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

blockchaintest: Optional partial state hash check #975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chfast
Copy link
Member

@chfast chfast commented Aug 19, 2024

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.

@chfast chfast added the tests Testing infrastructure label Aug 19, 2024
@chfast chfast requested review from gumb0 and pdobacz August 19, 2024 18:44
Copy link

codecov bot commented Aug 19, 2024

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

Copy link
Collaborator

@pdobacz pdobacz left a comment

Choose a reason for hiding this comment

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

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

@chfast
Copy link
Member Author

chfast commented Aug 20, 2024

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
Copy link
Collaborator

pdobacz commented Aug 20, 2024

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
Copy link
Member

gumb0 commented Aug 22, 2024

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
Copy link
Collaborator

pdobacz commented Aug 22, 2024

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
Copy link
Member Author

chfast commented Aug 22, 2024

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 chfast force-pushed the test/limit_blockchain_test_state_root_checks branch from 52e4530 to 5ccd847 Compare September 10, 2024 13:30
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.
@chfast chfast force-pushed the test/limit_blockchain_test_state_root_checks branch from 5ccd847 to 14ae941 Compare September 10, 2024 13:31
@chfast chfast changed the title blockchaintest: Only check state root for 5 first/last blocks blockchaintest: Optional partial state hash check Sep 10, 2024
@chfast
Copy link
Member Author

chfast commented Sep 10, 2024

Not needed for now so put on hold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Testing infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants