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

perf(evm): only store storage slot diffs and reconstruct full storage on request #3198

Merged
merged 13 commits into from
Sep 15, 2022

Conversation

shekhirin
Copy link
Contributor

@shekhirin shekhirin commented Sep 13, 2022

Motivation

continued work on top of #3187
relevant piece of geth code: https://github.com/ethereum/go-ethereum/blob/366d2169fbc0e0f803b68c042b77b6b480836dbc/eth/tracers/logger/logger.go#L181-L206

Demo

Deposit WETH and debug this transaction

~ cast send --rpc-url http://localhost:8545 --private-key 0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 deposit --value 1 --json |
jq -r .transactionHash |
xargs -I{} cast rpc --rpc-url http://localhost:8545 --raw debug_traceTransaction '["{}", {"enableMemory": true, "enableReturnData": true}]'

Check SLOAD instruction and storage slots associated with it (before, during, after)
image

Check SSTORE instruction and storage slots associated with it (before, during, after)
image

@shekhirin shekhirin changed the title perf(evm): only store changed storage entries perf(evm): only store storage slot diffs and reconstruct full storage on request Sep 13, 2022
@shekhirin shekhirin force-pushed the fix-state-storage-more branch from a2f4a7a to fc40e25 Compare September 13, 2022 20:35
Copy link
Member

@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.

this looks great.

populating the storage via geth_trace is a great solution here.
this will make the it correct and will also mem consumption a lot

Comment on lines 107 to 110
.last()
.expect("not enough contract journal entries")
.last()
.expect("not enough call journal entries");
Copy link
Member

Choose a reason for hiding this comment

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

this should always work since, revm initializes this with vec![vec![]] and has an entry at this point

Copy link
Member

Choose a reason for hiding this comment

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

let's add as comment in the code?

Comment on lines +353 to +354
/// Change of the contract state after step execution (effect of the SLOAD/SSTORE instructions)
pub state_diff: Option<(U256, U256)>,
Copy link
Member

Choose a reason for hiding this comment

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

nice

@shekhirin shekhirin force-pushed the fix-state-storage-more branch from a1ba35b to 6e5a613 Compare September 14, 2022 10:40
@shekhirin
Copy link
Contributor Author

There are still some bugs in debug_traceTransaction implementation but I think it'd better to resolve them in separate PRs and mark as fixes of #3206

@shekhirin
Copy link
Contributor Author

shekhirin commented Sep 14, 2022

@mattsse not sure if it's fine to pin revm to git for now or better to ask Dragan to make a new release?

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

it's OK to use the pinned revm and we can wait for new release np

Comment on lines 107 to 110
.last()
.expect("not enough contract journal entries")
.last()
.expect("not enough call journal entries");
Copy link
Member

Choose a reason for hiding this comment

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

let's add as comment in the code?

@shekhirin shekhirin force-pushed the fix-state-storage-more branch from ab9567d to 6098f45 Compare September 15, 2022 18:14
@shekhirin shekhirin requested a review from gakonst September 15, 2022 18:39
@shekhirin shekhirin marked this pull request as ready for review September 15, 2022 19:56
Copy link
Member

@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.

awesome

@mattsse mattsse merged commit 483843f into foundry-rs:master Sep 15, 2022
@mattsse
Copy link
Member

mattsse commented Sep 15, 2022

There are still some bugs in debug_traceTransaction implementation but I think it'd better to resolve them in separate PRs and mark as fixes of #3206

agree, merging since this is already a big improvement

iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
… on request (foundry-rs#3198)

* fix: breaking changes geth trace data

* fix: only clone contract state

* a bit more fixes n improvements

* revertme: bump revm to git

* track diffs, reconstruct the storage on request

* add comments

* omit gas refund if it's 0 (go has omitempty on this field)

* add comment regarding gas cost calculation & fix storage emptiness

* show storage in response only on SLOAD/SSTORE

* reorganize Inspector impl for Tracer

* improve unwrap comment

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
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.

3 participants