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, core/state: fix post-exec dump of state (statetests, blockchaintests) #28504

Merged
merged 11 commits into from Nov 28, 2023

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Nov 10, 2023

In #28484 (comment), I mentioned some failures that I encountered while trying to state-dump a blockchain test (ethereum/execution-spec-tests#332) . It turns out that there were several problems.

  • If a preimage was missing, even if we had set the OnlyWithAddresses to false, to export them anyway, the way the mapping was constructed (using common.Address as key) made the entries get lost anyway. This is fixed in the first commit, and concerns both state- and blockchain tests.
  • If preimages were present, but flushed from RAM (statedb) to disk, the dump operation didn't bother to look them up from disk. This concerns blockchain test exec, not sure if it concerns statetest exec. This is fixed in the second commit.
  • This also configures the blockchain tests to run with preimage enabled.

This PR goes pretty well hand in hand with #28484 .

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Feels a little weird to have mismatch in behavior between Dump and IteratorDump? Also more generally, does it make sense for DumpCollectorto return accounts we don't know the preimage of? Failing silently-ish doesn't seem like a great option (although we do log that there were some number of accounts with missing preimages).

I know Felix recently used this in the hivechain rewrite and there wouldn't be much use in outputing the preimages because it's expected that json will be consumed by programs which will process / antagonize the generated chain.

@holiman
Copy link
Contributor Author

holiman commented Nov 10, 2023

Feels a little weird to have mismatch in behavior between Dump and IteratorDump?

I agree, should be fixed.

Also more generally, does it make sense for DumpCollector to return accounts we don't know the preimage of?

Yes. The user/caller has passed in options, the options said OnlyWithAddresses : false -- meaning "yes I want the accounts even if you cannot provide the addresses". So the api at this point should just obey.

In general, it can be useful to dump state even if the address is unknown, for whatever reason.

@lightclient
Copy link
Member

Ahh okay I thought OnlyWithAddresses meant something like "don't provide the storage elements" 😬 okay that makes sense then. In that case, my only remaining thought is if pre(0x1234...1234) is clear enough. I don't love it. preimage(0x1234...1234) seems marginally clearer, but still feels kinda weird. Would it be extremely confusing to interleave 20 byte addresses and 32 byte secure keys w/o "pre" / "preimage" moniker?

@holiman
Copy link
Contributor Author

holiman commented Nov 10, 2023

pre(0x1234...1234) is clear enough. I don't love it.

Yeah, me neither. I could go with preimage. putting a 32-byte thing there -- not sure sure about that. If someone tries to parse it as an address, I want it to break hard, not, like, chomp off 12 bytes of hex and continue (which might happen if someone defines address as []byte, and later on just truncates it).

@holiman
Copy link
Contributor Author

holiman commented Nov 10, 2023

I pushed another commit now, turns out we don't need two different structs. We can just omit to set the Next field if we're not using using it, and that's already how it's implemented.

…ng preimages

This changes makes it so that the block test executor takes a callback, just like
the state test executor already does. This callback can be used to examine the
post-execution state, e.g. to aid debugging of test failures.

This change also modifies the mechanics of Dump: previously, all accounts for which
we did not have the preimage (address) were silently discarded.

Now they are output instead like this:
```
{
    "root": "3ef447699d08e3160c448903e77bd092c67ce3faeba773a3f83c500ab80f2400",
    "accounts": {
        "pre(0x03601462093b5945d1676df093446790fd31b20e7b12a2e8e5e09d068109616b)": {
            "balance": "10780210",
            "nonce": 1,
```
@holiman holiman added this to the 1.13.6 milestone Nov 17, 2023
core/state/dump.go Outdated Show resolved Hide resolved
core/state/dump.go Outdated Show resolved Hide resolved
@rjl493456442
Copy link
Member

		if conf.OnlyWithAddresses {
			fmt.Fprintf(os.Stderr, "If you want to include accounts with missing preimages, you need iterative output, since"+
				" otherwise the accounts will overwrite each other in the resulting mapping.")
			return errors.New("incompatible options")
		}

I guess it's no longer true? Even with OnlyWithAddresses config for non-interactive iteration, we can still iterate out the accounts with preimages and filter out the remaining.

@holiman
Copy link
Contributor Author

holiman commented Nov 28, 2023

Good catch, fixed!

@holiman holiman merged commit 63979bc into ethereum:master Nov 28, 2023
2 checks passed
fearlessfe pushed a commit to fearlessfe/go-ethereum that referenced this pull request Dec 11, 2023
…aintests) (ethereum#28504)

There were several problems related to dumping state. 

- If a preimage was missing, even if we had set the `OnlyWithAddresses` to `false`, to export them anyway, the way the mapping was constructed (using `common.Address` as key) made the entries get lost anyway. Concerns both state- and blockchain tests. 
- Blockchain test execution was not configured to store preimages.

This changes makes it so that the block test executor takes a callback, just like the state test executor already does. This callback can be used to examine the post-execution state, e.g. to aid debugging of test failures.
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
…aintests) (ethereum#28504)

There were several problems related to dumping state. 

- If a preimage was missing, even if we had set the `OnlyWithAddresses` to `false`, to export them anyway, the way the mapping was constructed (using `common.Address` as key) made the entries get lost anyway. Concerns both state- and blockchain tests. 
- Blockchain test execution was not configured to store preimages.

This changes makes it so that the block test executor takes a callback, just like the state test executor already does. This callback can be used to examine the post-execution state, e.g. to aid debugging of test failures.
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.

None yet

3 participants