Fixed-10108 : should only advertise snap/1 in handshake if snapsync-server-enabled#10182
Open
sagarkhandagre998 wants to merge 2 commits intobesu-eth:mainfrom
Open
Fixed-10108 : should only advertise snap/1 in handshake if snapsync-server-enabled#10182sagarkhandagre998 wants to merge 2 commits intobesu-eth:mainfrom
sagarkhandagre998 wants to merge 2 commits intobesu-eth:mainfrom
Conversation
…s active Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Signed-off-by: Sagar Khandagre <sagar.khandagre998@gmail.com>
Author
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
p2p: only advertise
snap/1when snap server or snap sync is activeProblem
Besu unconditionally advertised
snap/1in its devp2p handshake regardless of--snapsync-server-enabled. Remote peers probed it, received empty responses, classified it as a non-server, and stopped sending snap requests to it — wasted connections with no operator warning.Approach Decision
The non-trivial part was that
snap/1serves two roles:--snapsync-server-enabled)--sync-mode=SNAP)In devp2p, both sides must advertise a capability for it to be active on a connection. Blindly removing
snap/1when the server is disabled would silently break snap sync for nodes still catching up.So the fix gates advertisement on both conditions rather than just
snapServerEnabled:--snapsync-server-enabled--sync-modesnap/1?falsefalseSNAPtrueChanges
SnapProtocolManager—calculateCapabilities()now takessnapConfigandsyncMode; applies the three-way logic above; logs WARN when advertising for client-only useBesuControllerBuilder— passessyncConfig.getSyncMode()intoSnapProtocolManagerSnapProtocolManagerTest— addsSyncMode.SNAPto keep the existing test exercising a snap-capable nodeFixed Issue(s)
Fixed #10108
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