sync: recover from stale fork - #1036
Conversation
ch4r10t33r
left a comment
There was a problem hiding this comment.
The re-anchor idea is right, but I found two serious problems: an unbounded retry loop (with a u8 overflow) in the recovery path, and a liveness deadlock in the proposal suppression. Details inline.
| .peer_head_slot = snap.peer_head_slot, | ||
| .peer_head_root = snap.peer_head_root, | ||
| .our_head_root_at_start = anchor.root, | ||
| .attempt = snap.range_attempt + 1, |
There was a problem hiding this comment.
This can loop forever and then panic. syncEndDecision returns abort_fallback on ANY aborted range, before the attempt/max_attempts check (blocks_by_range_sync.zig:308), and this handler re-triggers recovery on every abort_fallback. If our justified anchor is not on the peer's chain (justification diverged, or the peer serves yet another fork), the recovery request's first chunk mismatches again, aborts again, and we re-issue the exact same request: forkMismatchRecoveryStart is deterministic in (anchor, peer_head), so nothing changes between rounds. attempt is a u8, so snap.range_attempt + 1 eventually hits 255+1 and panics in safe builds, or wraps and spins forever in ReleaseFast. Gate the recovery on snap.range_attempt < MAX_BLOCKS_BY_RANGE_SYNC_ATTEMPTS (or a one-shot recovery flag per wedge) so it degrades to the by-root walk instead of looping.
|
|
||
| const wall_head_lag = self.wall_head_lag_slots.load(.monotonic); | ||
| const latest_justified_slot = self.forkChoice.getLatestJustified().slot; | ||
| if (blocks_by_range_sync.shouldSuppressProposalForHeadLag( |
There was a problem hiding this comment.
This can permanently halt an otherwise healthy chain. The gate is pure wall-clock vs local head, with no peer evidence. If the whole network goes more than 4 slots without an imported block after first justification (coordinated restart of a devnet, 5 consecutive missed proposals on an all-zeam net, a long prover stall), then every node wakes up with lag > 4, every proposer skips, lag only grows, and nobody ever proposes again. No escape hatch. The existing peers_materially_ahead gate avoids this by requiring peers to actually be ahead. Suggest requiring evidence a fresher chain exists (some peer advertising head within N slots of wall clock) before suppressing, or an unconditional override once lag exceeds some large bound so the chain can restart itself.
|
|
||
| if (peer_head_slot >= min_slots_for_block_requests) { | ||
| const history_start = peer_head_slot - min_slots_for_block_requests; | ||
| if (recovery_start < history_start) return null; |
There was a problem hiding this comment.
Worth being explicit: in the incident this PR is fixing, the anchor was ~5800 slots behind the peer head, so this window check returns null and recovery lands on syncFetchPeerHeadByRoot, the same path that failed to recover in the incident (a 5000+ block parent walk). Your third test encodes exactly that. So the range re-anchor only helps when the wedge is caught within MIN_SLOTS_FOR_BLOCK_REQUESTS (3600 slots, ~4h). That is probably fine as the common case once detection is fast, but the PR description should say the observed devnet-5 wedge itself would still not recover via range, and whether the by-root walk is expected to handle gaps that large.
Fixes #1035.
Summary
blocks_by_rangerecovery to latest justified when the first chunk is on a sibling fork and that anchor is still inside the peer history windowMAX_BLOCKS_BY_RANGE_SYNC_ATTEMPTS; if the anchor is wrong or too old, degrade to the existingblocks_by_rootparent walk instead of loopingRange-history limitation
The devnet-5 incident described in #1035 had the likely anchor roughly 5800 slots behind the peer head, outside the current 3600-slot
blocks_by_rangeserving window. In that exact late-detected shape, the new ranged re-anchor intentionally returns null and recovery relies on the by-root parent walk from peer head. The ranged path is for faster recovery when the wedge is detected before the common anchor falls out of peer range history; large historical forks still need the by-root walk unless we add a deeper ancestor-search protocol.Validation
zig fmt --check pkgs/node/src/blocks_by_range_sync.zig pkgs/node/src/node.zig pkgs/node/src/chain.zig pkgs/node/src/constants.ziggit diff --check/home/node/.openclaw/workspace/.tools/zig-0.16.0/zig build test --summary allstill fails in this runner becauserustupis not installed (run rustuptransitive failures for node/network/etc.); non-Rust-dependent packages such as params, utils, and shadow_cost pass./usr/local/bin/zig build test --summary allis unusable here because system Zig 0.15.2 does not match cached deps using the older Build API (b.graph.io).