fix(l1): prevent snap sync crash on pivot update failure

[ElFantasma]

Motivation

Snap sync crashes ~20% of the time on mainnet (and deterministically on small networks like hoodi) when update_pivot exhausts its 15-failure budget by repeatedly asking the same 1–2 peers. The crash (process::exit(2)) also corrupts the DB, requiring a full removedb and resync.

Root cause analysis and full details in #6474.

Description

Three targeted fixes that prevent the crash while leaving deeper peer selection improvements for a follow-up:

1. Reclassify PeerHandler and NoBlockHeaders errors as recoverable — instead of process::exit(2), the sync cycle retries. This is the key fix: even if peer selection is suboptimal, the node stays alive.

2. Add get_best_peer_excluding() — update_pivot now tracks which peers already failed and excludes them from selection. When all eligible peers have been tried, the exclude list clears and a new rotation starts. This ensures every peer gets a chance before any is retried.

3. Bump MAX_TOTAL_FAILURES from 15 to 100 and stop counting "no peers available" rotation resets as failures. The old budget of 15 was exhausted in ~2 minutes, with 40–73% of failures being passive waits rather than actual requests.

Checklist

• Blockchain / VM / EVM tests pass
• cargo fmt clean
• No new warnings

[github-actions]

🤖 Claude Code Review

Now I have everything I need to write a thorough review.

---

PR Review: fix(l1): prevent snap sync crash on pivot update failure

Overall assessment: This is a solid, targeted fix that directly addresses a real crash. The motivation is well-documented, the three changes are cohesive, and the diff is clean. There are a few correctness concerns worth discussing before merge.

---

sync.rs — is_recoverable reclassification

Correct and important. Moving NoBlockHeaders and PeerHandler(_) to the recoverable arm is the right call. Both represent transient network/peer conditions, not corrupted local state.

One concern worth raising: all PeerHandlerError variants are now recoverable. If PeerHandlerError includes subtypes that indicate a fundamental protocol incompatibility (e.g., wrong network, unsupported version), treating those as transiently retryable could cause the sync loop to spin indefinitely rather than surfacing the real problem. Worth verifying that no fatal protocol-level variants live inside PeerHandlerError.

---

snap_sync.rs — update_pivot refactor

Correctness issue — PeerHandlerError bypasses peer rotation:

The new exclusion-list logic only activates when get_block_header returns Ok(None). When it returns Err(PeerHandlerError), the ? at snap_sync.rs:797 still propagates immediately out of update_pivot:

let result = peers
    .get_block_header(peer_id, &mut connection, new_pivot_block_number)
    .await
    .map_err(SyncError::PeerHandler)?;   // exits update_pivot entirely

So in the PeerHandlerError case:

• excluded_peers is never updated for that peer.
• The next update_pivot call starts with a fresh, empty excluded_peers.
• The same peer may be selected and fail again in an identical cycle.

The peer exclusion machinery only helps for None responses; the other half of the failure space (protocol errors) still bypasses it. Since PeerHandlerError is now recoverable, this won't crash — but it weakens the "rotate through all peers before retrying" guarantee. Consider handling the error path the same way:

match peers.get_block_header(...).await {
    Err(e) => {
        peers.peer_table.record_failure(peer_id)?;
        total_failures = total_failures.saturating_add(1);
        break; // move to next peer via exclusion
    }
    Ok(result) => { /* existing None/Some handling */ }
}

consecutive_failures_on_current is always MAX_RETRIES_PER_PEER at the log site:

The inner for loop runs the full 0..MAX_RETRIES_PER_PEER range and only exits early on success. Every time the peer is excluded, consecutive_failures_on_current equals MAX_RETRIES_PER_PEER (3). The debug log at snap_sync.rs:838 will always print "3 failures". Either use the constant directly or restructure to track only actual failures if the peer can also fail via PeerHandlerError (Point 1 above).

MAX_RETRY_DELAY is unreachable:

const MAX_RETRY_DELAY: Duration = Duration::from_secs(30);
// ...
let delay = INITIAL_RETRY_DELAY.saturating_mul(1 << (total_failures / 10).min(4));
let delay = delay.min(MAX_RETRY_DELAY);

The exponent is capped at 4, so the maximum multiplier is 2^4 = 16, giving 1s * 16 = 16s. The .min(MAX_RETRY_DELAY) (30s) is never triggered. Either raise the cap to 1 << 5 = 32s (just above 30s so the cap kicks in) or remove MAX_RETRY_DELAY and use the .min(4) directly. This was present before the PR but the refactor is a good opportunity to clean it up.

Backoff fires on rotation resets (minor, likely intentional):

After excluded_peers.clear() and continue, the next outer-loop iteration hits the if total_failures > 0 { sleep(...) } guard and sleeps again before selecting a peer. This means there are two consecutive sleeps when transitioning between rotations. The comment says "Don't count rotation resets as failures" (correct), but the backoff still fires. Probably fine as a throttle, but worth a comment to make the intent clear.

---

peer_table.rs — do_get_best_peer_excluding

Code duplication: do_get_best_peer and do_get_best_peer_excluding are identical except for the excluded.contains(id) guard. The simpler implementation:

fn do_get_best_peer(&self, capabilities: &[Capability]) -> Option<(H256, PeerConnection)> {
    self.do_get_best_peer_excluding(capabilities, &[])
}

This eliminates a divergence risk if the peer-selection logic changes in the future.

excluded.contains(id) is O(n) per peer per lookup. With typical peer counts (dozens) this is not a practical problem, but if excluded_peers is expected to grow (e.g., in denser networks), a HashSet<H256> would be more appropriate. Given H256 is Copy + Eq + Hash, this is a one-line change at the call site.

Unnecessary clone per loop iteration in snap_sync.rs:

.get_best_peer_excluding(SUPPORTED_ETH_CAPABILITIES.to_vec(), excluded_peers.clone())

The Vec<H256> is cloned on every outer-loop iteration because the protocol trait takes ownership. This is likely unavoidable with the actor-message model (which requires owned arguments for serialization), but worth confirming. If the trait could take &[H256] instead, the allocation is eliminated. Minor in practice.

---

Positive notes

• Reclassifying NoBlockHeaders and PeerHandler as recoverable directly solves the crash without touching unrelated code. Good surgical scope.
• The empty-vs-nonempty excluded_peers distinction in the None branch (snap_sync.rs:746-760) is correct and well-commented.
• MAX_TOTAL_FAILURES: 100 with the new accounting is much more appropriate than 15.
• Removing the unnecessary .clone() on the Ok(pivot) return is a good cleanup.
• The typo fix ("Succesfully" → "Successfully") is appreciated.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[greptile-apps]

Misleading backoff comment

The comment says "based on how many full rotations we've done" but the formula total_failures / 10 is not the number of full peer rotations — it's just total failures divided by 10. With 3 failures per peer and, say, 4 peers, a full rotation produces 12 failures, making the "per-rotation" interpretation off by 1.2×. Consider updating the comment to reflect the actual formula.

