fix(inspect): finalize journal on error to prevent EIP-2929 warm set leak (bluealloy#3780)

* fix(inspect): finalize journal on error to prevent EIP-2929 leak

`InspectEvm::inspect_tx` (and its sibling inspect/system-call/commit
variants) used `self.X(tx)?` to short-circuit before calling
`finalize()`. On error the handler calls `discard_tx`, which reverts
account values but keeps the entries in the state map and increments
`transaction_id`, so the EIP-2929 warm set and touched accounts leaked
into the next transaction.

This mirrors the same latent bug in `ExecuteEvm::transact_commit` and
`ExecuteEvm::replay`. Fix all of them to finalize the journal on the
error path, matching the existing correct pattern in
`ExecuteEvm::transact` and `transact_many`.

Adds regression tests asserting the journal is empty after a failed
`inspect_tx` / `inspect_tx_commit`.

Closes bluealloy#3779

---------

Author: KyrinCode

crates/handler/src/api.rs

@@ -139,9 +139,17 @@ pub trait ExecuteCommitEvm: ExecuteEvm {
     }
 
     /// Transact the transaction and commit to the state.
+    ///
+    /// # Outcome of Error
+    ///
+    /// If the transaction fails, the journal is finalized (not committed) so it
+    /// does not leak into the next transaction.
     #[inline]
     fn transact_commit(&mut self, tx: Self::Tx) -> Result<Self::ExecutionResult, Self::Error> {
-        let output = self.transact_one(tx)?;
+        let output = self.transact_one(tx).inspect_err(|_| {
+            // finalize (clear) the journal on error; do not commit it.
+            let _ = self.finalize();
+        })?;
         self.commit_inner();
         Ok(output)
     }
@@ -201,10 +209,17 @@ where
 
     #[inline]
     fn replay(&mut self) -> Result<ResultAndState<HaltReason>, Self::Error> {
-        MainnetHandler::default().run(self).map(|result| {
-            let state = self.finalize();
-            ResultAndState::new(result, state)
-        })
+        MainnetHandler::default()
+            .run(self)
+            // finalize (clear) the journal on error; on success the `map`
+            // branch below finalizes it.
+            .inspect_err(|_| {
+                let _ = self.finalize();
+            })
+            .map(|result| {
+                let state = self.finalize();
+                ResultAndState::new(result, state)
+            })
     }
 }
 

crates/inspector/src/inspect.rs

