ethereum / evmone

Fast Ethereum Virtual Machine implementation
Apache License 2.0
864 stars 286 forks source link

blockchaintest: Optional partial state hash check #975

Open chfast opened 3 months ago

chfast commented 3 months ago

This is blockchain tests execution optimization: for listed test names only check state root hash of first 5 blocks (to detect early problems) and last 5 blocks (to do the final check of the chain of blocks).

The current implementation of the MPT hash of the state builds the trie from scratch (no updates to the trie of the previous block). For the tests will a long chain of blocks the performance degrades significantly with 99% time spent in the keccak hash function.

This improves testing of EIP-2935 implemented in https://github.com/ethereum/evmone/pull/953.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.83%. Comparing base (a79218c) to head (14ae941).

Files with missing lines Patch % Lines
test/blockchaintest/blockchaintest_runner.cpp 87.50% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #975 +/- ## ========================================== - Coverage 93.84% 93.83% -0.01% ========================================== Files 146 146 Lines 15439 15444 +5 ========================================== + Hits 14488 14492 +4 - Misses 951 952 +1 ``` | [Flag](https://app.codecov.io/gh/ethereum/evmone/pull/975/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/975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `17.44% <87.50%> (+0.02%)` | :arrow_up: | | [ethereum_tests](https://app.codecov.io/gh/ethereum/evmone/pull/975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `27.85% <87.50%> (+0.01%)` | :arrow_up: | | [ethereum_tests_silkpre](https://app.codecov.io/gh/ethereum/evmone/pull/975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `19.50% <0.00%> (-0.01%)` | :arrow_down: | | [execution_spec_tests](https://app.codecov.io/gh/ethereum/evmone/pull/975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `18.83% <87.50%> (+0.01%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/ethereum/evmone/pull/975/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | `88.97% <0.00%> (-0.03%)` | :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/975?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum) | Coverage Δ | | |---|---|---| | [test/blockchaintest/blockchaintest\_runner.cpp](https://app.codecov.io/gh/ethereum/evmone/pull/975?src=pr&el=tree&filepath=test%2Fblockchaintest%2Fblockchaintest_runner.cpp&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ethereum#diff-dGVzdC9ibG9ja2NoYWludGVzdC9ibG9ja2NoYWludGVzdF9ydW5uZXIuY3Bw) | `77.55% <87.50%> (+0.13%)` | :arrow_up: |
chfast commented 3 months ago

Temporary but for long time :) Updating MTP seems quite a work and we only need this for testing.

Because the state in the final block is evolution of all previous states I think this is relatively fine. The only situation it can miss is when in a middle block state is incorrect but it corrects itself before the final block.

The other option is to list the test names for which we want to do the light check.

For the EEST stable tests the number of blocks is not bigger than 30.

pdobacz commented 3 months ago

Temporary but for long time :)

:)

Because the state in the final block is evolution of all previous states I think this is relatively fine. The only situation it can miss is when in a middle block state is incorrect but it corrects itself before the final block.

We had a very similar case of a self-cancelling test (IIRC EXCHANGE), hence my worries

The other option is to list the test names for which we want to do the light check.

For the EEST stable tests the number of blocks is not bigger than 30.

Or, can we assume a threshold (like 30), above which we automatically switch to the light check?

We could also check "every some-prime-number-th" or sth like that, in addition to first 5 + last 5. So that it still keeps us under the 30 blocks mark, WDYT?

gumb0 commented 3 months ago

We had a very similar case of a self-cancelling test (IIRC EXCHANGE), hence my worries

Given that system calls now modify state every block (beacon chain roots since Cancun and historical block hashes since Prague), it seems highly unlikely that state changes will cancel each other over several blocks? Actually I think this doesn't help

I think at least since Prague, as block hashes are saved into state, the wrong state changes cannot cancel out easily.

pdobacz commented 3 months ago

Hm, dunno, probably this is fine as is, however

I think at least since Prague, as block hashes are saved into state, the wrong state changes cannot cancel out easily.

true, unless they aren't saved because of some bug. But maybe this is paranoid. Maybe a flag to parametrize blockchain test runs, slow vs fast? And slow are run only on releases / with some lower cadence?

chfast commented 3 months ago

I think at least since Prague, as block hashes are saved into state, the wrong state changes cannot cancel out easily.

Hm... now I noticed this is weird: we need to compute the block hash which includes the state root hash for EIP-2935, so how all this works as we just disabled this computation...

chfast commented 2 months ago

Not needed for now so put on hold.