Check the bad block manager when receiving a new block from the network#10212
Open
daniellehrner wants to merge 5 commits intobesu-eth:mainfrom
Open
Check the bad block manager when receiving a new block from the network#10212daniellehrner wants to merge 5 commits intobesu-eth:mainfrom
daniellehrner wants to merge 5 commits intobesu-eth:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds safeguards to avoid repeatedly attempting to import blocks already marked as bad, preventing sync loops and enabling correct INVALID signaling for poisoned forks.
Changes:
- Short-circuit new block and new block hash handling when the block hash is already in
BadBlockManager. - Add a defensive short-circuit at the start of
importOrSavePendingBlockfor known-bad blocks. - Add unit tests around block propagation skipping, plus an integration test covering
engine_forkchoiceUpdatedbehavior with bad ancestors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java | Adds known-bad-block checks in network handlers and import path to avoid reprocessing invalid blocks. |
| ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/AbstractBlockPropagationManagerTest.java | Adds tests asserting known-bad blocks are skipped before entering the import/request pipeline. |
| ethereum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java | Adds end-to-end test verifying bad-ancestor propagation yields INVALID with correct latestValidHash. |
...m/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java
Show resolved
Hide resolved
...m/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java
Outdated
Show resolved
Hide resolved
...m/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java
Outdated
Show resolved
Hide resolved
...m/api/jsonrpc/internal/methods/engine/EngineForkchoiceUpdatedBadAncestorIntegrationTest.java
Outdated
Show resolved
Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java
Show resolved
Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/BlockPropagationManager.java
Show resolved
Hide resolved
e5fc0fd to
7e6a33e
Compare
…rk. This allows to mark a whole fork invalid and signaling this to the CL at the next fcu Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
7e6a33e to
ca65d08
Compare
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
Signed-off-by: daniellehrner <daniel.lehrner@consensys.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR description
When we get a fcu request and need to sync missing blocks from our peers, we currently are not checking if any of those blocks is already in the bad block manager. This leads to an endless loop where we try to import an invalid block every few milliseconds.
This PR adds this check and marks all following blocks on this specific forks as invalid as well. Like this we can respond with
INVALIDwhen the next fcu request is received and correctly signal to the CL that we consider this fork invalid.Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests