ethereum / evmone

Fast Ethereum Virtual Machine implementation
Apache License 2.0
863 stars 285 forks source link

statetest: Drop support for "fake" `BLOCKHASH` #1049

Closed chfast closed 1 month ago

chfast commented 1 month ago

The state tests had a procedure of generating non-empty BLOCKHASH outputs based on the block number. This is deprecated feature not used in any existing tests. Remove support for it in evmone.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.22%. Comparing base (8399303) to head (b778311). Report is 11 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1049 +/- ## ========================================== - Coverage 94.23% 94.22% -0.01% ========================================== Files 155 155 Lines 15968 15959 -9 ========================================== - Hits 15047 15038 -9 Misses 921 921 ``` | [Flag](https://app.codecov.io/gh/ethereum/evmone/pull/1049/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [eof_execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1049/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `17.70% <0.00%> (+0.01%)` | :arrow_up: | | [ethereum_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1049/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `27.50% <100.00%> (+<0.01%)` | :arrow_up: | | [ethereum_tests_silkpre](https://app.codecov.io/gh/ethereum/evmone/pull/1049/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `19.36% <0.00%> (+<0.01%)` | :arrow_up: | | [execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/1049/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `20.62% <0.00%> (+0.01%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/ethereum/evmone/pull/1049/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `89.01% <0.00%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files with missing lines](https://app.codecov.io/gh/ethereum/evmone/pull/1049?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [test/state/host.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1049?src=pr&el=tree&filepath=test%2Fstate%2Fhost.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC9zdGF0ZS9ob3N0LmNwcA==) | `100.00% <100.00%> (ø)` | | | [test/unittests/state\_transition\_block\_test.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/1049?src=pr&el=tree&filepath=test%2Funittests%2Fstate_transition_block_test.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC91bml0dGVzdHMvc3RhdGVfdHJhbnNpdGlvbl9ibG9ja190ZXN0LmNwcA==) | `100.00% <ø> (ø)` | |
winsvega commented 1 month ago

Perhaps error out of invalid test?

chfast commented 1 month ago

Perhaps error out of invalid test?

How exactly?

chfast commented 1 month ago

We may get a protection against this in EEST: https://github.com/ethereum/execution-spec-tests/issues/899.

But it may also break goevmlab fuzzer (cc @holiman). However, they may use "blockHashes" to provide the value for BLOCKHASH(0).

holiman commented 1 month ago

What do you mean "deprecated feature" - since the opcode BLOCKHASH is still not deprecated, what is a node supposed to do on that opcode, when executing a statetest?

chfast commented 1 month ago

What do you mean "deprecated feature" - since the opcode BLOCKHASH is still not deprecated, what is a node supposed to do on that opcode, when executing a statetest?

State tests try to avoid it at all cost to the point a state test with BLOCKHASH is considered invalid. See https://github.com/ethereum/execution-spec-tests/issues/899.

holiman commented 1 month ago

I think that is insufficient. Random tests are highly useful. I don't much care what the vm returns, but having all vms behave identically is important.

Why do this?

chfast commented 1 month ago

Why do this?

It bothers me this is mixed with the semi-production ready code. I'd prefer returning null hash but I think we can find a place for the old behavior...

chfast commented 1 month ago

The #1059 moves this feature "more" into "tests only". This should be fine for now.