Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

calculate gas_used of the transaction trace correctly, #9194

Closed
wants to merge 1 commit into from
Closed

calculate gas_used of the transaction trace correctly, #9194

wants to merge 1 commit into from

Conversation

debris
Copy link
Collaborator

@debris debris commented Jul 23, 2018

@debris debris added A0-pleasereview 🤓 Pull request needs code review. B0-patchthis M4-core ⛓ Core client code / Rust. labels Jul 23, 2018
@5chdn 5chdn added this to the 2.1 milestone Jul 24, 2018
This was referenced Jul 24, 2018
Merged
Merged
andresilva
andresilva approved these changes Jul 25, 2018
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM.

ethcore/vm/src/action_params.rs
/// Gas paid up front for transaction execution
/// Gas paid up front for entire transaction execution.
pub entire_gas: U256,
/// Gas paid up front for action execution.
Copy link
Contributor

Choose a reason for hiding this comment

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

Gas paid up front for action execution (entire gas minus action opcode and data cost). ?

This was referenced Jul 26, 2018
Merged
Merged
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 26, 2018
@sorpaas
Copy link
Collaborator

sorpaas commented Jul 27, 2018

I don't quite understand the meaning of entire_gas here:

  • For top-level call/create, it makes sense -- those are invoked by a transaction, and it indeed has the concept of transaction "entire gas".
  • However, for sub call/create, I think this concept might not have much meaning? It's currently always set to equal to just gas, and remain unused except in tracing.

Given that this entire_gas value is only for tracing, would it be possible to move it out of ActionParams struct?

@sorpaas sorpaas added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jul 27, 2018
@sorpaas
Copy link
Collaborator

sorpaas commented Aug 24, 2018 •
edited
Loading

Another method to fix #9178 and #6964 is to directly insert the transaction gas to tracing mod. I think that may be the better way, because the current gas used is actually correct if we consider a transaction is a wrapper on a call/create executive.

Also a note, this PR slightly conflicts with #9360.

This was referenced Aug 30, 2018
Merged
Merged
@5chdn 5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 4, 2018
@5chdn
Copy link
Contributor

5chdn commented Sep 4, 2018

What's the status of this?

@5chdn
Copy link
Contributor

5chdn commented Sep 8, 2018

Please reopen once you continue working on this.

@5chdn 5chdn closed this Sep 8, 2018
@5chdn 5chdn deleted the traces branch November 27, 2018 11:41
@zargit zargit mentioned this pull request Jul 19, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@andresilva andresilva andresilva approved these changes

Assignees
No one assigned
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.1 Whatever
Development

Successfully merging this pull request may close these issues.

Gas used according to transaction trace doesn't match transaction receipt Gas used got by trace_transaction
4 participants
@debris @sorpaas @5chdn @andresilva