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

core/vm: for tracing, do not report post-op memory #24867

Merged
merged 1 commit into from
May 24, 2022

Conversation

holiman
Copy link
Contributor

@holiman holiman commented May 12, 2022

closes #24109

Currently, if you try to trace a transaction which MSTOREs something at an offset previously not written to, then the debug_traceTransaction will pad the memory with zeros. In the trace, usually the reported values are what happens before running the opcode
...

On master

$ go run . --code 60406040526001 --json run 
{"pc":0,"op":96,"gas":"0x2540be400","gasCost":"0x3","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":2,"op":96,"gas":"0x2540be3fd","gasCost":"0x3","memSize":0,"stack":["0x40"],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":4,"op":82,"gas":"0x2540be3fa","gasCost":"0xc","memSize":96,"stack":["0x40","0x40"],"depth":1,"refund":0,"opName":"MSTORE"}
{"pc":5,"op":96,"gas":"0x2540be3ee","gasCost":"0x3","memSize":96,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":7,"op":0,"gas":"0x2540be3eb","gasCost":"0x0","memSize":96,"stack":["0x1"],"depth":1,"refund":0,"opName":"STOP"}
{"output":"","gasUsed":"0x15","time":739636}

With this PR:

[user@work evm]$ go run . --code 60406040526001 --json run 
{"pc":0,"op":96,"gas":"0x2540be400","gasCost":"0x3","memSize":0,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":2,"op":96,"gas":"0x2540be3fd","gasCost":"0x3","memSize":0,"stack":["0x40"],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":4,"op":82,"gas":"0x2540be3fa","gasCost":"0xc","memSize":0,"stack":["0x40","0x40"],"depth":1,"refund":0,"opName":"MSTORE"}
{"pc":5,"op":96,"gas":"0x2540be3ee","gasCost":"0x3","memSize":96,"stack":[],"depth":1,"refund":0,"opName":"PUSH1"}
{"pc":7,"op":0,"gas":"0x2540be3eb","gasCost":"0x0","memSize":96,"stack":["0x1"],"depth":1,"refund":0,"opName":"STOP"}
{"output":"","gasUsed":"0x15","time":128972}

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

LGTM

@alcuadrado
Copy link
Member

Sorry if these questions are obvious, I want to make sure that our implementation in Hardhat is equivalent to yours.

Does this PR affect the output of debug_traceTransaction? Will this be released with the next version of geth?

@s1na
Copy link
Contributor

s1na commented May 23, 2022

Hey @alcuadrado! Yes it'll affect the output of debug_traceTransaction slightly. The memory reported for each step is currently after the expansion necessary to execute that op. This PR changes it so that the step reports memory before expansion.

I'm not sure about the second question. There was a plan to cut the next release early this week. I'll bring up the PR tomorrow morning for discussion, see if we can get it merged.

@karalabe karalabe added this to the 1.10.18 milestone May 24, 2022
@karalabe karalabe merged commit 64d6c78 into ethereum:master May 24, 2022
@alcuadrado
Copy link
Member

Hey, @s1na! Thanks for your reply! Knowing that this is planned for an upcoming release, even if it's not the next one, is really helpful for our planning. We've been wanting to implement this same change for some time but didn't want to do it before geth.

gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 24, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 25, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 25, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 27, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Oct 17, 2024
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracing: memory output erroneously reports post-op memory size
4 participants