From 775f1f51c460764bd0886eff7c10bf1833706263 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 13 Aug 2026 07:52:02 -0700 Subject: [PATCH 1/3] fix(p2p): bound the inbound node-info exchange by the handshake deadline 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 --- sei-tendermint/internal/p2p/router.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sei-tendermint/internal/p2p/router.go b/sei-tendermint/internal/p2p/router.go index e0f5880bbe..0411e7654b 100644 --- a/sei-tendermint/internal/p2p/router.go +++ b/sei-tendermint/internal/p2p/router.go @@ -242,7 +242,9 @@ func (r *Router) acceptPeersRoutine(ctx context.Context) error { release() return giga.RunInboundConn(ctx, hConn) } - info, err := exchangeNodeInfo(ctx, hConn, *r.nodeInfoProducer()) + // handshakeCtx, not ctx: the accept slot is held until release() + // below, so an unbounded read here lets a peer hold it forever. + info, err := exchangeNodeInfo(handshakeCtx, hConn, *r.nodeInfoProducer()) if err != nil { return fmt.Errorf("exchangeNodeInfo(): %w", err) } From a804a3056b4f931b26065571a42c8086d861de42 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 13 Aug 2026 08:07:41 -0700 Subject: [PATCH 2/3] test(p2p): pin that the inbound node-info exchange is bounded 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 --- .../internal/p2p/handshake_deadline_test.go | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 sei-tendermint/internal/p2p/handshake_deadline_test.go diff --git a/sei-tendermint/internal/p2p/handshake_deadline_test.go b/sei-tendermint/internal/p2p/handshake_deadline_test.go new file mode 100644 index 0000000000..2ed1e5ab18 --- /dev/null +++ b/sei-tendermint/internal/p2p/handshake_deadline_test.go @@ -0,0 +1,81 @@ +package p2p + +import ( + "context" + "errors" + "io" + "testing" + "time" + + dbm "github.com/tendermint/tm-db" + "golang.org/x/time/rate" + + "github.com/sei-protocol/sei-chain/sei-tendermint/crypto/ed25519" + "github.com/sei-protocol/sei-chain/sei-tendermint/internal/p2p/conn" + "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils" + "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/require" + "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/scope" + "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/tcp" + "github.com/sei-protocol/sei-chain/sei-tendermint/types" +) + +// acceptPeersRoutine holds an accept-semaphore slot until the peer's node info +// has been exchanged, so every read before that point has to be covered by the +// handshake deadline. A peer that completes the handshake and then stops +// responding must be hung up on rather than left holding the slot. +func TestRouter_InboundNodeInfoBoundedByHandshakeDeadline(t *testing.T) { + ctx := t.Context() + + privKey := NodeSecretKey(ed25519.GenerateSecretKey()) + endpoint := Endpoint{AddrPort: tcp.TestReserveAddr()} + nodeInfo := types.NodeInfo{ + NodeID: privKey.Public().NodeID(), + ListenAddr: endpoint.String(), + Moniker: string(privKey.Public().NodeID()), + Network: "test", + } + router, err := NewRouter( + privKey, + func() *types.NodeInfo { return &nodeInfo }, + dbm.NewMemDB(), + &RouterOptions{ + Endpoint: endpoint, + Connection: conn.DefaultMConnConfig(), + IncomingConnectionWindow: utils.Some[time.Duration](0), + MaxAcceptRate: utils.Some(rate.Inf), + // Short, so the test fails fast rather than waiting out the 10s default. + HandshakeTimeout: utils.Some(100 * time.Millisecond), + }, + ) + require.NoError(t, err) + require.NoError(t, router.Start(ctx)) + require.NoError(t, router.WaitForStart(ctx)) + t.Cleanup(router.Stop) + + err = scope.Run(ctx, func(ctx context.Context, s scope.Scope) error { + tcpConn, err := tcp.Dial(ctx, endpoint.AddrPort) + if err != nil { + return err + } + s.SpawnBg(func() error { return tcpConn.Run(ctx) }) + + // Complete the handshake, which authenticates us, then send no node info. + if _, err := handshake(ctx, tcpConn, NodeSecretKey(ed25519.GenerateSecretKey()), handshakeSpec{}); err != nil { + return err + } + + // Keep a read outstanding so the connection pump observes the close. Without + // the deadline covering exchangeNodeInfo the router never hangs up and this + // never returns; the go test timeout is the backstop. + buf := make([]byte, 1) + for { + if err := tcpConn.Read(ctx, buf); err != nil { + return nil + } + } + }) + // The router hanging up on us surfaces as EOF from the connection pump. + if !errors.Is(err, io.EOF) { + t.Fatalf("want the router to hang up at the handshake deadline, got %v", err) + } +} From 4338572157ca3b0470cc7c9c76ae7d9b6262f23d Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 13 Aug 2026 08:21:09 -0700 Subject: [PATCH 3/3] address review: reframe the comment, tidy the test 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 --- .../internal/p2p/handshake_deadline_test.go | 17 +++++++---------- sei-tendermint/internal/p2p/router.go | 5 +++-- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/sei-tendermint/internal/p2p/handshake_deadline_test.go b/sei-tendermint/internal/p2p/handshake_deadline_test.go index 2ed1e5ab18..771975dd47 100644 --- a/sei-tendermint/internal/p2p/handshake_deadline_test.go +++ b/sei-tendermint/internal/p2p/handshake_deadline_test.go @@ -2,7 +2,6 @@ package p2p import ( "context" - "errors" "io" "testing" "time" @@ -19,10 +18,9 @@ import ( "github.com/sei-protocol/sei-chain/sei-tendermint/types" ) -// acceptPeersRoutine holds an accept-semaphore slot until the peer's node info -// has been exchanged, so every read before that point has to be covered by the -// handshake deadline. A peer that completes the handshake and then stops -// responding must be hung up on rather than left holding the slot. +// The node info exchange is part of the handshake and holds an accept-semaphore +// slot, so it has to run under the handshake deadline. A peer that goes quiet +// part way through must be hung up on rather than left holding the slot. func TestRouter_InboundNodeInfoBoundedByHandshakeDeadline(t *testing.T) { ctx := t.Context() @@ -68,14 +66,13 @@ func TestRouter_InboundNodeInfoBoundedByHandshakeDeadline(t *testing.T) { // the deadline covering exchangeNodeInfo the router never hangs up and this // never returns; the go test timeout is the backstop. buf := make([]byte, 1) - for { + for ctx.Err() == nil { if err := tcpConn.Read(ctx, buf); err != nil { - return nil + break } } + return nil }) // The router hanging up on us surfaces as EOF from the connection pump. - if !errors.Is(err, io.EOF) { - t.Fatalf("want the router to hang up at the handshake deadline, got %v", err) - } + require.ErrorIs(t, err, io.EOF) } diff --git a/sei-tendermint/internal/p2p/router.go b/sei-tendermint/internal/p2p/router.go index 0411e7654b..4b62fe8827 100644 --- a/sei-tendermint/internal/p2p/router.go +++ b/sei-tendermint/internal/p2p/router.go @@ -242,8 +242,9 @@ func (r *Router) acceptPeersRoutine(ctx context.Context) error { release() return giga.RunInboundConn(ctx, hConn) } - // handshakeCtx, not ctx: the accept slot is held until release() - // below, so an unbounded read here lets a peer hold it forever. + // The node info exchange is part of the handshake, so it runs under + // the same deadline. Without it a peer that loses connectivity + // mid-exchange holds an accept slot for as long as its socket lives. info, err := exchangeNodeInfo(handshakeCtx, hConn, *r.nodeInfoProducer()) if err != nil { return fmt.Errorf("exchangeNodeInfo(): %w", err)