[MegaRedHand]

🗒️ Description / Motivation

A heap profile of our mainnet follower (2.8 GB steady RSS) showed about 490 MB of gossipsub message copies that never went away: they sat in the send queues of a few peers whose gossipsub connection handler had disabled itself.

The mechanism, in the pinned rust-libp2p (same in upstream master):

• The handler counts every inbound and outbound substream event, successful ones included, and after MAX_SUBSTREAM_ATTEMPTS sets itself to Disabled(MaxSubstreamAttempts). In that state poll is Pending forever, so the send queue is never read again.
• The behaviour is not told. It keeps queueing forwards for the peer until the per-peer queue is full.
• A queued message is only dropped when the handler takes it off the queue, so the copies stay in memory until the connection closes. Other protocols keep using that connection, so nothing closes it.

Gossipsub does report such a peer: Event::SlowPeer every heartbeat. We dropped that event. Gossipsub's own score penalty for it (slow_peer_weight) left the peer in the negative band, nowhere near the graylist. Lighthouse handles the event in its peer manager instead.

What Changed

• crates/net/p2p/src/swarm_adapter.rs:
  • SlowPeers collects SlowPeer reports between the existing 10 s checks. A report counts only when its priority or its non-priority failures exceed SLOW_PEER_REPORT_THRESHOLD (10), the cut Lighthouse applies before it penalizes a slow peer.
  • disconnect_slow_peers runs on that tick, right after disconnect_graylisted_peers. It disconnects a peer reported in each of the last SLOW_PEER_CHECK_LIMIT (6) checks, so about a minute without a break.
  • One check disconnects at most MAX_SLOW_PEER_DISCONNECTS_PER_CHECK (10) peers, the ones with the most failed messages first. The others keep their streak, so a peer still failing at the next check is disconnected then. Peers held back by the cap are logged as a warning.
  • The event is still forwarded to the P2P actor as before.
• crates/net/p2p/src/metrics.rs: adds the lean_gossipsub_slow_peer_disconnects_total counter.
• docs/beacon_wire.md, docs/metrics.md, CLAUDE.md: describe the rule, its two limits and the metric.

Correctness / Behavior Guarantees

• Beacon only. The gate is the one the graylist check already uses: peer scoring on. Lean is unchanged.
• Only a persistent failure disconnects. A check with no counted report resets the peer's streak, so a peer that is briefly slow keeps its connection.
• Our own slowness does not drop the whole peer set (review comment). Gossipsub counts a forward that expired in a peer's queue (connection_handler_forward_duration) as the same non_priority failure as a full queue, so a saturated uplink or a stalled runtime on our side makes every peer look slow at once. The threshold filters short stalls, and the cap bounds how many peers one check can drop. A sustained slowdown can still cost up to MAX_SLOW_PEER_DISCONNECTS_PER_CHECK peers per check while discovery refills them.
• Known gaps:
  • Some peers keep their pinned queue until the connection closes, as today. These are peers whose handler is dead but whose failures stop within the minute, for example once they are pruned from the mesh and score below the gossip threshold.
  • A dead peer that gets few messages per heartbeat, for example one gossipsub has already pruned from the mesh, may stay under SLOW_PEER_REPORT_THRESHOLD and keep its connection.
• Since fix: cap gossipsub's log output at ERROR unless RUST_LOG raises it #719 caps libp2p_gossipsub logs at ERROR, gossipsub's "Send Queue full" WARNs no longer show by default. An operator now sees these peers through this change's info! disconnect line and the new counter.

Tests Added / Run

• Eight unit tests on SlowPeers:
  • a peer failing in every check is returned at the limit;
  • a quiet check resets the streak;
  • several reports in one check count once;
  • a returned peer starts a new streak;
  • a report at the threshold does not count, and resets the streak;
  • the threshold applies to each count on its own (priority alone is enough; the two added up are not);
  • one check returns at most the cap, most failed first, and a deferred peer that keeps failing is returned at the next check;
  • a deferred peer that stops failing loses its streak.
• Replayed the follower's log in 10 s bins (before the threshold and cap were added):
  • The three stuck peers had unbroken runs of "Send Queue full" of 29, 54 and 377 bins (5 to 63 minutes), so each would have been disconnected within about a minute.
  • A peer with 3 isolated failures would have kept its connection.
  • The bins do not show whether those peers had more than SLOW_PEER_REPORT_THRESHOLD failures within a single 700 ms heartbeat, so the replay does not yet confirm the threshold still catches them.
