blockchaintest: Optional partial state hash check

[chfast]

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 #953.

[codecov]

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 ⚠️

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%)	⬆️
ethereum_tests	27.85% <87.50%> (+0.01%)	⬆️
ethereum_tests_silkpre	19.50% <0.00%> (-0.01%)	⬇️
execution_spec_tests	18.83% <87.50%> (+0.01%)	⬆️
unittests	88.97% <0.00%> (-0.03%)	⬇️

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%)	⬆️

[pdobacz]

Is this temporary, until the MPT implementation is optimized, or forever? It sounds a tiny bit scary

[chfast]

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]

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]

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]

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]

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]

Not needed for now so put on hold.