Replay protection delay write

.changelog/unreleased/improvements/1905-replay-protection-improvements.md

@@ -0,0 +1,2 @@
+- Improved replay protection for invalid transactions.
+  ([\#1905](https://github.com/anoma/namada/pull/1905))

.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

core/src/ledger/tx_env.rs

@@ -64,4 +64,7 @@ pub trait TxEnv: StorageRead + StorageWrite {
         &self,
         event_type: impl AsRef<str>,
     ) -> Result<Vec<IbcEvent>, storage_api::Error>;
+
+    /// Set the sentinel for an invalid section commitment
+    fn set_commitment_sentinel(&mut self);
 }

core/src/proto/types.rs

@@ -26,7 +26,9 @@ use sha2::{Digest, Sha256};
 use thiserror::Error;
 
 use super::generated::types;
-use crate::ledger::gas::{GasMetering, VpGasMeter, VERIFY_TX_SIG_GAS_COST};
+use crate::ledger::gas::{
+    self, GasMetering, VpGasMeter, VERIFY_TX_SIG_GAS_COST,
+};
 use crate::ledger::storage::{KeccakHasher, Sha256Hasher, StorageHasher};
 #[cfg(any(feature = "tendermint", feature = "tendermint-abcipp"))]
 use crate::tendermint_proto::abci::ResponseDeliverTx;
@@ -74,8 +76,8 @@ pub enum Error {
     InvalidJSONDeserialization(String),
     #[error("The wrapper signature is invalid.")]
     InvalidWrapperSignature,
-    #[error("Signature verification went out of gas")]
-    OutOfGas,
+    #[error("Signature verification went out of gas: {0}")]
+    OutOfGas(gas::Error),
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -593,7 +595,7 @@ impl Signature {
                         if let Some(meter) = gas_meter {
                             meter
                                 .consume(VERIFY_TX_SIG_GAS_COST)
-                                .map_err(|_| VerifySigError::OutOfGas)?;
+                                .map_err(VerifySigError::OutOfGas)?;
                         }
                         common::SigScheme::verify_signature(
                             &pk,
@@ -618,7 +620,7 @@ impl Signature {
                         if let Some(meter) = gas_meter {
                             meter
                                 .consume(VERIFY_TX_SIG_GAS_COST)
-                                .map_err(|_| VerifySigError::OutOfGas)?;
+                                .map_err(VerifySigError::OutOfGas)?;
                         }
                         common::SigScheme::verify_signature(
                             pk,
@@ -1448,8 +1450,8 @@ impl Tx {
                             gas_meter,
                         )
                         .map_err(|e| {
-                            if let VerifySigError::OutOfGas = e {
-                                Error::OutOfGas
+                            if let VerifySigError::OutOfGas(inner) = e {
+                                Error::OutOfGas(inner)
                             } else {
                                 Error::InvalidSectionSignature(
                                     "found invalid signature.".to_string(),

core/src/types/key/mod.rs

@@ -124,8 +124,8 @@ pub enum VerifySigError {
     MissingData,
     #[error("Signature belongs to a different scheme from the public key.")]
     MismatchedScheme,
-    #[error("Signature verification went out of gas")]
-    OutOfGas,
+    #[error("Signature verification went out of gas: {0}")]
+    OutOfGas(crate::ledger::gas::Error),
 }
 
 #[allow(missing_docs)]

core/src/types/transaction/mod.rs

@@ -80,6 +80,8 @@ pub struct VpsResult {
     pub gas_used: VpsGas,
     /// Errors occurred in any of the VPs, if any
     pub errors: Vec<(Address, String)>,
+    /// Sentinel to signal an invalid transaction signature
+    pub invalid_sig: bool,
 }
 
 impl fmt::Display for TxResult {
@@ -166,6 +168,31 @@ impl TxType {
     }
 }
 
+/// Sentinel used in transactions to signal events that require special
+/// replay protection handling back to the protocol.
+#[derive(Debug, Default)]
+pub enum TxSentinel {
+    /// No action required
+    #[default]
+    None,
+    /// Exceeded gas limit
+    OutOfGas,
+    /// Found invalid commtiment to one of the transaction's sections
+    InvalidCommitment,
+}
+
+impl TxSentinel {
+    /// Set the sentinel for an out of gas error
+    pub fn set_out_of_gas(&mut self) {
+        *self = Self::OutOfGas
+    }
+
+    /// Set the sentinel for an invalid section commitment error
+    pub fn set_invalid_commitment(&mut self) {
+        *self = Self::InvalidCommitment
+    }
+}
+
 #[cfg(test)]
 mod test_process_tx {
     use super::*;

core/src/types/validity_predicate.rs

@@ -17,3 +17,38 @@ pub struct EvalVp {
     /// The input for the `eval`ed VP
     pub input: Tx,
 }
+
+/// Sentinel used in validity predicates to signal events that require special
+/// replay protection handling back to the protocol.
+#[derive(Debug, Default)]
+pub enum VpSentinel {
+    /// No action required
+    #[default]
+    None,
+    /// Exceeded gas limit
+    OutOfGas,
+    /// Found invalid transaction signature
+    InvalidSignature,
+}
+
+impl VpSentinel {
+    /// Check if the Vp ran out of gas
+    pub fn is_out_of_gas(&self) -> bool {
+        matches!(self, Self::OutOfGas)
+    }
+
+    /// Check if the Vp found an invalid signature
+    pub fn is_invalid_signature(&self) -> bool {
+        matches!(self, Self::InvalidSignature)
+    }
+
+    /// Set the sentinel for an out of gas error
+    pub fn set_out_of_gas(&mut self) {
+        *self = Self::OutOfGas
+    }
+
+    /// Set the sentinel for an invalid signature error
+    pub fn set_invalid_signature(&mut self) {
+        *self = Self::InvalidSignature
+    }
+}