From c2851775768a5aa058b7f6cc56e98d9baac1c9a9 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Wed, 26 Aug 2026 17:15:03 +0200 Subject: [PATCH 1/4] cl/sentinel: one status handshake per peer, and honour the ban on inbound 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 --- cl/sentinel/discovery.go | 13 +++++ cl/sentinel/handshake_gate.go | 61 +++++++++++++++++++++++ cl/sentinel/handshake_gate_test.go | 79 ++++++++++++++++++++++++++++++ cl/sentinel/sentinel.go | 16 +++--- 4 files changed, 162 insertions(+), 7 deletions(-) create mode 100644 cl/sentinel/handshake_gate.go create mode 100644 cl/sentinel/handshake_gate_test.go diff --git a/cl/sentinel/discovery.go b/cl/sentinel/discovery.go index 1035cfc6837..1706f503f67 100644 --- a/cl/sentinel/discovery.go +++ b/cl/sentinel/discovery.go @@ -557,6 +557,19 @@ func (s *Sentinel) onConnection(_ network.Network, conn network.Conn) { go func() { peerId := conn.RemotePeer() + // ConnectWithPeer refuses banned peers, but it only covers dials we initiate; + // connection events reach here either way. + if s.peers.BanStatus(peerId) { + s.closePeer(peerId) + return + } + // One handshake per peer at a time: a burst of events would otherwise start one + // each, all completing before the failure count could reach the ban threshold. + if !s.handshakeGate.tryAcquire(peerId) { + return + } + defer s.handshakeGate.release(peerId) + // Check if this peer helps any underserved subnets (< minimumPeersPerSubnet) peerHelpsSubnets := false if nodeVal, ok := s.pidToEnr.Load(peerId); ok { diff --git a/cl/sentinel/handshake_gate.go b/cl/sentinel/handshake_gate.go new file mode 100644 index 00000000000..1addcce2953 --- /dev/null +++ b/cl/sentinel/handshake_gate.go @@ -0,0 +1,61 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package sentinel + +import ( + "sync" + + "github.com/libp2p/go-libp2p/core/peer" +) + +// handshakeGate admits one status handshake per peer at a time. Connection events are +// handled on their own goroutine, so without this a burst of events for one peer starts a +// handshake for each, and the three-strike ban cannot intervene because every attempt +// completes before any of them raises the failure count to its threshold. +type handshakeGate struct { + mu sync.Mutex + inflight map[peer.ID]struct{} +} + +func newHandshakeGate() *handshakeGate { + return &handshakeGate{inflight: make(map[peer.ID]struct{})} +} + +// tryAcquire reports whether the caller may handshake this peer. A false return means +// another attempt is already running and this event should be dropped, not queued: the +// peer is about to be judged by the attempt already in flight. +func (g *handshakeGate) tryAcquire(pid peer.ID) bool { + g.mu.Lock() + defer g.mu.Unlock() + if _, busy := g.inflight[pid]; busy { + return false + } + g.inflight[pid] = struct{}{} + return true +} + +func (g *handshakeGate) release(pid peer.ID) { + g.mu.Lock() + defer g.mu.Unlock() + delete(g.inflight, pid) +} + +func (g *handshakeGate) inFlight() int { + g.mu.Lock() + defer g.mu.Unlock() + return len(g.inflight) +} diff --git a/cl/sentinel/handshake_gate_test.go b/cl/sentinel/handshake_gate_test.go new file mode 100644 index 00000000000..21586c9fa11 --- /dev/null +++ b/cl/sentinel/handshake_gate_test.go @@ -0,0 +1,79 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package sentinel + +import ( + "sync" + "sync/atomic" + "testing" + + "github.com/libp2p/go-libp2p/core/peer" + "github.com/stretchr/testify/require" +) + +// Every connection event is handled on its own goroutine, so a burst of events for one +// peer produced a burst of concurrent handshakes. The three-strike ban could not stop it: +// all of them completed before any pushed the counter to 3. Measured on a gnosis node at +// 101 attempts against a single peer in two minutes, until libp2p's own dialer refused. +func TestHandshakeGateAdmitsOneAttemptPerPeerAtATime(t *testing.T) { + g := newHandshakeGate() + pid := peer.ID("peer-a") + + require.True(t, g.tryAcquire(pid)) + require.False(t, g.tryAcquire(pid), "a second concurrent attempt on the same peer must be refused") + + g.release(pid) + require.True(t, g.tryAcquire(pid), "the peer is attemptable again once the first finishes") +} + +// Serialising per peer must not serialise the whole node: unrelated peers still proceed. +func TestHandshakeGateDoesNotBlockOtherPeers(t *testing.T) { + g := newHandshakeGate() + + require.True(t, g.tryAcquire(peer.ID("peer-a"))) + require.True(t, g.tryAcquire(peer.ID("peer-b"))) + require.True(t, g.tryAcquire(peer.ID("peer-c"))) +} + +// The burst is the thing being fixed, so drive it concurrently rather than in sequence. +func TestHandshakeGateUnderConcurrentBurst(t *testing.T) { + g := newHandshakeGate() + pid := peer.ID("peer-a") + + var admitted atomic.Int64 + var wg sync.WaitGroup + for range 64 { + wg.Go(func() { + if g.tryAcquire(pid) { + admitted.Add(1) + } + }) + } + wg.Wait() + + require.Equal(t, int64(1), admitted.Load(), "64 simultaneous events admitted %d attempts", admitted.Load()) +} + +func TestHandshakeGateForgetsReleasedPeers(t *testing.T) { + g := newHandshakeGate() + pid := peer.ID("peer-a") + + require.True(t, g.tryAcquire(pid)) + g.release(pid) + + require.Zero(t, g.inFlight(), "a released peer must not be retained") +} diff --git a/cl/sentinel/sentinel.go b/cl/sentinel/sentinel.go index a844e405e76..5274a5c098e 100644 --- a/cl/sentinel/sentinel.go +++ b/cl/sentinel/sentinel.go @@ -53,13 +53,14 @@ import ( ) type Sentinel struct { - started bool - listener *discover.UDPv5 // this is us in the network. - ctx context.Context - cancel context.CancelFunc - cfg *SentinelConfig - peers *peers.Pool - p2p p2p.P2PManager + started bool + listener *discover.UDPv5 // this is us in the network. + ctx context.Context + cancel context.CancelFunc + cfg *SentinelConfig + peers *peers.Pool + handshakeGate *handshakeGate + p2p p2p.P2PManager httpApi http.Handler @@ -143,6 +144,7 @@ func New( signal.Reset(syscall.SIGINT) s.peers = peers.NewPool(s.p2p.Host()) + s.handshakeGate = newHandshakeGate() mux := chi.NewRouter() mux.Get("/", httpreqresp.NewRequestHandler(s.p2p.Host())) From 69196c7302161a1db175e217d13e24460b3ec1c5 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Thu, 27 Aug 2026 10:30:18 +0200 Subject: [PATCH 2/4] cl/sentinel: read the ban inside the serialized handshake section 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. --- cl/sentinel/discovery.go | 87 ++++++++++--------- cl/sentinel/handshake_sequence_test.go | 111 +++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 37 deletions(-) create mode 100644 cl/sentinel/handshake_sequence_test.go diff --git a/cl/sentinel/discovery.go b/cl/sentinel/discovery.go index 1706f503f67..37b2f2e6412 100644 --- a/cl/sentinel/discovery.go +++ b/cl/sentinel/discovery.go @@ -557,19 +557,6 @@ func (s *Sentinel) onConnection(_ network.Network, conn network.Conn) { go func() { peerId := conn.RemotePeer() - // ConnectWithPeer refuses banned peers, but it only covers dials we initiate; - // connection events reach here either way. - if s.peers.BanStatus(peerId) { - s.closePeer(peerId) - return - } - // One handshake per peer at a time: a burst of events would otherwise start one - // each, all completing before the failure count could reach the ban threshold. - if !s.handshakeGate.tryAcquire(peerId) { - return - } - defer s.handshakeGate.release(peerId) - // Check if this peer helps any underserved subnets (< minimumPeersPerSubnet) peerHelpsSubnets := false if nodeVal, ok := s.pidToEnr.Load(peerId); ok { @@ -596,31 +583,57 @@ func (s *Sentinel) onConnection(_ network.Network, conn network.Conn) { return } - valid, err := s.handshaker.ValidatePeer(s.ctx, peerId) - if err != nil { - // Handshake transport error (stream reset, timeout, etc.) — keep the peer. - // The peer may still work for gossip even if status exchange failed. - log.Trace("[Sentinel] Handshake transport error (keeping connection)", "peer", peerId, "err", err) - } + s.exchangeStatus(peerId, + func() (bool, error) { return s.handshaker.ValidatePeer(s.ctx, peerId) }, + s.closePeer, + func(id peer.ID) { + s.p2p.Host().Peerstore().RemovePeer(id) + s.closePeer(id) + s.peers.RemovePeer(id) + }) + }() +} - if !valid && err == nil { - // Handshake succeeded but fork digest mismatched — peer is on a different fork. - // Must disconnect to avoid receiving incompatible blocks. - log.Debug("[Sentinel] Fork mismatch, disconnecting peer", "peer", peerId) - s.p2p.Host().Peerstore().RemovePeer(peerId) - s.closePeer(peerId) - s.peers.RemovePeer(peerId) - return - } +// exchangeStatus runs one status handshake for peerId and reports whether the peer was kept. +// +// Only one handshake per peer runs at a time: a burst of connection events would otherwise +// start one each, all completing before the failure count could reach the pool's ban +// threshold. The ban is read inside that serialized section, because a handshake that +// completed while this event waited may have installed it — and ConnectWithPeer only +// refuses banned peers on dials we initiate, not on events that arrive anyway. +func (s *Sentinel) exchangeStatus(peerId peer.ID, validate func() (bool, error), closePeer, dropPeer func(peer.ID)) bool { + if !s.handshakeGate.tryAcquire(peerId) { + return false + } + defer s.handshakeGate.release(peerId) - if !valid { - // Handshake had a transport error AND returned invalid — keep anyway. - s.peers.RecordHandshakeFailure(peerId) - } else { - // we were able to successfully connect, so add this peer to our pool - s.peers.AddPeer(peerId) + if s.peers.BanStatus(peerId) { + closePeer(peerId) + return false + } - log.Trace("[Sentinel] Peer validated and added", "peer", peerId) - } - }() + valid, err := validate() + if err != nil { + // Handshake transport error (stream reset, timeout, etc.) — keep the peer. + // The peer may still work for gossip even if status exchange failed. + log.Trace("[Sentinel] Handshake transport error (keeping connection)", "peer", peerId, "err", err) + } + + if !valid && err == nil { + // Handshake succeeded but fork digest mismatched — peer is on a different fork. + // Must disconnect to avoid receiving incompatible blocks. + log.Debug("[Sentinel] Fork mismatch, disconnecting peer", "peer", peerId) + dropPeer(peerId) + return false + } + + if !valid { + // Handshake had a transport error AND returned invalid — keep anyway. + s.peers.RecordHandshakeFailure(peerId) + return true + } + // we were able to successfully connect, so add this peer to our pool + s.peers.AddPeer(peerId) + log.Trace("[Sentinel] Peer validated and added", "peer", peerId) + return true } diff --git a/cl/sentinel/handshake_sequence_test.go b/cl/sentinel/handshake_sequence_test.go new file mode 100644 index 00000000000..2f438a4aca9 --- /dev/null +++ b/cl/sentinel/handshake_sequence_test.go @@ -0,0 +1,111 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package sentinel + +import ( + "errors" + "testing" + + "github.com/libp2p/go-libp2p/core/peer" + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/cl/sentinel/peers" +) + +func testSentinel() *Sentinel { + // The ban bookkeeping this exercises never touches the host. + return &Sentinel{peers: peers.NewPool(nil), handshakeGate: newHandshakeGate()} +} + +func noop(peer.ID) {} + +// The pool bans a peer after three handshake failures. Every connection event must run +// through the same serialized path, so the third failure is reached and the ban is applied +// to the next event instead of another handshake being started. +func TestExchangeStatusReachesTheBanAndThenRefusesThePeer(t *testing.T) { + s := testSentinel() + pid := peer.ID("peer-a") + + validated, closed, dropped := 0, 0, 0 + validate := func() (bool, error) { + validated++ + return false, errors.New("stream reset") + } + onClose := func(peer.ID) { closed++ } + onDrop := func(peer.ID) { dropped++ } + + for range 3 { + require.True(t, s.exchangeStatus(pid, validate, onClose, onDrop), + "a transport error keeps the peer: it may still serve gossip") + } + require.Equal(t, 3, validated) + require.True(t, s.peers.BanStatus(pid), "three handshake failures must ban the peer") + + require.False(t, s.exchangeStatus(pid, validate, onClose, onDrop)) + require.Equal(t, 3, validated, "a banned peer must not be handshaked again") + require.Equal(t, 1, closed) + require.Zero(t, dropped) + require.Zero(t, s.handshakeGate.inFlight(), "the gate must be released on the banned exit") +} + +// A ban installed by a handshake that completed while this event was waiting must be seen: +// the ban is read inside the serialized section, not before it. +func TestExchangeStatusRefusesAPeerBannedWhileTheEventWaited(t *testing.T) { + s := testSentinel() + pid := peer.ID("peer-a") + + s.peers.SetBanStatus(pid, true) + closed := 0 + require.False(t, s.exchangeStatus(pid, func() (bool, error) { + t.Fatal("a banned peer must not be handshaked") + return false, nil + }, func(peer.ID) { closed++ }, noop)) + require.Equal(t, 1, closed) +} + +// Serializing per peer is the point: while one handshake is in flight, a second event for +// the same peer must not start another. +func TestExchangeStatusAdmitsOneHandshakePerPeerAtATime(t *testing.T) { + s := testSentinel() + pid := peer.ID("peer-a") + + outer := 0 + require.True(t, s.exchangeStatus(pid, func() (bool, error) { + outer++ + require.False(t, s.exchangeStatus(pid, func() (bool, error) { + t.Fatal("a second handshake started while one was in flight") + return false, nil + }, noop, noop), "the concurrent event must be refused") + return true, nil + }, noop, noop)) + require.Equal(t, 1, outer) + require.Zero(t, s.handshakeGate.inFlight()) +} + +// A completed handshake reporting the wrong fork is not a failure to retry: the peer is +// dropped outright and must not count toward the ban. +func TestExchangeStatusDropsAForkMismatchWithoutRecordingAFailure(t *testing.T) { + s := testSentinel() + pid := peer.ID("peer-a") + + dropped := 0 + require.False(t, s.exchangeStatus(pid, func() (bool, error) { return false, nil }, + noop, func(peer.ID) { dropped++ })) + require.Equal(t, 1, dropped) + require.False(t, s.peers.BanStatus(pid)) + require.Zero(t, s.handshakeGate.inFlight(), "the gate must be released on the fork-mismatch exit") +} From f116f2dc6b436c241cff32400c606f2e98bc76f7 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Thu, 27 Aug 2026 13:51:42 +0200 Subject: [PATCH 3/4] cl/sentinel: pin the ban-after-admission ordering with a sequence test The previous tests passed with BanStatus moved back above tryAcquire, so they did not pin the fix at all. They only observed outcomes, and the outcome is the same either way when admission is non-blocking. Injects the ban reader into exchangeStatus, so a test can assert the peer's handshake slot is already held at the moment the ban is read - which is false if the read moves back above admission. Splits onConnection's body into handleNewConnection so the handler itself is reachable without a live handshaker. The sequence test pauses one handshake mid-flight, submits a second event for the same peer and one for an unrelated peer, releases, drives two more failures to reach the pool ban, and asserts the next event closes the peer without handshaking it. Violations are counted rather than asserted inside the handshake goroutine: a failed require there would Goexit and turn a regression into a ten-minute timeout instead of a named failure. Verified both regressions fail fast: reading the ban before admission, and deleting the handleNewConnection wiring. --- cl/sentinel/discovery.go | 37 ++++-- cl/sentinel/handshake_gate.go | 8 ++ cl/sentinel/handshake_sequence_test.go | 175 ++++++++++++++++++------- cl/sentinel/testhost_test.go | 47 +++++++ 4 files changed, 202 insertions(+), 65 deletions(-) create mode 100644 cl/sentinel/testhost_test.go diff --git a/cl/sentinel/discovery.go b/cl/sentinel/discovery.go index 37b2f2e6412..dbce481c929 100644 --- a/cl/sentinel/discovery.go +++ b/cl/sentinel/discovery.go @@ -554,9 +554,17 @@ func (s *Sentinel) listenForPeers() { } func (s *Sentinel) onConnection(_ network.Network, conn network.Conn) { - go func() { - peerId := conn.RemotePeer() + peerId := conn.RemotePeer() + go s.handleNewConnection(peerId, func() (bool, error) { + return s.handshaker.ValidatePeer(s.ctx, peerId) + }) +} +// handleNewConnection admits or rejects a peer that has just connected, then runs its status +// handshake. validate is injected so the sequence can be driven without a live handshaker. +// Reports whether the peer was kept. +func (s *Sentinel) handleNewConnection(peerId peer.ID, validate func() (bool, error)) bool { + { // Check if this peer helps any underserved subnets (< minimumPeersPerSubnet) peerHelpsSubnets := false if nodeVal, ok := s.pidToEnr.Load(peerId); ok { @@ -580,18 +588,17 @@ func (s *Sentinel) onConnection(_ network.Network, conn network.Conn) { s.p2p.Host().Peerstore().RemovePeer(peerId) s.closePeer(peerId) s.peers.RemovePeer(peerId) - return + return false } + } - s.exchangeStatus(peerId, - func() (bool, error) { return s.handshaker.ValidatePeer(s.ctx, peerId) }, - s.closePeer, - func(id peer.ID) { - s.p2p.Host().Peerstore().RemovePeer(id) - s.closePeer(id) - s.peers.RemovePeer(id) - }) - }() + return s.exchangeStatus(peerId, s.peers.BanStatus, validate, + s.closePeer, + func(id peer.ID) { + s.p2p.Host().Peerstore().RemovePeer(id) + s.closePeer(id) + s.peers.RemovePeer(id) + }) } // exchangeStatus runs one status handshake for peerId and reports whether the peer was kept. @@ -601,13 +608,15 @@ func (s *Sentinel) onConnection(_ network.Network, conn network.Conn) { // threshold. The ban is read inside that serialized section, because a handshake that // completed while this event waited may have installed it — and ConnectWithPeer only // refuses banned peers on dials we initiate, not on events that arrive anyway. -func (s *Sentinel) exchangeStatus(peerId peer.ID, validate func() (bool, error), closePeer, dropPeer func(peer.ID)) bool { +// banned is injected so a test can assert that the gate is already held when the ban is read; +// reading it before admission is the bug this exists to prevent. +func (s *Sentinel) exchangeStatus(peerId peer.ID, banned func(peer.ID) bool, validate func() (bool, error), closePeer, dropPeer func(peer.ID)) bool { if !s.handshakeGate.tryAcquire(peerId) { return false } defer s.handshakeGate.release(peerId) - if s.peers.BanStatus(peerId) { + if banned(peerId) { closePeer(peerId) return false } diff --git a/cl/sentinel/handshake_gate.go b/cl/sentinel/handshake_gate.go index 1addcce2953..7ab833fe151 100644 --- a/cl/sentinel/handshake_gate.go +++ b/cl/sentinel/handshake_gate.go @@ -59,3 +59,11 @@ func (g *handshakeGate) inFlight() int { defer g.mu.Unlock() return len(g.inflight) } + +// holds reports whether a handshake slot for pid is currently taken. +func (g *handshakeGate) holds(pid peer.ID) bool { + g.mu.Lock() + defer g.mu.Unlock() + _, busy := g.inflight[pid] + return busy +} diff --git a/cl/sentinel/handshake_sequence_test.go b/cl/sentinel/handshake_sequence_test.go index 2f438a4aca9..a40858ebad2 100644 --- a/cl/sentinel/handshake_sequence_test.go +++ b/cl/sentinel/handshake_sequence_test.go @@ -18,11 +18,15 @@ package sentinel import ( "errors" + "sync" + "sync/atomic" "testing" + "time" "github.com/libp2p/go-libp2p/core/peer" "github.com/stretchr/testify/require" + "github.com/erigontech/erigon/cl/p2p" "github.com/erigontech/erigon/cl/sentinel/peers" ) @@ -33,79 +37,148 @@ func testSentinel() *Sentinel { func noop(peer.ID) {} -// The pool bans a peer after three handshake failures. Every connection event must run -// through the same serialized path, so the third failure is reached and the ban is applied -// to the next event instead of another handshake being started. -func TestExchangeStatusReachesTheBanAndThenRefusesThePeer(t *testing.T) { - s := testSentinel() - pid := peer.ID("peer-a") +// banObserver wraps the pool's BanStatus and counts every read that happens without this +// peer's handshake slot being held. Reading the ban before admission is the defect under +// test, and it is invisible to a test that only checks outcomes. Violations are counted +// rather than asserted on the spot: this runs on handshake goroutines, where a failed +// require would Goexit and deadlock the test instead of reporting. +type banObserver struct { + s *Sentinel + violations atomic.Int64 +} - validated, closed, dropped := 0, 0, 0 - validate := func() (bool, error) { - validated++ - return false, errors.New("stream reset") +func (b *banObserver) read(pid peer.ID) bool { + if !b.s.handshakeGate.holds(pid) { + b.violations.Add(1) } - onClose := func(peer.ID) { closed++ } - onDrop := func(peer.ID) { dropped++ } + return b.s.peers.BanStatus(pid) +} + +func (b *banObserver) assertAlwaysHeld(t *testing.T) { + t.Helper() + require.Zero(t, b.violations.Load(), + "the ban must be read while this peer's handshake slot is held, not before admission") +} - for range 3 { - require.True(t, s.exchangeStatus(pid, validate, onClose, onDrop), - "a transport error keeps the peer: it may still serve gossip") +func waitForInFlight(t *testing.T, s *Sentinel, want int) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for s.handshakeGate.inFlight() != want { + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for %d handshake(s) in flight, have %d", want, s.handshakeGate.inFlight()) + } + time.Sleep(time.Millisecond) } - require.Equal(t, 3, validated) - require.True(t, s.peers.BanStatus(pid), "three handshake failures must ban the peer") - - require.False(t, s.exchangeStatus(pid, validate, onClose, onDrop)) - require.Equal(t, 3, validated, "a banned peer must not be handshaked again") - require.Equal(t, 1, closed) - require.Zero(t, dropped) - require.Zero(t, s.handshakeGate.inFlight(), "the gate must be released on the banned exit") } -// A ban installed by a handshake that completed while this event was waiting must be seen: -// the ban is read inside the serialized section, not before it. -func TestExchangeStatusRefusesAPeerBannedWhileTheEventWaited(t *testing.T) { +// The full sequence: one handshake paused mid-flight, a second event for the same peer +// arriving while it runs, an unrelated peer proceeding regardless, three failures reaching +// the pool ban, and the next event for the banned peer closing it without a handshake. +func TestHandshakeSequenceSerializesPerPeerAndReachesTheBan(t *testing.T) { s := testSentinel() - pid := peer.ID("peer-a") + obs := &banObserver{s: s} + banned := obs.read + pidA, pidB := peer.ID("peer-a"), peer.ID("peer-b") + + var validatedA, validatedB, closedA int + var mu sync.Mutex + countA := func() { mu.Lock(); validatedA++; mu.Unlock() } + + resume := make(chan struct{}) + entered := make(chan struct{}) + var wg sync.WaitGroup + wg.Go(func() { + // First event for peer A: holds the slot until released. + s.exchangeStatus(pidA, banned, func() (bool, error) { + countA() + close(entered) + <-resume + return false, errors.New("stream reset") + }, noop, noop) + }) + + <-entered + waitForInFlight(t, s, 1) + + // A second event for the same peer, arriving while the first handshake is in flight. + require.False(t, s.exchangeStatus(pidA, banned, func() (bool, error) { + t.Error("a second handshake started for a peer already being handshaked") + return false, nil + }, noop, noop)) + + // An unrelated peer must not be held up by peer A's in-flight handshake. + require.True(t, s.exchangeStatus(pidB, banned, func() (bool, error) { + validatedB++ + return true, nil + }, noop, noop)) + require.Equal(t, 1, validatedB) - s.peers.SetBanStatus(pid, true) - closed := 0 - require.False(t, s.exchangeStatus(pid, func() (bool, error) { - t.Fatal("a banned peer must not be handshaked") + close(resume) + wg.Wait() + + // Two more failures reach the pool's three-strike ban. + for range 2 { + require.True(t, s.exchangeStatus(pidA, banned, func() (bool, error) { + countA() + return false, errors.New("stream reset") + }, noop, noop)) + } + require.Equal(t, 3, validatedA) + require.True(t, s.peers.BanStatus(pidA), "three handshake failures must ban the peer") + + // The next event closes the banned peer instead of handshaking it again. + require.False(t, s.exchangeStatus(pidA, banned, func() (bool, error) { + t.Error("a banned peer must not be handshaked") return false, nil - }, func(peer.ID) { closed++ }, noop)) - require.Equal(t, 1, closed) + }, func(peer.ID) { closedA++ }, noop)) + require.Equal(t, 3, validatedA) + require.Equal(t, 1, closedA) + require.Zero(t, s.handshakeGate.inFlight(), "every exit path must release the slot") + obs.assertAlwaysHeld(t) } -// Serializing per peer is the point: while one handshake is in flight, a second event for -// the same peer must not start another. -func TestExchangeStatusAdmitsOneHandshakePerPeerAtATime(t *testing.T) { +// The connection handler must route through the serialized path: a banned peer entering +// handleNewConnection is closed without its handshake being attempted. +func TestHandleNewConnectionRefusesABannedPeerWithoutHandshaking(t *testing.T) { s := testSentinel() - pid := peer.ID("peer-a") + s.p2p = stubP2P{host: newTestHost(t)} + s.cfg = &SentinelConfig{P2PConfig: p2p.P2PConfig{MaxPeerCount: 100}} + pidA := peer.ID("peer-a") + s.peers.SetBanStatus(pidA, true) - outer := 0 - require.True(t, s.exchangeStatus(pid, func() (bool, error) { - outer++ - require.False(t, s.exchangeStatus(pid, func() (bool, error) { - t.Fatal("a second handshake started while one was in flight") - return false, nil - }, noop, noop), "the concurrent event must be refused") - return true, nil - }, noop, noop)) - require.Equal(t, 1, outer) + require.False(t, s.handleNewConnection(pidA, func() (bool, error) { + t.Error("a banned peer must not be handshaked") + return false, nil + })) require.Zero(t, s.handshakeGate.inFlight()) } -// A completed handshake reporting the wrong fork is not a failure to retry: the peer is -// dropped outright and must not count toward the ban. +// A peer that is not banned reaches its handshake through the handler. +func TestHandleNewConnectionHandshakesAnUnbannedPeer(t *testing.T) { + s := testSentinel() + s.p2p = stubP2P{host: newTestHost(t)} + s.cfg = &SentinelConfig{P2PConfig: p2p.P2PConfig{MaxPeerCount: 100}} + + validated := 0 + require.True(t, s.handleNewConnection(peer.ID("peer-a"), func() (bool, error) { + validated++ + return true, nil + })) + require.Equal(t, 1, validated) +} + +// A completed handshake reporting the wrong fork is dropped and must not count toward the ban. func TestExchangeStatusDropsAForkMismatchWithoutRecordingAFailure(t *testing.T) { s := testSentinel() pid := peer.ID("peer-a") dropped := 0 - require.False(t, s.exchangeStatus(pid, func() (bool, error) { return false, nil }, + obs := &banObserver{s: s} + require.False(t, s.exchangeStatus(pid, obs.read, + func() (bool, error) { return false, nil }, noop, func(peer.ID) { dropped++ })) require.Equal(t, 1, dropped) require.False(t, s.peers.BanStatus(pid)) - require.Zero(t, s.handshakeGate.inFlight(), "the gate must be released on the fork-mismatch exit") + require.Zero(t, s.handshakeGate.inFlight(), "the fork-mismatch exit must release the slot") + obs.assertAlwaysHeld(t) } diff --git a/cl/sentinel/testhost_test.go b/cl/sentinel/testhost_test.go new file mode 100644 index 00000000000..63d3613779b --- /dev/null +++ b/cl/sentinel/testhost_test.go @@ -0,0 +1,47 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package sentinel + +import ( + "testing" + + "github.com/libp2p/go-libp2p" + pubsub "github.com/libp2p/go-libp2p-pubsub" + "github.com/libp2p/go-libp2p/core/host" + "github.com/libp2p/go-libp2p/core/metrics" + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/p2p/discover" +) + +// stubP2P satisfies p2p.P2PManager with only the host the connection handler needs. +type stubP2P struct{ host host.Host } + +func (s stubP2P) Pubsub() *pubsub.PubSub { return nil } +func (s stubP2P) Host() host.Host { return s.host } +func (s stubP2P) BandwidthCounter() *metrics.BandwidthCounter { return nil } +func (s stubP2P) UDPv5Listener() *discover.UDPv5 { return nil } +func (s stubP2P) UpdateENRAttSubnets(subnetIndex int, on bool) {} +func (s stubP2P) UpdateENRSyncNets(subnetIndex int, on bool) {} + +func newTestHost(t *testing.T) host.Host { + t.Helper() + h, err := libp2p.New(libp2p.NoListenAddrs) + require.NoError(t, err) + t.Cleanup(func() { _ = h.Close() }) + return h +} From 46e880f9e23c12db3c6daf8b2c13e52d7eace974 Mon Sep 17 00:00:00 2001 From: Oleksandr Lystopad Date: Thu, 27 Aug 2026 22:16:19 +0200 Subject: [PATCH 4/4] cl/sentinel: trim the new comments and unblock the paused handshake on failure Cuts the production comments to the invariant and drops the incident measurements from the test docstring, per the repository comment policy. The sequence test closed its resume channel only after several assertions, so a regression reaching FailNow first left the paused handshake goroutine parked and the failure surfaced as a timeout. Registers the release as cleanup, which runs after Goexit: a forced early failure now reports in under a second. --- cl/sentinel/discovery.go | 12 ++---------- cl/sentinel/handshake_gate.go | 11 ++++------- cl/sentinel/handshake_gate_test.go | 6 ++---- cl/sentinel/handshake_sequence_test.go | 8 +++++++- 4 files changed, 15 insertions(+), 22 deletions(-) diff --git a/cl/sentinel/discovery.go b/cl/sentinel/discovery.go index dbce481c929..8bb71e14155 100644 --- a/cl/sentinel/discovery.go +++ b/cl/sentinel/discovery.go @@ -561,8 +561,7 @@ func (s *Sentinel) onConnection(_ network.Network, conn network.Conn) { } // handleNewConnection admits or rejects a peer that has just connected, then runs its status -// handshake. validate is injected so the sequence can be driven without a live handshaker. -// Reports whether the peer was kept. +// handshake. Reports whether the peer was kept. func (s *Sentinel) handleNewConnection(peerId peer.ID, validate func() (bool, error)) bool { { // Check if this peer helps any underserved subnets (< minimumPeersPerSubnet) @@ -602,14 +601,7 @@ func (s *Sentinel) handleNewConnection(peerId peer.ID, validate func() (bool, er } // exchangeStatus runs one status handshake for peerId and reports whether the peer was kept. -// -// Only one handshake per peer runs at a time: a burst of connection events would otherwise -// start one each, all completing before the failure count could reach the pool's ban -// threshold. The ban is read inside that serialized section, because a handshake that -// completed while this event waited may have installed it — and ConnectWithPeer only -// refuses banned peers on dials we initiate, not on events that arrive anyway. -// banned is injected so a test can assert that the gate is already held when the ban is read; -// reading it before admission is the bug this exists to prevent. +// The ban must be read while the slot is held: a handshake completing meanwhile may install it. func (s *Sentinel) exchangeStatus(peerId peer.ID, banned func(peer.ID) bool, validate func() (bool, error), closePeer, dropPeer func(peer.ID)) bool { if !s.handshakeGate.tryAcquire(peerId) { return false diff --git a/cl/sentinel/handshake_gate.go b/cl/sentinel/handshake_gate.go index 7ab833fe151..56c2a8d489f 100644 --- a/cl/sentinel/handshake_gate.go +++ b/cl/sentinel/handshake_gate.go @@ -22,10 +22,8 @@ import ( "github.com/libp2p/go-libp2p/core/peer" ) -// handshakeGate admits one status handshake per peer at a time. Connection events are -// handled on their own goroutine, so without this a burst of events for one peer starts a -// handshake for each, and the three-strike ban cannot intervene because every attempt -// completes before any of them raises the failure count to its threshold. +// handshakeGate admits one status handshake per peer at a time, so a burst of connection +// events cannot outrun the pool's failure count before it reaches the ban threshold. type handshakeGate struct { mu sync.Mutex inflight map[peer.ID]struct{} @@ -35,9 +33,8 @@ func newHandshakeGate() *handshakeGate { return &handshakeGate{inflight: make(map[peer.ID]struct{})} } -// tryAcquire reports whether the caller may handshake this peer. A false return means -// another attempt is already running and this event should be dropped, not queued: the -// peer is about to be judged by the attempt already in flight. +// tryAcquire reports whether the caller may handshake this peer. A false return means the +// event should be dropped, not queued: the attempt in flight will judge the peer. func (g *handshakeGate) tryAcquire(pid peer.ID) bool { g.mu.Lock() defer g.mu.Unlock() diff --git a/cl/sentinel/handshake_gate_test.go b/cl/sentinel/handshake_gate_test.go index 21586c9fa11..f4c0b85addd 100644 --- a/cl/sentinel/handshake_gate_test.go +++ b/cl/sentinel/handshake_gate_test.go @@ -25,10 +25,8 @@ import ( "github.com/stretchr/testify/require" ) -// Every connection event is handled on its own goroutine, so a burst of events for one -// peer produced a burst of concurrent handshakes. The three-strike ban could not stop it: -// all of them completed before any pushed the counter to 3. Measured on a gnosis node at -// 101 attempts against a single peer in two minutes, until libp2p's own dialer refused. +// A burst of events for one peer must not become a burst of concurrent handshakes: the +// three-strike ban cannot stop them if they all complete before the counter reaches 3. func TestHandshakeGateAdmitsOneAttemptPerPeerAtATime(t *testing.T) { g := newHandshakeGate() pid := peer.ID("peer-a") diff --git a/cl/sentinel/handshake_sequence_test.go b/cl/sentinel/handshake_sequence_test.go index a40858ebad2..530e0bc9c3b 100644 --- a/cl/sentinel/handshake_sequence_test.go +++ b/cl/sentinel/handshake_sequence_test.go @@ -86,6 +86,12 @@ func TestHandshakeSequenceSerializesPerPeerAndReachesTheBan(t *testing.T) { resume := make(chan struct{}) entered := make(chan struct{}) + // Unblock the paused handshake unconditionally: an assertion below reaching FailNow + // before the explicit close would otherwise leave its goroutine parked forever. + var once sync.Once + release := func() { once.Do(func() { close(resume) }) } + t.Cleanup(release) + var wg sync.WaitGroup wg.Go(func() { // First event for peer A: holds the slot until released. @@ -113,7 +119,7 @@ func TestHandshakeSequenceSerializesPerPeerAndReachesTheBan(t *testing.T) { }, noop, noop)) require.Equal(t, 1, validatedB) - close(resume) + release() wg.Wait() // Two more failures reach the pool's three-strike ban.