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

gasUsed value is incorrect in eth_getTransactionReceipt response #4956

Closed
gumb0 opened this Issue Apr 12, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@gumb0
Member

gumb0 commented Apr 12, 2018

We currently return the same value for gasUsed and cumulativeGasUsed, see https://github.com/ethereum/cpp-ethereum/blob/a17f30a41aa2af506b6b453f052c53bd323a36e0/libweb3jsonrpc/JsonHelper.cpp#L203

For both values we return TransactionReceipt::gasUsed(), which is in fact cumulative gas.

We need to

  1. rename TransactionReceipt::gasUsed() to cumulativeGasUsed()
  2. add gasUsed() to LocalizedTransactionReceipt, calculate it by subtracting the previous transaction receipt's gas
  3. fill it correctly in toJson() function linked above
@halfalicious

This comment has been minimized.

Contributor

halfalicious commented Apr 14, 2018

I'm interested in taking a crack at this as a part of learning more about Ethereum. The bug and recommended fix seem pretty straight-forward, with the exception of calculating the correct gasUsed value - I don't see a way to obtain the previous transaction's gas from within the TransactionReceipt / LocalizedTransactionReceipt classes, though it's very possible that I'm missing something since I'm brand new to this code base. Do you have any recommendations for how to retrieve this information, perhaps using the state root (m_statusCodeOrStateRoot)?

On a related note, it looks like the root cause of the issue is that in State::Execute the cumulative gas used is computed and passed into the TransactionReceipt constructor:

https://github.com/ethereum/cpp-ethereum/blob/aa73807c9a6f79114d36ce738658dcba0bc7fbb3/libethereum/State.cpp#L587-L590

Presumably I cannot simply add a new constructor _gasUsed argument (and rename the existing one to _cumulativeGasUsed) because there are too many dependencies on the existing constructor signature?

@gumb0

This comment has been minimized.

Member

gumb0 commented Apr 16, 2018

Hi @halfalicious, thank you for your interest!

We shouldn't add new members to TransactionReceipt here, because it's supposed to represent the "Receipt" as defined by the Yellow Paper spec.
LocalizedTransactionReceipt is on the other hand something of a helper class that allows us to associate additional usueful data with the Receipt, so we should add gas value to it I think. See where its instantiated in ClientBase::localisedTransactionReceipt(), you can get the previous receipt from the blockchain (bc()) there, then get previous cumulative gas from it, subtract from current one and pass it to LocalizedTransactionReceipt constructor.

@halfalicious

This comment has been minimized.

Contributor

halfalicious commented Apr 17, 2018

Thanks gumb0! I've forked the repo and have a change ready which builds clean on Ubuntu, just need to see if I can test it locally before creating the PR.

@halfalicious

This comment has been minimized.

Contributor

halfalicious commented Apr 19, 2018

Update - ran into some issues trying to sync on Ubuntu (presumably due to running it in a Hyper-V container) so I've rebuilt my changes on Windows and kicked off a sync last night. Assuming I don't hit any other issues and testing goes smoothly I should be able to create the PR this weekend.

@gumb0

This comment has been minimized.

Member

gumb0 commented Apr 20, 2018

@halfalicious Note - you don't need to sync to the top, you can request any transaction among already downloaded blocks

pirapira added a commit that referenced this issue Apr 23, 2018

Merge pull request #4974 from halfalicious/develop
Add "gasUsed" data to the LocalisedTransactionReceipt class (fix #4956)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment