@niran Good catch from the review bot. The SynchronizeTask::execute returning Ok(()) on Syncing means reset_to_ancestor needs explicit response checking. A simple fix would be:

rust
let result = task.execute(&mut state).await.map_err(FollowError::engine_task)?;
// Verify the forkchoice update was actually applied
if !state.sync_state.safe_head().map_or(false, |h| h.block_info.hash == ancestor.block_info.hash) {
return Err(FollowError::RecoveryFailed(
"reset_to_ancestor: EL returned Syncing, head not updated".into()
));
}

This way recovery fails loudly instead of silently stalling the node.

🟡 Heimdall Review Status

Solid design overall @niran. The reset_to_ancestor → replay flow is clean and the stateful EL mock properly validates the Valid-vs-Syncing distinction. Two observations the bots didn't flag:

1. local_finalized can be stale when passed to recovery::recover

In update_safe_and_finalized (runtime.rs:~190), local_finalized is fetched before evaluate_label is called for the safe head. If evaluate_label returns Diverged, the same local_finalized which may now be stale is passed to recovery::recover → find_common_ancestor:

// runtime.rs:~190 — fetched early
let local_finalized = local.block_info(BlockNumberOrTag::Finalized).await?;

// runtime.rs:~207 — evaluate_label might trigger recovery later
LabelOutcome::Diverged { number, .. } => {
    generation.cancel();
    let finalized = local_finalized.as_ref().ok_or(...)?;  // <-- stale?
    let ancestor = recovery::recover(..., finalized, number).await?;

If the local's finalized head advances between the local.block_info(Finalized) call and the recovery::recover call, the finalized parameter points to a block below the actual finalized head. The binary search floor in find_common_ancestor is then too low, producing a lower common ancestor than necessary. Impact is benign the ancestor is still valid (on both chains) and reset_to_ancestor independently checks the live sync_state.finalized_head() for the finality guard. The cost is a slightly longer replay window, not a correctness issue.

Not requesting a change this is conservative by design, and re-fetching inside the Diverged arm would trade one TOCTOU for another. Worth documenting for future readers though.

2. SynchronizeTask's EngineSyncStateUpdate with finalized_head: None

In reset_to_ancestor (engine.rs:~124), the update uses ..Default::default():

EngineSyncStateUpdate {
    unsafe_head: Some(ancestor),
    local_safe_head: Some(ancestor),
    safe_head: Some(ancestor),
    ..Default::default()  // finalized_head = None
}

When SynchronizeTask::execute sends the fork_choice_updated_v3 to the EL with finalized_block_hash absent/zero, different EL implementations may behave differently. The Engine API spec says forkchoiceState requires headBlockHash; safeBlockHash and finalizedBlockHash are "optional" in the JSON-RPC spec but ForkchoiceState in alloy/alloy-rpc-types-engine serializes them as B256 (not Option<B256>), so they always transmit.

If the non-zero finalized hash from before the reset is sent (which is what actually happens since SynchronizeTask uses the state's current finalized_head from sync_state, not the update), the EL sees a finalized block above the new unsafe head which could trigger a SYNCING or INVALID response depending on how strict the EL's forkchoice validation is.

From the engine tests, the stateful EL mock accepts any forkchoice update to a known block as Valid regardless of the safe/finalized fields, so this doesn't surface in tests. But against a real EL (geth/reth), a finalized_block_hash above the current head would be rejected with InvalidPayloadAttributes or Syncing. Might be worth verifying on the local devnet that the replayed forkchoice updates during the recovery window carry the right finalized hash (the ancestor or below).

Neither is blocking the recovery logic is sound. Mostly surfacing these so the next person reading the code doesn't have to trace them separately.

Review Summary

The recovery design is sound: binary-search for the common ancestor, reset the EL to a block it already has (Valid forkchoice), then replay source payloads forward. The reset_to_ancestor confirmation check (engine.rs:126) correctly detects Syncing responses by verifying the sync state actually advanced. Error variants are well-modeled and tests exercise the key invariants (ancestor reset + replay, unknown-tip rejection, below-finalized refusal).

Finding

generation.cancel() before recover() tears down insert/fetch on recovery failure (runtime.rs:201) — If recover() fails after the generation token is already cancelled, the insert/fetch loops die and select! picks up their exit as either a clean shutdown or a misleading BlocksToInsertChannelClosed error. The actual recovery failure is only logged as warn!, not propagated. Moving generation.cancel() after a successful recover() would let failed attempts be retried on the next safety tick without tearing down the running insert loop.

Notes on prior bot comments

Several earlier inline comments on this PR are based on incorrect premises:

• The comment on recovery.rs claiming reset_to_ancestor silently succeeds on Syncing is wrong — the confirmation check at engine.rs:126 catches this and returns ResetToAncestorUnconfirmed.
• The comment on runtime.rs suggesting generation should be cancelled before recover — it already is (line 201).
• The comment on recovery.rs about re-fetching finalized — finalized is passed as a parameter from the caller; the only fetch is from the source to verify agreement.