fix(syncer): refetch latest da height instead of da height +1#3201
fix(syncer): refetch latest da height instead of da height +1#3201tac0turtle merged 11 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughSyncer startup DA-height initialization was changed to derive the daRetrieverHeight from the last executed block's included DA heights (when available) instead of directly using the cache-wide Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 2m 57s —— View job Code Review
SummaryThe PR's intent is correct — restarting from 🔴 Critical:
|
|
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) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3201 +/- ##
==========================================
- Coverage 61.45% 61.42% -0.03%
==========================================
Files 120 120
Lines 12443 12449 +6
==========================================
Hits 7647 7647
- Misses 3939 3942 +3
- Partials 857 860 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
block/internal/syncing/syncer_test.go (1)
370-379:⚠️ Potential issue | 🟡 MinorType mismatch:
mockHeaderStoreandmockDataStoreuse wrong type parameters.In this test,
mockHeaderStoreis typed asMockStore[*types.SignedHeader]andmockDataStoreasMockStore[*types.Data], butNewSyncerexpectsheader.Store[*types.P2PSignedHeader]andheader.Store[*types.P2PData]. The correct mocks (mockP2PHeaderStoreandmockP2PDataStore) are created on lines 376-379 and correctly used inNewSyncercalls, butmockHeaderStoreandmockDataStoreon lines 370-374 appear to be unused artifacts.🧹 Suggested cleanup: remove unused mock declarations
- // Create mock stores for P2P - mockHeaderStore := extmocks.NewMockStore[*types.SignedHeader](t) - mockHeaderStore.EXPECT().Height().Return(uint64(0)).Maybe() - - mockDataStore := extmocks.NewMockStore[*types.Data](t) - mockDataStore.EXPECT().Height().Return(uint64(0)).Maybe() - mockP2PHeaderStore := extmocks.NewMockStore[*types.P2PSignedHeader](t) mockP2PHeaderStore.EXPECT().Height().Return(uint64(0)).Maybe() mockP2PDataStore := extmocks.NewMockStore[*types.P2PData](t) mockP2PDataStore.EXPECT().Height().Return(uint64(0)).Maybe()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer_test.go` around lines 370 - 379, Remove the unused, incorrectly-typed mocks by deleting the mockHeaderStore and mockDataStore declarations (symbols: mockHeaderStore, mockDataStore) in syncer_test.go; ensure the test relies only on the existing mockP2PHeaderStore and mockP2PDataStore used when constructing NewSyncer so there are no leftover mock artifacts or type mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/syncing/syncer.go`:
- Around line 358-363: The computed daHeight uses min(state.DAHeight-1, 0) which
always yields 0 for non-negative DA heights; change this to clamp the underflow
instead by using max(state.DAHeight-1, 0) so the line becomes daHeight :=
max(s.genesis.DAStartHeight, max(state.DAHeight-1, 0)); update the expression in
syncer.go (referencing daHeight, s.genesis.DAStartHeight, state.DAHeight,
s.cache.DaHeight(), s.headerStore.Height(), and s.daRetrieverHeight.Store) and
ensure any necessary type conversions are applied so the subtraction and
comparison do not underflow for unsigned types.
---
Outside diff comments:
In `@block/internal/syncing/syncer_test.go`:
- Around line 370-379: Remove the unused, incorrectly-typed mocks by deleting
the mockHeaderStore and mockDataStore declarations (symbols: mockHeaderStore,
mockDataStore) in syncer_test.go; ensure the test relies only on the existing
mockP2PHeaderStore and mockP2PDataStore used when constructing NewSyncer so
there are no leftover mock artifacts or type mismatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a076008c-af59-4027-9645-bc9cab4f1083
📒 Files selected for processing (4)
CHANGELOG.mdblock/internal/syncing/syncer.goblock/internal/syncing/syncer_forced_inclusion_test.goblock/internal/syncing/syncer_test.go
block/internal/syncing/syncer.go
Outdated
| // Only use cache.DaHeight() when P2P is actively syncing (headerStore has higher height than current state). | ||
| daHeight := max(s.genesis.DAStartHeight, min(state.DAHeight-1, 0)) | ||
| if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight { | ||
| daHeight = max(daHeight, s.cache.DaHeight()) | ||
| } | ||
| s.daRetrieverHeight.Store(daHeight) |
There was a problem hiding this comment.
Critical: min(state.DAHeight-1, 0) always evaluates to 0, ignoring the stored DA height.
The expression min(state.DAHeight-1, 0) will always return 0 when state.DAHeight >= 1 because min(positive_value, 0) is 0. This makes line 359 effectively equivalent to:
daHeight := max(s.genesis.DAStartHeight, 0) // state.DAHeight is ignoredThis contradicts the PR objective of refetching DA height - 1. The intent appears to be using max to clamp the underflow, not min.
🐛 Proposed fix to correctly compute DA height - 1 with underflow protection
// Set DA height to the maximum of the genesis start height, the state's DA height, and the cached DA height.
// The cache's DaHeight() is initialized from store metadata, so it's always correct even after cache clear.
// Only use cache.DaHeight() when P2P is actively syncing (headerStore has higher height than current state).
- daHeight := max(s.genesis.DAStartHeight, min(state.DAHeight-1, 0))
+ var daHeight uint64
+ if state.DAHeight > 0 {
+ daHeight = state.DAHeight - 1
+ }
+ daHeight = max(s.genesis.DAStartHeight, daHeight)
if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight {
daHeight = max(daHeight, s.cache.DaHeight())
}
s.daRetrieverHeight.Store(daHeight)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/syncing/syncer.go` around lines 358 - 363, The computed
daHeight uses min(state.DAHeight-1, 0) which always yields 0 for non-negative DA
heights; change this to clamp the underflow instead by using
max(state.DAHeight-1, 0) so the line becomes daHeight :=
max(s.genesis.DAStartHeight, max(state.DAHeight-1, 0)); update the expression in
syncer.go (referencing daHeight, s.genesis.DAStartHeight, state.DAHeight,
s.cache.DaHeight(), s.headerStore.Height(), and s.daRetrieverHeight.Store) and
ensure any necessary type conversions are applied so the subtraction and
comparison do not underflow for unsigned types.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
block/internal/syncing/syncer.go (1)
359-359:⚠️ Potential issue | 🟠 MajorLine 359 still drops the persisted DA height on cache miss.
min(state.DAHeight-1, 0)always yields0for auint64, andstate.DAHeight == 0underflows beforeminruns. So if the per-block cache lookup misses, startup falls back tos.genesis.DAStartHeightinstead ofstate.DAHeight-1, which defeats the recovery path this change is trying to add.🐛 Proposed fix
- daHeight := max(s.genesis.DAStartHeight, min(state.DAHeight-1, 0)) + daHeight := s.genesis.DAStartHeight + if state.DAHeight > 0 { + daHeight = max(daHeight, state.DAHeight-1) + }In Go, when `stateDAHeight` is a `uint64`, what are the results of `min(stateDAHeight-1, 0)` for (a) `stateDAHeight = 5` and (b) `stateDAHeight = 0`? Please cite the rules for unsigned integer arithmetic and the predeclared `min` function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer.go` at line 359, The expression daHeight := max(s.genesis.DAStartHeight, min(state.DAHeight-1, 0)) is incorrect for uint64: state.DAHeight-1 underflows when state.DAHeight==0 and min(...,0) always yields 0, dropping the intended state.DAHeight-1 fallback; fix by computing the previous DA height safely (e.g., prev := uint64(0); if state.DAHeight > 0 { prev = state.DAHeight - 1 }) and then use daHeight := max(s.genesis.DAStartHeight, prev) so the code in syncer.go uses a safe prev value derived from state.DAHeight rather than relying on min/max to handle unsigned underflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@block/internal/syncing/syncer.go`:
- Line 359: The expression daHeight := max(s.genesis.DAStartHeight,
min(state.DAHeight-1, 0)) is incorrect for uint64: state.DAHeight-1 underflows
when state.DAHeight==0 and min(...,0) always yields 0, dropping the intended
state.DAHeight-1 fallback; fix by computing the previous DA height safely (e.g.,
prev := uint64(0); if state.DAHeight > 0 { prev = state.DAHeight - 1 }) and then
use daHeight := max(s.genesis.DAStartHeight, prev) so the code in syncer.go uses
a safe prev value derived from state.DAHeight rather than relying on min/max to
handle unsigned underflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ebcf320-c385-48b7-b906-5022b5c12b28
📒 Files selected for processing (1)
block/internal/syncing/syncer.go
3 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.
Summary by CodeRabbit
Bug Fixes
Documentation