evmone icon indicating copy to clipboard operation
evmone copied to clipboard

blockchaintest: Optional partial state hash check

Open chfast opened this issue 1 year ago • 7 comments

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.

chfast avatar Aug 19 '24 18:08 chfast

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
@@            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 Coverage Δ
eof_execution_spec_tests 17.44% <87.50%> (+0.02%) :arrow_up:
ethereum_tests 27.85% <87.50%> (+0.01%) :arrow_up:
ethereum_tests_silkpre 19.50% <0.00%> (-0.01%) :arrow_down:
execution_spec_tests 18.83% <87.50%> (+0.01%) :arrow_up:
unittests 88.97% <0.00%> (-0.03%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
test/blockchaintest/blockchaintest_runner.cpp 77.55% <87.50%> (+0.13%) :arrow_up:

codecov[bot] avatar Aug 19 '24 18:08 codecov[bot]

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.

chfast avatar Aug 20 '24 07:08 chfast

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?

pdobacz avatar Aug 20 '24 07:08 pdobacz

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.

gumb0 avatar Aug 22 '24 10:08 gumb0

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?

pdobacz avatar Aug 22 '24 11:08 pdobacz

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 avatar Aug 22 '24 11:08 chfast

Not needed for now so put on hold.

chfast avatar Sep 10 '24 15:09 chfast