fix(op): Handler deposit tx halt, catch_error handle

crates/database/src/in_memory_db.rs

@@ -430,26 +430,28 @@ impl BenchmarkDB {
 /// BYTECODE address
 pub const FFADDRESS: Address = address!("ffffffffffffffffffffffffffffffffffffffff");
 pub const BENCH_TARGET: Address = FFADDRESS;
+pub const BENCH_TARGET_BALANCE: U256 = U256::from_limbs([10000000, 0, 0, 0]);
 /// CALLER address
 pub const EEADDRESS: Address = address!("eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee");
 pub const BENCH_CALLER: Address = EEADDRESS;
+pub const BENCH_CALLER_BALANCE: U256 = U256::from_limbs([10000000, 0, 0, 0]);
 
 impl Database for BenchmarkDB {
     type Error = Infallible;
     /// Get basic account information.
     fn basic(&mut self, address: Address) -> Result<Option<AccountInfo>, Self::Error> {
-        if address == FFADDRESS {
+        if address == BENCH_TARGET {
             return Ok(Some(AccountInfo {
                 nonce: 1,
-                balance: U256::from(10000000),
+                balance: BENCH_TARGET_BALANCE,
                 code: Some(self.0.clone()),
                 code_hash: self.1,
             }));
         }
-        if address == EEADDRESS {
+        if address == BENCH_CALLER {
             return Ok(Some(AccountInfo {
                 nonce: 0,
-                balance: U256::from(10000000),
+                balance: BENCH_CALLER_BALANCE,
                 code: None,
                 code_hash: KECCAK_EMPTY,
             }));

crates/handler/src/handler.rs

@@ -115,12 +115,25 @@ pub trait Handler {
     fn run(
         &mut self,
         evm: &mut Self::Evm,
+    ) -> Result<ResultAndState<Self::HaltReason>, Self::Error> {
+        // run inner handler and catch all errors to handle cleanup.
+        match self.run_without_catch_error(evm) {
+            Ok(output) => Ok(output),
+            Err(e) => self.catch_error(evm, e),
+        }
+    }
+
+    #[inline]
+    fn run_without_catch_error(
+        &mut self,
+        evm: &mut Self::Evm,
     ) -> Result<ResultAndState<Self::HaltReason>, Self::Error> {
         let init_and_floor_gas = self.validate(evm)?;
         let eip7702_refund = self.pre_execution(evm)? as i64;
         let exec_result = self.execution(evm, &init_and_floor_gas)?;
         self.post_execution(evm, exec_result, init_and_floor_gas, eip7702_refund)
     }
+
     /// Call all validation functions
     #[inline]
     fn validate(&self, evm: &mut Self::Evm) -> Result<InitialAndFloorGas, Self::Error> {
@@ -174,11 +187,7 @@ pub trait Handler {
         // Reward beneficiary
         self.reward_beneficiary(evm, &mut exec_result)?;
         // Prepare output of transaction.
-        let output = self.output(evm, exec_result)?;
-        // Clear any internal state.
-        self.clear(evm);
-        // Return output
-        Ok(output)
+        self.output(evm, exec_result)
     }
 
     /* VALIDATION */
@@ -371,28 +380,24 @@ pub trait Handler {
     ) -> Result<ResultAndState<Self::HaltReason>, Self::Error> {
         let ctx = evm.ctx();
         mem::replace(ctx.error(), Ok(()))?;
-        Ok(post_execution::output(ctx, result))
+        let output = post_execution::output(ctx, result);
+
+        // clear journal
+        evm.ctx().journal().clear();
+        Ok(output)
     }
 
-    /// Called when execution ends.
+    /// Called every time at the end of execution. Used for clearing the journal.
     ///
     /// End handle in comparison to output handle will be called every time after execution.
-    ///
-    /// While output will be omitted in case of the error.
     #[inline]
-    fn end(
+    fn catch_error(
         &self,
-        _evm: &mut Self::Evm,
-        end_output: Result<ResultAndState<Self::HaltReason>, Self::Error>,
+        evm: &mut Self::Evm,
+        error: Self::Error,
     ) -> Result<ResultAndState<Self::HaltReason>, Self::Error> {
-        end_output
-    }
-
-    /// Clean handler. It resets internal Journal state to default one.
-    ///
-    /// This handle is called every time regardless of the result of the transaction.
-    #[inline]
-    fn clear(&self, evm: &mut Self::Evm) {
+        // do the cleanup of journal if error is caught
         evm.ctx().journal().clear();
+        Err(error)
     }
 }

crates/inspector/src/inspector.rs

@@ -190,6 +190,16 @@ where
     fn inspect_run(
         &mut self,
         evm: &mut Self::Evm,
+    ) -> Result<ResultAndState<Self::HaltReason>, Self::Error> {
+        match self.inspect_run_without_catch_error(evm) {
+            Ok(output) => Ok(output),
+            Err(e) => self.catch_error(evm, e),
+        }
+    }
+
+    fn inspect_run_without_catch_error(
+        &mut self,
+        evm: &mut Self::Evm,
     ) -> Result<ResultAndState<Self::HaltReason>, Self::Error> {
         let init_and_floor_gas = self.validate(evm)?;
         let eip7702_refund = self.pre_execution(evm)? as i64;

crates/optimism/src/evm.rs

@@ -75,3 +75,78 @@ where
         (&mut self.0.data.ctx, &mut self.0.precompiles)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::{
+        transaction::deposit::DEPOSIT_TRANSACTION_TYPE, DefaultOp, OpBuilder, OpHaltReason,
+        OpSpecId,
+    };
+    use database::{BenchmarkDB, BENCH_CALLER, BENCH_CALLER_BALANCE, BENCH_TARGET};
+    use precompile::Address;
+    use revm::{
+        bytecode::opcode,
+        context::result::ExecutionResult,
+        primitives::{TxKind, U256},
+        state::Bytecode,
+        Context, ExecuteEvm,
+    };
+
+    #[test]
+    fn test_deposit_tx() {
+        let ctx = Context::op()
+            .modify_tx_chained(|tx| {
+                tx.enveloped_tx = None;
+                tx.deposit.mint = Some(100);
+                tx.base.tx_type = DEPOSIT_TRANSACTION_TYPE;
+            })
+            .modify_cfg_chained(|cfg| cfg.spec = OpSpecId::HOLOCENE);
+
+        let mut evm = ctx.build_op();
+
+        let output = evm.transact_previous().unwrap();
+
+        // balance should be 100
+        assert_eq!(
+            output
+                .state
+                .get(&Address::default())
+                .map(|a| a.info.balance),
+            Some(U256::from(100))
+        );
+    }
+
+    #[test]
+    fn test_halted_deposit_tx() {
+        let ctx = Context::op()
+            .modify_tx_chained(|tx| {
+                tx.enveloped_tx = None;
+                tx.deposit.mint = Some(100);
+                tx.base.tx_type = DEPOSIT_TRANSACTION_TYPE;
+                tx.base.caller = BENCH_CALLER;
+                tx.base.kind = TxKind::Call(BENCH_TARGET);
+            })
+            .modify_cfg_chained(|cfg| cfg.spec = OpSpecId::HOLOCENE)
+            .with_db(BenchmarkDB::new_bytecode(Bytecode::new_legacy(
+                [opcode::POP].into(),
+            )));
+
+        // POP would return a halt.
+        let mut evm = ctx.build_op();
+
+        let output = evm.transact_previous().unwrap();
+
+        // balance should be 100 + previous balance
+        assert_eq!(
+            output.result,
+            ExecutionResult::Halt {
+                reason: OpHaltReason::FailedDeposit,
+                gas_used: 30_000_000
+            }
+        );
+        assert_eq!(
+            output.state.get(&BENCH_CALLER).map(|a| a.info.balance),
+            Some(U256::from(100) + BENCH_CALLER_BALANCE)
+        );
+    }
+}

crates/optimism/src/handler.rs

@@ -387,79 +387,76 @@ where
                 return Err(ERROR::from(OpTransactionError::HaltedDepositPostRegolith));
             }
         }
+        evm.ctx().chain().clear_tx_l1_cost();
         Ok(result)
     }
 
-    fn end(
+    fn catch_error(
         &self,
         evm: &mut Self::Evm,
-        end_output: Result<ResultAndState<Self::HaltReason>, Self::Error>,
+        error: Self::Error,
     ) -> Result<ResultAndState<Self::HaltReason>, Self::Error> {
-        //end_output
-
         let is_deposit = evm.ctx().tx().tx_type() == DEPOSIT_TRANSACTION_TYPE;
-        end_output.or_else(|err| {
-            if err.is_tx_error() && is_deposit {
-                let ctx = evm.ctx();
-                let spec = ctx.cfg().spec();
-                let tx = ctx.tx();
-                let caller = tx.caller();
-                let mint = tx.mint();
-                let is_system_tx = tx.is_system_transaction();
-                let gas_limit = tx.gas_limit();
-                // If the transaction is a deposit transaction and it failed
-                // for any reason, the caller nonce must be bumped, and the
-                // gas reported must be altered depending on the Hardfork. This is
-                // also returned as a special Halt variant so that consumers can more
-                // easily distinguish between a failed deposit and a failed
-                // normal transaction.
-
-                // Increment sender nonce and account balance for the mint amount. Deposits
-                // always persist the mint amount, even if the transaction fails.
-                let account = {
-                    let mut acc = Account::from(
-                        evm.ctx()
-                            .db()
-                            .basic(caller)
-                            .unwrap_or_default()
-                            .unwrap_or_default(),
-                    );
-                    acc.info.nonce = acc.info.nonce.saturating_add(1);
-                    acc.info.balance = acc
-                        .info
-                        .balance
-                        .saturating_add(U256::from(mint.unwrap_or_default()));
-                    acc.mark_touch();
-                    acc
-                };
-                let state = HashMap::from_iter([(caller, account)]);
-
-                // The gas used of a failed deposit post-regolith is the gas
-                // limit of the transaction. pre-regolith, it is the gas limit
-                // of the transaction for non system transactions and 0 for system
-                // transactions.
-                let gas_used = if spec.is_enabled_in(OpSpecId::REGOLITH) || !is_system_tx {
-                    gas_limit
-                } else {
-                    0
-                };
-
-                Ok(ResultAndState {
-                    result: ExecutionResult::Halt {
-                        reason: OpHaltReason::FailedDeposit,
-                        gas_used,
-                    },
-                    state,
-                })
+        let output = if error.is_tx_error() && is_deposit {
+            let ctx = evm.ctx();
+            let spec = ctx.cfg().spec();
+            let tx = ctx.tx();
+            let caller = tx.caller();
+            let mint = tx.mint();
+            let is_system_tx = tx.is_system_transaction();
+            let gas_limit = tx.gas_limit();
+            // If the transaction is a deposit transaction and it failed
+            // for any reason, the caller nonce must be bumped, and the
+            // gas reported must be altered depending on the Hardfork. This is
+            // also returned as a special Halt variant so that consumers can more
+            // easily distinguish between a failed deposit and a failed
+            // normal transaction.
+
+            // Increment sender nonce and account balance for the mint amount. Deposits
+            // always persist the mint amount, even if the transaction fails.
+            let account = {
+                let mut acc = Account::from(
+                    evm.ctx()
+                        .db()
+                        .basic(caller)
+                        .unwrap_or_default()
+                        .unwrap_or_default(),
+                );
+                acc.info.nonce = acc.info.nonce.saturating_add(1);
+                acc.info.balance = acc
+                    .info
+                    .balance
+                    .saturating_add(U256::from(mint.unwrap_or_default()));
+                acc.mark_touch();
+                acc
+            };
+            let state = HashMap::from_iter([(caller, account)]);
+
+            // The gas used of a failed deposit post-regolith is the gas
+            // limit of the transaction. pre-regolith, it is the gas limit
+            // of the transaction for non system transactions and 0 for system
+            // transactions.
+            let gas_used = if spec.is_enabled_in(OpSpecId::REGOLITH) || !is_system_tx {
+                gas_limit
             } else {
-                Err(err)
-            }
-        })
-    }
-
-    fn clear(&self, evm: &mut Self::Evm) {
+                0
+            };
+            // clear the journal
+            Ok(ResultAndState {
+                result: ExecutionResult::Halt {
+                    reason: OpHaltReason::FailedDeposit,
+                    gas_used,
+                },
+                state,
+            })
+        } else {
+            Err(error)
+        };
+        // do cleanup
         evm.ctx().chain().clear_tx_l1_cost();
-        self.mainnet.clear(evm);
+        evm.ctx().journal().clear();
+
+        output
     }
 }