Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds helpers to read/write 8-byte little-endian uint64 metadata and replaces manual encoding/decoding with them; test setups switch from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 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 |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task in 1m 27s —— View job Code Review
Overall, this is a clean deduplication PR. The helper functions are a good idea. However, there are two substantive bugs and one missed deduplication that are worth addressing. Critical: Persist before advancing in-memory state (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3139 +/- ##
=======================================
Coverage 61.12% 61.12%
=======================================
Files 113 113
Lines 11676 11674 -2
=======================================
- Hits 7137 7136 -1
- Misses 3737 3738 +1
+ Partials 802 800 -2
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: 2
🤖 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/cache/manager.go`:
- Around line 390-405: getMetadataUint64 currently collapses backend errors and
malformed values into a missing result; change its signature to return (uint64,
bool, error) so it returns the raw error when st.GetMetadata fails or when
len(b) != 8 (e.g., return an fmt.Errorf describing the malformed length), and
only use ok==false for the explicit not-found case; then update
initDAHeightFromStore to call value, ok, err := getMetadataUint64(ctx, m.store,
store.DAIncludedHeightKey), handle/log err distinctly (e.g.,
m.logger.Error/return) and only treat the not-found (ok==false) path as a
harmless skip, preserving transient read failures for diagnostics.
In `@block/internal/submitting/submitter.go`:
- Around line 357-359: The DA included height is advanced and the per-height
cache is deleted even when putUint64Metadata(s.ctx, s.store,
store.DAIncludedHeightKey, nextHeight) fails; modify the flow so you only update
s.daIncludedHeight and remove the per-height cache after putUint64Metadata
returns nil. Concretely, check the error from putUint64Metadata and on non-nil
return/propagate the error (or retry) without mutating s.daIncludedHeight or
performing the per-height cache deletion; only after a successful
putUint64Metadata call set s.daIncludedHeight = nextHeight and then delete the
per-height cache. Ensure you log the failure as you do now via
s.logger.Error().Err(err)... but avoid dropping in-memory state when persistence
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c63884b7-8812-49b2-aa3e-9a1c928062f8
📒 Files selected for processing (5)
block/internal/cache/manager.goblock/internal/cache/manager_test.goblock/internal/cache/pending_data_test.goblock/internal/cache/pending_headers_test.goblock/internal/submitting/submitter.go
| // getMetadataUint64 reads an 8-byte little-endian uint64 from store metadata. | ||
| // Returns 0, false if the key is absent, errors, or the value is not 8 bytes. | ||
| func getMetadataUint64(ctx context.Context, st store.Store, key string) (uint64, bool) { | ||
| b, err := st.GetMetadata(ctx, key) | ||
| if err != nil || len(b) != 8 { | ||
| return 0, false | ||
| } | ||
| return binary.LittleEndian.Uint64(b), true | ||
| } | ||
|
|
||
| // initDAHeightFromStore seeds maxDAHeight from the HeightToDAHeight metadata | ||
| // written by the submitter for the last finalized block. This ensures | ||
| // DaHeight() is non-zero on restart even when the in-flight snapshot is empty. | ||
| func (m *implementation) initDAHeightFromStore(ctx context.Context) { | ||
| daIncludedBytes, err := m.store.GetMetadata(ctx, store.DAIncludedHeightKey) | ||
| if err != nil || len(daIncludedBytes) != 8 { | ||
| return | ||
| } | ||
| daIncludedHeight := binary.LittleEndian.Uint64(daIncludedBytes) | ||
| if daIncludedHeight == 0 { | ||
| daIncludedHeight, ok := getMetadataUint64(ctx, m.store, store.DAIncludedHeightKey) | ||
| if !ok || daIncludedHeight == 0 { |
There was a problem hiding this comment.
Preserve metadata read failures here instead of treating them as “missing”.
getMetadataUint64 now maps not-found, backend read errors, and malformed values to the same false result. initDAHeightFromStore then silently skips seeding DaHeight(), so a transient store failure can look like “no metadata” and reset startup state with no diagnostic.
Suggested fix
-// getMetadataUint64 reads an 8-byte little-endian uint64 from store metadata.
-// Returns 0, false if the key is absent, errors, or the value is not 8 bytes.
-func getMetadataUint64(ctx context.Context, st store.Store, key string) (uint64, bool) {
+// getMetadataUint64 reads an 8-byte little-endian uint64 from store metadata.
+// Returns ok=false only when the key is genuinely absent.
+func getMetadataUint64(ctx context.Context, st store.Store, key string) (uint64, bool, error) {
b, err := st.GetMetadata(ctx, key)
- if err != nil || len(b) != 8 {
- return 0, false
+ if err != nil {
+ return 0, false, fmt.Errorf("read metadata %q: %w", key, err)
}
+ if len(b) != 8 {
+ return 0, false, fmt.Errorf("invalid metadata length for %q: %d", key, len(b))
+ }
- return binary.LittleEndian.Uint64(b), true
+ return binary.LittleEndian.Uint64(b), true, nil
}Then have initDAHeightFromStore log/handle the error separately and only ignore the real not-found case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/cache/manager.go` around lines 390 - 405, getMetadataUint64
currently collapses backend errors and malformed values into a missing result;
change its signature to return (uint64, bool, error) so it returns the raw error
when st.GetMetadata fails or when len(b) != 8 (e.g., return an fmt.Errorf
describing the malformed length), and only use ok==false for the explicit
not-found case; then update initDAHeightFromStore to call value, ok, err :=
getMetadataUint64(ctx, m.store, store.DAIncludedHeightKey), handle/log err
distinctly (e.g., m.logger.Error/return) and only treat the not-found
(ok==false) path as a harmless skip, preserving transient read failures for
diagnostics.
| if err := putUint64Metadata(s.ctx, s.store, store.DAIncludedHeightKey, nextHeight); err != nil { | ||
| s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("failed to persist DA included height") | ||
| } |
There was a problem hiding this comment.
Don't advance past this height unless DAIncludedHeightKey was persisted.
Line 363 still deletes the per-height cache even when this write fails, but s.daIncludedHeight has already been advanced. That leaves durable state behind memory, and on restart the node can no longer reconstruct inclusion from the cache it just dropped.
Suggested fix
- // Update DA included height
- s.SetDAIncludedHeight(nextHeight)
- currentDAIncluded = nextHeight
-
- // Persist DA included height
- if err := putUint64Metadata(s.ctx, s.store, store.DAIncludedHeightKey, nextHeight); err != nil {
+ // Persist DA included height before advancing in-memory state
+ if err := putUint64Metadata(s.ctx, s.store, store.DAIncludedHeightKey, nextHeight); err != nil {
s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("failed to persist DA included height")
+ break
}
+ // Update DA included height
+ s.SetDAIncludedHeight(nextHeight)
+ currentDAIncluded = nextHeight
+
// Delete height cache for that height
// This can only be performed after the height has been persisted to store
s.cache.DeleteHeight(nextHeight)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/submitting/submitter.go` around lines 357 - 359, The DA
included height is advanced and the per-height cache is deleted even when
putUint64Metadata(s.ctx, s.store, store.DAIncludedHeightKey, nextHeight) fails;
modify the flow so you only update s.daIncludedHeight and remove the per-height
cache after putUint64Metadata returns nil. Concretely, check the error from
putUint64Metadata and on non-nil return/propagate the error (or retry) without
mutating s.daIncludedHeight or performing the per-height cache deletion; only
after a successful putUint64Metadata call set s.daIncludedHeight = nextHeight
and then delete the per-height cache. Ensure you log the failure as you do now
via s.logger.Error().Err(err)... but avoid dropping in-memory state when
persistence fails.
2 participants
Overview
this pr attempts a simple deduplication of some functions
Summary by CodeRabbit
Refactor
Tests
Chores