client connection issues by vladimir-ea · Pull Request #192 · gattaca-com/silver

vladimir-ea · 2026-08-21T16:16:58Z

a set of fixes from debugging client connection issues (specifically lighthouse):

tracking of rpc request outcomes
fix ByRoot rpc requests (previously not handled)
monotonically increasing ENR seq across restarts
advertise fake tcp support in ENR (otherwise Lighthouse will not call us)
stuck streams and stream timeouts
gossip mesh management: reduce mesh churn
additional surfer metrics: per client counts, mark inbound connections

vladimir-ea · 2026-08-21T18:05:24Z

clanky-gattaca · 2026-08-21T18:05:26Z

🤖 PR review

This PR is a large P2P reliability/observability pass: exact-size gossip attestation check, ENR seq/tcp advertisement changes, QUIC stream setup + inbound-RPC lifetime timeouts with distinguishable reset codes, a missing increment_offset fix that made all by-root request bodies parse empty, correct multi-identifier DataColumnSidecarsByRootRequest parsing in storage, extraction of the gossip domain from manager.rs into manager/gossip.rs with looser graft/IHAVE policy plus quick-prune backoff escalation, and new counters/surfer panes. Most of it is well-reasoned and the refactor looks behaviour-preserving where it claims to be. One issue should be fixed before merge: the new 30s inbound-RPC reap combines with the retained P7 penalty on StreamGone to graylist innocent peers after three orphaned response chunks.

Blocking (1)

crates/peer/src/manager.rs:504 — StreamGone (a response whose inbound stream no longer exists) still charges a 1.0 P7 penalty, but this PR adds a mechanism that produces it systematically: when INBOUND_RPC_TIMEOUT reaps an inbound RPC stream, the storage tile's PendingQuery for that stream is untouched and keeps emitting one response per remaining unit — each one becomes a StreamGone and another 1.0 penalty. With behaviour_penalty_threshold=0, behaviour_penalty_weight=-10 and graylist_threshold=-80, three such chunks graylist and evict the peer. The same happens whenever a peer legitimately cancels a by-range/by-root request. This is exactly the "penalising it blames the wrong side" case the PR exempts a few lines below for incoming P2pStreamClosed.

_{10 inline comment(s) on the diff.}

_{opus · high effort · 675s}

Re-run — tick an effort:

low
medium
high

clanky-gattaca

Inline notes from the review comment on this PR.

clanky-gattaca · 2026-08-21T18:16:48Z

+                let offence = if stream_gone {
+                    crate::PeerCounters::ResponseStreamGone.inc();
+                    "response stream gone"
+                } else {
+                    crate::PeerCounters::StreamCreditExhausted.inc();
+                    "stream credit exhausted"
+                };
+                self.add_behaviour_penalty(p2p_peer, 1.0, offence);


Blocking — StreamGone (a response whose inbound stream no longer exists) still charges a 1.0 P7 penalty, but this PR adds a mechanism that produces it systematically: when INBOUND_RPC_TIMEOUT reaps an inbound RPC stream, the storage tile's PendingQuery for that stream is untouched and keeps emitting one response per remaining unit — each one becomes a StreamGone and another 1.0 penalty. With behaviour_penalty_threshold=0, behaviour_penalty_weight=-10 and graylist_threshold=-80, three such chunks graylist and evict the peer. The same happens whenever a peer legitimately cancels a by-range/by-root request. This is exactly the "penalising it blames the wrong side" case the PR exempts a few lines below for incoming P2pStreamClosed.

Suggested change

let offence = if stream_gone {

crate::PeerCounters::ResponseStreamGone.inc();

"response stream gone"

} else {

crate::PeerCounters::StreamCreditExhausted.inc();

"stream credit exhausted"

};

self.add_behaviour_penalty(p2p_peer, 1.0, offence);

if stream_gone {

crate::PeerCounters::ResponseStreamGone.inc();

tracing::debug!(p2p_peer, ?protocol, "response stream gone");

} else {

crate::PeerCounters::StreamCreditExhausted.inc();

self.add_behaviour_penalty(p2p_peer, 1.0, "stream credit exhausted");

}

clanky-gattaca · 2026-08-21T18:16:48Z

+const STREAM_ERR_CODE_RESPONSE_TIMEOUT: u32 = 2;
+/// Whole-life budget for an inbound RPC stream (negotiate + request +
+/// serve). Generous so a large by-range serve to a slow peer survives.
+const INBOUND_RPC_TIMEOUT: Duration = Duration::from_secs(30);


Suggestion — INBOUND_RPC_TIMEOUT is a whole-life budget measured from stream creation rather than an idle/no-progress deadline, so a serve that is making steady progress is reset at 30s. This PR simultaneously widens DataColumnsByRoot from one identifier to up to MAX_REQUEST_BLOCKS_DENEB identifiers × 128 columns per request, so multi-thousand-unit serves are now reachable and will be torn down mid-response. An inactivity deadline (reset on each chunk written) would reap the stuck streams this targets without cutting off healthy long serves.

clanky-gattaca · 2026-08-21T18:16:48Z

+        let boot_seq =
+            SystemTime::now().duration_since(UNIX_EPOCH).unwrap_or_default().as_millis() as u64;
+        builder.seq(boot_seq);


