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

cmd/evm: allow state dump regardless if test passes in statetest #28484

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

sorpaas
Copy link
Contributor

@sorpaas sorpaas commented Nov 8, 2023

This just makes it simpler for other evm implementations testing against geth.

@holiman
Copy link
Contributor

holiman commented Nov 9, 2023

In theory I agree, and I've been on the verge of making this PR myself, but when I've actually experimented with it, I've noticed that the state dump after a statetest cannot be relied upon. I started digging in that, but then got busy with other tasks.

As I recall it,

  • The preimages aren't collected during statetest execution, so the dump misses addresses,
  • There's something about flushing that is wrong, so when we iterate the trie we aren't actually getting everything.

The postCheck method get's a new state:

	statedb, _ = state.New(root, statedb.Database(), snaps)

And is invoked with the fresh 'state' but reused snaps`.

		postCheck(result, snaps, statedb)

However, before we constructed this new state, we did a Commit operation and discarded the nodes, so we never actually stored them to db.

	// Commit state mutations into database.
	root, _ := statedb.Commit(block.NumberU64(), config.IsEIP158(block.Number()))

A good start here would be to add some testcases that shows the failure of the current mechanism. Having proper tests here would be great (and making it work would probably not be too hard once we have tests)

@holiman
Copy link
Contributor

holiman commented Nov 10, 2023

Hm, I might be misremembering. When I was investigating this: ethereum/execution-spec-tests#332 , I noticed the shortcomings. However, that was regarding the blocktest command, not the statetest command.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.13.5 milestone Nov 10, 2023
cmd/evm/staterunner.go Outdated Show resolved Hide resolved
@holiman holiman merged commit 2f4833b into ethereum:master Nov 10, 2023
2 checks passed
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…ereum#28484)

This change makes it so that when executing state tess, state is always dumped out if the corresponding flag is set.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
…ereum#28484)

This change makes it so that when executing state tess, state is always dumped out if the corresponding flag is set.
colinlyguo pushed a commit to scroll-tech/go-ethereum that referenced this pull request Oct 31, 2024
…ereum#28484)

This change makes it so that when executing state tess, state is always dumped out if the corresponding flag is set.
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.

2 participants