Merge branch 'grarco/replay-protection-delay-write' (#2104)

.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))

apps/src/lib/node/ledger/shell/mod.rs

@@ -933,6 +933,19 @@ where
         wrapper: &Tx,
         temp_wl_storage: &mut TempWlStorage<D, H>,
     ) -> 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()))
     }
 

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

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"),
         }
     }
 

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<bool, super::Error> {
+        self.storage.has_replay_protection_entry(hash)
+    }
 }
 
 /// Common trait for [`WlStorage`] and [`TempWlStorage`], used to implement

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);

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();