Skip to content

fix(p2p): reclaim connTracker lastConnect entries once their window elapses #3918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bdchatham wants to merge 1 commit into
base: main
Choose a base branch
from
+50 −0
Open

fix(p2p): reclaim connTracker lastConnect entries once their window elapses #3918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions sei-tendermint/internal/p2p/conn_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ type connTracker struct {
mutex sync.RWMutex
max uint
window time.Duration
nextSweep time.Time
}

func newConnTracker(max uint, window time.Duration) *connTracker {
Expand All @@ -30,10 +31,29 @@ func (rat *connTracker) Len() int {
return len(rat.cache)
}

// sweepLocked drops lastConnect entries whose window has elapsed.
func (rat *connTracker) sweepLocked(now time.Time) {
// At most once per window. An entry is only consulted while it is inside the
// window, so anything older is dead weight. RemoveConn drops an entry itself
// when the connection outlived the window, which leaves exactly the addresses
// whose connections died inside it: without this sweep those stay for the life
// of the process, and on a public listener that set is unbounded.
if now.Before(rat.nextSweep) {
return
}
rat.nextSweep = now.Add(rat.window)
for address, last := range rat.lastConnect {
if now.Sub(last) > rat.window {
delete(rat.lastConnect, address)
}
}
}

func (rat *connTracker) AddConn(addrPort netip.AddrPort) error {
address := addrPort.Addr()
rat.mutex.Lock()
defer rat.mutex.Unlock()
rat.sweepLocked(time.Now())

if num := rat.cache[address]; num >= rat.max {
return fmt.Errorf("%q has %d connections [max=%d]", address, num, rat.max)
Expand All @@ -56,6 +76,7 @@ func (rat *connTracker) RemoveConn(addrPort netip.AddrPort) {
address := addrPort.Addr()
rat.mutex.Lock()
defer rat.mutex.Unlock()
rat.sweepLocked(time.Now())

if num := rat.cache[address]; num > 0 {
rat.cache[address]--
Expand Down
29 changes: 29 additions & 0 deletions sei-tendermint/internal/p2p/conn_tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,32 @@ func TestConnTracker(t *testing.T) {
})

}

// A connection that dies inside the window keeps its lastConnect entry, because
// the window has not elapsed and a reconnect still has to be refused. Nothing
// revisited those entries afterwards, so on a public listener every address whose
// connection was short-lived stayed in the map for the life of the process.
func TestConnTrackerShortLivedConnsDoNotAccumulate(t *testing.T) {
const conns = 100_000

ct := newConnTracker(10, time.Millisecond)
for range conns {
ip := randLocalAddr()
require.NoError(t, ct.AddConn(ip))
ct.RemoveConn(ip)
}

// Bounded by the addresses seen within one window rather than by every address
// seen. The margin is wide because the sweep is driven by elapsed time.
require.Less(t, len(ct.lastConnect), conns/10)

@seidroid seidroid Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This bound is a function of loop throughput rather than of the sweep: the surviving entries are those added since the last sweep, i.e. roughly window / per-iteration cost. At an estimated ~0.5–1µs per iteration that lands around 1–3k, comfortably under 10k, and -race in CI only widens the margin — but the relationship is inverted from what you want (a faster machine retains more), so the headroom shrinks precisely where the test is cheapest to run.

You can make it deterministic and much stronger at the same time by forcing the final sweep instead of sampling mid-stream: after the loop, time.Sleep(2 * time.Millisecond) then AddConn/RemoveConn one fresh address. That call sweeps (nextSweep has certainly elapsed) and every loop entry is now older than the window, so require.Len(t, ct.lastConnect, 1) holds exactly, and it still fails without the sweep.

}

// Reclaiming entries must not let an address reconnect inside its window.
func TestConnTrackerSweepPreservesWindow(t *testing.T) {
ct := newConnTracker(10, time.Hour)

@seidroid seidroid Bot Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] With window = time.Hour this test never runs the sweep against a populated map, so it does not guard what its comment claims. Trace it: the first AddConn sweeps an empty map and sets nextSweep = now + 1h; the following RemoveConn and AddConn both hit now.Before(rat.nextSweep) and return early. Replacing the loop body in sweepLocked with an unconditional delete(rat.lastConnect, address) still leaves this test passing (as it does TestConnTracker/Window and /VeryShort), so the now.Sub(last) > rat.window guard is currently untested.

The case that exercises it needs an entry created after the last sweep but still inside its window when the next sweep fires — e.g. with a short window, prime nextSweep with a throwaway address, sleep until just before it elapses, AddConn/RemoveConn the address under test, then sleep past nextSweep and make one more call so the sweep runs while that entry is only a fraction of a window old, and assert the reconnect is still refused.

ip := randLocalAddr()

require.NoError(t, ct.AddConn(ip))
ct.RemoveConn(ip)
require.Error(t, ct.AddConn(ip))
}
Loading