From fe9b8ccb3d8aaa6d84b016754d4eea7408db58fd Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 6 Nov 2023 14:33:17 +0100 Subject: [PATCH 1/6] Check inner tx hash only against the storage --- apps/src/lib/node/ledger/shell/mod.rs | 9 ++++----- core/src/ledger/storage/wl_storage.rs | 8 ++++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index 4c924400fc..2157e94586 100644 --- a/apps/src/lib/node/ledger/shell/mod.rs +++ b/apps/src/lib/node/ledger/shell/mod.rs @@ -950,8 +950,10 @@ where .map_err(|e| Error::ReplayAttempt(e.to_string()))?; let inner_tx_hash = wrapper.raw_header_hash(); + // Do the inner tx hash check only against the storage, skip the write + // log if temp_wl_storage - .has_replay_protection_entry(&inner_tx_hash) + .has_committed_replay_protection_entry(&inner_tx_hash) .expect("Error while checking inner tx hash key in storage") { return Err(Error::ReplayAttempt(format!( @@ -960,10 +962,7 @@ where ))); } - // Write inner hash to WAL - temp_wl_storage - .write_tx_hash(inner_tx_hash) - .map_err(|e| Error::ReplayAttempt(e.to_string())) + Ok(()) } /// If a handle to an Ethereum oracle was provided to the [`Shell`], attempt 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 From ff54240993ecda16dbd40c0b8514ee4a28595657 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 6 Nov 2023 14:37:03 +0100 Subject: [PATCH 2/6] Delayed inner tx hash write --- .../lib/node/ledger/shell/finalize_block.rs | 53 ++++++++----------- shared/src/ledger/protocol/mod.rs | 37 +++++++------ 2 files changed, 44 insertions(+), 46 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index f4386cba16..8799e45c1a 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")))] @@ -314,9 +310,6 @@ where "Tx with hash {} was un-decryptable", tx_in_queue.tx.header_hash() ); - // Remove inner tx hash from storage - self.allow_tx_replay(tx_in_queue.tx); - event["info"] = "Transaction is invalid.".into(); event["log"] = "Transaction could not be \ @@ -456,6 +449,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 +493,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 +515,19 @@ 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(_) ) ) { - self.allow_tx_replay(wrapper); + self.commit_inner_tx_hash(wrapper); } - } 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 +940,16 @@ 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. 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"); } } 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); From 6cc6e3d3d946443746676e58a7b1639050e0e4ed Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 6 Nov 2023 16:47:51 +0100 Subject: [PATCH 3/6] Fixes and updates tests --- .../lib/node/ledger/shell/finalize_block.rs | 183 ++++++++++++++++-- .../lib/node/ledger/shell/prepare_proposal.rs | 46 ++--- .../lib/node/ledger/shell/process_proposal.rs | 21 +- 3 files changed, 178 insertions(+), 72 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 8799e45c1a..4dbb117326 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -525,8 +525,22 @@ where | Error::TxApply( protocol::Error::MissingSection(_) ) + | Error::TxApply( + protocol::Error::ReplayAttempt(_) + ) ) { 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 + self.wl_storage + .delete_tx_hash(wrapper.header_hash()) + .expect( + "Error while deleting tx hash from storage", + ); } } @@ -2251,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 @@ -2286,7 +2300,7 @@ mod test_finalize_block { .shell .wl_storage .write_log - .has_replay_protection_entry(&wrapper_tx.raw_header_hash()) + .has_replay_protection_entry(&wrapper_tx.header_hash()) .unwrap_or_default() ); // Check that the hash is present in the merkle tree @@ -2297,15 +2311,145 @@ mod test_finalize_block { .storage .block .tree - .has_key(&decrypted_hash_key) + .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 + .wl_storage + .storage + .write_replay_protection_entry(&mut batch, &hash_subkey) + .expect("Test failed"); + } + + 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(&inner.raw_header_hash()) + .unwrap_or_default() + ); + assert!( + !shell + .wl_storage + .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(); @@ -2372,17 +2516,17 @@ 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 @@ -2433,7 +2577,6 @@ 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(); assert_eq!(code, String::from(ErrorCodes::WasmRuntimeError).as_str()); @@ -2460,13 +2603,15 @@ mod test_finalize_block { !shell .wl_storage .write_log - .has_replay_protection_entry(&invalid_inner.header_hash()) + .has_replay_protection_entry( + &invalid_inner.raw_header_hash() + ) .unwrap_or_default() ); assert!( shell .wl_storage - .write_log + .storage .has_replay_protection_entry(&valid_wrapper.header_hash()) .unwrap_or_default() ); @@ -2474,8 +2619,8 @@ mod test_finalize_block { assert!( shell .wl_storage - .storage - .has_replay_protection_entry(&failing_inner.header_hash()) + .write_log + .has_replay_protection_entry(&failing_inner.raw_header_hash()) .expect("test failed") ); assert!( diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs index 2df4b520c5..94e8190048 100644 --- a/apps/src/lib/node/ledger/shell/prepare_proposal.rs +++ b/apps/src/lib/node/ledger/shell/prepare_proposal.rs @@ -248,7 +248,7 @@ where // 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 + // same hash can be deemed valid self.replay_protection_checks(&tx, temp_wl_storage) .map_err(|_| ())?; @@ -1201,14 +1201,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 +1236,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 +1283,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 +1329,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 +1338,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"), } } From 14b76a9c3b73cd1680d6fe751e540a4fd1dc3845 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Mon, 6 Nov 2023 16:58:54 +0100 Subject: [PATCH 4/6] Inverts the order of check for replay protection --- apps/src/lib/node/ledger/shell/mod.rs | 30 +++++++++---------- .../lib/node/ledger/shell/prepare_proposal.rs | 4 --- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/mod.rs b/apps/src/lib/node/ledger/shell/mod.rs index 2157e94586..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,7 @@ 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(); - // Do the inner tx hash check 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, - ))); - } - - Ok(()) + .map_err(|e| Error::ReplayAttempt(e.to_string())) } /// If a handle to an Ethereum oracle was provided to the [`Shell`], attempt diff --git a/apps/src/lib/node/ledger/shell/prepare_proposal.rs b/apps/src/lib/node/ledger/shell/prepare_proposal.rs index 94e8190048..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 be deemed valid self.replay_protection_checks(&tx, temp_wl_storage) .map_err(|_| ())?; From df30cd1845289d99ecdeb6d23eefebf21e6018b6 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 7 Nov 2023 10:37:27 +0100 Subject: [PATCH 5/6] Improves comments on wrapper tx hash removal --- apps/src/lib/node/ledger/shell/finalize_block.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/apps/src/lib/node/ledger/shell/finalize_block.rs b/apps/src/lib/node/ledger/shell/finalize_block.rs index 4dbb117326..15d1aea2c7 100644 --- a/apps/src/lib/node/ledger/shell/finalize_block.rs +++ b/apps/src/lib/node/ledger/shell/finalize_block.rs @@ -535,7 +535,9 @@ where ) = msg { // Remove the wrapper hash but keep the inner tx - // hash + // 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( @@ -955,8 +957,9 @@ where } // Write the inner tx hash to storage and remove the corresponding wrapper - // hash since it's redundant. Requires the wrapper transaction as argument - // to recover both the hashes. + // 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.raw_header_hash()) From 6c1d784fb1e064f8fcebd9c0e3cdb0913ab5b249 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Tue, 7 Nov 2023 13:48:52 +0100 Subject: [PATCH 6/6] Changelog #2104 --- .../improvements/2104-replay-protection-delay-write.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/2104-replay-protection-delay-write.md 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