Fix8725: Do not ask for chain head header when using eth69#8756
Closed
pinges wants to merge 12 commits intobesu-eth:mainfrom
Closed
Fix8725: Do not ask for chain head header when using eth69#8756pinges wants to merge 12 commits intobesu-eth:mainfrom
pinges wants to merge 12 commits intobesu-eth:mainfrom
Conversation
Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>
Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>
Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>
Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes Eth69 peer handling by skipping redundant chain head requests and refactors peer checking logic for clarity and performance.
- Simplified SnapServerChecker by making its constructor private and streamlining its
checkmethod. - Added debug logging in
ChainHeadTracker#getBestHeaderFromPeerCompletableFuture. - Refactored
EthPeers.ethPeerStatusExchangedto extract and reusecheckHeadBlockandcheckSnapServer, with conditional skipping for Eth69 peers.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/SnapServerChecker.java | Made constructor private; simplified check to use thenApply. |
| ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/ChainHeadTracker.java | Added debug log before fetching headers from peer. |
| ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java | Refactored ethPeerStatusExchanged, extracted helper methods, and skipped chain head fetch for Eth69 peers. |
| LOG.atTrace() | ||
| .setMessage("Checking whether peer {} is a snap server ...") | ||
| .addArgument(peer::getLoggableId) | ||
| .log(); |
There was a problem hiding this comment.
Passing a null headBlockHeader for Eth69-compatible peers will be forwarded to getAccountRangeFromPeer and may cause an NPE or unexpected behavior. Consider handling the null case explicitly (e.g., using the status message header) before calling this method.
Suggested change
| .log(); | |
| .log(); | |
| if (headBlockHeader == null) { | |
| LOG.atWarn() | |
| .setMessage("headBlockHeader is null. Cannot check if peer {} is a snap server.") | |
| .addArgument(peer::getLoggableId) | |
| .log(); | |
| return CompletableFuture.completedFuture(false); | |
| } |
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java
Outdated
Show resolved
Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/EthPeers.java
Outdated
Show resolved
Hide resolved
pinges
changed the title
…anager/EthPeers.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Stefan Pingel <16143240+pinges@users.noreply.github.com>
…anager/EthPeers.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Stefan Pingel <16143240+pinges@users.noreply.github.com>
Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@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
This PR is a slight optimisation in case we are using eth69 with a peer, where we don't have to ask for their chain head header after receiving their status message.
Fixed Issue(s)
#8725