• cargo test --profile release-fast -p ethlambda-p2p --lib: 370 passed, 1 ignored.

Related Issues / PRs

• One of three memory fixes from the same profile; the other two are separate PRs (vote-table growth, state encoding).

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean. Ran cargo clippy --locked -p ethlambda-p2p --all-targets -- -D warnings (clean), not the full workspace target
• Ran make test (test-consensus plus test-node, at release-fast) — all passing. Ran only the p2p lib tests above

[github-actions]

🤖 Claude Code Review

Review: PR 739, disconnect peers whose gossipsub send queue never drains

The change is small, self-contained and well reasoned. I found no blocking issues. I read the diff only and did not build it or run the tests.

Correctness

• SlowPeers::check is sound. It drains reported on every call, so that map can't grow. streaks.retain drops any peer not reported since the last check, which gives the "consecutive" semantics. Peers that disconnect on their own are cleaned up on the next tick.
• Several SlowPeer events inside one 10 s window count as one check. This is covered by the test several_reports_within_one_check_count_once_towards_the_streak.
• The streak is removed once a peer is returned. If disconnect_peer_id fails because the peer is already gone, nothing leaks.
• swarm.disconnect_peer_id closes every connection to the peer. That is the intended way to free the handler's queue, and a reconnect gets a fresh handler.
• The event is recorded in swarm_loop before being forwarded to the actor, so the actor still sees SlowPeer. The tracking has no dependency on actor behaviour.

Points worth a look (non-blocking)

1. Beacon-only gating (swarm_adapter.rs, disconnect_slow_peers): peer_score(local_peer_id).is_none() is used as a stand-in for "scoring is off, so this is lean". This presumably mirrors disconnect_graylisted_peers, and the doc comment says so. It is indirect, though. If gossipsub ever returns Some for the local id, or None for an unrelated reason, the gate flips silently. A small shared helper such as scoring_enabled(&swarm) used by both functions would make the coupling explicit.
2. False positives on a healthy-but-loaded peer: the doc notes that the SlowPeer report doesn't distinguish a refused message from an expired one. Six consecutive 10 s checks, about a minute, is a reasonable guard. A peer on a congested link during a long mainnet burst could still hit it. This is a trade-off I'd accept, since the lean_gossipsub_slow_peer_disconnects_total metric will show if it fires too often. Making the limit a named constant, as the PR already does, makes tuning easy.
3. Tick timing: with MissedTickBehavior::Skip, a stalled loop can make "consecutive checks" cover more than 10 s each. This only makes disconnects slightly slower, so it is harmless.
4. Log level: info! per disconnect is fine at expected rates. If a bad actor can trigger it repeatedly by reconnecting, consider debug! or rate-limiting. The metric already covers that case.

Tests and docs

• The four unit tests cover the limit, a reset on a gap, the same-window collapse, and a restart after a return. That is the full state machine.
• docs/beacon_wire.md, docs/metrics.md and CLAUDE.md are updated consistently, and the metric name follows the lean_ convention.
• The FailedMessages sum priority + non_priority is a plain usize add. It can't realistically overflow, and the value is only used for logging.

Looks good to merge. Point 1 is the only change I'd suggest making.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. The change is narrowly scoped to P2P hygiene, not consensus logic, and the approach is reasonable for mitigating the documented gossipsub queue retention issue.

• No consensus-layer concerns found: nothing here touches fork choice, attestation validation, state transition, finalization, XMSS, or SSZ.
• Security/perf direction looks good: disconnecting persistently SlowPeer peers should bound retained queued-message memory and reduce a plausible resource-exhaustion vector.

One thing I’d flag:

• crates/net/p2p/src/swarm_adapter.rs:414 — disconnect_peer_id(peer_id).is_ok() only increments the metric/logs on success, but a failed disconnect leaves the peer out of slow_peers.streaks because check() forgets persistent peers before the disconnect attempt. If disconnect_peer_id fails transiently, that peer gets a full reset and may retain the pinned queue for another minute before being retried. Consider keeping the peer in the tracked set until the disconnect succeeds, or re-inserting it on failure.

Nice-to-have:

• crates/net/p2p/src/swarm_adapter.rs:370 — priority + non_priority is fine in practice, but saturating_add would be a slightly more defensive choice for bookkeeping code.
• crates/net/p2p/src/swarm_adapter.rs:637 — the test named “count once towards the streak” only checks that many reports in one interval do not immediately trip the threshold; it does not verify the streak increments by exactly one across successive checks. A follow-up assertion over two intervals would make the behavior more explicit.

