fix(l1): pre-check sync head height to skip snap on low-block networks#6536
fix(l1): pre-check sync head height to skip snap on low-block networks#6536
Conversation
When joining a fresh network whose head is only a few blocks deep, snap sync would stall in the header download loop before reaching the existing `head_close_to_0` fallback. Probe the sync head's header up-front; if its number is below MIN_FULL_BLOCKS, switch to full sync immediately. Falls through to the existing snap path if the probe can't reach any peer.
🤖 Kimi Code ReviewThis PR adds a pre-check to avoid snap sync on fresh devnets where the chain head is below Highlights
Minor suggestions
Security/Performance: No concerns. The probe adds minimal latency (max 6s) and the early full-sync return avoids expensive snap sync setup for short chains. LGTM. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR adds an upfront probe in Confidence Score: 4/5Safe to merge — fix is correct and consistent with existing snap/full transition logic; only P2 style notes remain. No P0 or P1 issues found. The probe logic is sound: the header hash is cryptographically bound so a single-peer response is trustworthy, single-element header responses pass the chaining check (vacuous windows(2).all), and the fallback on probe failure is conservative. The two P2 comments are minor — one is a missing attempt number in a debug log, the other is a documentation gap around the permanent nature of the snap-disable decision. No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/networking/p2p/sync.rs | Adds a pre-check probe before snap sync that fetches the sync head header by hash; if block number < MIN_FULL_BLOCKS it permanently disables snap and dispatches directly to full sync. Logic is correct and consistent with existing snap_sync.rs behaviour; two minor style/documentation P2s noted. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[sync_cycle called] --> B{snap_enabled?}
B -- No --> F[full::sync_cycle_full]
B -- Yes --> C[probe_sync_head_number\nup to 3 attempts]
C -- None / probe failed --> E[snap::sync_cycle_snap]
C -- Some number --> D{number < MIN_FULL_BLOCKS\n10 000}
D -- No --> E
D -- Yes --> G[snap_enabled = false\nfull::sync_cycle_full]
E --> H{head_close_to_0\nor sync_head found?}
H -- Yes --> I[snap_enabled = false\nfull::sync_cycle_full]
H -- No --> J[continue snap sync loop]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/networking/p2p/sync.rs
Line: 274-278
Comment:
**Missing attempt number in debug log**
When `Ok(Some(headers))` is returned but the target header isn't found, the debug message doesn't include the attempt number, unlike the other two branches. In a scenario where all 3 attempts hit this case (e.g. a peer consistently returns a response anchored to a different fork root), the three identical log lines with no attempt context make it harder to diagnose.
```suggestion
debug!("Sync head probe attempt {attempt}/{PROBE_SYNC_HEAD_ATTEMPTS}: response did not contain target header");
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/networking/p2p/sync.rs
Line: 219
Comment:
**Permanent snap-disable on first eligible sync_head**
`snap_enabled` is set to `false` here before dispatching to full sync, and it is never reset to `true`. If the node is started while the devnet chain is short (< `MIN_FULL_BLOCKS`) but then the network advances past that threshold before the first full sync cycle completes, subsequent `sync_cycle` calls will still skip the snap path entirely.
This matches the existing behaviour in `snap_sync.rs` (line 225 sets the same flag permanently on `head_close_to_0`), so it is consistent — but worth documenting explicitly here since the pre-check fires much earlier and at a point where the "permanent" decision may be harder to notice.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(l1): pre-check sync head height to s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Ok(Some(headers)) => { | ||
| if let Some(header) = headers.iter().find(|h| h.hash() == sync_head) { | ||
| return Some(header.number); | ||
| } | ||
| debug!("Sync head probe: response did not contain target header"); |
There was a problem hiding this comment.
When Ok(Some(headers)) is returned but the target header isn't found, the debug message doesn't include the attempt number, unlike the other two branches. In a scenario where all 3 attempts hit this case (e.g. a peer consistently returns a response anchored to a different fork root), the three identical log lines with no attempt context make it harder to diagnose.
| Ok(Some(headers)) => { | |
| if let Some(header) = headers.iter().find(|h| h.hash() == sync_head) { | |
| return Some(header.number); | |
| } | |
| debug!("Sync head probe: response did not contain target header"); | |
| debug!("Sync head probe attempt {attempt}/{PROBE_SYNC_HEAD_ATTEMPTS}: response did not contain target header"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/p2p/sync.rs
Line: 274-278
Comment:
**Missing attempt number in debug log**
When `Ok(Some(headers))` is returned but the target header isn't found, the debug message doesn't include the attempt number, unlike the other two branches. In a scenario where all 3 attempts hit this case (e.g. a peer consistently returns a response anchored to a different fork root), the three identical log lines with no attempt context make it harder to diagnose.
```suggestion
debug!("Sync head probe attempt {attempt}/{PROBE_SYNC_HEAD_ATTEMPTS}: response did not contain target header");
```
How can I resolve this? If you propose a fix, please make it concise.| sync_head_number, | ||
| "Sync head below MIN_FULL_BLOCKS ({MIN_FULL_BLOCKS}), using full sync" | ||
| ); | ||
| self.snap_enabled.store(false, Ordering::Relaxed); |
There was a problem hiding this comment.
snap_enabled is set to false here before dispatching to full sync, and it is never reset to true. If the node is started while the devnet chain is short (< MIN_FULL_BLOCKS) but then the network advances past that threshold before the first full sync cycle completes, subsequent sync_cycle calls will still skip the snap path entirely.
This matches the existing behaviour in snap_sync.rs (line 225 sets the same flag permanently on head_close_to_0), so it is consistent — but worth documenting explicitly here since the pre-check fires much earlier and at a point where the "permanent" decision may be harder to notice.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/p2p/sync.rs
Line: 219
Comment:
**Permanent snap-disable on first eligible sync_head**
`snap_enabled` is set to `false` here before dispatching to full sync, and it is never reset to `true`. If the node is started while the devnet chain is short (< `MIN_FULL_BLOCKS`) but then the network advances past that threshold before the first full sync cycle completes, subsequent `sync_cycle` calls will still skip the snap path entirely.
This matches the existing behaviour in `snap_sync.rs` (line 225 sets the same flag permanently on `head_close_to_0`), so it is consistent — but worth documenting explicitly here since the pre-check fires much earlier and at a point where the "permanent" decision may be harder to notice.
How can I resolve this? If you propose a fix, please make it concise.
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewPR #6536 Review:
|
🤖 Codex Code ReviewFindings
Otherwise the change is small and I don’t see EVM, gas-accounting, trie, or consensus-rule issues in the touched logic. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
2 participants
Summary
head_close_to_0fallback insync_cycle_snap(peers are barely synced themselves andrequest_block_headersreturns no usable batch).Syncer::sync_cycle: fetch the sync head's header by hash; ifheader.number < MIN_FULL_BLOCKS, flipsnap_enabledto false and dispatch directly tosync_cycle_full.Test plan
>= MIN_FULL_BLOCKS, snap path runs as before).