fix(network): gate find-response stall tracking on per-peer height - #10910
fix(network): gate find-response stall tracking on per-peer height#10910upbqdn wants to merge 1 commit into
Conversation
fb3f36c to
286a93a
Compare
|
Do we still need this now that #11122 has merged? |
|
Yes. The distance still comes from the local tip's block time against the wall clock, so a tip that stops Scope, to be explicit: this gates per peer on Needs a rebase either way — #11122 touched the same test vectors. |
PR #10732 stops the find-response stall tracker from disconnecting peers at the chain tip, but gates it on `is_at_or_near_network_tip`, which estimates the distance to the network tip from the local tip block's timestamp. A stale tip — mining paused, or the whole network parked at a shared tip — makes that estimate report the node as far behind, re-arming the tracker and disconnecting healthy at-tip peers again. Gate the tracker per-peer on the peer's advertised handshake height versus our tip instead: only penalise empty or failed find responses from a peer that claimed a height above ours. This is robust to a stale tip, preserves the GHSA-h9hm-m2xj-4rq9 property (a peer that promises height but delivers nothing is still dropped during real sync), and means a peer lying high about its height can only get itself dropped, never an honest peer. The trade-off — a peer claiming a height at or below our tip is never tracked — is closed by the contribution-based accounting tracked in #11080. The zcashd-compat sidecar exemption from #10952 is preserved; its stall test now arms the per-peer gate so the exemption is what it exercises.
286a93a to
d18c789
Compare
Motivation
Follow-up to #10732. That PR stops the find-response stall tracker from disconnecting peers at the chain tip, but gates it on
ChainTip::is_at_or_near_network_tip, which estimates the distance to the network tip purely from the local tip block's timestamp. When the local tip is stale — mining paused on a test network, or the whole network parked at a shared tip — that wall-clock estimate reports the node as thousands of blocks behind, so the gate re-arms the stall tracker and starts disconnecting healthy at-tip peers again. On a network where every node runs this code, a stall at a shared tip re-arms every node's tracker and they mutually disconnect, which can keep a cohort from catching up to a peer that is ahead.#11080 tracked the complementary half of this problem (what counts as a stall) and has since been closed as a duplicate of #11101, which is the live issue. Note that #11101 asks for the latest advertised height plus a freshness policy, while this PR gates on
start_height, which is captured at handshake and never updated — peer-specific but not fresh. So #11101's eventual design may supersede this gate rather than build on it. This PR is the narrower fix for when a peer is eligible for stall tracking, and #11122 (threshold 5 → 1,000) does not cover it: that moves the false-positive boundary to ~20.8 hours, which a paused test network or a network parked at a shared tip exceeds routinely.Solution
Gate the stall tracker per-peer on the peer's advertised handshake height versus our own tip, instead of a single wall-clock estimate. An empty or timed-out find response is only counted against a peer that advertised a height above our tip — a peer that claimed to have blocks and then delivered nothing. A peer at or below our tip legitimately has nothing to send, so its empty responses are never penalised, regardless of how old our tip is.
LoadTrackedClient::remote_start_height()exposes the peer's version-handshake height (already stored inconnection_info.remote).PeerSet::route_p2ctracks stalls only whenremote_start_height() - our_tip > AT_OR_NEAR_TIP_THRESHOLD.This is robust to a stale local tip, preserves the GHSA-h9hm-m2xj-4rq9 property (a peer that promises height during real sync but only returns empty responses is still dropped after the threshold), and is DoS-resistant: deciding per-peer means a peer lying high about its height can only get itself dropped, never an honest peer — unlike a global "max advertised height" gate. The wire
start_heightis an arbitraryu32, andHeight - Heightsubtracts exactly ini64, so a hostile extreme value cannot overflow.start_heightis captured at handshake and not updated, which only ever makes us stop tracking slightly early near the tip, never falsely disconnect a good peer — including a peer we have since outrun.Security trade-off (for explicit sign-off)
The per-peer gate narrows the tracker's reach in one attacker-selectable way: a peer that advertises a height at or below our tip is never tracked, so it can serve empty find responses indefinitely without being struck (on
main, any peer was tracked whenever the wall-clock estimate said "syncing"). Related: once our tip passes a connection's frozen handshake height, tracking stays off for that connection's lifetime. Both are the deliberate cost of fixing the false-positive direction — the wall-clock gate disconnects every healthy peer on a parked network, which is strictly worse, and an untracked useless peer costs sync only a retry. The residual gap (peers contributing nothing while staying connected) is exactly what #11101 is scoped to close.Interaction with #10952 (zcashd-compat sidecars)
The sidecar exemption from #10952 is preserved and layered with the new gate:
track_stalls = is_find_request && !zcashd_compat && peer_claims_ahead(). The exemption still matters under the per-peer gate: a sidecar's handshake height can exceed a fresh local tip before this node loads its state.Tests
find_blocks_stall_tracked_when_syncing: peer advertises a height above our tip → still tracked and disconnected after the threshold (GHSA property preserved), even with the wall-clock estimate set to "at tip".find_blocks_stall_not_tracked_when_at_tip: peer at our tip, with the wall-clock estimate set to100_000to simulate a stale/frozen tip → not tracked, peer stays connected. This is the direct regression test for the gap above: undermain's wall-clock gate this scenario tracks and disconnects the peer.find_blocks_stall_tracked_when_tip_unknownandfind_blocks_stall_count_preserved_across_tip_transitionupdated to drive the peer's advertised height (newClientTestHarnessBuilder::with_start_heightplus a single-peer discovery helper).find_blocks_stall_not_tracked_for_zcashd_compat(from feat(zebrad): add zcashd-compat mode with a supervised zcashd wallet sidecar #10952) now gives the sidecar a handshake height above the local tip, so it arms the per-peer gate and the sidecar exemption is what the test exercises — it would otherwise pass vacuously under the new gate.find_blocks_stall_gate_boundary: a peer exactlyAT_OR_NEAR_TIP_THRESHOLDabove our tip is not tracked; one block further is tracked and disconnected. Mutation-checked: flipping the gate's>to>=fails this test (the rest of the suite passes that mutation silently).Specifications & References
AT_OR_NEAR_TIP_THRESHOLD5 → 1,000. Retunes the same wall-clock signal this PR replaces, so it delays the false positive rather than removing it.AI Disclosure
PR Checklist
type(scope): description