Skip to content

Commit

Permalink
fix: gas and gasUsed in trace root only for ParityTrace (#171)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
ZzPoLariszZ authored Jul 25, 2024
1 parent 403164c commit 9a54f21
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 44 deletions.
10 changes: 1 addition & 9 deletions src/tracing/builder/parity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,19 +150,11 @@ impl ParityTraceBuilder {
res: &ExecutionResult,
trace_types: &HashSet<TraceType>,
) -> 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
Expand Down
46 changes: 24 additions & 22 deletions src/tracing/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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]
Expand All @@ -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 {
Expand Down Expand Up @@ -268,7 +288,7 @@ impl TracingInspector {
value: U256,
kind: CallKind,
caller: Address,
mut gas_limit: u64,
gas_limit: u64,
maybe_precompile: Option<bool>,
) {
// This will only be true if the inspector is configured to exclude precompiles and the call
Expand All @@ -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 <https://github.com/paradigmxyz/reth/issues/3678> and <https://github.com/ethereum/go-ethereum/pull/27029>
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,
Expand Down Expand Up @@ -319,7 +327,7 @@ impl TracingInspector {
/// This expects an existing trace [Self::start_trace_on_call]
fn fill_trace_on_call_end<DB: Database>(
&mut self,
context: &mut EvmContext<DB>,
_context: &mut EvmContext<DB>,
result: &InterpreterResult,
created_address: Option<Address>,
) {
Expand All @@ -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();
Expand Down
27 changes: 23 additions & 4 deletions tests/it/geth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}
14 changes: 5 additions & 9 deletions tests/it/parity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 9a54f21

Please sign in to comment.