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

* origin/grarco/replay-protection-delay-write:
  Changelog #2104
  Improves comments on wrapper tx hash removal
  Inverts the order of check for replay protection
  Fixes and updates tests
  Delayed inner tx hash write
  Check inner tx hash only against the storage

.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/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,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 +956,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");
     }
 }
@@ -2260,9 +2268,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
@@ -2295,7 +2303,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
@@ -2306,15 +2314,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<ProcessedTx> = 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();
@@ -2381,17 +2519,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
@@ -2442,7 +2580,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());
@@ -2469,22 +2606,24 @@ 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()
             );
         }
         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!(