fix: selfdestructs once and for all

src/tracing/mod.rs

@@ -209,6 +209,13 @@ impl TracingInspector {
         self.trace_stack.last().copied().expect("can't start step without starting a trace first")
     }
 
+    /// Returns a mutable reference to the last trace [CallTrace] from the stack.
+    #[track_caller]
+    fn last_trace(&mut self) -> &mut CallTraceNode {
+        let idx = self.last_trace_idx();
+        &mut self.traces.arena[idx]
+    }
+
     /// _Removes_ the last trace [CallTrace] index from the stack.
     ///
     /// # Panics
@@ -450,11 +457,8 @@ where
 
     fn log(&mut self, context: &mut EvmContext<DB>, log: &Log) {
         self.gas_inspector.log(context, log);
-
-        let trace_idx = self.last_trace_idx();
-        let trace = &mut self.traces.arena[trace_idx];
-
         if self.config.record_logs {
+            let trace = self.last_trace();
             trace.ordering.push(LogCallOrder::Log(trace.logs.len()));
             trace.logs.push(log.data.clone());
         }
@@ -553,16 +557,15 @@ where
         outcome: CreateOutcome,
     ) -> CreateOutcome {
         let outcome = self.gas_inspector.create_end(context, inputs, outcome);
-
         self.fill_trace_on_call_end(context, outcome.result.clone(), outcome.address);
-
         outcome
     }
 
-    fn selfdestruct(&mut self, _contract: Address, target: Address, _value: U256) {
-        let trace_idx = self.last_trace_idx();
-        let trace = &mut self.traces.arena[trace_idx].trace;
-        trace.selfdestruct_refund_target = Some(target)
+    fn selfdestruct(&mut self, contract: Address, target: Address, value: U256) {
+        let node = self.last_trace();
+        node.trace.address = contract;
+        node.trace.selfdestruct_refund_target = Some(target);
+        node.trace.value = value;
     }
 }
 

src/tracing/types.rs

@@ -32,12 +32,12 @@ pub struct CallTrace {
     ///
     /// Note: This is an Option because not all tracers make use of this
     pub maybe_precompile: Option<bool>,
-    /// Holds the target for the __selfdestruct__ refund target
+    /// Holds the target for the selfdestruct refund target.
     ///
-    /// This is only set if a selfdestruct was executed.
+    /// This is only `Some` if a selfdestruct was executed and the call is executed before the
+    /// Cancun hardfork.
     ///
-    /// Note: This not necessarily guarantees that the status is [InstructionResult::SelfDestruct]
-    /// There's an edge case where a new created contract is immediately selfdestructed.
+    /// See [`is_selfdestruct`](Self::is_selfdestruct) for more information.
     pub selfdestruct_refund_target: Option<Address>,
     /// The kind of call this is
     pub kind: CallKind,
@@ -73,6 +73,22 @@ impl CallTrace {
         self.status == InstructionResult::Revert
     }
 
+    /// Returns `true` if this trace was a selfdestruct.
+    ///
+    /// See also `TracingInspector::selfdestruct`.
+    ///
+    /// We can't rely entirely on [`Self::status`] being [`InstructionResult::SelfDestruct`]
+    /// because there's an edge case where a new created contract (CREATE) is immediately
+    /// selfdestructed.
+    ///
+    /// We also can't rely entirely on `selfdestruct_refund_target` being `Some` as the
+    /// `selfdestruct` inspector function will not be called after the Cancun hardfork.
+    #[inline]
+    pub const fn is_selfdestruct(&self) -> bool {
+        matches!(self.status, InstructionResult::SelfDestruct)
+            || self.selfdestruct_refund_target.is_some()
+    }
+
     /// Returns the error message if it is an erroneous result.
     pub(crate) fn as_error_msg(&self, kind: TraceStyle) -> Option<String> {
         // See also <https://github.com/ethereum/go-ethereum/blob/34d507215951fb3f4a5983b65e127577989a6db8/eth/tracers/native/call_flat.go#L39-L55>
@@ -179,17 +195,12 @@ impl CallTraceNode {
         self.trace.status
     }
 
-    /// Returns true if the call was a selfdestruct
-    ///
-    /// A selfdestruct is marked by the refund target being set.
-    ///
-    /// See also `TracingInspector::selfdestruct`
+    /// Returns `true` if this trace was a selfdestruct.
     ///
-    /// Note: We can't rely in the [Self::status] being [InstructionResult::SelfDestruct] because
-    /// there's an edge case where a new created contract (CREATE) is immediately selfdestructed.
+    /// See [`CallTrace::is_selfdestruct`] for more details.
     #[inline]
     pub const fn is_selfdestruct(&self) -> bool {
-        self.trace.selfdestruct_refund_target.is_some()
+        self.trace.is_selfdestruct()
     }
 
     /// Converts this node into a parity `TransactionTrace`

tests/it/parity.rs

@@ -1,38 +1,50 @@
 //! Parity tests
 
 use crate::utils::{inspect, print_traces};
-use alloy_primitives::{hex, Address};
+use alloy_primitives::{address, hex, Address, U256};
 use alloy_rpc_types::TransactionInfo;
+use alloy_rpc_types_trace::parity::{Action, SelfdestructAction};
 use revm::{
     db::{CacheDB, EmptyDB},
     interpreter::CreateScheme,
     primitives::{
-        BlockEnv, CfgEnv, CfgEnvWithHandlerCfg, EnvWithHandlerCfg, ExecutionResult, HandlerCfg,
-        Output, SpecId, TransactTo, TxEnv,
+        AccountInfo, BlockEnv, CfgEnv, CfgEnvWithHandlerCfg, EnvWithHandlerCfg, ExecutionResult,
+        HandlerCfg, Output, SpecId, TransactTo, TxEnv,
     },
     DatabaseCommit,
 };
 use revm_inspectors::tracing::{TracingInspector, TracingInspectorConfig};
 
 #[test]
-fn test_parity_selfdestruct() {
+fn test_parity_selfdestruct_london() {
+    test_parity_selfdestruct(SpecId::LONDON);
+}
+
+#[test]
+fn test_parity_selfdestruct_cancun() {
+    test_parity_selfdestruct(SpecId::CANCUN);
+}
+
+fn test_parity_selfdestruct(spec_id: SpecId) {
     /*
     contract DummySelfDestruct {
+        constructor() payable {}
         function close() public {
             selfdestruct(payable(msg.sender));
         }
     }
     */
 
     // simple contract that selfdestructs when a function is called
-    let code = hex!("6080604052348015600f57600080fd5b50606a80601d6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c806343d726d614602d575b600080fd5b603233ff5b00fea2646970667358221220e52c8372ad24b20a6f7e4a13772dbc1d00f7eb5d0934ed6d635d3f5bf47dbc9364736f6c634300080d0033");
+    let code = hex!("608080604052606b908160108239f3fe6004361015600c57600080fd5b6000803560e01c6343d726d614602157600080fd5b346032578060031936011260325733ff5b80fdfea2646970667358221220f393fc6be90126d52315ccd38ae6608ac4fd5bef4c59e119e280b2a2b149d0dc64736f6c63430008190033");
 
-    let deployer = Address::ZERO;
+    let deployer = address!("341348115259a8bf69f1f50101c227fced83bac6");
+    let value = U256::from(69);
 
     let mut db = CacheDB::new(EmptyDB::default());
+    db.insert_account_info(deployer, AccountInfo { balance: value, ..Default::default() });
 
-    let cfg = CfgEnvWithHandlerCfg::new(CfgEnv::default(), HandlerCfg::new(SpecId::LONDON));
-
+    let cfg = CfgEnvWithHandlerCfg::new(CfgEnv::default(), HandlerCfg::new(spec_id));
     let env = EnvWithHandlerCfg::new_with_cfg_env(
         cfg.clone(),
         BlockEnv::default(),
@@ -41,14 +53,15 @@ fn test_parity_selfdestruct() {
             gas_limit: 1000000,
             transact_to: TransactTo::Create(CreateScheme::Create),
             data: code.into(),
+            value,
             ..Default::default()
         },
     );
 
     let mut insp = TracingInspector::new(TracingInspectorConfig::default_parity());
 
     let (res, _) = inspect(&mut db, env, &mut insp).unwrap();
-    let addr = match res.result {
+    let contract_address = match res.result {
         ExecutionResult::Success { output, .. } => match output {
             Output::Create(_, addr) => addr.unwrap(),
             _ => panic!("Create failed"),
@@ -65,22 +78,42 @@ fn test_parity_selfdestruct() {
         TxEnv {
             caller: deployer,
             gas_limit: 1000000,
-            transact_to: TransactTo::Call(addr),
+            transact_to: TransactTo::Call(contract_address),
             data: hex!("43d726d6").into(),
             ..Default::default()
         },
     );
 
     let (res, _) = inspect(&mut db, env, &mut insp).unwrap();
-    assert!(res.result.is_success());
+    assert!(res.result.is_success(), "{res:#?}");
+
+    // TODO: Transfer still happens in Cancun, but this is not reflected in the trace.
+    let (expected_value, expected_target) =
+        if spec_id < SpecId::CANCUN { (value, Some(deployer)) } else { (U256::ZERO, None) };
+
+    {
+        assert_eq!(insp.get_traces().nodes().len(), 1);
+        let node = &insp.get_traces().nodes()[0];
+        assert!(node.is_selfdestruct(), "{node:#?}");
+        assert_eq!(node.trace.address, contract_address);
+        assert_eq!(node.trace.selfdestruct_refund_target, expected_target);
+        assert_eq!(node.trace.value, expected_value);
+    }
 
     let traces = insp
         .with_transaction_gas_used(res.result.gas_used())
         .into_parity_builder()
         .into_localized_transaction_traces(TransactionInfo::default());
 
     assert_eq!(traces.len(), 2);
-    assert!(traces[1].trace.action.is_selfdestruct())
+    assert_eq!(
+        traces[1].trace.action,
+        Action::Selfdestruct(SelfdestructAction {
+            address: contract_address,
+            refund_address: expected_target.unwrap_or_default(),
+            balance: expected_value,
+        })
+    );
 }
 
 // Minimal example of <https://etherscan.io/tx/0xd81725127173cf1095a722cbaec118052e2626ddb914d61967fb4bf117969be0>