Skip to content

Commit

Permalink
fix(tracing): fix the parity vmTrace stack pushes #5561 (#5563)
Browse files Browse the repository at this point in the history 
Co-authored-by: fake <fake@fake>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
  • Loading branch information
@gbrew @mattsse
3 people authored Nov 25, 2023
1 parent 5f38a5f commit 4679bce
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 114 deletions.
153 changes: 73 additions & 80 deletions crates/revm/revm-inspectors/src/tracing/builder/parity.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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: <https://github.com/ledgerwatch/erigon/blob/9b74cf0384385817459f88250d1d9c459a18eab1/turbo/jsonrpc/trace_adhoc.go#L451>
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,
}
}
85 changes: 58 additions & 27 deletions crates/revm/revm-inspectors/src/tracing/config.rs
View file
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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.
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -93,9 +74,11 @@ impl TracingInspectorConfig {
#[inline]
pub fn from_parity_config(trace_types: &HashSet<TraceType>) -> 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)
}

Expand All @@ -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()
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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::*;
Expand Down
17 changes: 10 additions & 7 deletions crates/revm/revm-inspectors/src/tracing/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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(|| {
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 4679bce

Please sign in to comment.