Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: gas and gasUsed in trace root only for ParityTrace #171

Merged
merged 4 commits into from
Jul 25, 2024
Merged

fix: gas and gasUsed in trace root only for ParityTrace #171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19478a6
Fix gas and gasUsed in trace root only for ParityTrace
ZzPoLariszZ Jul 24, 2024
f58f161
Do some cleanup for review
ZzPoLariszZ Jul 24, 2024
df71413
Do some cleanup for review
ZzPoLariszZ Jul 24, 2024
53b5eb3
Do some cleanup for review
ZzPoLariszZ Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions src/tracing/builder/parity.rs
View file
Open in desktop
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
View file
Open in desktop
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.
mattsse marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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());
Comment on lines -290 to -292
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this got lost, or do we no longer need this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused, we also have unused variables in the various constructors

Copy link
Contributor Author

@ZzPoLariszZ ZzPoLariszZ Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double check the code. This is self.spec_id for TracingInspector is used to set the gas limit for the Parity trace root only once. For Geth Debug trace, this will be set in the newly constructed functions set_transaction_gas_limit() and with_transaction_gas_limit(). Therefore, we on longer need this.

}

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
View file
Open in desktop
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
View file
Open in desktop
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
Loading