fix(network): evict stale BSC protocol peers from registry on disconnect#349
Open
tsutsu wants to merge 1 commit into
Open
fix(network): evict stale BSC protocol peers from registry on disconnect#349tsutsu wants to merge 1 commit into
tsutsu wants to merge 1 commit into
Conversation
Remove stale UnboundedSender entries when the BSC subprotocol stream drops, when GetBlocksByRange send fails, and on vote broadcast failures. Use per-connection tokens so same-PeerId reconnects are not removed by older sessions. Co-authored-by: Cursor <cursoragent@cursor.com>
Pull Request ReviewThis PR fixes stale peer handling in a Rust-based blockchain networking module (BSC protocol extension) by introducing per-connection registry tokens and explicit cleanup paths. It refactors the global peer sender registry to store Sensitive ContentNo sensitive content detected. Security IssuesNo serious security issues detected. Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
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
1 participant
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.
Resolves #348.
Description
Unregisters BSC protocol-extension peers from the global command-sender registry when their connection stream drops or when a send to their channel fails, preventing the registry from accumulating stale entries that can never succeed.
Refactors the command-sender registry to use per-connection tokens, preventing a data race where a peer reconnecting with the same
PeerIdcould have its new (live) registry entry removed by the de-registration of an older (dead) session.Rationale
The BSC protocol-extension peer registry (
REGISTRY) mapsPeerIds toUnboundedSender<BscCommand>s. Entries are inserted when a BSC subprotocol connection is established, but are never fully removed. The map entry is not removed by peer disconnect, nor byBscCommandsend failure.I observed code comments that noted that "failed sends will lazily clean up entries," but this cleanup was incomplete:
request_blocks_by_rangedid not remove the peer on send failure, and there was noDrop-based eviction at all. (The only code path that can currently trigger peer removal from the registry is vote-broadcast processing.)When peers disconnect (especially in bulk, e.g. due to a netsplit, or due to a bug like #312), the registry retains dead senders. If this happens during fork recovery, then fork recovery picks peers from this stale registry, fails to send
GetBlocksByRangethrough the closed channels, and so fails to ever make progress.Testing
Existing unit tests pass.