8000 Fix for world state root mismatch after parallel preprocessing with zero block reward by mirgee · Pull Request #8353 · hyperledger/besu · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix for world state root mismatch after parallel preprocessing with zero block reward #8353

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Mar 25, 2025
Merged

Fix for world state root mismatch after parallel preprocessing with zero block reward #8353

merged 10 commits into from
Mar 25, 2025

Conversation

mirgee
Copy link
Contributor
@mirgee mirgee commented Feb 26, 2025

This PR fixes a bug in the implementation of the parallel block processor, leading to frequent world state root mismatches during block persistence, similar to #7844.

When the block reward is zero and the mining beneficiary account doesn't exist yet, if the parallel preprocessing finishes in time to be applied, the resulting world state differs from the one produced by the sequential transaction processing, because, during application of the transaction processing result, parallel transaction processor creates the mining beneficiary address, whereas plain sequential processing, at least with clearEmptyAccounts on, does not, leading to mismatch between world states computed during block creation and parallelized block processing.

This PR also contains an integration test covering this scenario. The test fails without the fix and passes with it.

A complete fix would probably account for the value of clearEmptyAccounts in parallel transaction processor, but this flag seems to be in place for legacy purposes, so I am not sure it is worth further code change.

It is quite possible that this is not the only discrepancy between parallel and sequential processing leading to world state root mismatches.

This PR has been contributed on behalf of Absa Group Ltd.

@mirgee
Fix for world state root mismatch after parallel preprocessing with z…
c06704b 
…ero block reward

Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
@matkt
Copy link
Contributor
matkt commented Feb 26, 2025

will take a look thanks

@matkt
Copy link
Contributor
matkt commented Feb 27, 2025 •
edited
Loading

Thans for that it's really a good catch and I think it can clearly explain some issues

I noticed that the call to the constructor of ParallelTransactionPreprocessing was moved to the constructor of MainnetParallelBlockProcessor. This change creates the instance only once when the node starts for each specific fork, rather than per block. Why I created a new one for each block before it"s because of the ParallelizedConcurrentTransactionProcessor and more for parallelizedTransactionContextByLocation map

This map is saving the result of each background transaction execution of the block but if you have the same instance for each block you can have some transaction execution for the block before and it can create inconsistency

For example, consider block 5 with transactions tx0, tx1, and tx2:

  • Background Processing: Transactions tx0, tx1, and tx2 start executing in the background.
  • Sequential Processing: The sequential processing begins. However, due to timing, the ParallelizedConcurrentTransactionProcessor might retrieve the result for tx2 and not for tx0 and tx1 because the sequential process reached tx0 and tx1 before their background results were available.
  • Residual Data: At the end of block 5, results for tx0 and tx1 remain in the map.
  • Then, when block 6 (with tx0', tx1', and tx2') is processed, the system detects the results from block 5 (for tx0 and tx1) and applies them to the world state, leading to an inconsistency.

It is an important modification ?

A complete fix would probably account for the value of clearEmptyAccounts in parallel transaction processor, but this flag seems to be in place for legacy purposes, so I am not sure it is worth further code change.

It will be nice to manage it if we want to do a fullsync or if we have a configuration that take care of that.

@mirgee
Reinstantiate ParallelTransactionPreprocessing per block, manage exec…
7399fe8 
…utor in ParallelTransactionPreprocessing, simplify

Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
@mirgee
Account for clearEmptyAccounts flag in ParallelizedConcurrentTransact…
54fa89f 
…ionProcessor

Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
@mirgee
Copy link
Contributor Author
mirgee commented Feb 28, 2025

@matkt Good point regarding ParallelizedConcurrentTransactionProcessor, that was an unnecessary leftover. Both that and clearEmptyAccounts should be addressed now.

matkt and others added 4 commits March 4, 2025 21:33
@matkt
Merge branch 'main' into bugfix/world-state-root-mismatch
46dac36
@mirgee
Merge branch 'main' into bugfix/world-state-root-mismatch
ee15de8 
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
@mirgee
Initialize one instance of executor per instance of MainnetParallelBl…
0476b12 
…ockProcessor

Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
@mirgee
Merge branch 'main' into bugfix/world-state-root-mismatch
3534332 
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
ahamlat
ahamlat reviewed Mar 21, 2025
View reviewed changes
Copy link
Contributor
@ahamlat ahamlat left a comment

Choose a reason for hiding this comment

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

I haven't finished the review, as I need to double check some GC overhead.

mirgee added 2 commits March 21, 2025 20:40
@mirgee
Make executor static in MainnetParallelBlockProcessor, rename testBlo…
f2b7f4b 
…ckProcessingConsistency to testProcessBlockZeroReward

Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
@mirgee
Merge branch 'main' into bugfix/world-state-root-mismatch
90cd9ba 
Signed-off-by: Miroslav Kovar <miroslavkovar@protonmail.com>
ahamlat
ahamlat approved these changes Mar 24, 2025
View reviewed changes
Copy link
Contributor
@ahamlat ahamlat left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 , @matkt could you take a last look on your side ?

@matkt matkt enabled auto-merge (squash) March 25, 2025 08:48
@matkt matkt merged commit 0088505 into hyperledger:main Mar 25, 2025
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahamlat ahamlat ahamlat approved these changes

@matkt matkt matkt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mirgee @matkt @ahamlat
0