From 4679bce41ac83534482186080d27a3db5633dc71 Mon Sep 17 00:00:00 2001 From: gbrew <127057440+gbrew@users.noreply.github.com> Date: Fri, 24 Nov 2023 22:40:59 -0700 Subject: [PATCH] fix(tracing): fix the parity vmTrace stack pushes #5561 (#5563) Co-authored-by: fake Co-authored-by: Matthias Seitz --- .../src/tracing/builder/parity.rs | 153 +++++++++--------- .../revm-inspectors/src/tracing/config.rs | 85 ++++++---- .../revm/revm-inspectors/src/tracing/mod.rs | 17 +- 3 files changed, 141 insertions(+), 114 deletions(-) diff --git a/crates/revm/revm-inspectors/src/tracing/builder/parity.rs b/crates/revm/revm-inspectors/src/tracing/builder/parity.rs index 943a85ee2b..d091a019b2 100644 --- a/crates/revm/revm-inspectors/src/tracing/builder/parity.rs +++ b/crates/revm/revm-inspectors/src/tracing/builder/parity.rs @@ -8,7 +8,10 @@ use alloy_primitives::{Address, U64}; use reth_rpc_types::{trace::parity::*, TransactionInfo}; use revm::{ db::DatabaseRef, - interpreter::opcode::{self, spec_opcode_gas}, + interpreter::{ + opcode::{self, spec_opcode_gas}, + OpCode, + }, primitives::{Account, ExecutionResult, ResultAndState, SpecId, KECCAK_EMPTY}, }; use std::collections::{HashSet, VecDeque}; @@ -385,87 +388,9 @@ impl ParityTraceBuilder { }) }; - // Calculate the stack items at this step - let push_stack = { - let step_op = step.op.get(); - let show_stack: usize; - if (opcode::PUSH0..=opcode::PUSH32).contains(&step_op) { - show_stack = 1; - } else if (opcode::SWAP1..=opcode::SWAP16).contains(&step_op) { - show_stack = (step_op - opcode::SWAP1) as usize + 2; - } else if (opcode::DUP1..=opcode::DUP16).contains(&step_op) { - show_stack = (step_op - opcode::DUP1) as usize + 2; - } else { - show_stack = match step_op { - opcode::CALLDATALOAD | - opcode::SLOAD | - opcode::MLOAD | - opcode::CALLDATASIZE | - opcode::LT | - opcode::GT | - opcode::DIV | - opcode::SDIV | - opcode::SAR | - opcode::AND | - opcode::EQ | - opcode::CALLVALUE | - opcode::ISZERO | - opcode::ADD | - opcode::EXP | - opcode::CALLER | - opcode::KECCAK256 | - opcode::SUB | - opcode::ADDRESS | - opcode::GAS | - opcode::MUL | - opcode::RETURNDATASIZE | - opcode::NOT | - opcode::SHR | - opcode::SHL | - opcode::EXTCODESIZE | - opcode::SLT | - opcode::OR | - opcode::NUMBER | - opcode::PC | - opcode::TIMESTAMP | - opcode::BALANCE | - opcode::SELFBALANCE | - opcode::MULMOD | - opcode::ADDMOD | - opcode::BASEFEE | - opcode::BLOCKHASH | - opcode::BYTE | - opcode::XOR | - opcode::ORIGIN | - opcode::CODESIZE | - opcode::MOD | - opcode::SIGNEXTEND | - opcode::GASLIMIT | - opcode::DIFFICULTY | - opcode::SGT | - opcode::GASPRICE | - opcode::MSIZE | - opcode::EXTCODEHASH | - opcode::SMOD | - opcode::CHAINID | - opcode::COINBASE => 1, - _ => 0, - } - }; - let mut push_stack = step.push_stack.clone().unwrap_or_default(); - if let Some(stack) = step.stack.as_ref() { - for idx in (0..show_stack).rev() { - if stack.len() > idx { - push_stack.push(stack[stack.len() - idx - 1]) - } - } - } - push_stack - }; - let maybe_execution = Some(VmExecutedOperation { used: step.gas_remaining, - push: push_stack, + push: step.push_stack.clone().unwrap_or_default(), mem: maybe_memory, store: maybe_storage, }); @@ -637,3 +562,71 @@ where Ok(()) } + +/// Returns the number of items pushed on the stack by a given opcode. +/// This used to determine how many stack etries to put in the `push` element +/// in a parity vmTrace. +/// The value is obvious for most opcodes, but SWAP* and DUP* are a bit weird, +/// and we handle those as they are handled in parity vmtraces. +/// For reference: +pub(crate) fn stack_push_count(step_op: OpCode) -> usize { + let step_op = step_op.get(); + match step_op { + opcode::PUSH0..=opcode::PUSH32 => 1, + opcode::SWAP1..=opcode::SWAP16 => (step_op - opcode::SWAP1) as usize + 2, + opcode::DUP1..=opcode::DUP16 => (step_op - opcode::DUP1) as usize + 2, + opcode::CALLDATALOAD | + opcode::SLOAD | + opcode::MLOAD | + opcode::CALLDATASIZE | + opcode::LT | + opcode::GT | + opcode::DIV | + opcode::SDIV | + opcode::SAR | + opcode::AND | + opcode::EQ | + opcode::CALLVALUE | + opcode::ISZERO | + opcode::ADD | + opcode::EXP | + opcode::CALLER | + opcode::KECCAK256 | + opcode::SUB | + opcode::ADDRESS | + opcode::GAS | + opcode::MUL | + opcode::RETURNDATASIZE | + opcode::NOT | + opcode::SHR | + opcode::SHL | + opcode::EXTCODESIZE | + opcode::SLT | + opcode::OR | + opcode::NUMBER | + opcode::PC | + opcode::TIMESTAMP | + opcode::BALANCE | + opcode::SELFBALANCE | + opcode::MULMOD | + opcode::ADDMOD | + opcode::BASEFEE | + opcode::BLOCKHASH | + opcode::BYTE | + opcode::XOR | + opcode::ORIGIN | + opcode::CODESIZE | + opcode::MOD | + opcode::SIGNEXTEND | + opcode::GASLIMIT | + opcode::DIFFICULTY | + opcode::SGT | + opcode::GASPRICE | + opcode::MSIZE | + opcode::EXTCODEHASH | + opcode::SMOD | + opcode::CHAINID | + opcode::COINBASE => 1, + _ => 0, + } +} diff --git a/crates/revm/revm-inspectors/src/tracing/config.rs b/crates/revm/revm-inspectors/src/tracing/config.rs index 689fa16de9..6bc85f76c9 100644 --- a/crates/revm/revm-inspectors/src/tracing/config.rs +++ b/crates/revm/revm-inspectors/src/tracing/config.rs @@ -1,25 +1,6 @@ use reth_rpc_types::trace::{geth::GethDefaultTracingOptions, parity::TraceType}; use std::collections::HashSet; -/// What kind of tracing style this is. -/// -/// This affects things like error messages. -#[derive(Debug, Clone, Copy, Eq, PartialEq)] -pub(crate) enum TraceStyle { - /// Parity style tracer - Parity, - /// Geth style tracer - #[allow(unused)] - Geth, -} - -impl TraceStyle { - /// Returns true if this is a parity style tracer. - pub(crate) const fn is_parity(self) -> bool { - matches!(self, Self::Parity) - } -} - /// Gives guidance to the [TracingInspector](crate::tracing::TracingInspector). /// /// Use [TracingInspectorConfig::default_parity] or [TracingInspectorConfig::default_geth] to get @@ -31,7 +12,7 @@ pub struct TracingInspectorConfig { /// Whether to record individual memory snapshots. pub record_memory_snapshots: bool, /// Whether to record individual stack snapshots. - pub record_stack_snapshots: bool, + pub record_stack_snapshots: StackSnapshotType, /// Whether to record state diffs. pub record_state_diff: bool, /// Whether to ignore precompile calls. @@ -48,7 +29,7 @@ impl TracingInspectorConfig { Self { record_steps: true, record_memory_snapshots: true, - record_stack_snapshots: true, + record_stack_snapshots: StackSnapshotType::Full, record_state_diff: false, exclude_precompile_calls: false, record_call_return_data: false, @@ -63,7 +44,7 @@ impl TracingInspectorConfig { Self { record_steps: false, record_memory_snapshots: false, - record_stack_snapshots: false, + record_stack_snapshots: StackSnapshotType::None, record_state_diff: false, exclude_precompile_calls: true, record_call_return_data: false, @@ -78,7 +59,7 @@ impl TracingInspectorConfig { Self { record_steps: true, record_memory_snapshots: true, - record_stack_snapshots: true, + record_stack_snapshots: StackSnapshotType::Full, record_state_diff: true, exclude_precompile_calls: false, record_call_return_data: false, @@ -93,9 +74,11 @@ impl TracingInspectorConfig { #[inline] pub fn from_parity_config(trace_types: &HashSet) -> Self { let needs_vm_trace = trace_types.contains(&TraceType::VmTrace); + let snap_type = + if needs_vm_trace { StackSnapshotType::Pushes } else { StackSnapshotType::None }; TracingInspectorConfig::default_parity() .set_steps(needs_vm_trace) - .set_stack_snapshots(needs_vm_trace) + .set_stack_snapshots(snap_type) .set_memory_snapshots(needs_vm_trace) } @@ -104,7 +87,11 @@ impl TracingInspectorConfig { pub fn from_geth_config(config: &GethDefaultTracingOptions) -> Self { Self { record_memory_snapshots: config.enable_memory.unwrap_or_default(), - record_stack_snapshots: !config.disable_stack.unwrap_or_default(), + record_stack_snapshots: if config.disable_stack.unwrap_or_default() { + StackSnapshotType::None + } else { + StackSnapshotType::Full + }, record_state_diff: !config.disable_storage.unwrap_or_default(), ..Self::default_geth() } @@ -130,8 +117,8 @@ impl TracingInspectorConfig { self } - /// Configure whether the tracer should record stack snapshots - pub fn set_stack_snapshots(mut self, record_stack_snapshots: bool) -> Self { + /// Configure how the tracer should record stack snapshots + pub fn set_stack_snapshots(mut self, record_stack_snapshots: StackSnapshotType) -> Self { self.record_stack_snapshots = record_stack_snapshots; self } @@ -164,6 +151,50 @@ impl TracingInspectorConfig { } } +/// How much of the stack to record. Nothing, just the items pushed, or the full stack +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub enum StackSnapshotType { + /// Don't record stack snapshots + None, + /// Record only the items pushed to the stack + Pushes, + /// Record the full stack + Full, +} + +impl StackSnapshotType { + /// Returns true if this is the [StackSnapshotType::Full] variant + #[inline] + pub fn is_full(self) -> bool { + matches!(self, Self::Full) + } + + /// Returns true if this is the [StackSnapshotType::Pushes] variant + #[inline] + pub fn is_pushes(self) -> bool { + matches!(self, Self::Pushes) + } +} + +/// What kind of tracing style this is. +/// +/// This affects things like error messages. +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub(crate) enum TraceStyle { + /// Parity style tracer + Parity, + /// Geth style tracer + #[allow(unused)] + Geth, +} + +impl TraceStyle { + /// Returns true if this is a parity style tracer. + pub(crate) const fn is_parity(self) -> bool { + matches!(self, Self::Parity) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/revm/revm-inspectors/src/tracing/mod.rs b/crates/revm/revm-inspectors/src/tracing/mod.rs index 31d7c29c2e..0f1ba19147 100644 --- a/crates/revm/revm-inspectors/src/tracing/mod.rs +++ b/crates/revm/revm-inspectors/src/tracing/mod.rs @@ -22,6 +22,7 @@ mod fourbyte; mod opcount; pub mod types; mod utils; +use self::parity::stack_push_count; use crate::tracing::{ arena::PushTraceKind, types::{CallTraceNode, RecordedMemory, StorageChange, StorageChangeReason}, @@ -282,7 +283,11 @@ impl TracingInspector { .record_memory_snapshots .then(|| RecordedMemory::new(interp.shared_memory.context_memory().to_vec())) .unwrap_or_default(); - let stack = self.config.record_stack_snapshots.then(|| interp.stack.data().clone()); + let stack = if self.config.record_stack_snapshots.is_full() { + Some(interp.stack.data().clone()) + } else { + None + }; let op = OpCode::new(interp.current_opcode()) .or_else(|| { @@ -325,12 +330,10 @@ impl TracingInspector { self.step_stack.pop().expect("can't fill step without starting a step first"); let step = &mut self.traces.arena[trace_idx].trace.steps[step_idx]; - if let Some(stack) = step.stack.as_ref() { - // only check stack changes if record stack snapshots is enabled: if stack is Some - if interp.stack.len() > stack.len() { - // if the stack grew, we need to record the new values - step.push_stack = Some(interp.stack.data()[stack.len()..].to_vec()); - } + if self.config.record_stack_snapshots.is_pushes() { + let num_pushed = stack_push_count(step.op); + let start = interp.stack.len() - num_pushed; + step.push_stack = Some(interp.stack.data()[start..].to_vec()); } if self.config.record_memory_snapshots {