fix(p2p): bound the inbound node-info exchange by the handshake deadline - #3916
fix(p2p): bound the inbound node-info exchange by the handshake deadline#3916bdchatham wants to merge 3 commits into
Conversation
acceptPeersRoutine holds an accept-semaphore slot from before AcceptOrClose until after the peer's node info is exchanged. handshakeCtx carries handshake-timeout and covers handshake(), but exchangeNodeInfo on the next line took the connection context instead, and it blocks on ReadSizedMsg with no deadline of its own. A peer that completes the handshake and then stops responding holds its slot for as long as it keeps the socket open, and the node stops acquiring inbound peers while every health signal still reports green. Run it under handshakeCtx, which already covers the handshake itself. The default stays at 10s. Bounding the read is the fix here; tuning the deadline is a separate judgement about slow links. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR SummaryLow Risk Overview
Adds Reviewed by Cursor Bugbot for commit 4338572. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3916 +/- ##
==========================================
- Coverage 59.48% 58.47% -1.01%
==========================================
Files 2325 2229 -96
Lines 198647 188004 -10643
==========================================
- Hits 118160 109936 -8224
+ Misses 69258 67682 -1576
+ Partials 11229 10386 -843
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
One-line fix that bounds the inbound exchangeNodeInfo read by handshakeCtx, closing a slot-exhaustion vector where a peer could hold an accept-semaphore slot indefinitely; the change is correct and makes the accept path symmetric with the dial path, which already covers both handshake and node-info exchange under the same timeout. The only gap is the absence of a regression test, which the author already called out.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No regression test covers the fix: a peer that completes the handshake and then stops sending node info should have its connection torn down and its accept-semaphore slot released once
HandshakeTimeoutexpires. The existing router test setsHandshakeTimeout: utils.Some(time.Hour)(sei-tendermint/internal/p2p/router_test.go:283), so nothing exercises expiry and this one-line change could silently regress. The test sketch in the PR description (build the router directly, dial withtcp.Dial, runhandshakeclient-side, send no node info, assert the connection closes within the deadline) looks like the right shape.
Builds a router with a short handshake-timeout, dials it, completes the handshake so the peer is authenticated, then sends no node info. The router must hang up rather than leave the accept slot held, which surfaces to the dialer as EOF from the connection pump. Verified it discriminates: with exchangeNodeInfo back on the connection context the test hangs and go test's own timeout fires, which is the repo's convention rather than an artificial timeout inside the test. Two details worth knowing for anyone extending this. tcp.Conn.Run is demand-driven, so a read has to stay outstanding for the pump to observe the close at all. And a failed read blocks on ctx.Done() until the pump cancels the scope, so the pump's error is the terminal signal rather than something to swallow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greg: the node info exchange is part of the handshake, which is why it belongs under the handshake deadline. What this guards is a peer losing connectivity part way through, not a malicious one, since a malicious peer can complete the exchange and hold the connection open with pings while sending nothing useful. Comment rewritten to say that instead of framing it as an indefinite hold. Masih: the read loop is bounded on ctx rather than looping forever, and the final assertion uses require.ErrorIs. Still discriminates: with exchangeNodeInfo back on the connection context the test hangs and go test's timeout fires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Problem
Router.acceptPeersRoutineholds an accept-semaphore slot from beforeAcceptOrCloseuntil after the peer's node info is exchanged.handshakeCtxcarrieshandshake-timeoutand covershandshake(), but the next line takes the connection context:exchangeNodeInfoblocks onconn.ReadSizedMsg, which has no deadline of its own and reads the length varint a byte at a time. The node info exchange is part of the handshake, so it should run under the same deadline; as written, a peer that goes quiet part way through holds its slot for as long as its socket survives.What that guards is a peer losing connectivity mid-exchange, not a malicious one. A peer that completes the exchange releases the accept slot and moves into the inbound peer budget, where it can hold a connection open by answering pings while sending nothing useful, so this bound does not stop anyone determined.
Change
handshakeCtxalready exists and already covers the handshake. This extends it over the rest of the region where the slot is held.context.WithTimeoutis an absolute deadline rather than an idle one, so a peer that drips bytes slowly is still cut off athandshake-timeout. Teardown was already correct: on cancellationtcp.Conn.ReadcallsCloseRead()and waits for the read pump to return data ownership, so the aborted read leaks neither a goroutine nor a half-open socket.The default stays at 10s. Bounding the read is the fix; tuning the deadline down is a separate judgement about slow links.
Notes for review
MakeTestNetwork: dial its endpoint withtcp.Dial, runhandshakeclient-side to completion, send no node info, and assert the connection closes within the deadline. Happy to add it here if you would rather not merge without it.Advertise()andnodeInfoProducer()both complete before the I/O begins, andconnTracker.AddConn's critical section is map operations only, so the slot is the only resource involved.go build ./...,go test ./internal/p2p/,gofmt,goimportsandgolangci-lint(v2.8.0) are clean.