Suggestion — Seeding the ENR seq from the wall clock assumes boot time is monotonic across restarts, which fails on an NTP step backwards, a VM/container snapshot restore, or a host without RTC sync at boot: the new record then has a lower seq than the one peers cached and no subsequent set_* bump can catch up, leaving the node advertising a stale address indefinitely. Persisting the last-used seq (or clamping to max(persisted + 1, now_millis)) removes the clock dependency. Also note unwrap_or_default() silently yields seq 0.

hmm really?

clanky-gattaca · 2026-08-21T18:16:48Z

 fn out_buffer(id: &P2pStreamId, incoming: bool) -> OutboundBuffer {
    match id.protocol() {
-        StreamProtocol::GossipSub => OutboundBuffer::Gossip(OutBuffer::new(1024)),
+        StreamProtocol::GossipSub => OutboundBuffer::Gossip(OutBuffer::new(4096)),


Suggestion — Quadrupling the gossip out-buffer means a write-blocked peer can now accumulate 4096 queued messages that will still be forwarded once credit returns, long after they are useful — deeper staleness rather than fewer real drops. Since P2pOutboundMessageDropped no longer carries a penalty, the shallower buffer's shed-and-move-on behaviour may be preferable to a 4x longer stale queue per stream (two per peer).

clanky-gattaca · 2026-08-21T18:16:48Z

+    /// a stuck state names itself. Steady state is 2 gossip + transient
+    /// RPC.
+    #[allow(dead_code)]
+    pub(crate) fn log_stream_census(&mut self, now: Instant) {


Suggestion — log_stream_census is added behind #[allow(dead_code)] and never called from anywhere, and P2p::sample_stats was switched to peers.get_mut (line 149 of p2p/mod.rs) even though Peer::stats takes &self — presumably the intended call site. Either wire the census in or drop it rather than shipping unreachable diagnostics.

clanky-gattaca · 2026-08-21T18:16:48Z

    }

    /// Remote peer has called 'stop' on their recv stream (our send side).
+    fn timed_out<E>(


Nit — timed_out was inserted directly under the /// Remote peer has called 'stop' on their recv stream (our send side). doc comment, which belongs to stop_send; that comment now documents timed_out instead. Move it back down onto stop_send.

clanky-gattaca · 2026-08-21T18:16:48Z

+        let _ = connection.send_stream(id).reset(VarInt::from_u32(1));
+        let _ = connection.recv_stream(id).stop(VarInt::from_u32(1));


Nit — These hardcode 1 instead of the STREAM_ERR_CODE_PROTOCOL constant introduced in this diff, which also means a timeout teardown is reported to the remote as a protocol violation rather than a timeout.

Suggested change

let _ = connection.send_stream(id).reset(VarInt::from_u32(1));

let _ = connection.recv_stream(id).stop(VarInt::from_u32(1));

let _ = connection.send_stream(id).reset(VarInt::from_u32(STREAM_ERR_CODE_PROTOCOL));

let _ = connection.recv_stream(id).stop(VarInt::from_u32(STREAM_ERR_CODE_PROTOCOL));

clanky-gattaca · 2026-08-21T18:16:48Z

+        let key = cfg.keypair().unwrap();
+        enr.set_attnets([0xff; 8], key.secret_key()).unwrap();
+        // Unpadded base64: 4 chars per 3 bytes.


Nit — Enr::size() already returns the encoded record length, and Enr::builder().build() already rejects records over MAX_ENR_SIZE (300), so the base64 length arithmetic here is both redundant and approximate.

Suggested change

let key = cfg.keypair().unwrap();

enr.set_attnets([0xff; 8], key.secret_key()).unwrap();

// Unpadded base64: 4 chars per 3 bytes.

let bytes = enr.size();

vladimir-ea added 2 commits August 21, 2026 17:10

markups

e386139

clean up logs

3cd7602

clanky-gattaca reviewed Aug 21, 2026

View reviewed changes

vladimir-ea added 9 commits August 21, 2026 19:46

score fix

dea9e46

fix bad score timeout

f9a9ffa

attestation unknown root counter

d9026f1

default idel timeout

db5a425

gossip getting sped past

fe40d45

gossip getting sped past

546976d

manage over population

9882f8a

manage over population

1a25fdd

reject inbound if over capacity

10acb1e

Navigation Menu

Uh oh!

client connection issues - #192

client connection issues#192
vladimir-ea wants to merge 11 commits into
mainfrom
ve/lh_issues

vladimir-ea commented Aug 21, 2026

Uh oh!

vladimir-ea commented Aug 21, 2026

Uh oh!

clanky-gattaca commented Aug 21, 2026 •

edited

Loading

Uh oh!

clanky-gattaca left a comment

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

vladimir-ea Aug 21, 2026

Uh oh!

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

clanky-gattaca Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let _ = connection.send_stream(id).reset(VarInt::from_u32(1));
		let _ = connection.recv_stream(id).stop(VarInt::from_u32(1));

Uh oh!

Conversation

vladimir-ea commented Aug 21, 2026

Uh oh!

vladimir-ea commented Aug 21, 2026

Uh oh!

clanky-gattaca commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 PR review

Uh oh!

clanky-gattaca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

clanky-gattaca commented Aug 21, 2026 •

edited

Loading