Suggested change

	// Exponential backoff based on how many full rotations we've done
	if total_failures > 0 {
	let delay = INITIAL_RETRY_DELAY.saturating_mul(1 << (total_failures / 10).min(4));
	let delay = delay.min(MAX_RETRY_DELAY);
	// Exponential backoff: delay doubles every 10 total failures, capped at MAX_RETRY_DELAY
	if total_failures > 0 {
	let delay = INITIAL_RETRY_DELAY.saturating_mul(1 << (total_failures / 10).min(4));
	let delay = delay.min(MAX_RETRY_DELAY);

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/networking/p2p/sync/snap_sync.rs
Line: 725-728

Comment:
**Misleading backoff comment**

The comment says "based on how many full rotations we've done" but the formula `total_failures / 10` is not the number of full peer rotations — it's just total failures divided by 10. With 3 failures per peer and, say, 4 peers, a full rotation produces 12 failures, making the "per-rotation" interpretation off by 1.2×. Consider updating the comment to reflect the actual formula.

```suggestion
        // Exponential backoff: delay doubles every 10 total failures, capped at MAX_RETRY_DELAY
        if total_failures > 0 {
            let delay = INITIAL_RETRY_DELAY.saturating_mul(1 << (total_failures / 10).min(4));
            let delay = delay.min(MAX_RETRY_DELAY);
```

How can I resolve this? If you propose a fix, please make it concise.

[ElFantasma]

Addressed in 87880b0. Replaced total_failures / 10 with explicit rotation_count; backoff now doubles per rotation, comment matches implementation.

[greptile-apps]

Busy peers can trigger premature rotation reset

get_best_peer_excluding returns None for two distinct reasons: (a) every non-excluded peer was tried this rotation, or (b) non-excluded peers exist but are all over the MAX_CONCURRENT_REQUESTS_PER_PEER limit or lack ETH capabilities. The code treats both as "rotation complete" because excluded_peers.is_empty() is false in either case.

In case (b), excluded_peers.clear() re-admits recently-failed peers to the candidate pool before the busy peers become available, potentially looping through the same failed peer(s) again while perfectly healthy-but-momentarily-busy peers wait. The rotation isn't truly exhausted — it just has no current slot.

A tighter guard (e.g., comparing excluded_peers.len() against eligible_count from the diagnostics snapshot already fetched a few lines down) would distinguish the two cases, though this is minor given the 100-failure budget.

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/networking/p2p/sync/snap_sync.rs
Line: 740-756

Comment:
**Busy peers can trigger premature rotation reset**

`get_best_peer_excluding` returns `None` for two distinct reasons: (a) every non-excluded peer was tried this rotation, or (b) non-excluded peers exist but are all over the `MAX_CONCURRENT_REQUESTS_PER_PEER` limit or lack ETH capabilities. The code treats both as "rotation complete" because `excluded_peers.is_empty()` is false in either case.

In case (b), `excluded_peers.clear()` re-admits recently-failed peers to the candidate pool before the busy peers become available, potentially looping through the same failed peer(s) again while perfectly healthy-but-momentarily-busy peers wait. The rotation isn't truly exhausted — it just has no current slot.

A tighter guard (e.g., comparing `excluded_peers.len()` against `eligible_count` from the diagnostics snapshot already fetched a few lines down) would distinguish the two cases, though this is minor given the 100-failure budget.

How can I resolve this? If you propose a fix, please make it concise.

[ElFantasma]

Addressed in 87880b0. When get_best_peer_excluding returns None we now probe get_best_peer (no exclusions) to tell whether (a) no peer is eligible at all (wait), (b) peers exist but none selectable (retry), or (c) all non-excluded peers were tried (rotation complete, clear list and bump rotation_count). Busy peers no longer trigger a premature reset.

[greptile-apps]

Duplicated peer-selection logic

do_get_best_peer_excluding is a strict superset of do_get_best_peer — only one extra excluded.contains(id) condition. Consider delegating to a shared helper to keep the scoring/eligibility logic in one place:

fn do_get_best_peer(&self, capabilities: &[Capability]) -> Option<(H256, PeerConnection)> {
    self.do_get_best_peer_excluding(capabilities, &[])
}

This avoids future drift if the eligibility check (e.g., can_try_more_requests) changes in one method but not the other.

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/networking/p2p/peer_table.rs
Line: 912-936

Comment:
**Duplicated peer-selection logic**

`do_get_best_peer_excluding` is a strict superset of `do_get_best_peer` — only one extra `excluded.contains(id)` condition. Consider delegating to a shared helper to keep the scoring/eligibility logic in one place:

```rust
fn do_get_best_peer(&self, capabilities: &[Capability]) -> Option<(H256, PeerConnection)> {
    self.do_get_best_peer_excluding(capabilities, &[])
}
```

This avoids future drift if the eligibility check (e.g., `can_try_more_requests`) changes in one method but not the other.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[ElFantasma]

Addressed in 87880b0. do_get_best_peer now delegates to do_get_best_peer_excluding(caps, &[]).

[ElFantasma]

Review feedback addressed (commit 87880b0)

Thanks for the thorough reviews. All eight findings across Codex, Claude, and Greptile are addressed.

Codex

1. MAX_TOTAL_FAILURES = 100 too low for large peer sets — Fixed. Replaced the hardcoded failure count with MAX_ROTATIONS = 5. The cap now scales naturally with peer count: one rotation = every eligible peer tried once.

2. Backoff formula scaled with failures, not rotations — Fixed. Now uses an explicit rotation_count; backoff doubles per rotation (1s, 2s, 4s, 8s, 16s, 30s).

3. PeerHandlerError wholesale recoverable — Fixed. Added PeerHandlerError::is_recoverable() and made SyncError::is_recoverable() delegate. Only transient peer-interaction variants (BlockHeaders, SendMessageToPeer, EmptyResponseFromPeer, timeouts, etc.) retry. Actor failures (PeerTableError), StorageFull, and Snap(_) stay fatal — a dead peer-table actor now exits the node instead of looping forever.

Claude

4. PeerHandlerError bypassed peer rotation — Fixed. The inner loop now matches the outcome: Ok(Some) succeeds, Ok(None) and recoverable errors both count as a failure (advance retries, exclude peer on exhaustion), non-recoverable errors propagate. This closes the rotation gap you identified.

5. consecutive_failures_on_current always printed 3 — Fixed. Variable is now peer_failures, counted separately from the retry loop index. After docs: update milestones. #4 it reflects actual failure attempts (not just the loop cap), though in the error-free case it will still equal MAX_RETRIES_PER_PEER.

Greptile (inline replies above)

6. Misleading backoff comment — fixed with docs: add milestones #1/chore: create project structure #2.
7. Busy peers triggering premature rotation reset — fixed by probing get_best_peer (no exclusions) to distinguish "rotation done" from "all at capacity".
8. Duplicated peer-selection logic — do_get_best_peer now delegates to do_get_best_peer_excluding(caps, &[]).

Not addressed

Claude's note about MAX_RETRY_DELAY being unreachable is now addressed as a side-effect of #2: the new per-rotation backoff (1s << rotation_count) reaches 16s at rotation 4 and hits the 30s cap on rotation 5.

[iovoid]

Why not call e.is_recoverable() here?

[iovoid]

get_block_header doesn't actually propagate most error types, it returns Ok(None)

(it seems like it could return PeerHandlerError::BlockHeaders, but that is unreachable)

[iovoid]

Moved to #6506