fix: align sync status heuristic with leanSpec#417
Conversation
Greptile SummaryThis PR replaces the simplistic "head == current slot" sync heuristic with a stateful
Confidence Score: 4/5Safe to merge for the stated observability goal; the state-machine logic follows leanSpec PR #708 and is well-covered by the new unit tests. The state-machine logic and test coverage are solid. Two small issues are worth tidying before this feeds into validator-duty gating: the live-chain full-HashMap allocation on every 800 ms tick, and the bare unsigned subtraction in the hysteresis recovery condition which would silently break hysteresis in a release build if the constants are ever reordered. crates/blockchain/src/lib.rs — the update_sync_status helper and the hysteresis recovery expression on line 89.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/lib.rs | Core change: adds SyncStatusTracker with correct threshold/hysteresis logic; two minor issues — full HashMap allocation on every 800 ms tick and an unguarded unsigned subtraction in the recovery condition. |
| crates/blockchain/src/metrics.rs | Adds #[derive(Debug, Clone, Copy, PartialEq, Eq)] to SyncStatus to support equality assertions in the new unit tests; no logic changes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([Every Tick 800ms]) --> B{network_lag > 8?}
B -- Yes --> C[syncing = false]
B -- No --> D{currently syncing?}
D -- Yes --> E{head_lag > 2?}
E -- Yes --> F[syncing = true]
E -- No --> G[syncing = false]
D -- No --> H{head_lag > 4?}
H -- Yes --> I[syncing = true]
H -- No --> J[syncing = false]
C & G & J --> K([Synced])
F & I --> L([Syncing])
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/blockchain/src/lib.rs:721-727
`get_live_chain()` collects the entire `LiveChain` table into a `HashMap<H256, (u64, H256)>` only to compute the maximum slot. This allocation runs 5 times per slot (once per 800 ms tick). During finality-delay events the live chain can grow to hundreds of entries. Adding a dedicated `Store::max_live_chain_slot()` that iterates without collecting would avoid the allocation entirely.
```suggestion
let max_seen_slot = self
.store
.max_live_chain_slot()
.unwrap_or(head_slot);
```
### Issue 2 of 2
crates/blockchain/src/lib.rs:89
The expression `SYNC_LAG_THRESHOLD - SYNC_HYSTERESIS_BAND` is a bare `u64` subtraction. In a debug build this would panic if `SYNC_HYSTERESIS_BAND` were ever raised above `SYNC_LAG_THRESHOLD`; in a release build it silently wraps to a huge value, making the condition `head_lag > u64::MAX - delta` always false — the syncing flag would reset on the very next tick regardless of actual lag, silently breaking hysteresis. Using `saturating_sub` costs nothing and makes the intent explicit.
```suggestion
self.syncing = head_lag > SYNC_LAG_THRESHOLD.saturating_sub(SYNC_HYSTERESIS_BAND);
```
Reviews (1): Last reviewed commit: "fix: align sync status heuristic with le..." | Re-trigger Greptile
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
🗒️ Description / Motivation
lean_node_sync_statusto follow leanSpec PR feat: add validator sync-lag duty gate leanEthereum/leanSpec#708.What Changed
Correctness / Behavior Guarantees
syncingwhen its head is more than four slots behind while fresher validated blocks are known.synced.Tests Added / Run
cargo fmt --all -- --check.git diff --check.leanVMdependency is available.Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleancargo test --workspace --release— all passing