Open
Conversation
Contributor
📝 WalkthroughWalkthroughReworks how the DA retriever start height is determined during syncer initialization: derive baseline from genesis+persisted state, only elevate it from cache when P2P syncing is active (headerStore present and ahead), and allow an explicit override via Changes
Sequence Diagram(s)sequenceDiagram
participant Syncer
participant State
participant Cache
participant HeaderStore
participant Config
Syncer->>State: read persisted state (LastBlockHeight, DAHeight, genesis)
Syncer->>Config: read DA.StartHeight
alt Config.StartHeight > 0
Syncer->>Syncer: set daHeight = Config.StartHeight
else
Syncer->>Syncer: baseline daHeight = genesis.DAStartHeight
alt state.DAHeight > genesis.DAStartHeight
Syncer->>Syncer: baseline daHeight = state.DAHeight - 1
end
HeaderStore->>Syncer: provide headerStore (may be nil)
alt headerStore exists and HeaderStore.Height() > state.LastBlockHeight
Syncer->>Cache: request cached DA height (s.cache.DaHeight())
Cache-->>Syncer: cachedDaHeight
Syncer->>Syncer: daHeight = max(baseline, cachedDaHeight)
else
Syncer->>Syncer: keep baseline daHeight
end
end
Syncer->>Syncer: store s.daRetrieverHeight = daHeight
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
Contributor
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Contributor
|
Claude Code is working… I'll analyze this and get back to you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 359-362: The daHeight computation uses min(state.DAHeight-1, 0)
which always yields 0; change it to compute a safe candidate from state.DAHeight
(use candidate = state.DAHeight-1 when state.DAHeight > 0, else candidate = 0)
and then set daHeight = max(s.genesis.DAStartHeight, candidate). Keep the
subsequent conditional that bumps daHeight to max(daHeight, s.cache.DaHeight())
when s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight;
update the code around s.genesis.DAStartHeight, state.DAHeight, s.headerStore,
s.cache.DaHeight(), and state.LastBlockHeight accordingly.
🪄 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: 08094f15-06b3-4f6a-bf87-9ba1939c5d75
📒 Files selected for processing (1)
block/internal/syncing/syncer.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3204 +/- ##
==========================================
- Coverage 61.42% 61.41% -0.02%
==========================================
Files 120 120
Lines 12449 12456 +7
==========================================
+ Hits 7647 7650 +3
- Misses 3942 3946 +4
Partials 860 860
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:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
block/internal/syncing/syncer.go (1)
359-362:⚠️ Potential issue | 🔴 CriticalBug:
min(state.DAHeight-1, 0)always evaluates to 0 foruint64.This is a critical logic error that was flagged in a previous review but remains unfixed:
- When
state.DAHeight > 0: e.g., ifstate.DAHeight = 5, thenmin(4, 0) = 0- When
state.DAHeight = 0: underflows toMaxUint64, thenmin(MaxUint64, 0) = 0The result is that
daHeightalways equalss.genesis.DAStartHeight, completely ignoring the persistedstate.DAHeight. This contradicts the comment at line 356-358 which states the intent is to use the maximum of genesis, state, and cache DA heights.🐛 Proposed fix
// 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)) + daHeight := s.genesis.DAStartHeight + if state.DAHeight > 0 { + // Use state.DAHeight - 1 to re-fetch the last DA height in case it wasn't fully processed + daHeight = max(daHeight, state.DAHeight-1) + } if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight { daHeight = max(daHeight, s.cache.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 359 - 362, The computation for daHeight incorrectly uses min(state.DAHeight-1, 0) on a uint64 (always 0); instead compute a safe state candidate by checking if state.DAHeight > 0 then use state.DAHeight-1 else use 0, then set daHeight = max(s.genesis.DAStartHeight, stateCandidate) and finally, if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight keep the existing behavior of daHeight = max(daHeight, s.cache.DaHeight()); update the logic around the daHeight variable and reference s.genesis.DAStartHeight, state.DAHeight, s.cache.DaHeight(), s.headerStore.Height(), and state.LastBlockHeight when making the change.
🧹 Nitpick comments (1)
block/internal/syncing/syncer.go (1)
357-358: Comment may be misleading about cache behavior after clear.The comment states "The cache's DaHeight() is initialized from store metadata, so it's always correct even after cache clear." However, based on the relevant code snippets:
RestoreFromStore()(context snippet 1) only runs once at initializationmaxDAHeight(context snippet 3) is updated viasetMaxDAHeight()only when items are added- After a cache clear during syncing,
maxDAHeightcan regress if no new items with higher DA heights are subsequently addedThis means
s.cache.DaHeight()may return a lower value than expected after a cache clear, which could cause re-fetching of already-processed DA heights. Consider clarifying the comment or verifying the intended behavior.🤖 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 357 - 358, The comment incorrectly guarantees cache.DaHeight() stays correct after a clear; because RestoreFromStore() runs only at init and maxDAHeight is updated only via setMaxDAHeight() when items are added, clearing the cache can leave maxDAHeight too low and cause re-fetches. Fix by either updating the comment to state that DaHeight may regress after a cache clear unless restored from persistent metadata, or change the logic: on cache.Clear()/during syncing ensure RestoreFromStore() or a persistent max DA readback is invoked (or retain/restore maxDAHeight from store metadata) so that s.cache.DaHeight() reflects the true maximum; reference RestoreFromStore, setMaxDAHeight, maxDAHeight and cache.DaHeight in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/config.go`:
- Line 570: Fix the typo in the flag help string passed to cmd.Flags().String
for FlagDBPath in config.go: change the duplicated wording "path for for node
database" to a correct phrase such as "path for node database" (or "path to node
database"); ensure the call creating the flag remains
cmd.Flags().String(FlagDBPath, def.DBPath, "<corrected help text>").
---
Duplicate comments:
In `@block/internal/syncing/syncer.go`:
- Around line 359-362: The computation for daHeight incorrectly uses
min(state.DAHeight-1, 0) on a uint64 (always 0); instead compute a safe state
candidate by checking if state.DAHeight > 0 then use state.DAHeight-1 else use
0, then set daHeight = max(s.genesis.DAStartHeight, stateCandidate) and finally,
if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight keep
the existing behavior of daHeight = max(daHeight, s.cache.DaHeight()); update
the logic around the daHeight variable and reference s.genesis.DAStartHeight,
state.DAHeight, s.cache.DaHeight(), s.headerStore.Height(), and
state.LastBlockHeight when making the change.
---
Nitpick comments:
In `@block/internal/syncing/syncer.go`:
- Around line 357-358: The comment incorrectly guarantees cache.DaHeight() stays
correct after a clear; because RestoreFromStore() runs only at init and
maxDAHeight is updated only via setMaxDAHeight() when items are added, clearing
the cache can leave maxDAHeight too low and cause re-fetches. Fix by either
updating the comment to state that DaHeight may regress after a cache clear
unless restored from persistent metadata, or change the logic: on
cache.Clear()/during syncing ensure RestoreFromStore() or a persistent max DA
readback is invoked (or retain/restore maxDAHeight from store metadata) so that
s.cache.DaHeight() reflects the true maximum; reference RestoreFromStore,
setMaxDAHeight, maxDAHeight and cache.DaHeight in your change.
🪄 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: 7b99f109-c982-4c0c-a05c-190556abf106
📒 Files selected for processing (3)
block/internal/syncing/syncer.gopkg/config/config.gopkg/config/config_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/config/config_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/config/config.go (1)
570-570:⚠️ Potential issue | 🟡 MinorFix the duplicated word in the DB path help text.
--helpcurrently printspath for for node database.📝 Suggested fix
- cmd.Flags().String(FlagDBPath, def.DBPath, "path for for node database") + cmd.Flags().String(FlagDBPath, def.DBPath, "path to node database")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config.go` at line 570, The help text for the DB path flag contains a duplicated word; update the call to cmd.Flags().String(FlagDBPath, def.DBPath, ...) to remove the extra "for" so the description reads "path for node database" (or a clearer alternative like "path to node database"); edit the string literal passed to cmd.Flags().String where FlagDBPath and def.DBPath are used to fix the typo.
🧹 Nitpick comments (1)
block/internal/syncing/syncer.go (1)
367-374: Treat the manual DA start-height as an unsafe override and validate it.A mistyped value here can move the follower before
s.genesis.DAStartHeightor beyond the recovered watermark and silently skip historical DA heights. Please reject impossible values, and at least emit a warning when operators opt into an unsafe jump.As per coding guidelines "Validate all inputs from external sources in Go code".
🤖 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 367 - 374, The manual DA start-height override (s.config.DA.StartHeight) must be validated: ensure startHeight >= s.genesis.DAStartHeight and startHeight <= daHeight (the recovered watermark) and reject (return an error) if out of that range; if the value is in-range but not equal to the current daHeight emit a warning via s.logger.Warn() that this is an unsafe jump/override; keep the existing info log when accepted but do not silently accept impossible values—use s.logger to warn and return a clear error when validation fails.
🤖 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 356-365: The current logic uses s.cache.DaHeight() (a global max)
to bump daHeight when headerStore is ahead, which can skip DA ranges; instead,
derive the bump from the per-block included-by entry for state.LastBlockHeight
(the persisted per-block inclusion height written by the submitter) or leave
daHeight unchanged if that per-block entry is missing. Concretely: replace the
branch that uses s.cache.DaHeight() with a lookup of the included-height for
state.LastBlockHeight (or call the cache/manager API that returns the
included-by height for a specific block) and max that value into daHeight only
when that per-block value exists; otherwise do not use the cache-wide
s.cache.DaHeight(). Ensure you still respect s.genesis.DAStartHeight and the
state.DAHeight-1 logic.
---
Duplicate comments:
In `@pkg/config/config.go`:
- Line 570: The help text for the DB path flag contains a duplicated word;
update the call to cmd.Flags().String(FlagDBPath, def.DBPath, ...) to remove the
extra "for" so the description reads "path for node database" (or a clearer
alternative like "path to node database"); edit the string literal passed to
cmd.Flags().String where FlagDBPath and def.DBPath are used to fix the typo.
---
Nitpick comments:
In `@block/internal/syncing/syncer.go`:
- Around line 367-374: The manual DA start-height override
(s.config.DA.StartHeight) must be validated: ensure startHeight >=
s.genesis.DAStartHeight and startHeight <= daHeight (the recovered watermark)
and reject (return an error) if out of that range; if the value is in-range but
not equal to the current daHeight emit a warning via s.logger.Warn() that this
is an unsafe jump/override; keep the existing info log when accepted but do not
silently accept impossible values—use s.logger to warn and return a clear error
when validation fails.
🪄 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: 63b1d6c5-1133-4102-8ea6-6061279613ef
📒 Files selected for processing (2)
block/internal/syncing/syncer.gopkg/config/config.go
Comment on lines
356
to
365
| // Set DA height to the maximum of the genesis start height, the state's DA height, and the cached DA height. | ||
| // Use the DA height from the last executed block instead of the maximum from all blocks, | ||
| // because P2P-fetched heights may be lost on restart. | ||
| daHeight := max(s.genesis.DAStartHeight, min(state.DAHeight-1, 0)) | ||
| if state.LastBlockHeight > 0 { | ||
| if lastHeaderDA, ok := s.cache.GetHeaderDAIncludedByHeight(state.LastBlockHeight); ok { | ||
| daHeight = max(daHeight, lastHeaderDA) | ||
| } | ||
| if lastDataDA, ok := s.cache.GetDataDAIncludedByHeight(state.LastBlockHeight); ok { | ||
| daHeight = max(daHeight, lastDataDA) | ||
| } | ||
| // 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 := s.genesis.DAStartHeight | ||
| if state.DAHeight > s.genesis.DAStartHeight { | ||
| daHeight = max(daHeight, state.DAHeight-1) | ||
| } | ||
| if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight { | ||
| daHeight = max(daHeight, s.cache.DaHeight()) | ||
| } |
Contributor
There was a problem hiding this comment.
Resume from the last applied block's DA height, not the cache-wide max.
block/internal/cache/manager.go:174-176 defines DaHeight() as the highest DA height ever seen across the caches, while block/internal/submitting/submitter.go:440-458 still persists per-block inclusion heights. Using the global max here can jump the retriever past untouched DA ranges whenever later submissions are already in cache metadata, and those skipped heights are no longer covered by the sequential catchup path when P2P priority hints are dropped. Please derive this bump from the included-by-height entries for state.LastBlockHeight, or leave the recovered watermark unchanged when they are missing.
🛠️ Safer resume logic
- if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight {
- daHeight = max(daHeight, s.cache.DaHeight())
- }
+ if s.headerStore != nil && s.headerStore.Height() > state.LastBlockHeight {
+ if headerDAHeight, ok := s.cache.GetHeaderDAIncludedByHeight(state.LastBlockHeight); ok {
+ daHeight = max(daHeight, headerDAHeight)
+ }
+ if dataDAHeight, ok := s.cache.GetDataDAIncludedByHeight(state.LastBlockHeight); ok {
+ daHeight = max(daHeight, dataDAHeight)
+ }
+ }🤖 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 356 - 365, The current logic
uses s.cache.DaHeight() (a global max) to bump daHeight when headerStore is
ahead, which can skip DA ranges; instead, derive the bump from the per-block
included-by entry for state.LastBlockHeight (the persisted per-block inclusion
height written by the submitter) or leave daHeight unchanged if that per-block
entry is missing. Concretely: replace the branch that uses s.cache.DaHeight()
with a lookup of the included-height for state.LastBlockHeight (or call the
cache/manager API that returns the included-by height for a specific block) and
max that value into daHeight only when that per-block value exists; otherwise do
not use the cache-wide s.cache.DaHeight(). Ensure you still respect
s.genesis.DAStartHeight and the state.DAHeight-1 logic.
julienrbrt
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
My previous PR got merged with a test commit (longer version of this). This should have been merged instead.
Same behavior.
Summary by CodeRabbit