p2p: size the inbound throttle to one peer's connections - #1018
Open
hyunsooda wants to merge 1 commit into
Open
Conversation
The per-IP inbound throttle admitted one connection per 30s window, which fits a single listener but not a multichannel node. Discovery carries one TCP port per node, so a peer's first dial is single-channel; the handshake then reports the extra listen ports and the peer redials all of them. One peer therefore opens one connection per listener plus that first dial, from the same IP within microseconds, and every connection after the first was closed before its handshake. handleAddPeerConn only builds a Peer once all channels arrive, so a peer on a public IP never assembled and expired out of CandidateConns instead. LAN addresses are exempt, which is why loopback tests and same-subnet deployments never saw it. Count the entries per IP instead of testing for presence, and let each server declare how many belong to one peer: one for a single-channel node, one per listener plus the initial dial for a multichannel one. Constraint: discovery carries one TCP port per node, so the first dial to a multichannel peer is always single-channel and is always retried Rejected: give each listener its own history | listener 0 still sees the redial as a second attempt from the same IP Rejected: drop the history entry when its connection closes | turns the throttle into a concurrency cap that a connect/disconnect flood evades Confidence: high Scope-risk: narrow Directive: the allowance is derived from the listener count, not a constant - adding a listener must not require touching the throttle Not-tested: the listenLoop call sites; the test drives inboundAllowance() and checkInboundConn directly Not-tested: two nodes behind one public IP, which still share one allowance Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Proposed changes
The inbound throttle is keyed on the remote IP and allowed one connection per 30s
window, but a multichannel peer opens one connection to every listen port from that
same IP. Every channel after the first was closed before its handshake, and a peer is
only complete once all of them arrive, so multichannel peering could not form at all
between public IPs — LAN is exempt, which is why loopback tests never showed it. The
throttle now allows one peer's worth of connections:
len(ListenAddrs) + 1for amultichannel node — one per listener, plus the single-channel dial a peer makes before
the handshake tells it there are more ports — and 1 for a single-channel node.
Types of changes
Checklist
I have read the CLA Document and I hereby sign the CLAin first time contribute after having read CLA$ make test)Related issues
Regression from #926, present in
v3.0.0-rc.1.Further comments
Giving each listener its own throttle history does not work: the initial
single-channel dial and its retry both land on the main port, so that listener still
sees the same IP twice. Two nodes behind one public IP still share an allowance, as
upstream.