-
Notifications
You must be signed in to change notification settings - Fork 39
sync: recover from stale fork #1036
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ const RcBeamState = rc_beam_state.RcBeamState; | |
| const chain_worker = @import("./chain_worker.zig"); | ||
| const invalid_block_cache = @import("./invalid_block_cache.zig"); | ||
| const InvalidBlockSet = invalid_block_cache.InvalidBlockSet; | ||
| const blocks_by_range_sync = @import("./blocks_by_range_sync.zig"); | ||
|
|
||
| /// Bound on the in-memory invalid-block-roots cache (see | ||
| /// `BeamChain.invalid_block_roots`). 10 000 32-byte roots ≈ 320 KB plus | ||
|
|
@@ -5272,6 +5273,21 @@ pub const BeamChain = struct { | |
| /// publishBlock) runs on a `thread_pool` worker so the multi-second | ||
| /// prod-scheme merge never freezes the slot loop. At most one propose is in | ||
| /// flight; a second trigger is dropped (the next slot re-proposes). | ||
| fn hasFresherPeerNearWall(self: *Self, our_head_slot: types.Slot, wall_head_lag_slots: u64) bool { | ||
| const wall_slot = our_head_slot +| wall_head_lag_slots; | ||
| var peer_guard = self.connected_peers.iterateLocked(); | ||
| defer peer_guard.deinit(); | ||
| var peer_iter = peer_guard.iter; | ||
| while (peer_iter.next()) |entry| { | ||
| const status = entry.value_ptr.latest_status orelse continue; | ||
| if (status.head_slot <= our_head_slot) continue; | ||
| if (status.head_slot +| constants.BLOCK_PROPOSAL_MAX_HEAD_LAG_SLOTS >= wall_slot) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| pub fn submitPropose(self: *Self, node: *@import("./node.zig").BeamNode, slot: usize, proposer_id: usize) void { | ||
| // Sync gating (the checks the old on-loop maybeDoProposal performed). | ||
| switch (self.getSyncStatus()) { | ||
|
|
@@ -5303,6 +5319,22 @@ pub const BeamChain = struct { | |
| }, | ||
| } | ||
|
|
||
| const wall_head_lag = self.wall_head_lag_slots.load(.monotonic); | ||
| const latest_justified_slot = self.forkChoice.getLatestJustified().slot; | ||
| const head = self.forkChoice.getHead(); | ||
| if (blocks_by_range_sync.shouldSuppressProposalForHeadLag( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| wall_head_lag, | ||
| constants.BLOCK_PROPOSAL_MAX_HEAD_LAG_SLOTS, | ||
| latest_justified_slot, | ||
| self.hasFresherPeerNearWall(head.slot, wall_head_lag), | ||
| )) { | ||
| self.logger.warn( | ||
| "skipping block production for slot={d} proposer={d}: local head is {d} wall-clock slots behind (head_slot={d}, latest_justified_slot={d})", | ||
| .{ slot, proposer_id, wall_head_lag, head.slot, latest_justified_slot }, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| // Single-flight: at most one propose at a time. fetchAdd-then-compare is a soft ceiling, | ||
| // released on every early return below. | ||
| const prev = self.propose_inflight.fetchAdd(1, .acq_rel); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1718,6 +1718,50 @@ pub const BeamNode = struct { | |
| }; | ||
| } | ||
|
|
||
| fn initiateForkMismatchRangeRecovery( | ||
| self: *Self, | ||
| snap: networkFactory.Network.PendingRequestSnapshot, | ||
| ) bool { | ||
| if (!blocks_by_range_sync.shouldAttemptForkMismatchRangeRecovery( | ||
| snap.range_attempt, | ||
| constants.MAX_BLOCKS_BY_RANGE_SYNC_ATTEMPTS, | ||
| )) return false; | ||
|
|
||
| const anchor = self.chain.forkChoice.getLatestJustified(); | ||
| const recovery_start = blocks_by_range_sync.forkMismatchRecoveryStart( | ||
| snap.start_slot, | ||
| anchor.slot, | ||
| snap.peer_head_slot, | ||
| constants.MIN_SLOTS_FOR_BLOCK_REQUESTS, | ||
| ) orelse return false; | ||
|
|
||
| if (!self.network.peerSupportsBlocksByRange(snap.peer_id_copy)) return false; | ||
|
|
||
| const remaining = snap.peer_head_slot - recovery_start + 1; | ||
| const requested_count: u64 = @min(remaining, params.MAX_REQUEST_BLOCKS); | ||
| self.logger.warn( | ||
| "blocks_by_range fork recovery: re-anchoring peer {s}{f} from justified slot={d} start_slot={d} count={d} after failed start_slot={d}", | ||
| .{ | ||
| snap.peer_id_copy, | ||
| self.node_registry.getNodeNameFromPeerId(snap.peer_id_copy), | ||
| anchor.slot, | ||
| recovery_start, | ||
| requested_count, | ||
| snap.start_slot, | ||
| }, | ||
| ); | ||
| self.initiateBlocksByRangeCatchUp(.{ | ||
| .peer_id = snap.peer_id_copy, | ||
| .start_slot = recovery_start, | ||
| .count = requested_count, | ||
| .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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| }); | ||
| return true; | ||
| } | ||
|
|
||
| /// `blocks_by_root` catch-up: fetch the peer head and walk parents via the existing | ||
| /// batched parent-fetch path (used when `blocks_by_range` is unavailable or gap is small). | ||
| fn initiateCatchUpViaBlocksByRoot(self: *Self, status: CatchUpPeerStatus, our_head_slot: types.Slot) void { | ||
|
|
@@ -1922,7 +1966,9 @@ pub const BeamNode = struct { | |
| }, | ||
| ); | ||
| self.network.finalizePendingRequest(request_id); | ||
| self.syncFetchPeerHeadByRoot(snap.peer_id_copy, snap.peer_head_root); | ||
| if (action != .abort_fallback or !self.initiateForkMismatchRangeRecovery(snap)) { | ||
| self.syncFetchPeerHeadByRoot(snap.peer_id_copy, snap.peer_head_root); | ||
| } | ||
| }, | ||
| .pre_finalized_complete => { | ||
| self.recordRangeSyncOutcome("pre_finalized_noop"); | ||
|
|
@@ -2027,7 +2073,7 @@ pub const BeamNode = struct { | |
| !std.mem.eql(u8, &signed_block.block.parent_root, &view.our_head_root_at_start)) | ||
| { | ||
| self.logger.warn( | ||
| "blocks_by_range: fork mismatch on first chunk slot={d} start_slot={d} (parent 0x{x} != our head-at-start 0x{x}); aborting range batch", | ||
| "blocks_by_range: fork mismatch on first chunk slot={d} start_slot={d} (parent 0x{x} != expected anchor 0x{x}); aborting range batch", | ||
| .{ | ||
| signed_block.block.slot, | ||
| view.start_slot, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.