fix(l1): recover instead of stalling when the head's post-state is unreachable - #7196
Conversation
When a deep reorg unwinds past the window of retained state, the canonical chain and the state history can end up on different branches, leaving no canonical block whose post-state we can still read. Full sync then walked to genesis, found no stateful parent, and returned Ok to pause "until a reconcilable forkchoice head arrives" — a head that can never arrive, because the state is gone rather than merely unreferenced. Meanwhile every forkchoiceUpdated whose finalized block we had not downloaded was rejected with -38002 through an arm that, unlike Syncing and StateNotReachable, never starts a sync, so the missing blocks were never fetched and the node followed the chain no further. Report the exhausted full-sync walk as SyncError::StateUnrecoverable and escalate to snap sync, which is the only in-protocol way to obtain state we do not hold. Treat a safe/finalized block we simply do not have as missing data: start a sync and answer SYNCING, keeping -38002 for elements we do hold but that are ordered wrongly or sit on a disjoint branch. Also report the forkchoice element that is actually absent instead of always naming the finalized one, and correct the comment at the deep-reorg cache-edge bail, which claimed the shallow path should already have succeeded when that path having failed is precisely how execution reaches it.
|
🤖 Kimi Code ReviewThis PR addresses critical liveness issues in fork choice handling and sync recovery. The changes are well-structured and include good test coverage. crates/blockchain/fork_choice.rs
crates/networking/p2p/sync.rs
crates/networking/p2p/sync/full.rs
crates/networking/rpc/engine/fork_choice.rs
Minor suggestions:
Overall: The PR correctly identifies and fixes three related failure modes: incorrect error attribution, indefinite sync pausing, and failure to trigger sync on missing elements. The code is safe to merge. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewFindings
The I couldn’t run the targeted tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewReview: PR 7196 — Recover instead of stalling when the head's post-state is unreachableSummaryThis is a well-scoped, well-documented fix for a real (and painful) failure mode: a full-sync node whose retained state falls behind the canonical chain after a deep reorg had no path back to liveness. The changes are surgical, the reasoning in commit/PR text matches what's implemented, and unit tests were added for the one piece of logic that's cheaply testable ( Findings1. Stale error message on The entire point of this PR is that a missing safe/finalized element is now treated as a sync problem ( 2. 3. PR description's claim about the guard's exclusivity is slightly stronger than the code guarantees (informational) Things verified as correct
No security, gas-accounting, or consensus-rule concerns — this PR only touches sync orchestration and FCU error routing, not block/state validation logic itself. Automated review by Claude (Anthropic) · sonnet · custom prompt |
| // operator notices and runs `ethrex removedb`. | ||
| warn!( | ||
| %sync_head, | ||
| "Full sync has no reachable state to resume from; escalating to snap sync" |
There was a problem hiding this comment.
| "Full sync has no reachable state to resume from; escalating to snap sync" | |
| "Full sync has no reachable state to resume from; switching to snap sync" |
| self.snap_enabled.store(true, Ordering::Relaxed); | ||
| self.run_snap_cycle(sync_head, store).await |
There was a problem hiding this comment.
Should we check here if the sync-mode was set to snap-sync/default? If the sync-mode is full-sync, switching to snap-sync here can result in lost data.
| // Returning Ok here would pause the cycle and wait for a forkchoice head that | ||
| // reconciles to state we retain. When the walk has already bottomed out at | ||
| // genesis no such head exists — the state is gone, not merely unreferenced — | ||
| // so the wait never ends and the node stops following the chain entirely. | ||
| // Report it so the caller can escalate to snap sync. |
There was a problem hiding this comment.
| // Returning Ok here would pause the cycle and wait for a forkchoice head that | |
| // reconciles to state we retain. When the walk has already bottomed out at | |
| // genesis no such head exists — the state is gone, not merely unreferenced — | |
| // so the wait never ends and the node stops following the chain entirely. | |
| // Report it so the caller can escalate to snap sync. | |
| // Returning Ok here would pause the cycle. | |
| // Report it so the caller can switch to snap sync. |
|
|
||
| if !safe_hash.is_zero() { | ||
| check_order(&safe_res, &head_res)?; | ||
| check_order(&safe_res, &head_res, error::ForkChoiceElement::Safe)?; |
There was a problem hiding this comment.
Let's import ForkChoiceElement here
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Should we put this in the tests directory?
An earlier revision of this PR mapped ElementNotFound to a SYNCING response and started a sync, to avoid wedging a node that has fallen behind. But ElementNotFound only fires when the block the missing element is compared against (the head, or the safe block) is already present — the ordering checks run before the head-absent syncing path — so the node is not behind on it. An unknown safe/finalized hash against a known head is the -38002 case the engine spec mandates, which the Hive 'Unknown SafeBlockHash' and 'Unknown FinalizedBlockHash' Cancun tests enforce. The genuine fell-behind wedge this PR targets surfaces as the head-absent Syncing path or StateNotReachable, which are unaffected. The check_order change that names the actually-missing element in the error is kept.
…ress review Review feedback on the unreachable-state recovery: - Gate the snap-sync escalation on the configured mode. At the recovery point snap_enabled is false both for an explicit --syncmode full node and for a snap-default node that auto-switched to full after initial sync; escalating the former silently runs a snap cycle that wipes the leaves folder, discarding state the operator chose to keep. Thread the configured intent (snap_permitted) down to the Syncer and, when snap is not permitted, surface the unrecoverable state for operator action instead of switching. A snap-default node still escalates as before. - Import ForkChoiceElement instead of spelling error::ForkChoiceElement at every call site (per review).
2 participants
Motivation
A devnet node stopped following the chain and could not recover, not even across restarts. A deep reorg unwound past the window of retained state and left the canonical chain and the state history pointing at different branches, so no canonical block had a post-state we could still read.
From there three independent paths all declined to make progress:
Okto pause "until a reconcilable forkchoice head arrives". That head can never arrive — the state is gone, not merely unreferenced — so the wait never ends.forkchoiceUpdatednaming a finalized block we had not downloaded was rejected with-38002through an arm that, unlikeSyncingandStateNotReachable, never starts a sync. The blocks we were missing were therefore never fetched, and each later FCU failed identically.regenerate_head_stateruns the same walk and refuses to boot.The node logged 2836 forkchoice rejections over nine hours while the chain moved ~6500 blocks ahead, then died and crash-looped. The trigger was a CL-side bug producing repeated invalid heads, but the reorg churn only exposed the condition; nothing in the execution client could get out of it.
Description
SyncError::StateUnrecoverableinstead of pausing, andsync_cycleescalates to snap sync — the only in-protocol way to obtain state we do not hold. The guard fires only once the walk has bottomed out with no stateful parent anywhere, so it cannot trigger while a usable base still exists.SYNCING.-38002is kept for elements we do hold but that are ordered wrongly (Unordered) or sit on a disjoint branch (Disconnected). Without this, the escalation above is unreachable, since no sync is ever started.check_orderreports the element the caller actually looked up. It hardcodedFinalized, so a missing safe block was also reported as "Finalized" — meaning those 2836 log lines may not have been about the finalized block at all.apply_fork_choice"should have succeeded as a shallow reorg" when that path having failed is precisely how execution reaches it. The bail itself is right: with an empty overlay range the state is genuinely absent locally, and deferring to sync is the honest answer — it just needed a sync that can recover.Deliberately out of scope: the boot-time refusal in
regenerate_head_state. A node that dies before the escalation completes still needsethrex removedb, so that path is worth a follow-up.Tests
Unit tests for
check_ordercover the missing-element label for both call sites, known blocks in the wrong order, and equal block numbers.The escalation path itself has no automated coverage — reaching it requires a store whose retained state has been pruned out from under a canonical chain plus live peers to snap-sync from, which the current sync tests cannot set up. It is exercised by
cargo clippy/checkonly. Reproducing it in CI would need a p2p harness that does not exist yet.