diff --git a/crates/revm/revm-inspectors/src/tracing/builder/parity.rs b/crates/revm/revm-inspectors/src/tracing/builder/parity.rs index 246f608b4..ebbe28c48 100644 --- a/crates/revm/revm-inspectors/src/tracing/builder/parity.rs +++ b/crates/revm/revm-inspectors/src/tracing/builder/parity.rs @@ -146,22 +146,17 @@ impl ParityTraceBuilder { /// Consumes the inspector and returns the trace results according to the configured trace /// types. /// - /// Warning: If `trace_types` contains [TraceType::StateDiff] the returned [StateDiff] will only - /// contain accounts with changed state, not including their balance changes because this is not - /// tracked during inspection and requires the State map returned after inspection. Use - /// [ParityTraceBuilder::into_trace_results_with_state] to populate the balance and nonce - /// changes for the [StateDiff] using the [DatabaseRef]. + /// Warning: If `trace_types` contains [TraceType::StateDiff] the returned [StateDiff] will not + /// be filled. Use [ParityTraceBuilder::into_trace_results_with_state] or + /// [populate_state_diff] to populate the balance and nonce changes for the [StateDiff] + /// using the [DatabaseRef]. pub fn into_trace_results( self, - res: ExecutionResult, + res: &ExecutionResult, trace_types: &HashSet, ) -> TraceResults { let gas_used = res.gas_used(); - let output = match res { - ExecutionResult::Success { output, .. } => output.into_data(), - ExecutionResult::Revert { output, .. } => output, - ExecutionResult::Halt { .. } => Default::default(), - }; + let output = res.output().cloned().unwrap_or_default(); let (trace, vm_trace, state_diff) = self.into_trace_type_traces(trace_types); @@ -186,14 +181,14 @@ impl ParityTraceBuilder { /// with the [TracingInspector](crate::tracing::TracingInspector). pub fn into_trace_results_with_state( self, - res: ResultAndState, + res: &ResultAndState, trace_types: &HashSet, db: DB, ) -> Result where DB: DatabaseRef, { - let ResultAndState { result, state } = res; + let ResultAndState { ref result, ref state } = res; let breadth_first_addresses = if trace_types.contains(&TraceType::VmTrace) { CallTraceNodeWalkerBF::new(&self.nodes) @@ -207,7 +202,7 @@ impl ParityTraceBuilder { // check the state diff case if let Some(ref mut state_diff) = trace_res.state_diff { - populate_account_balance_nonce_diffs(state_diff, &db, state.iter())?; + populate_state_diff(state_diff, &db, state.iter())?; } // check the vm trace case @@ -218,7 +213,12 @@ impl ParityTraceBuilder { Ok(trace_res) } - /// Returns the tracing types that are configured in the set + /// Returns the tracing types that are configured in the set. + /// + /// Warning: if [TraceType::StateDiff] is provided this does __not__ fill the state diff, since + /// this requires access to the account diffs. + /// + /// See [Self::into_trace_results_with_state] and [populate_state_diff]. pub fn into_trace_type_traces( self, trace_types: &HashSet, @@ -234,7 +234,6 @@ impl ParityTraceBuilder { if trace_types.contains(&TraceType::VmTrace) { Some(self.vm_trace()) } else { None }; let mut traces = Vec::with_capacity(if with_traces { self.nodes.len() } else { 0 }); - let mut diff = StateDiff::default(); for node in self.iter_traceable_nodes() { let trace_address = self.trace_address(node.idx); @@ -262,13 +261,10 @@ impl ParityTraceBuilder { } } } - if with_diff { - node.parity_update_state_diff(&mut diff); - } } let traces = with_traces.then_some(traces); - let diff = with_diff.then_some(diff); + let diff = with_diff.then_some(StateDiff::default()); (traces, vm_trace, diff) } @@ -565,9 +561,7 @@ where /// /// It's expected that `DB` is a revm [Database](revm::db::Database) which at this point already /// contains all the accounts that are in the state map and never has to fetch them from disk. -/// -/// This is intended to be called after inspecting a transaction with the returned state. -pub fn populate_account_balance_nonce_diffs<'a, DB, I>( +pub fn populate_state_diff<'a, DB, I>( state_diff: &mut StateDiff, db: DB, account_diffs: I, @@ -586,7 +580,7 @@ where let entry = state_diff.entry(addr).or_default(); // we check if this account was created during the transaction - if changed_acc.is_created() { + if changed_acc.is_created() || changed_acc.is_loaded_as_not_existing() { entry.balance = Delta::Added(changed_acc.info.balance); entry.nonce = Delta::Added(U64::from(changed_acc.info.nonce)); if changed_acc.info.code_hash == KECCAK_EMPTY { @@ -594,13 +588,27 @@ where // marked as added entry.code = Delta::Added(Default::default()); } + + // new storage values + for (key, slot) in changed_acc.storage.iter() { + entry.storage.insert((*key).into(), Delta::Added(slot.present_value.into())); + } } else { // account already exists, we need to fetch the account from the db let db_acc = db.basic(addr)?.unwrap_or_default(); + // update _changed_ storage values + for (key, slot) in changed_acc.storage.iter().filter(|(_, slot)| slot.is_changed()) { + entry.storage.insert( + (*key).into(), + Delta::changed( + slot.previous_or_original_value.into(), + slot.present_value.into(), + ), + ); + } + // check if the account was changed at all - // NOTE: changed storage values are set by the the - // `CallTraceNode::parity_update_state_diff` if entry.storage.is_empty() && db_acc == changed_acc.info && !changed_acc.is_selfdestructed() diff --git a/crates/revm/revm-inspectors/src/tracing/types.rs b/crates/revm/revm-inspectors/src/tracing/types.rs index 8d2529029..95e84a3d0 100644 --- a/crates/revm/revm-inspectors/src/tracing/types.rs +++ b/crates/revm/revm-inspectors/src/tracing/types.rs @@ -8,15 +8,15 @@ use reth_rpc_types::trace::{ AccountState, CallFrame, CallLogFrame, DiffStateKind, GethDefaultTracingOptions, StructLog, }, parity::{ - Action, ActionType, CallAction, CallOutput, CallType, ChangedType, CreateAction, - CreateOutput, Delta, SelfdestructAction, StateDiff, TraceOutput, TransactionTrace, + Action, ActionType, CallAction, CallOutput, CallType, CreateAction, CreateOutput, + SelfdestructAction, TraceOutput, TransactionTrace, }, }; use revm::interpreter::{ opcode, CallContext, CallScheme, CreateScheme, InstructionResult, Memory, OpCode, Stack, }; use serde::{Deserialize, Serialize}; -use std::collections::{btree_map::Entry, BTreeMap, VecDeque}; +use std::collections::{BTreeMap, VecDeque}; /// A unified representation of a call #[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Serialize, Deserialize)] @@ -308,74 +308,6 @@ impl CallTraceNode { self.status() == InstructionResult::SelfDestruct } - /// Updates the values of the state diff - pub(crate) fn parity_update_state_diff(&self, diff: &mut StateDiff) { - let addr = self.trace.address; - let acc = diff.entry(addr).or_default(); - - if self.kind().is_any_create() { - let code = self.trace.output.clone(); - if acc.code == Delta::Unchanged { - acc.code = Delta::Added(code) - } - } - - // iterate over all storage diffs - for change in self.trace.steps.iter().filter_map(|s| s.storage_change) { - let StorageChange { key, value, had_value, .. } = change; - let b256_value = B256::from(value); - match acc.storage.entry(key.into()) { - Entry::Vacant(entry) => { - if let Some(had_value) = had_value { - if value != had_value { - entry.insert(Delta::Changed(ChangedType { - from: had_value.into(), - to: b256_value, - })); - } - } else { - entry.insert(Delta::Added(b256_value)); - } - } - Entry::Occupied(mut entry) => { - let value = match entry.get() { - Delta::Unchanged => { - if let Some(had_value) = had_value { - if value != had_value { - Delta::Changed(ChangedType { - from: had_value.into(), - to: b256_value, - }) - } else { - Delta::Unchanged - } - } else { - Delta::Added(b256_value) - } - } - Delta::Added(added) => { - if added == &b256_value { - Delta::Added(*added) - } else { - Delta::Changed(ChangedType { from: *added, to: b256_value }) - } - } - Delta::Removed(_) => Delta::Added(b256_value), - Delta::Changed(c) => { - if c.from == b256_value { - // remains unchanged if the value is the same - Delta::Unchanged - } else { - Delta::Changed(ChangedType { from: c.from, to: b256_value }) - } - } - }; - entry.insert(value); - } - } - } - } - /// Converts this node into a parity `TransactionTrace` pub(crate) fn parity_transaction_trace(&self, trace_address: Vec) -> TransactionTrace { let action = self.parity_action(); diff --git a/crates/rpc/rpc-types/src/eth/trace/parity.rs b/crates/rpc/rpc-types/src/eth/trace/parity.rs index d9d78e1e4..e375d7be4 100644 --- a/crates/rpc/rpc-types/src/eth/trace/parity.rs +++ b/crates/rpc/rpc-types/src/eth/trace/parity.rs @@ -85,6 +85,11 @@ pub enum Delta { // === impl Delta === impl Delta { + /// Creates a new [Delta::Changed] variant + pub fn changed(from: T, to: T) -> Self { + Self::Changed(ChangedType { from, to }) + } + /// Returns true if the value is unchanged pub fn is_unchanged(&self) -> bool { matches!(self, Delta::Unchanged) diff --git a/crates/rpc/rpc/src/trace.rs b/crates/rpc/rpc/src/trace.rs index bbf31d77b..52bbe49b5 100644 --- a/crates/rpc/rpc/src/trace.rs +++ b/crates/rpc/rpc/src/trace.rs @@ -15,9 +15,7 @@ use reth_provider::{BlockReader, ChainSpecProvider, EvmEnvProvider, StateProvide use reth_revm::{ database::StateProviderDatabase, env::tx_env_with_recovered, - tracing::{ - parity::populate_account_balance_nonce_diffs, TracingInspector, TracingInspectorConfig, - }, + tracing::{parity::populate_state_diff, TracingInspector, TracingInspectorConfig}, }; use reth_rpc_api::TraceApiServer; use reth_rpc_types::{ @@ -26,7 +24,7 @@ use reth_rpc_types::{ BlockError, BlockOverrides, CallRequest, Index, }; use revm::{db::CacheDB, primitives::Env}; -use revm_primitives::{db::DatabaseCommit, ResultAndState}; +use revm_primitives::db::DatabaseCommit; use std::{collections::HashSet, sync::Arc}; use tokio::sync::{AcquireError, OwnedSemaphorePermit}; @@ -84,7 +82,7 @@ where .spawn_with_call_at(call, at, overrides, move |db, env| { let (res, _, db) = inspect_and_return_db(db, env, &mut inspector)?; let trace_res = inspector.into_parity_builder().into_trace_results_with_state( - res, + &res, &trace_types, &db, )?; @@ -116,7 +114,7 @@ where .eth_api .spawn_trace_at_with_state(env, config, at, move |inspector, res, db| { Ok(inspector.into_parity_builder().into_trace_results_with_state( - res, + &res, &trace_types, &db, )?) @@ -158,16 +156,12 @@ where let config = tracing_config(&trace_types); let mut inspector = TracingInspector::new(config); let (res, _) = inspect(&mut db, env, &mut inspector)?; - let ResultAndState { result, state } = res; - let mut trace_res = - inspector.into_parity_builder().into_trace_results(result, &trace_types); - - // If statediffs were requested, populate them with the account balance and - // nonce from pre-state - if let Some(ref mut state_diff) = trace_res.state_diff { - populate_account_balance_nonce_diffs(state_diff, &db, state.iter())?; - } + let trace_res = inspector.into_parity_builder().into_trace_results_with_state( + &res, + &trace_types, + &db, + )?; results.push(trace_res); @@ -176,7 +170,7 @@ where if calls.peek().is_some() { // need to apply the state changes of this call before executing // the next call - db.commit(state) + db.commit(res.state) } } @@ -196,7 +190,7 @@ where .eth_api .spawn_trace_transaction_in_block(hash, config, move |_, inspector, res, db| { let trace_res = inspector.into_parity_builder().into_trace_results_with_state( - res, + &res, &trace_types, &db, )?; @@ -417,12 +411,12 @@ where tracing_config(&trace_types), move |tx_info, inspector, res, state, db| { let mut full_trace = - inspector.into_parity_builder().into_trace_results(res, &trace_types); + inspector.into_parity_builder().into_trace_results(&res, &trace_types); // If statediffs were requested, populate them with the account balance and // nonce from pre-state if let Some(ref mut state_diff) = full_trace.state_diff { - populate_account_balance_nonce_diffs(state_diff, db, state.iter())?; + populate_state_diff(state_diff, db, state.iter())?; } let trace = TraceResultsWithTransactionHash { @@ -567,12 +561,13 @@ struct TraceApiInner { } /// Returns the [TracingInspectorConfig] depending on the enabled [TraceType]s +/// +/// Note: the parity statediffs can be populated entirely via the execution result, so we don't need +/// statediff recording #[inline] fn tracing_config(trace_types: &HashSet) -> TracingInspectorConfig { - let needs_diff = trace_types.contains(&TraceType::StateDiff); let needs_vm_trace = trace_types.contains(&TraceType::VmTrace); - let needs_steps = needs_vm_trace || needs_diff; - TracingInspectorConfig::default_parity().set_steps(needs_steps).set_state_diffs(needs_diff) + TracingInspectorConfig::default_parity().set_steps(needs_vm_trace) } /// Helper to construct a [`LocalizedTransactionTrace`] that describes a reward to the block @@ -602,8 +597,9 @@ mod tests { let mut s = HashSet::new(); s.insert(TraceType::StateDiff); let config = tracing_config(&s); - assert!(config.record_steps); - assert!(config.record_state_diff); + // not required + assert!(!config.record_steps); + assert!(!config.record_state_diff); let mut s = HashSet::new(); s.insert(TraceType::VmTrace); @@ -616,6 +612,7 @@ mod tests { s.insert(TraceType::StateDiff); let config = tracing_config(&s); assert!(config.record_steps); - assert!(config.record_state_diff); + // not required for StateDiff + assert!(!config.record_state_diff); } }