From 9a54f21dd303f1bfe42b2a6839dc0664c46e4d1b Mon Sep 17 00:00:00 2001 From: ZzPoLariszZ <39354078+ZzPoLariszZ@users.noreply.github.com> Date: Thu, 25 Jul 2024 12:49:01 +0100 Subject: [PATCH] fix: gas and gasUsed in trace root only for ParityTrace (#171) ## Description This pull request is Part 1/2 of fixing the bug where the `gas` and `gasUsed` fields in Parity Trace root are incorrect. Part 2/2 paradigmxyz/reth#9761 ## Related Issues and Pull Requests - Follow: ethereum/go-ethereum#27029 - Improve: paradigmxyz/reth#3678 and paradigmxyz/reth#3719 - Fix: paradigmxyz/reth#9142 with #170 - Update: paradigmxyz/reth#3782 ## Problem The `gas` and `gasUsed` fields in Geth Debug Trace root should be the gas limit and gas used for the entire transaction. However, two fields in Parity Trace root should be the original ones. ### Reproducible Example With the latest version Reth v1.0.3, using `trace_transaction()` to trace the transaction `0x03128677ee3a9623d20f3c677f423ccc592d126374bf32e331343dd1bdf38b61` ``` curl http://localhost:8545 \ -X POST \ -H "Content-Type: application/json" \ --data '{"method":"trace_transaction","params":["0x03128677ee3a9623d20f3c677f423ccc592d126374bf32e331343dd1bdf38b61"],"id":1,"jsonrpc":"2.0"}' ``` **From Reth** ``` gas: 0x55493 (349331) gasUsed: 0x32d16 (208150) ``` **From [Etherscan](https://etherscan.io/vmtrace?txhash=0x03128677ee3a9623d20f3c677f423ccc592d126374bf32e331343dd1bdf38b61&type=parity#raw) and QuickNode** ``` gas: 0x4f227 (324135) gasUsed: 0x36622 (222754) ``` ## Solution for `revm-inspectors` 1. Not modify `gas_limit` and `gas_used` in the trace root ```diff - gas_limit = context.env.tx.gas_limit; - trace.set_root_trace_gas_used(gas_used); - trace.gas_used = gas_used(context.spec_id(), gas.spent(), gas.refunded() as u64); ``` 2. The modification in Step 1 will cause another problem The `gas` field for Geth Debug Trace root will also be reset (not the gas limitation for the entire transaction). therefore, can define `set_transaction_gas_limit()` and `with_transaction_gas_limit()` for Geth Debug, which is similar to current `set_transaction_gas_used()` and `with_transaction_gas_used()` for Parity. 3. Then, modify the Reth Part: `crates/rpc/rpc/src/trace.rs` and `crates/rpc/rpc/src/debug.rs` to completely fix the bug. ## Miscellaneous - Actually, I love the current design, but the results are inconsistent with those of others. - When I used `make pr` to test the Reth Part, the issue paradigmxyz/reth#9381 still exists for me. I should only skip tests for `lockfile` and test them seperately. --- src/tracing/builder/parity.rs | 10 +------- src/tracing/mod.rs | 46 ++++++++++++++++++----------------- tests/it/geth.rs | 27 +++++++++++++++++--- tests/it/parity.rs | 14 ++++------- 4 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/tracing/builder/parity.rs b/src/tracing/builder/parity.rs index 56fe4fd..acb7813 100644 --- a/src/tracing/builder/parity.rs +++ b/src/tracing/builder/parity.rs @@ -150,19 +150,11 @@ impl ParityTraceBuilder { res: &ExecutionResult, trace_types: &HashSet, ) -> TraceResults { - let gas_used = res.gas_used(); let output = res.output().cloned().unwrap_or_default(); let (trace, vm_trace, state_diff) = self.into_trace_type_traces(trace_types); - let mut trace = - TraceResults { output, trace: trace.unwrap_or_default(), vm_trace, state_diff }; - - // we're setting the gas used of the root trace explicitly to the gas used of the execution - // result - trace.set_root_trace_gas_used(gas_used); - - trace + TraceResults { output, trace: trace.unwrap_or_default(), vm_trace, state_diff } } /// Consumes the inspector and returns the trace results according to the configured trace diff --git a/src/tracing/mod.rs b/src/tracing/mod.rs index eebe623..965ed73 100644 --- a/src/tracing/mod.rs +++ b/src/tracing/mod.rs @@ -160,7 +160,7 @@ impl TracingInspector { self.traces } - /// Manually the gas used of the root trace. + /// Manually set the gas used of the root trace. /// /// This is useful if the root trace's gasUsed should mirror the actual gas used by the /// transaction. @@ -173,6 +173,19 @@ impl TracingInspector { } } + /// Manually set the gas limit of the debug root trace. + /// + /// This is useful if the debug root trace's gasUsed should mirror the actual gas used by the + /// transaction. + /// + /// This allows setting it manually by consuming the execution result's gas for example. + #[inline] + pub fn set_transaction_gas_limit(&mut self, gas_limit: u64) { + if let Some(node) = self.traces.arena.first_mut() { + node.trace.gas_limit = gas_limit; + } + } + /// Convenience function for [ParityTraceBuilder::set_transaction_gas_used] that consumes the /// type. #[inline] @@ -181,6 +194,13 @@ impl TracingInspector { self } + /// Work with [TracingInspector::set_transaction_gas_limit] function + #[inline] + pub fn with_transaction_gas_limit(mut self, gas_limit: u64) -> Self { + self.set_transaction_gas_limit(gas_limit); + self + } + /// Consumes the Inspector and returns a [ParityTraceBuilder]. #[inline] pub fn into_parity_builder(self) -> ParityTraceBuilder { @@ -268,7 +288,7 @@ impl TracingInspector { value: U256, kind: CallKind, caller: Address, - mut gas_limit: u64, + gas_limit: u64, maybe_precompile: Option, ) { // This will only be true if the inspector is configured to exclude precompiles and the call @@ -280,18 +300,6 @@ impl TracingInspector { PushTraceKind::PushAndAttachToParent }; - if self.trace_stack.is_empty() { - // this is the root call which should get the original gas limit of the transaction, - // because initialization costs are already subtracted from gas_limit - // For the root call this value should use the transaction's gas limit - // See and - gas_limit = context.env.tx.gas_limit; - - // we set the spec id here because we only need to do this once and this condition is - // hit exactly once - self.spec_id = Some(context.spec_id()); - } - self.trace_stack.push(self.traces.push_trace( 0, push_kind, @@ -319,7 +327,7 @@ impl TracingInspector { /// This expects an existing trace [Self::start_trace_on_call] fn fill_trace_on_call_end( &mut self, - context: &mut EvmContext, + _context: &mut EvmContext, result: &InterpreterResult, created_address: Option
, ) { @@ -328,13 +336,7 @@ impl TracingInspector { let trace_idx = self.pop_trace_idx(); let trace = &mut self.traces.arena[trace_idx].trace; - if trace_idx == 0 { - // this is the root call which should get the gas used of the transaction - // refunds are applied after execution, which is when the root call ends - trace.gas_used = gas_used(context.spec_id(), gas.spent(), gas.refunded() as u64); - } else { - trace.gas_used = gas.spent(); - } + trace.gas_used = gas.spent(); trace.status = result; trace.success = trace.status.is_ok(); diff --git a/tests/it/geth.rs b/tests/it/geth.rs index a68ec3e..bba6752 100644 --- a/tests/it/geth.rs +++ b/tests/it/geth.rs @@ -304,16 +304,35 @@ fn test_geth_inspector_reset() { assert_eq!(insp.traces().nodes().first().unwrap().trace.gas_limit, 0); // first run inspector - let (res, _) = inspect(&mut db, env.clone(), &mut insp).unwrap(); + let (res, env) = inspect(&mut db, env.clone(), &mut insp).unwrap(); assert!(res.result.is_success()); - assert_eq!(insp.traces().nodes().first().unwrap().trace.gas_limit, 1000000); + assert_eq!( + insp.clone() + .with_transaction_gas_limit(env.tx.gas_limit) + .traces() + .nodes() + .first() + .unwrap() + .trace + .gas_limit, + 1000000 + ); // reset the inspector insp.fuse(); assert_eq!(insp.traces().nodes().first().unwrap().trace.gas_limit, 0); // second run inspector after reset - let (res, _) = inspect(&mut db, env, &mut insp).unwrap(); + let (res, env) = inspect(&mut db, env, &mut insp).unwrap(); assert!(res.result.is_success()); - assert_eq!(insp.traces().nodes().first().unwrap().trace.gas_limit, 1000000); + assert_eq!( + insp.with_transaction_gas_limit(env.tx.gas_limit) + .traces() + .nodes() + .first() + .unwrap() + .trace + .gas_limit, + 1000000 + ); } diff --git a/tests/it/parity.rs b/tests/it/parity.rs index 823ee04..9747aab 100644 --- a/tests/it/parity.rs +++ b/tests/it/parity.rs @@ -104,10 +104,8 @@ fn test_parity_selfdestruct(spec_id: SpecId) { assert_eq!(node.trace.selfdestruct_transferred_value, expected_value); } - let traces = insp - .with_transaction_gas_used(res.result.gas_used()) - .into_parity_builder() - .into_localized_transaction_traces(TransactionInfo::default()); + let traces = + insp.into_parity_builder().into_localized_transaction_traces(TransactionInfo::default()); assert_eq!(traces.len(), 2); assert_eq!( @@ -188,10 +186,8 @@ fn test_parity_constructor_selfdestruct() { assert!(res.result.is_success()); print_traces(&insp); - let traces = insp - .with_transaction_gas_used(res.result.gas_used()) - .into_parity_builder() - .into_localized_transaction_traces(TransactionInfo::default()); + let traces = + insp.into_parity_builder().into_localized_transaction_traces(TransactionInfo::default()); assert_eq!(traces.len(), 3); assert!(traces[1].trace.action.is_create()); @@ -277,7 +273,7 @@ fn test_parity_call_selfdestruct() { Action::Call(CallAction { from: caller, call_type: CallType::Call, - gas: 100000000, + gas: traces.trace[0].action.as_call().unwrap().gas, input: input.into(), to, value: U256::ZERO,