If you want, I can turn the first point into a suggested patch.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds slow peer disconnection for gossipsub peers with dead streams that accumulate queued messages in memory.

Overall Assessment

This is a well-structured, targeted fix for a memory leak caused by gossipsub connection handlers that disable themselves but leave queues pinned in memory. The logic is sound, tests are comprehensive, and documentation is thorough. I have a few minor concerns.

---

Issues Found

1. Race condition in SlowPeers::check() — stale FailedMessages aggregation (swarm_adapter.rs:372-380)

fn check(&mut self) -> Vec<(PeerId, usize)> {
    let reported = std::mem::take(&mut self.reported);
    self.streaks
        .retain(|peer_id, _| reported.contains_key(peer_id));  // line 374

Problem: The retain on line 374 uses reported.contains_key(peer_id), but reported was just taken. This is correct for removing peers not in the new report. However, there's a subtle issue: if a peer was in streaks with (checks=5, total=10), and is reported again with failed=0 (possible? FailedMessages sums to 0?), the streak continues with no new failures. But record() uses += failed, so failed=0 would still create a reported entry via or_default().

Actually, re-reading: failed_messages.priority + failed_messages.non_priority could be 0 if both are 0. The or_default() would make reported[peer] = 0, and contains_key would be true. This seems harmless but slightly wasteful.

More serious: The retain happens before updating streaks. Consider: peer A has streak (5, 10), not reported this check. retain removes it. Correct. But what if between take and retain, we cared about consistency? We don't—this is single-threaded.

Actual concern: The failed count returned includes only messages from checks where the peer was consecutively reported. If a peer fails check 1, misses check 2, fails checks 3-8, the streak resets. This is documented behavior, but the total in the returned tuple only sums the successful streak. The log message says "failed" without clarifying it's only the streak's failed count, not lifetime failures. Minor.

2. Missing peer_score check logic inconsistency (swarm_adapter.rs:402-408)

if swarm
    .behaviour()
    .gossipsub
    .peer_score(swarm.local_peer_id())
    .is_none()
{
    return;
}

Problem: This checks if our own peer has a score, not if gossipsub scoring is enabled for the peer being disconnected. The comment says "Beacon only, like disconnect_graylisted_peers", but disconnect_graylisted_peers doesn't have this check—it checks peer_score(peer_id) for each peer to get the score.

Wait, let me re-read disconnect_graylisted_peers... Actually it does:

let Some(score) = swarm.behaviour().gossipsub.peer_score(&peer_id) else {
    continue;
};

So disconnect_graylisted_peers checks per-peer score existence. The slow peer check uses peer_score(swarm.local_peer_id()) which returns None if scoring isn't enabled at all (e.g., lean mode). This is a proxy for "is this beacon mode?"

This is correct but fragile. If gossipsub ever returns a score for local_peer_id() in lean mode, this breaks. A more explicit mode check would be clearer. However, this matches the existing pattern elsewhere, so acceptable.

3. Test a_peer_failing_in_every_check_is_returned_at_the_limit off-by-one risk (swarm_adapter.rs:622-631)

for _ in 1..SLOW_PEER_CHECK_LIMIT {  // runs 5 times for limit=6
    slow_peers.record(peer, &failed(2));
    assert!(slow_peers.check().is_empty());
}
slow_peers.record(peer, &failed(2));
let expected = 2 * usize::try_from(SLOW_PEER_CHECK_LIMIT).unwrap();
assert_eq!(slow_peers.check(), vec![(peer, expected)]);

Problem: The loop runs SLOW_PEER_CHECK_LIMIT - 1 times (1..6 = 5 iterations for limit=6). Then one more record + check triggers at limit. Total records: 6, checks: 6. The streak reaches 6, and *checks >= SLOW_PEER_CHECK_LIMIT (6 >= 6) triggers.

But wait—the first check() returns empty, and checks becomes 1. After 5 loop iterations, checks is 5. The 6th record + check makes it 6, triggers. This is correct.

However, the test name says "at the limit" but the assertion checks >= limit. The test verifies exactly at limit. Good.

4. Test a_returned_peer_starts_a_new_streak has suspicious loop (swarm_adapter.rs:668-677)

for _ in 0..SLOW_PEER_CHECK_LIMIT {
    slow_peers.record(peer, &failed(1));
    slow_peers.check();
}
slow_peers.record(peer, &failed(1));
assert!(slow_peers.check().is_empty());

Problem: Each iteration: record then check. On iteration 6 (0-indexed, SLOW_PEER_CHECK_LIMIT=6), record makes checks=6, check returns the peer and removes it. So after the loop, the peer is gone from streaks. Then record + check starts fresh at 1, returns empty. Correct.

But this test doesn't verify the peer was actually returned in the loop! It should assert something about the loop iterations. As written, if check() never returned anything (bug), the test would still pass because the final state is empty. This test has a hole.

Suggested fix:

#[test]
fn a_returned_peer_starts_a_new_streak() {
    let peer = PeerId::random();
    let mut slow_peers = SlowPeers::default();
    for i in 0..SLOW_PEER_CHECK_LIMIT {
        slow_peers.record(peer, &failed(1));
        let result = slow_peers.check();
        if i < SLOW_PEER_CHECK_LIMIT - 1 {
            assert!(result.is_empty(), "should not trigger before limit");
        } else {
            assert_eq!(result, vec![(peer, SLOW_PEER_CHECK_LIMIT as usize)]);
        }
    }
    slow_peers.record(peer, &failed(1));
    assert!(slow_peers.check().is_empty(), "new streak should not trigger immediately");
}

5. Memory growth in SlowPeers::reported between checks (swarm_adapter.rs:357)

fn record(&mut self, peer_id: PeerId, failed_messages: &FailedMessages) {
    let failed = failed_messages.priority + failed_messages.non_priority;
    *self.reported.entry(peer_id).or_default() += failed;
}

Problem: If gossipsub emits SlowPeer for many unique peers between checks, reported grows unbounded. The check() clears it, but if the event loop is busy and check is delayed, this could accumulate. However, SlowPeer is emitted per heartbeat (700ms), and checks are every 10s, so max ~14 heartbeats of reports. With mesh size 8, this is bounded. Acceptable.

6. No upper bound on failed count (swarm_adapter.rs:358)

failed_messages.priority + failed_messages.non_priority as usize could theoretically overflow if gossipsub reports huge numbers. In practice, queue capacities are small (thousands), so this won't happen. Not a real concern.

---

Positive Observations

1. Excellent documentation — The SlowPeers struct doc comment (lines 339-358) clearly explains the gossipsub handler bug, memory impact ("490 MB"), and design rationale.

2. Conservative threshold — SLOW_PEER_CHECK_LIMIT = 6 (60 seconds) avoids disconnecting catch-up peers with transient queue pressure.

3. Proper metrics and logging — Counter incremented only on successful disconnect, with structured logging.

4. Test coverage — Four tests cover: exact limit triggering, streak reset, coalescing within one check, and fresh start after disconnect.

5. Lean mode safety — The peer_score check prevents disconnections on lean chain, matching existing pattern.

---

Summary

Item	Severity	File:Lines	Description
1	Low	swarm_adapter.rs:668-677	Test a_returned_peer_starts_a_new_streak doesn't verify peer was actually returned during loop
2	Info	swarm_adapter.rs:402-408	peer_score(local_peer_id) mode check is correct but implicit; consider explicit mode enum
3	Info	swarm_adapter.rs:339-358	Doc comment mentions "490 MB" — consider if this is mainnet-specific and may need updating

Recommendation: Fix test in Item 1 to assert on loop behavior. Otherwise, this PR is ready to merge.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[pablodeymo]

One availability risk worth weighing before merging.

In the pinned fork, a forward that sits in the handler's queue longer than
connection_handler_forward_duration (1 s) is reported as non_priority:
handler.rs hits the timeout and emits HandlerEvent::MessageDropped, and
behaviour.rs counts it as failed_messages.non_priority += 1. So a
SlowPeer report also means our side did not flush in time, not only that
the remote handler is dead.

If the swarm task is starved for a while, or the uplink is saturated, every
peer gets reported in the same heartbeat. After six consecutive checks
disconnect_slow_peers then drops the whole peer set in one tick. Discovery
and the bootnode redial refill it, but the node eclipses itself for a while.

disconnect_graylisted_peers has the same shape, but graylisting needs
sustained invalid behaviour, so this new rule is the easier one to trip.

Two cheap ways to close it:

• cap the number of peers one check may disconnect, or
• skip the pass when more than some fraction of connected peers were
  reported, since that points at us rather than at them.

[MegaRedHand]

@pablodeymo done