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.

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

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

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

ZzPoLariszZ
Copy link
Contributor

@ZzPoLariszZ ZzPoLariszZ commented Jul 24, 2024

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

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 and QuickNode

gas: 0x4f227 (324135)
gasUsed: 0x36622 (222754)

Solution for revm-inspectors

  1. Not modify gas_limit and gas_used in the trace root

    - 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 make pr failed of lockfile reth#9381 still exists for me.

    I should only skip tests for lockfile and test them seperately.

Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

tysm for looking into this.

it seems like this is still a wip and I'll take a closer look once cleaned up.
but very supportive, your write up made a lot of sense

src/tracing/mod.rs Outdated Show resolved Hide resolved
@ZzPoLariszZ ZzPoLariszZ requested a review from mattsse July 24, 2024 14:09
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty!
last question

src/tracing/mod.rs Show resolved Hide resolved
Comment on lines -290 to -292
// 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());
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

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.

@ZzPoLariszZ ZzPoLariszZ requested a review from mattsse July 25, 2024 10:57
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

tysm!

doing a new release and we can finish the reth pr

@mattsse mattsse merged commit 9a54f21 into paradigmxyz:main Jul 25, 2024
11 checks passed
@ZzPoLariszZ ZzPoLariszZ deleted the fix-gas-used-trace-root branch July 28, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect traces order for suicide call, and gas and value for create call
3 participants