cl/sentinel: one status handshake per peer, and honour the ban on inbound connections - #23606
cl/sentinel: one status handshake per peer, and honour the ban on inbound connections#23606lystopad wants to merge 2 commits into
Conversation
…ound onConnection handles every connection event on its own goroutine, so a burst of events for one peer started a handshake for each. The three-strike ban could not intervene: all of them completed before any raised the failure count to its threshold. It also only consulted the ban in ConnectWithPeer, which covers dials we initiate and not the events that arrive anyway. A gnosis archive node reached 101 attempts against a single peer in two minutes, and 13,827 handshake failures across 1,638 peers in ninety minutes, until libp2p's own dialer began refusing with "rate limit exceeded". Banned peers are now closed before the handshake, and a per-peer gate admits one attempt at a time so the ban threshold is reachable. Concurrent events for the same peer are dropped rather than queued: the peer is about to be judged by the attempt already running. Closes #23605
domiwei
left a comment
domiwei
left a comment
There was a problem hiding this comment.
Requesting changes for two related lifecycle issues.
-
cl/sentinel/discovery.go:562-568checksBanStatusbefore gate admission, leaving a TOCTOU window. Event B can observe not-banned and pause; event A can complete the third failing handshake, install the ban, and release; then B can acquire and callValidatePeerfor the now-banned peer. Acquire first and check or recheck the ban inside the serialized section before handshaking. -
The new tests exercise only
handshakeGate; removing the newonConnectionwiring leaves all of them green. The burst test also never releases its winner, so serial execution still admits exactly one. Because the issue and PR both say the observed production ordering is unverified, please add a deterministic production-sequence test coveringonConnection -> BanStatus -> ValidatePeer -> RecordHandshakeFailure, three failures reaching the ban, release on all exits, and the next inbound event closing without another handshake.
The focused cl/sentinel suite passes normally and under -race, but it does not cover these sequences.
Checking BanStatus before admission left a window: an event could observe the peer as not banned, a concurrent handshake could complete the third failure and install the ban, and the first event could then acquire the gate and handshake a peer that is already banned. Extracts the status exchange into exchangeStatus so the gate is taken first and the ban is read while it is held, and so the sequence is reachable from a test. The connection handler keeps its subnet and peer-limit checks. The previous tests only exercised handshakeGate in isolation, leaving the wiring unpinned: deleting it kept them green. The new tests drive the whole sequence against a real peers pool - three transport failures reaching the ban, the next event closing the peer without another handshake, a concurrent event for the same peer refused, and a fork mismatch dropped without counting toward the ban - and each exit path leaving the gate released.
|
Both findings addressed in 69196c7. TOCTOU — you are right, and the window is not where I would have guessed it. The gate is now taken first and the ban is read while it is held, so a ban installed by a handshake that completed while this event waited is visible before Tests — your critique is correct and it is the second time I have made it: the four gate tests exercised
I verified they bite by mutation rather than assuming:
One limit worth stating plainly: these tests pin that the ban is consulted and that a ban installed during an earlier handshake is honoured by the next event. They do not deterministically pin the ordering of acquire-versus-check, because that interleaving needs a hook inside the function to reproduce. If you want that guaranteed against future edits, I can add a test seam for it — say the word and I will. |
domiwei
left a comment
domiwei
left a comment
There was a problem hiding this comment.
The production TOCTOU is fixed correctly at 69196c7: gate admission now precedes BanStatus, so a same-peer event cannot carry a stale ban observation past the third failing handshake. One blocking regression-test gap remains.
The new sequence tests still pass if BanStatus is moved back above tryAcquire, which restores the exact bug, and they also pass if the onConnection -> exchangeStatus wiring is deleted. TestExchangeStatusRefusesAPeerBannedWhileTheEventWaited pre-bans the peer before the call; no event actually waits or crosses the check/admission boundary because busy admission is dropped.
Please add a deterministic production-sequence seam that distinguishes the ordering: prove gate ownership exists when ban status is observed, and enter through onConnection or a directly invoked production handler. The sequence should pause one validation, submit a same-peer event and an unrelated peer, complete the third failure, then assert the next inbound event closes without validation. This should fail if ban observation is moved before admission or if the onConnection wiring is removed.
The full cl/sentinel package passes normally and under -race; this request is specifically about pinning the production regression.
Closes #23605.
onConnectionhandles every connection event on its own goroutine, so a burst of events for one peer starts a handshake for each. The three-strike ban inpeers.Poolcannot intervene, because all of them complete before any raises the failure count to its threshold. Separately, the ban was only consulted inConnectWithPeer— which covers dials we initiate, not the events that arrive regardless.Measured on a gnosis archive node: 101 attempts against a single peer in two minutes, 13,827 handshake failures across 1,638 distinct peers in ninety minutes, and 460 dials refused by libp2p's own dialer with
rate limit exceeded.Change
Concurrent events for the same peer are dropped rather than queued: the peer is about to be judged by the attempt already in flight, so queueing would only re-run the same verdict.
The retained-peer behaviour is untouched. Keeping a peer whose status exchange failed (
discovery.go, "may still work for gossip") is deliberate and this does not change it — only the rate at which such a peer is re-attempted.Notes for review
The mechanism above is my reading of it, and the issue records it as unverified — I could not reproduce the burst against a live network, only in the unit tests. If someone can confirm the ordering with instrumentation on
RecordHandshakeFailure, that would settle whether the gate is the whole fix or only part of it.cl/sentinelpasses with-race; the burst test drives 64 concurrent acquires and asserts exactly one is admitted.