Conversation
Summary of ChangesHello @alpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how pending blocks are handled within the system. It transitions from storing unconfirmed blocks as regular block data with empty signatures to using a dedicated metadata store. This change enhances the robustness of block production, particularly during node restarts, and includes a crucial migration path to ensure compatibility with existing data formats, thereby preventing potential issues like double-signing after a system upgrade. Additionally, module replacement directives were enabled in application-level Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @alpe's task in 2m 21s —— View job Code Review CompleteI've completed a comprehensive review of PR #3073, which refactors pending block storage to use dedicated metadata keys instead of storing unconfirmed blocks as regular block data with empty signatures. This is a significant architectural improvement that enhances restart robustness and prevents double-signing issues. Summary of ChangesPositive aspects:
Areas requiring attention:
Critical Issues1. Silent Failure on Pending Block Deletion (High Priority)Location: if err := e.deletePendingBlock(batch); err != nil {
e.logger.Warn().Err(err).Uint64("height", newHeight).Msg("failed to delete pending block metadata")
}Issue: If Impact:
Recommendation: Treat this as a critical error and abort the commit: if err := e.deletePendingBlock(batch); err != nil {
return fmt.Errorf("failed to delete pending block metadata for height %d: %w", newHeight, err)
}This aligns with Gemini's review comment and ensures state consistency. Fix this issue 2. Incomplete Error Detection in Pending Block Retrieval (Medium Priority)Location: dataBytes, err := e.store.GetMetadata(ctx, dataKey)
if err != nil {
if errors.Is(err, ds.ErrNotFound) {
return nil, nil, fmt.Errorf("pending header exists but data is missing: corrupt state")
}
return nil, nil, err
}Issue: When a pending header exists without corresponding data, this indicates an inconsistent state. While the code correctly returns an error, it doesn't log this condition, making debugging difficult. Recommendation: Add warning log before returning the error: dataBytes, err := e.store.GetMetadata(ctx, dataKey)
if err != nil {
if errors.Is(err, ds.ErrNotFound) {
e.logger.Warn().Msg("found pending header without pending data: corrupt state detected")
return nil, nil, fmt.Errorf("pending header exists but data is missing: corrupt state")
}
return nil, nil, err
}This helps with operational observability. Fix this issue Medium Priority Issues3. Context Not Passed to deletePendingBlockLocation: Issue: The Current signature: func (e *Executor) deletePendingBlock(batch store.Batch) errorRecommended signature: func (e *Executor) deletePendingBlock(ctx context.Context, batch store.Batch) errorRationale: Consistent with Go best practices and other methods like 4. Migration Logic Doesn't Handle Partial FailuresLocation: Issue: The
If the second operation fails after the first succeeds, you'll have duplicate data in both old and new formats. Recommendation: Consider using a single batch for atomicity: batch, err := e.store.NewBatch(ctx)
if err != nil {
return fmt.Errorf("create migration batch: %w", err)
}
// Write to new metadata keys
headerBytes, err := pendingHeader.MarshalBinary()
if err != nil {
return fmt.Errorf("marshal header: %w", err)
}
dataBytes, err := pendingData.MarshalBinary()
if err != nil {
return fmt.Errorf("marshal data: %w", err)
}
if err := batch.Put(ds.NewKey(store.GetMetaKey(headerKey)), headerBytes); err != nil {
return fmt.Errorf("save pending header: %w", err)
}
if err := batch.Put(ds.NewKey(store.GetMetaKey(dataKey)), dataBytes); err != nil {
return fmt.Errorf("save pending data: %w", err)
}
// Delete old keys in same batch
for _, key := range []string{...} {
if err := batch.Delete(ds.NewKey(key)); err != nil && !errors.Is(err, ds.ErrNotFound) {
return fmt.Errorf("delete legacy key %s: %w", key, err)
}
}
if err := batch.Commit(); err != nil {
return fmt.Errorf("commit migration: %w", err)
}This ensures atomic migration. Fix this issue 5. Missing TODO Link Per Maintainer RequestLocation: Issue: Reviewer julienrbrt requested linking issue #2795 as a TODO comment for the migration code. Current: // Migrate any old-style pending block (stored at height N+1 via SaveBlockData
// with empty signature) to the new metadata-key format.
// Todo remove in the future: https://github.com/evstack/ev-node/issues/2795Recommended: // Migrate any old-style pending block (stored at height N+1 via SaveBlockData
// with empty signature) to the new metadata-key format.
// TODO(#2795): Remove migration logic in future release after all nodes have upgradedThis provides better IDE integration and clearer action items. Fix this issue Architecture & DesignStrengths
Potential Concerns
Error Handling & ResilienceStrengths
Issues
Security ConsiderationsPositive
Concerns
Performance & Resource EfficiencyObservations
Recommendations
Testing & CoverageCurrent StatePer Codecov report:
Existing TestsThe PR includes
Missing Test CoverageCritical scenarios not covered:
Recommended Additional Testsfunc TestPendingBlock_CorruptState(t *testing.T) {
// Test header without data
// Test data without header
// Test malformed data
}
func TestMigrateLegacyPendingBlock_EdgeCases(t *testing.T) {
// Test with signed block (should error)
// Test with no legacy block (should no-op)
// Test cleanup of all legacy keys
}
func TestPendingBlock_BatchFailures(t *testing.T) {
// Mock batch.Commit() failure
// Mock batch.Delete() failure
}Code Quality & Go Best PracticesStrengths
Minor Issues
Recommendations SummaryMust Fix (Before Merge)
Should Fix (High Priority)
Nice to Have (Future Improvements)
ConclusionThis is a solid architectural improvement that addresses a real issue (double-signing on restart). The core logic is sound, but there are critical error handling issues that must be addressed before merging. Overall Assessment: ✅ Approve with required changes The refactoring correctly separates pending blocks from committed blocks and includes a well-designed migration path. However, the silent failure on pending block deletion (Issue #1) is a blocker that could lead to data inconsistency. Once the must-fix issues are addressed and test coverage improves, this will be a valuable enhancement to the codebase. |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3073 +/- ##
==========================================
- Coverage 61.25% 61.00% -0.25%
==========================================
Files 112 113 +1
Lines 11355 11431 +76
==========================================
+ Hits 6956 6974 +18
- Misses 3630 3676 +46
- Partials 769 781 +12
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:
|
| e.logger.Info().Uint64("height", state.LastBlockHeight). | ||
| Str("chain_id", state.ChainID).Msg("initialized state") | ||
|
|
||
| // Migrate any old-style pending block (stored at height N+1 via SaveBlockData |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the storage of pending blocks by moving them from the main block storage to a separate metadata area. This is a good architectural improvement that isolates uncommitted data. The changes include a new pending.go file with helpers for managing pending blocks, a migration path for old-style pending blocks, and updates to the block production logic to use the new mechanism.
My review includes a couple of suggestions to improve robustness and observability. One suggestion is to log a warning when an inconsistent pending block state is detected (e.g., a header without data). Another is to handle failures in deleting pending blocks more strictly by aborting the block commit, which would prevent leaving stale data in the store.
block/internal/executing/executor.go
Outdated
| if err := e.deletePendingBlock(e.ctx, batch); err != nil { | ||
| e.logger.Warn().Err(err).Uint64("height", newHeight).Msg("failed to delete pending block metadata") | ||
| } |
There was a problem hiding this comment.
If deletePendingBlock fails, the current implementation logs a warning and proceeds to commit the batch. This leaves a stale pending block in the metadata store. While this might not cause issues in the next block production cycle (due to height checks), it indicates an underlying problem with the batch or store and pollutes the storage. It would be safer to treat this as a critical error and abort the commit by returning an error. This ensures that the state remains consistent.
| if err := e.deletePendingBlock(e.ctx, batch); err != nil { | |
| e.logger.Warn().Err(err).Uint64("height", newHeight).Msg("failed to delete pending block metadata") | |
| } | |
| if err := e.deletePendingBlock(e.ctx, batch); err != nil { | |
| return fmt.Errorf("failed to delete pending block metadata for height %d: %w", newHeight, err) | |
| } |
| dataBytes, err := e.store.GetMetadata(ctx, dataKey) | ||
| if err != nil { | ||
| if errors.Is(err, ds.ErrNotFound) { | ||
| return nil, nil, nil | ||
| } | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
The current logic correctly handles the case where pending data is not found after a pending header has been found, but it does so silently. If a pending header exists without corresponding data, it indicates an inconsistent state that should be logged as a warning. This would help in debugging potential issues where pending blocks are not saved correctly.
| dataBytes, err := e.store.GetMetadata(ctx, dataKey) | |
| if err != nil { | |
| if errors.Is(err, ds.ErrNotFound) { | |
| return nil, nil, nil | |
| } | |
| return nil, nil, err | |
| } | |
| dataBytes, err := e.store.GetMetadata(ctx, dataKey) | |
| if err != nil { | |
| if errors.Is(err, ds.ErrNotFound) { | |
| e.logger.Warn().Msg("found pending header without pending data, ignoring") | |
| return nil, nil, nil | |
| } | |
| return nil, nil, err | |
| } |
(cherry picked from commit 354bc76e32eb7a150d1437aa9674b697cd95af09)
* main: chore: Fix mismatched comment in TestCache_WithNilStore function (#3074)
julienrbrt
changed the title
2 participants
No description provided.