@@ -21,23 +21,36 @@ pub trait InspectEvm: ExecuteEvm {
     fn inspect_one_tx(&mut self, tx: Self::Tx) -> Result<Self::ExecutionResult, Self::Error>;
 
     /// Inspect the EVM and finalize the state.
+    ///
+    /// # Outcome of Error
+    ///
+    /// If the transaction fails, the journal is finalized (cleared) so that the
+    /// next transaction starts from a clean state. This mirrors [`ExecuteEvm::transact`].
     fn inspect_tx(
         &mut self,
         tx: Self::Tx,
     ) -> Result<ExecResultAndState<Self::ExecutionResult, Self::State>, Self::Error> {
-        let output = self.inspect_one_tx(tx)?;
+        // finalize the journal unconditionally, even on error, so the
+        // EIP-2929 warm set and state do not leak into the next transaction.
+        let output_or_error = self.inspect_one_tx(tx);
         let state = self.finalize();
+        let output = output_or_error?;
         Ok(ExecResultAndState::new(output, state))
     }
 
     /// Inspect the EVM with the given inspector and transaction, and finalize the state.
+    ///
+    /// # Outcome of Error
+    ///
+    /// Same as [`InspectEvm::inspect_tx`]: the journal is finalized on error.
     fn inspect(
         &mut self,
         tx: Self::Tx,
         inspector: Self::Inspector,
     ) -> Result<ExecResultAndState<Self::ExecutionResult, Self::State>, Self::Error> {
-        let output = self.inspect_one(tx, inspector)?;
+        let output_or_error = self.inspect_one(tx, inspector);
         let state = self.finalize();
+        let output = output_or_error?;
         Ok(ExecResultAndState::new(output, state))
     }
 
@@ -59,20 +72,34 @@ pub trait InspectEvm: ExecuteEvm {
 pub trait InspectCommitEvm: InspectEvm + ExecuteCommitEvm {
     /// Inspect the EVM with the current inspector and previous transaction by replaying, similar to [`InspectEvm::inspect_tx`]
     /// and commit the state diff to the database.
+    ///
+    /// # Outcome of Error
+    ///
+    /// If the transaction fails, the journal is finalized (not committed) so it
+    /// does not leak into the next transaction.
     fn inspect_tx_commit(&mut self, tx: Self::Tx) -> Result<Self::ExecutionResult, Self::Error> {
-        let output = self.inspect_one_tx(tx)?;
+        let output = self.inspect_one_tx(tx).inspect_err(|_| {
+            // finalize (clear) the journal on error; do not commit it.
+            let _ = self.finalize();
+        })?;
         self.commit_inner();
         Ok(output)
     }
 
     /// Inspect the EVM with the given transaction and inspector similar to [`InspectEvm::inspect`]
     /// and commit the state diff to the database.
+    ///
+    /// # Outcome of Error
+    ///
+    /// Same as [`InspectCommitEvm::inspect_tx_commit`]: the journal is finalized on error.
     fn inspect_commit(
         &mut self,
         tx: Self::Tx,
         inspector: Self::Inspector,
     ) -> Result<Self::ExecutionResult, Self::Error> {
-        let output = self.inspect_one(tx, inspector)?;
+        let output = self.inspect_one(tx, inspector).inspect_err(|_| {
+            let _ = self.finalize();
+        })?;
         self.commit_inner();
         Ok(output)
     }
@@ -109,28 +136,38 @@ pub trait InspectSystemCallEvm: InspectEvm + SystemCallEvm {
     /// Inspect a system call and finalize the state.
     ///
     /// Similar to [`InspectEvm::inspect_tx`] but for system calls.
+    ///
+    /// # Outcome of Error
+    ///
+    /// Same as [`InspectEvm::inspect_tx`]: the journal is finalized on error.
     fn inspect_system_call(
         &mut self,
         system_contract_address: Address,
         data: Bytes,
     ) -> Result<ExecResultAndState<Self::ExecutionResult, Self::State>, Self::Error> {
-        let output = self.inspect_one_system_call(system_contract_address, data)?;
+        let output_or_error = self.inspect_one_system_call(system_contract_address, data);
         let state = self.finalize();
+        let output = output_or_error?;
         Ok(ExecResultAndState::new(output, state))
     }
 
     /// Inspect a system call with a custom caller and finalize the state.
     ///
     /// Similar to [`InspectEvm::inspect_tx`] but for system calls with a custom caller.
+    ///
+    /// # Outcome of Error
+    ///
+    /// Same as [`InspectEvm::inspect_tx`]: the journal is finalized on error.
     fn inspect_system_call_with_caller(
         &mut self,
         caller: Address,
         system_contract_address: Address,
         data: Bytes,
     ) -> Result<ExecResultAndState<Self::ExecutionResult, Self::State>, Self::Error> {
-        let output =
-            self.inspect_one_system_call_with_caller(caller, system_contract_address, data)?;
+        let output_or_error =
+            self.inspect_one_system_call_with_caller(caller, system_contract_address, data);
         let state = self.finalize();
+        let output = output_or_error?;
         Ok(ExecResultAndState::new(output, state))
     }
 
@@ -150,15 +187,20 @@ pub trait InspectSystemCallEvm: InspectEvm + SystemCallEvm {
     /// Inspect a system call with a given inspector and finalize the state.
     ///
     /// Similar to [`InspectEvm::inspect`] but for system calls.
+    ///
+    /// # Outcome of Error
+    ///
+    /// Same as [`InspectEvm::inspect_tx`]: the journal is finalized on error.
     fn inspect_system_call_with_inspector(
         &mut self,
         system_contract_address: Address,
         data: Bytes,
         inspector: Self::Inspector,
     ) -> Result<ExecResultAndState<Self::ExecutionResult, Self::State>, Self::Error> {
-        let output =
-            self.inspect_one_system_call_with_inspector(system_contract_address, data, inspector)?;
+        let output_or_error =
+            self.inspect_one_system_call_with_inspector(system_contract_address, data, inspector);
         let state = self.finalize();
+        let output = output_or_error?;
         Ok(ExecResultAndState::new(output, state))
     }
 
@@ -179,20 +221,25 @@ pub trait InspectSystemCallEvm: InspectEvm + SystemCallEvm {
     /// Inspect a system call with a given inspector and finalize the state.
     ///
     /// Similar to [`InspectEvm::inspect`] but for system calls.
+    ///
+    /// # Outcome of Error
+    ///
+    /// Same as [`InspectEvm::inspect_tx`]: the journal is finalized on error.
     fn inspect_system_call_with_inspector_and_caller(
         &mut self,
         caller: Address,
         system_contract_address: Address,
         data: Bytes,
         inspector: Self::Inspector,
     ) -> Result<ExecResultAndState<Self::ExecutionResult, Self::State>, Self::Error> {
-        let output = self.inspect_one_system_call_with_inspector_and_caller(
+        let output_or_error = self.inspect_one_system_call_with_inspector_and_caller(
             caller,
             system_contract_address,
             data,
             inspector,
-        )?;
+        );
         let state = self.finalize();
+        let output = output_or_error?;
         Ok(ExecResultAndState::new(output, state))
     }
 }

crates/inspector/src/inspector_tests.rs

@@ -1,9 +1,11 @@
 #[cfg(test)]
 mod tests {
-    use crate::{InspectEvm, InspectSystemCallEvm, InspectorEvent, TestInspector};
+    use crate::{
+        InspectCommitEvm, InspectEvm, InspectSystemCallEvm, InspectorEvent, TestInspector,
+    };
     use context::{Context, TxEnv};
     use database::{BenchmarkDB, BENCH_CALLER, BENCH_TARGET};
-    use handler::{MainBuilder, MainContext};
+    use handler::{ExecuteEvm, MainBuilder, MainContext};
     use primitives::{address, Address, Bytes, TxKind, U256};
     use state::{bytecode::opcode, AccountInfo, Bytecode};
 
@@ -772,4 +774,102 @@ mod tests {
             "system_call state_gas_spent must match between inspect and non-inspect paths",
         );
     }
+
+    /// Regression test for https://github.com/bluealloy/revm/issues/3779
+    ///
+    /// `inspect_tx` used to short-circuit on `inspect_one_tx` returning `Err`
+    /// and skip `finalize()`, leaving the journal (EIP-2929 warm set, touched
+    /// accounts) in a dirty state that leaked into the next transaction. The
+    /// handler's error path calls `discard_tx`, which reverts account *values*
+    /// but keeps the account *entries* in the state map, so a subsequent
+    /// `finalize()` drains those leftovers — making the leak observable.
+    ///
+    /// Here a nonce mismatch triggers an `InvalidTransaction` *after* the
+    /// caller and coinbase have been loaded and warmed in `load_accounts`.
+    /// After `inspect_tx` returns `Err`, the journal must already be cleared,
+    /// i.e. the drained state must be empty.
+    #[test]
+    fn test_inspect_tx_finalizes_journal_on_error() {
+        use database::{CacheDB, EmptyDB};
+
+        // Caller account exists in the DB with nonce = 1.
+        let mut db = CacheDB::<EmptyDB>::default();
+        db.insert_account_info(
+            BENCH_CALLER,
+            AccountInfo {
+                balance: U256::from(1_000_000_000_000_000_000u64),
+                nonce: 1,
+                ..Default::default()
+            },
+        );
+        let ctx = Context::mainnet().with_db(db);
+        let mut evm = ctx.build_mainnet_with_inspector(TestInspector::new());
+
+        // Send a tx with nonce = 0 -> InvalidTransaction::NonceTooLow, raised
+        // after load_accounts warmed the caller/coinbase.
+        let result = evm.inspect_tx(
+            TxEnv::builder()
+                .caller(BENCH_CALLER)
+                .kind(TxKind::Call(BENCH_TARGET))
+                .gas_limit(100_000)
+                .nonce(0)
+                .build()
+                .unwrap(),
+        );
+        assert!(
+            result.is_err(),
+            "tx with stale nonce must error so the error path is exercised"
+        );
+
+        // `inspect_tx` must have finalized the journal on error, so draining
+        // it now yields an empty state. Before the fix this contained the
+        // warmed caller/coinbase accounts and was non-empty.
+        let leftover = evm.finalize();
+        assert!(
+            leftover.is_empty(),
+            "journal must be cleared after a failed inspect_tx, but found {} leftover accounts",
+            leftover.len(),
+        );
+    }
+
+    /// Companion to [`Self::test_inspect_tx_finalizes_journal_on_error`] for the
+    /// `inspect_tx_commit` path: on error the journal must be finalized (not
+    /// committed), so a subsequent drain yields an empty state.
+    #[test]
+    fn test_inspect_tx_commit_finalizes_journal_on_error() {
+        use database::{CacheDB, EmptyDB};
+
+        let mut db = CacheDB::<EmptyDB>::default();
+        db.insert_account_info(
+            BENCH_CALLER,
+            AccountInfo {
+                balance: U256::from(1_000_000_000_000_000_000u64),
+                nonce: 1,
+                ..Default::default()
+            },
+        );
+        let ctx = Context::mainnet().with_db(db);
+        let mut evm = ctx.build_mainnet_with_inspector(TestInspector::new());
+
+        let result = evm.inspect_tx_commit(
+            TxEnv::builder()
+                .caller(BENCH_CALLER)
+                .kind(TxKind::Call(BENCH_TARGET))
+                .gas_limit(100_000)
+                .nonce(0)
+                .build()
+                .unwrap(),
+        );
+        assert!(
+            result.is_err(),
+            "tx with stale nonce must error so the error path is exercised"
+        );
+
+        let leftover = evm.finalize();
+        assert!(
+            leftover.is_empty(),
+            "journal must be cleared after a failed inspect_tx_commit, but found {} leftover accounts",
+            leftover.len(),
+        );
+    }
 }