diff --git a/.changelog/unreleased/improvements/2104-replay-protection-delay-write.md b/.changelog/unreleased/improvements/2104-replay-protection-delay-write.md new file mode 100644 index 0000000000..8ab0ba0377 --- /dev/null +++ b/.changelog/unreleased/improvements/2104-replay-protection-delay-write.md @@ -0,0 +1,2 @@ +- Moved the inner transaction replay check at execution time. + ([\#2104](https://github.com/anoma/namada/pull/2104)) \ No newline at end of file diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index f4386cba16..beb1c33aa1 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -210,17 +210,13 @@ where tx_event["gas_used"] = "0".into(); response.events.push(tx_event); // if the rejected tx was decrypted, remove it - // from the queue of txs to be processed, remove its hash - // from storage and write the hash of the corresponding wrapper + // from the queue of txs to be processed if let TxType::Decrypted(_) = &tx_header.tx_type { - let wrapper_tx = self - .wl_storage + self.wl_storage .storage .tx_queue .pop() - .expect("Missing wrapper tx in queue") - .tx; - self.allow_tx_replay(wrapper_tx); + .expect("Missing wrapper tx in queue"); } #[cfg(not(any(feature = "abciplus", feature = "abcipp")))] @@ -456,6 +452,9 @@ where result ); stats.increment_successful_txs(); + if let Some(wrapper) = embedding_wrapper { + self.commit_inner_tx_hash(wrapper); + } } self.wl_storage.commit_tx(); if !tx_event.contains_key("code") { @@ -497,10 +496,11 @@ where ); if let Some(wrapper) = embedding_wrapper { - if result.vps_result.invalid_sig { - // Invalid signature was found, remove the tx - // hash from storage to allow replay - self.allow_tx_replay(wrapper); + // If decrypted tx failed for any reason but invalid + // signature, commit its hash to storage, otherwise + // allow for a replay + if !result.vps_result.invalid_sig { + self.commit_inner_tx_hash(wrapper); } } @@ -518,26 +518,35 @@ where msg ); - // If transaction type is Decrypted and failed because of - // out of gas or invalid section commtiment, remove its hash - // from storage to allow rewrapping it + // If transaction type is Decrypted and didn't failed + // because of out of gas nor invalid + // section commitment, commit its hash to prevent replays if let Some(wrapper) = embedding_wrapper { - if matches!( + if !matches!( msg, Error::TxApply(protocol::Error::GasError(_)) | Error::TxApply( protocol::Error::MissingSection(_) ) + | Error::TxApply( + protocol::Error::ReplayAttempt(_) + ) ) { - self.allow_tx_replay(wrapper); + self.commit_inner_tx_hash(wrapper); + } else if let Error::TxApply( + protocol::Error::ReplayAttempt(_), + ) = msg + { + // Remove the wrapper hash but keep the inner tx + // hash. A replay of the wrapper is impossible since + // the inner tx hash is committed to storage and + // we validate the wrapper against that hash too + self.wl_storage + .delete_tx_hash(wrapper.header_hash()) + .expect( + "Error while deleting tx hash from storage", + ); } - } else if let Some(wrapper) = wrapper { - // If transaction type was Wrapper and failed, write its - // hash to storage to prevent - // replay - self.wl_storage - .write_tx_hash(wrapper.header_hash()) - .expect("Error while writing tx hash to storage"); } stats.increment_errored_txs(); @@ -950,15 +959,17 @@ where Ok(()) } - // Allow to replay a specific wasm transaction. Needs as argument the - // corresponding wrapper transaction to avoid replay of that in the process - fn allow_tx_replay(&mut self, wrapper_tx: Tx) { + // Write the inner tx hash to storage and remove the corresponding wrapper + // hash since it's redundant (we check the inner tx hash too when validating + // the wrapper). Requires the wrapper transaction as argument to recover + // both the hashes. + fn commit_inner_tx_hash(&mut self, wrapper_tx: Tx) { self.wl_storage - .write_tx_hash(wrapper_tx.header_hash()) + .write_tx_hash(wrapper_tx.raw_header_hash()) .expect("Error while writing tx hash to storage"); self.wl_storage - .delete_tx_hash(wrapper_tx.raw_header_hash()) + .delete_tx_hash(wrapper_tx.header_hash()) .expect("Error while deleting tx hash from storage"); } } @@ -2071,11 +2082,9 @@ mod test_finalize_block { // won't receive votes from TM since we receive votes at a 1-block // delay, so votes will be empty here next_block_for_inflation(&mut shell, pkh1.clone(), vec![], None); - assert!( - rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap()); // FINALIZE BLOCK 2. Tell Namada that val1 is the block proposer. // Include votes that correspond to block 1. Make val2 the next block's @@ -2085,11 +2094,9 @@ mod test_finalize_block { assert!(rewards_prod_2.is_empty(&shell.wl_storage).unwrap()); assert!(rewards_prod_3.is_empty(&shell.wl_storage).unwrap()); assert!(rewards_prod_4.is_empty(&shell.wl_storage).unwrap()); - assert!( - !rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(!rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap()); // Val1 was the proposer, so its reward should be larger than all // others, which should themselves all be equal let acc_sum = get_rewards_sum(&shell.wl_storage); @@ -2203,11 +2210,9 @@ mod test_finalize_block { None, ); } - assert!( - rewards_accumulator_handle() - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(rewards_accumulator_handle() + .is_empty(&shell.wl_storage) + .unwrap()); let rp1 = rewards_prod_1 .get(&shell.wl_storage, &Epoch::default()) .unwrap() @@ -2260,9 +2265,9 @@ mod test_finalize_block { let (wrapper_tx, processed_tx) = mk_wrapper_tx(&shell, &crate::wallet::defaults::albert_keypair()); - let decrypted_hash_key = + let wrapper_hash_key = replay_protection::get_replay_protection_last_key( - &wrapper_tx.raw_header_hash(), + &wrapper_tx.header_hash(), ); // merkle tree root before finalize_block @@ -2290,31 +2295,153 @@ mod test_finalize_block { assert_eq!(root_pre.0, root_post.0); // Check transaction's hash in storage - assert!( + assert!(shell + .shell + .wl_storage + .write_log + .has_replay_protection_entry(&wrapper_tx.header_hash()) + .unwrap_or_default()); + // Check that the hash is present in the merkle tree + assert!(!shell + .shell + .wl_storage + .storage + .block + .tree + .has_key(&wrapper_hash_key) + .unwrap()); + } + + /// Test that a decrypted tx that has already been applied in the same block + /// doesn't get reapplied + #[test] + fn test_duplicated_decrypted_tx_same_block() { + let (mut shell, _, _, _) = setup(); + let keypair = gen_keypair(); + let keypair_2 = gen_keypair(); + let mut batch = + namada::core::ledger::storage::testing::TestStorage::batch(); + + let tx_code = TestWasms::TxNoOp.read_bytes(); + let mut wrapper = + Tx::from_type(TxType::Wrapper(Box::new(WrapperTx::new( + Fee { + amount_per_gas_unit: 1.into(), + token: shell.wl_storage.storage.native_token.clone(), + }, + keypair.ref_to(), + Epoch(0), + GAS_LIMIT_MULTIPLIER.into(), + None, + )))); + wrapper.header.chain_id = shell.chain_id.clone(); + wrapper.set_code(Code::new(tx_code)); + wrapper.set_data(Data::new( + "Decrypted transaction data".as_bytes().to_owned(), + )); + + let mut new_wrapper = wrapper.clone(); + new_wrapper.update_header(TxType::Wrapper(Box::new(WrapperTx::new( + Fee { + amount_per_gas_unit: 1.into(), + token: shell.wl_storage.storage.native_token.clone(), + }, + keypair_2.ref_to(), + Epoch(0), + GAS_LIMIT_MULTIPLIER.into(), + None, + )))); + new_wrapper.add_section(Section::Signature(Signature::new( + new_wrapper.sechashes(), + [(0, keypair_2)].into_iter().collect(), + None, + ))); + wrapper.add_section(Section::Signature(Signature::new( + wrapper.sechashes(), + [(0, keypair)].into_iter().collect(), + None, + ))); + + let mut inner = wrapper.clone(); + let mut new_inner = new_wrapper.clone(); + + for inner in [&mut inner, &mut new_inner] { + inner.update_header(TxType::Decrypted(DecryptedTx::Decrypted)); + } + + // Write wrapper hashes in storage + for tx in [&wrapper, &new_wrapper] { + let hash_subkey = + replay_protection::get_replay_protection_last_subkey( + &tx.header_hash(), + ); shell - .shell + .wl_storage + .storage + .write_replay_protection_entry(&mut batch, &hash_subkey) + .unwrap(); + } + + let mut processed_txs: Vec = vec![]; + for inner in [&inner, &new_inner] { + processed_txs.push(ProcessedTx { + tx: inner.to_bytes(), + result: TxResult { + code: ErrorCodes::Ok.into(), + info: "".into(), + }, + }) + } + + shell.enqueue_tx(wrapper.clone(), GAS_LIMIT_MULTIPLIER.into()); + shell.enqueue_tx(new_wrapper.clone(), GAS_LIMIT_MULTIPLIER.into()); + // merkle tree root before finalize_block + let root_pre = shell.shell.wl_storage.storage.block.tree.root(); + + let event = &shell + .finalize_block(FinalizeBlock { + txs: processed_txs, + ..Default::default() + }) + .expect("Test failed"); + + // the merkle tree root should not change after finalize_block + let root_post = shell.shell.wl_storage.storage.block.tree.root(); + assert_eq!(root_pre.0, root_post.0); + + assert_eq!(event[0].event_type.to_string(), String::from("applied")); + let code = event[0] + .attributes + .get("code") + .expect("Testfailed") + .as_str(); + assert_eq!(code, String::from(ErrorCodes::Ok).as_str()); + assert_eq!(event[1].event_type.to_string(), String::from("applied")); + let code = event[1] + .attributes + .get("code") + .expect("Testfailed") + .as_str(); + assert_eq!(code, String::from(ErrorCodes::WasmRuntimeError).as_str()); + + for (inner, wrapper) in [(inner, wrapper), (new_inner, new_wrapper)] { + assert!(shell .wl_storage .write_log - .has_replay_protection_entry(&wrapper_tx.raw_header_hash()) - .unwrap_or_default() - ); - // Check that the hash is present in the merkle tree - assert!( - !shell - .shell + .has_replay_protection_entry(&inner.raw_header_hash()) + .unwrap_or_default()); + assert!(!shell .wl_storage - .storage - .block - .tree - .has_key(&decrypted_hash_key) - .unwrap() - ); + .write_log + .has_replay_protection_entry(&wrapper.header_hash()) + .unwrap_or_default()); + } } /// Test that if a decrypted transaction fails because of out-of-gas, /// undecryptable, invalid signature or wrong section commitment, its hash - /// is removed from storage. Also checks that a tx failing for other - /// reason has its hash persisted to storage. + /// is not committed to storage. Also checks that a tx failing for other + /// reason has its hash written to storage. #[test] fn test_tx_hash_handling() { let (mut shell, _, _, _) = setup(); @@ -2381,23 +2508,23 @@ mod test_finalize_block { inner.update_header(TxType::Decrypted(DecryptedTx::Decrypted)); } - // Write inner hashes in storage - for inner in [ - &out_of_gas_inner, - &undecryptable_inner, - &unsigned_inner, - &wrong_commitment_inner, - &failing_inner, + // Write wrapper hashes in storage + for wrapper in [ + &out_of_gas_wrapper, + &undecryptable_wrapper, + &unsigned_wrapper, + &wrong_commitment_wrapper, + &failing_wrapper, ] { let hash_subkey = replay_protection::get_replay_protection_last_subkey( - &inner.header_hash(), + &wrapper.header_hash(), ); shell .wl_storage .storage .write_replay_protection_entry(&mut batch, &hash_subkey) - .unwrap(); + .expect("Test failed"); } let mut processed_txs: Vec = vec![]; @@ -2442,21 +2569,40 @@ mod test_finalize_block { let root_post = shell.shell.wl_storage.storage.block.tree.root(); assert_eq!(root_pre.0, root_post.0); - // Check inner tx hash has been removed from storage assert_eq!(event[0].event_type.to_string(), String::from("applied")); - let code = event[0].attributes.get("code").unwrap().as_str(); + let code = event[0] + .attributes + .get("code") + .expect("Testfailed") + .as_str(); assert_eq!(code, String::from(ErrorCodes::WasmRuntimeError).as_str()); assert_eq!(event[1].event_type.to_string(), String::from("applied")); - let code = event[1].attributes.get("code").unwrap().as_str(); + let code = event[1] + .attributes + .get("code") + .expect("Testfailed") + .as_str(); assert_eq!(code, String::from(ErrorCodes::Undecryptable).as_str()); assert_eq!(event[2].event_type.to_string(), String::from("applied")); - let code = event[2].attributes.get("code").unwrap().as_str(); + let code = event[2] + .attributes + .get("code") + .expect("Testfailed") + .as_str(); assert_eq!(code, String::from(ErrorCodes::InvalidTx).as_str()); assert_eq!(event[3].event_type.to_string(), String::from("applied")); - let code = event[3].attributes.get("code").unwrap().as_str(); + let code = event[3] + .attributes + .get("code") + .expect("Testfailed") + .as_str(); assert_eq!(code, String::from(ErrorCodes::WasmRuntimeError).as_str()); assert_eq!(event[4].event_type.to_string(), String::from("applied")); - let code = event[4].attributes.get("code").unwrap().as_str(); + let code = event[4] + .attributes + .get("code") + .expect("Testfailed") + .as_str(); assert_eq!(code, String::from(ErrorCodes::WasmRuntimeError).as_str()); for (invalid_inner, valid_wrapper) in [ @@ -2465,35 +2611,27 @@ mod test_finalize_block { (unsigned_inner, unsigned_wrapper), (wrong_commitment_inner, wrong_commitment_wrapper), ] { - assert!( - !shell - .wl_storage - .write_log - .has_replay_protection_entry(&invalid_inner.header_hash()) - .unwrap_or_default() - ); - assert!( - shell - .wl_storage - .write_log - .has_replay_protection_entry(&valid_wrapper.header_hash()) - .unwrap_or_default() - ); - } - assert!( - shell - .wl_storage - .storage - .has_replay_protection_entry(&failing_inner.header_hash()) - .expect("test failed") - ); - assert!( - !shell + assert!(!shell .wl_storage .write_log - .has_replay_protection_entry(&failing_wrapper.header_hash()) - .unwrap_or_default() - ); + .has_replay_protection_entry(&invalid_inner.raw_header_hash()) + .unwrap_or_default()); + assert!(shell + .wl_storage + .storage + .has_replay_protection_entry(&valid_wrapper.header_hash()) + .unwrap_or_default()); + } + assert!(shell + .wl_storage + .write_log + .has_replay_protection_entry(&failing_inner.raw_header_hash()) + .expect("test failed")); + assert!(!shell + .wl_storage + .write_log + .has_replay_protection_entry(&failing_wrapper.header_hash()) + .unwrap_or_default()); } #[test] @@ -2559,20 +2697,16 @@ mod test_finalize_block { .as_str(); assert_eq!(code, String::from(ErrorCodes::InvalidTx).as_str()); - assert!( - shell - .wl_storage - .write_log - .has_replay_protection_entry(&wrapper_hash) - .unwrap_or_default() - ); - assert!( - !shell - .wl_storage - .write_log - .has_replay_protection_entry(&wrapper.raw_header_hash()) - .unwrap_or_default() - ); + assert!(shell + .wl_storage + .write_log + .has_replay_protection_entry(&wrapper_hash) + .unwrap_or_default()); + assert!(!shell + .wl_storage + .write_log + .has_replay_protection_entry(&wrapper.raw_header_hash()) + .unwrap_or_default()); } // Test that if the fee payer doesn't have enough funds for fee payment the @@ -2860,11 +2994,9 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Consensus) ); - assert!( - enqueued_slashes_handle() - .at(&Epoch::default()) - .is_empty(&shell.wl_storage)? - ); + assert!(enqueued_slashes_handle() + .at(&Epoch::default()) + .is_empty(&shell.wl_storage)?); assert_eq!( get_num_consensus_validators(&shell.wl_storage, Epoch::default()) .unwrap(), @@ -2883,21 +3015,17 @@ mod test_finalize_block { .unwrap(), Some(ValidatorState::Jailed) ); - assert!( - enqueued_slashes_handle() - .at(&epoch) - .is_empty(&shell.wl_storage)? - ); + assert!(enqueued_slashes_handle() + .at(&epoch) + .is_empty(&shell.wl_storage)?); assert_eq!( get_num_consensus_validators(&shell.wl_storage, epoch).unwrap(), 5_u64 ); } - assert!( - !enqueued_slashes_handle() - .at(&processing_epoch) - .is_empty(&shell.wl_storage)? - ); + assert!(!enqueued_slashes_handle() + .at(&processing_epoch) + .is_empty(&shell.wl_storage)?); // Advance to the processing epoch loop { @@ -2920,11 +3048,9 @@ mod test_finalize_block { // println!("Reached processing epoch"); break; } else { - assert!( - enqueued_slashes_handle() - .at(&shell.wl_storage.storage.block.epoch) - .is_empty(&shell.wl_storage)? - ); + assert!(enqueued_slashes_handle() + .at(&shell.wl_storage.storage.block.epoch) + .is_empty(&shell.wl_storage)?); let stake1 = read_validator_stake( &shell.wl_storage, ¶ms, @@ -3486,15 +3612,13 @@ mod test_finalize_block { ) .unwrap(); assert_eq!(last_slash, Some(Epoch(4))); - assert!( - namada_proof_of_stake::is_validator_frozen( - &shell.wl_storage, - &val1.address, - current_epoch, - ¶ms - ) - .unwrap() - ); + assert!(namada_proof_of_stake::is_validator_frozen( + &shell.wl_storage, + &val1.address, + current_epoch, + ¶ms + ) + .unwrap()); assert!( namada_proof_of_stake::validator_slashes_handle(&val1.address) .is_empty(&shell.wl_storage) @@ -4003,12 +4127,14 @@ mod test_finalize_block { assert!(!consensus_val_set.at(&ep).is_empty(storage).unwrap()); // assert!(!below_cap_val_set.at(&ep).is_empty(storage). // unwrap()); - assert!( - !validator_positions.at(&ep).is_empty(storage).unwrap() - ); - assert!( - !all_validator_addresses.at(&ep).is_empty(storage).unwrap() - ); + assert!(!validator_positions + .at(&ep) + .is_empty(storage) + .unwrap()); + assert!(!all_validator_addresses + .at(&ep) + .is_empty(storage) + .unwrap()); } }; @@ -4047,33 +4173,25 @@ mod test_finalize_block { Epoch(1), Epoch(params.pipeline_len + default_past_epochs + 1), ); - assert!( - !consensus_val_set - .at(&Epoch(0)) - .is_empty(&shell.wl_storage) - .unwrap() - ); - assert!( - validator_positions - .at(&Epoch(0)) - .is_empty(&shell.wl_storage) - .unwrap() - ); - assert!( - all_validator_addresses - .at(&Epoch(0)) - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(!consensus_val_set + .at(&Epoch(0)) + .is_empty(&shell.wl_storage) + .unwrap()); + assert!(validator_positions + .at(&Epoch(0)) + .is_empty(&shell.wl_storage) + .unwrap()); + assert!(all_validator_addresses + .at(&Epoch(0)) + .is_empty(&shell.wl_storage) + .unwrap()); // Advance to the epoch `consensus_val_set_len` + 1 loop { - assert!( - !consensus_val_set - .at(&Epoch(0)) - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(!consensus_val_set + .at(&Epoch(0)) + .is_empty(&shell.wl_storage) + .unwrap()); let votes = get_default_true_votes( &shell.wl_storage, shell.wl_storage.storage.block.epoch, @@ -4084,12 +4202,10 @@ mod test_finalize_block { } } - assert!( - consensus_val_set - .at(&Epoch(0)) - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(consensus_val_set + .at(&Epoch(0)) + .is_empty(&shell.wl_storage) + .unwrap()); // Advance one more epoch let votes = get_default_true_votes( @@ -4098,23 +4214,19 @@ mod test_finalize_block { ); current_epoch = advance_epoch(&mut shell, &pkh1, &votes, None); for ep in Epoch::default().iter_range(2) { - assert!( - consensus_val_set - .at(&ep) - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(consensus_val_set + .at(&ep) + .is_empty(&shell.wl_storage) + .unwrap()); } for ep in Epoch::iter_bounds_inclusive( Epoch(2), current_epoch + params.pipeline_len, ) { - assert!( - !consensus_val_set - .at(&ep) - .is_empty(&shell.wl_storage) - .unwrap() - ); + assert!(!consensus_val_set + .at(&ep) + .is_empty(&shell.wl_storage) + .unwrap()); } Ok(()) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index 4c924400fc..50d84dc807 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -933,6 +933,19 @@ where wrapper: &Tx, temp_wl_storage: &mut TempWlStorage, ) -> Result<()> { + let inner_tx_hash = wrapper.raw_header_hash(); + // Check the inner tx hash only against the storage, skip the write + // log + if temp_wl_storage + .has_committed_replay_protection_entry(&inner_tx_hash) + .expect("Error while checking inner tx hash key in storage") + { + return Err(Error::ReplayAttempt(format!( + "Inner transaction hash {} already in storage", + &inner_tx_hash, + ))); + } + let wrapper_hash = wrapper.header_hash(); if temp_wl_storage .has_replay_protection_entry(&wrapper_hash) @@ -947,22 +960,6 @@ where // Write wrapper hash to WAL temp_wl_storage .write_tx_hash(wrapper_hash) - .map_err(|e| Error::ReplayAttempt(e.to_string()))?; - - let inner_tx_hash = wrapper.raw_header_hash(); - if temp_wl_storage - .has_replay_protection_entry(&inner_tx_hash) - .expect("Error while checking inner tx hash key in storage") - { - return Err(Error::ReplayAttempt(format!( - "Inner transaction hash {} already in storage", - &inner_tx_hash, - ))); - } - - // Write inner hash to WAL - temp_wl_storage - .write_tx_hash(inner_tx_hash) .map_err(|e| Error::ReplayAttempt(e.to_string())) } diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs index 2df4b520c5..0f652d5130 100644 --- a/apps/src/lib/node/ledger/shell/prepare_proposal.rs +++ b/apps/src/lib/node/ledger/shell/prepare_proposal.rs @@ -245,10 +245,6 @@ where let mut tx_gas_meter = TxGasMeter::new(wrapper.gas_limit); tx_gas_meter.add_tx_size_gas(tx_bytes).map_err(|_| ())?; - // Check replay protection, safe to do here. Even if the tx is a - // replay attempt, we can leave its hashes in the write log since, - // having we already checked the signature, no other tx with the - // same hash can ba deemed valid self.replay_protection_checks(&tx, temp_wl_storage) .map_err(|_| ())?; @@ -1201,14 +1197,8 @@ mod test_prepare_proposal { ..Default::default() }; - let received = - shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { - Tx::try_from(tx_bytes.as_slice()) - .expect("Test failed") - .data() - .expect("Test failed") - }); - assert_eq!(received.len(), 0); + let received_txs = shell.prepare_proposal(req).txs; + assert_eq!(received_txs.len(), 0); } /// Test that if two identical wrapper txs are proposed for this block, only @@ -1242,14 +1232,8 @@ mod test_prepare_proposal { txs: vec![wrapper.to_bytes(); 2], ..Default::default() }; - let received = - shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { - Tx::try_from(tx_bytes.as_slice()) - .expect("Test failed") - .data() - .expect("Test failed") - }); - assert_eq!(received.len(), 1); + let received_txs = shell.prepare_proposal(req).txs; + assert_eq!(received_txs.len(), 1); } /// Test that if the unsigned inner tx hash is known (replay attack), the @@ -1295,18 +1279,12 @@ mod test_prepare_proposal { ..Default::default() }; - let received = - shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { - Tx::try_from(tx_bytes.as_slice()) - .expect("Test failed") - .data() - .expect("Test failed") - }); - assert_eq!(received.len(), 0); + let received_txs = shell.prepare_proposal(req).txs; + assert_eq!(received_txs.len(), 0); } /// Test that if two identical decrypted txs are proposed for this block, - /// only one gets accepted + /// both get accepted #[test] fn test_inner_tx_hash_same_block() { let (shell, _recv, _, _) = test_utils::setup(); @@ -1347,7 +1325,7 @@ mod test_prepare_proposal { None, )))); new_wrapper.add_section(Section::Signature(Signature::new( - wrapper.sechashes(), + new_wrapper.sechashes(), [(0, keypair_2)].into_iter().collect(), None, ))); @@ -1356,14 +1334,8 @@ mod test_prepare_proposal { txs: vec![wrapper.to_bytes(), new_wrapper.to_bytes()], ..Default::default() }; - let received = - shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| { - Tx::try_from(tx_bytes.as_slice()) - .expect("Test failed") - .data() - .expect("Test failed") - }); - assert_eq!(received.len(), 1); + let received_txs = shell.prepare_proposal(req).txs; + assert_eq!(received_txs.len(), 2); } /// Test that expired wrapper transactions are not included in the block diff --git a/apps/src/lib/node/ledger/shell/process_proposal.rs b/apps/src/lib/node/ledger/shell/process_proposal.rs index 4968faa050..e337a7b5af 100644 --- a/apps/src/lib/node/ledger/shell/process_proposal.rs +++ b/apps/src/lib/node/ledger/shell/process_proposal.rs @@ -2257,7 +2257,7 @@ mod test_process_proposal { } /// Test that a block containing two identical inner transactions is - /// rejected + /// accepted #[test] fn test_inner_tx_hash_same_block() { let (shell, _recv, _, _) = test_utils::setup(); @@ -2285,7 +2285,6 @@ mod test_process_proposal { [(0, keypair)].into_iter().collect(), None, ))); - let inner_unsigned_hash = wrapper.raw_header_hash(); new_wrapper.update_header(TxType::Wrapper(Box::new(WrapperTx::new( Fee { @@ -2308,22 +2307,8 @@ mod test_process_proposal { txs: vec![wrapper.to_bytes(), new_wrapper.to_bytes()], }; match shell.process_proposal(request) { - Ok(_) => panic!("Test failed"), - Err(TestError::RejectProposal(response)) => { - assert_eq!(response[0].result.code, u32::from(ErrorCodes::Ok)); - assert_eq!( - response[1].result.code, - u32::from(ErrorCodes::ReplayTx) - ); - assert_eq!( - response[1].result.info, - format!( - "Transaction replay attempt: Inner transaction hash \ - {} already in storage", - inner_unsigned_hash - ) - ); - } + Ok(received) => assert_eq!(received.len(), 2), + Err(_) => panic!("Test failed"), } } diff --git a/core/src/ledger/storage/wl_storage.rs b/core/src/ledger/storage/wl_storage.rs index 6d30e77c73..4d613365c0 100644 --- a/core/src/ledger/storage/wl_storage.rs +++ b/core/src/ledger/storage/wl_storage.rs @@ -69,6 +69,14 @@ where self.storage.has_replay_protection_entry(hash) } + + /// Check if the given tx hash has already been committed to storage + pub fn has_committed_replay_protection_entry( + &self, + hash: &Hash, + ) -> Result { + self.storage.has_replay_protection_entry(hash) + } } /// Common trait for [`WlStorage`] and [`TempWlStorage`], used to implement diff --git a/shared/src/ledger/protocol/mod.rs b/shared/src/ledger/protocol/mod.rs index e2292bd1a7..07430c1ccf 100644 --- a/shared/src/ledger/protocol/mod.rs +++ b/shared/src/ledger/protocol/mod.rs @@ -29,7 +29,7 @@ use crate::ledger::pgf::PgfVp; use crate::ledger::pos::{self, PosVP}; use crate::ledger::storage::write_log::WriteLog; use crate::ledger::storage::{DBIter, Storage, StorageHasher, WlStorage, DB}; -use crate::ledger::{replay_protection, storage_api}; +use crate::ledger::storage_api; use crate::proto::{self, Tx}; use crate::types::address::{Address, InternalAddress}; use crate::types::storage; @@ -62,6 +62,10 @@ pub enum Error { FeeError(String), #[error("Invalid transaction signature")] InvalidTxSignature, + #[error( + "The decrypted transaction {0} has already been applied in this block" + )] + ReplayAttempt(Hash), #[error("Error executing VP for addresses: {0:?}")] VpRunnerError(vm::wasm::run::Error), #[error("The address {0} doesn't exist")] @@ -212,12 +216,11 @@ where } /// Performs the required operation on a wrapper transaction: +/// - replay protection /// - fee payment /// - gas accounting -/// - replay protection /// -/// Returns the set of changed storage keys. The caller should write the hash of -/// the wrapper header to storage in case of failure. +/// Returns the set of changed storage keys. pub(crate) fn apply_wrapper_tx<'a, D, H, CA, WLS>( tx: Tx, wrapper: &WrapperTx, @@ -234,6 +237,12 @@ where { let mut changed_keys = BTreeSet::default(); + // Write wrapper tx hash to storage + shell_params + .wl_storage + .write_tx_hash(tx.header_hash()) + .expect("Error while writing tx hash to storage"); + // Charge fee before performing any fallible operations charge_fee( wrapper, @@ -249,15 +258,6 @@ where .add_tx_size_gas(tx_bytes) .map_err(|err| Error::GasError(err.to_string()))?; - // If wrapper was succesful, write inner tx hash to storage - shell_params - .wl_storage - .write_tx_hash(tx.raw_header_hash()) - .expect("Error while writing tx hash to storage"); - changed_keys.insert(replay_protection::get_replay_protection_last_key( - &tx.raw_header_hash(), - )); - Ok(changed_keys) } @@ -578,6 +578,13 @@ where ) }; + let tx_hash = tx.raw_header_hash(); + if let Some(true) = write_log.has_replay_protection_entry(&tx_hash) { + // If the same transaction has already been applied in this block, skip + // execution and return + return Err(Error::ReplayAttempt(tx_hash)); + } + let verifiers = execute_tx( &tx, tx_index, @@ -1035,10 +1042,10 @@ where Err(err) => match err { // Execution of VPs can (and must) be short-circuited // only in case of a gas overflow to prevent the - // transaction from conuming resources that have not + // transaction from consuming resources that have not // been acquired in the corresponding wrapper tx. For // all the other errors we keep evaluating the vps. This - // allow to display a consistent VpsResult accross all + // allows to display a consistent VpsResult accross all // nodes and find any invalid signatures Error::GasError(_) => { return Err(err); diff --git a/shared/src/vm/wasm/run.rs b/shared/src/vm/wasm/run.rs index d4ce052124..bb7b5e5cd8 100644 --- a/shared/src/vm/wasm/run.rs +++ b/shared/src/vm/wasm/run.rs @@ -733,6 +733,7 @@ mod tests { let mut gas_meter = VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), ); + let _invalid_sig = false; let keys_changed = BTreeSet::new(); let verifiers = BTreeSet::new(); let tx_index = TxIndex::default(); @@ -836,6 +837,7 @@ mod tests { let mut gas_meter = VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), ); + let _invalid_sig = false; let keys_changed = BTreeSet::new(); let verifiers = BTreeSet::new(); let tx_index = TxIndex::default(); @@ -968,6 +970,7 @@ mod tests { let mut gas_meter = VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), ); + let _invalid_sig = false; let keys_changed = BTreeSet::new(); let verifiers = BTreeSet::new(); let tx_index = TxIndex::default(); @@ -1089,6 +1092,7 @@ mod tests { let mut gas_meter = VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), ); + let _invalid_sig = false; let keys_changed = BTreeSet::new(); let verifiers = BTreeSet::new(); let tx_index = TxIndex::default(); @@ -1148,6 +1152,7 @@ mod tests { let mut gas_meter = VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), ); + let _invalid_sig = false; let keys_changed = BTreeSet::new(); let verifiers = BTreeSet::new(); let tx_index = TxIndex::default(); @@ -1317,6 +1322,7 @@ mod tests { let mut gas_meter = VpGasMeter::new_from_tx_meter( &TxGasMeter::new_from_sub_limit(TX_GAS_LIMIT.into()), ); + let _invalid_sig = false; let keys_changed = BTreeSet::new(); let verifiers = BTreeSet::new(); let (vp_cache, _) = wasm::compilation_cache::common::testing::cache();