fix(syncer): refetch latest da height instead of da height +1#3201
fix(syncer): refetch latest da height instead of da height +1#3201julienrbrt wants to merge 7 commits intomainfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @julienrbrt's task in 1m 54s —— View job Code Review
SummaryThe fix correctly identifies that on restart without P2P, the syncer may get stuck because it advanced past a DA height that still had unprocessed blocks. Re-fetching from However, there is a critical bug in the implementation that silently breaks the main case. Critical Bug:
|
state.DAHeight |
state.DAHeight-1 (uint64) |
min(..., 0) |
Final daHeight |
|---|---|---|---|
| 0 | maxUint64 (underflow) |
0 | DAStartHeight |
| 1 | 0 | 0 | DAStartHeight |
| 5 | 4 | 0 ← wrong | DAStartHeight ← wrong |
| 100 | 99 | 0 ← wrong | DAStartHeight ← wrong |
The syncer will always restart from genesis DA height rather than state.DAHeight - 1. This means re-scanning all DA blocks from the beginning on every restart once the chain has progressed — it defeats the purpose of the fix entirely and could introduce significant startup latency on mature chains.
The underflow guard was likely intended to protect against state.DAHeight == 0, but min is the wrong function here. The correct way to compute state.DAHeight - 1 with underflow protection for uint64 is:
daHeight := max(s.genesis.DAStartHeight, max(state.DAHeight, 1)-1)This correctly gives:
state.DAHeight = 0→max(0, 1)-1 = 0→DAStartHeightstate.DAHeight = 1→max(1, 1)-1 = 0→DAStartHeightstate.DAHeight = 5→max(5, 1)-1 = 4→max(DAStartHeight, 4)state.DAHeight = 100→99→max(DAStartHeight, 99)
Minor Observation: P2P condition logic
The condition s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight is a reasonable heuristic for detecting "P2P is ahead of execution", but it's worth noting that headerStore.Height() may return a stale value if the node is restarting after a crash mid-sync. The comment accurately documents the intent, so this is acceptable as-is, but worth keeping in mind for future edge cases.
CHANGELOG
The entry is well-formed and accurate. ✓
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
| if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight { | ||
| daHeight = max(daHeight, s.cache.DaHeight()) | ||
| } | ||
| s.daRetrieverHeight.Store(daHeight) |
2 participants
Overview
If the latest da height contained more blocks, and one of those blocks got executed, without p2p the node will be stuck as it won't refetch the da heigth. We should re fetch daHeight-1 on start.
In our case, the cache was as well further, which is fine, but because we don't hold blocks in cache, we don't need this at all.