Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @pthmas's task in 2m 33s —— View job Comprehensive Code Review Complete ✅I've performed a comprehensive code review of the block pruning implementation focusing on code quality, security, error handling, performance, testing, and documentation. SummaryThis is a well-designed and thoughtfully implemented feature that adds height-based pruning to ev-node. The implementation follows Go best practices, includes proper error handling, and demonstrates good architectural decisions. However, there are some concerns related to error handling resilience, test coverage, and configuration validation that should be addressed. Overall Assessment: The code is production-ready with minor improvements recommended. Detailed Findings1. Code Quality & Architecture ⭐⭐⭐⭐☆ (4/5)Strengths:
Concerns:
Recommendations: // In submitter.go, add metrics for pruning failures:
if err := s.store.PruneBlocks(s.ctx, targetHeight); err != nil {
s.logger.Error().Err(err).Uint64("target_height", targetHeight).Msg("failed to prune old block data")
s.metrics.PruningFailures.Inc() // Add this metric
}2. Error Handling & Resilience ⭐⭐⭐☆☆ (3/5)Strengths:
Critical Concerns:
3. Security ⭐⭐⭐⭐⭐ (5/5)Strengths:
No security concerns identified. The implementation is secure. 4. Performance & Resource Efficiency ⭐⭐⭐⭐☆ (4/5)Strengths:
Concerns:
5. Testing & Reliability ⭐⭐☆☆☆ (2/5)Critical Issue: Only 23.36% patch coverage with 82 lines missing coverage (per Codecov report). Test Coverage Analysis: ✅ Good coverage:
Required Tests: // Add to pkg/store/store_test.go:
func TestPruneBlocks_DeletesAllBlockComponents(t *testing.T) {
// Test that pruning deletes headers, data, signatures, hash index, and DA metadata
}
func TestPruneBlocks_Idempotent(t *testing.T) {
// Test that calling PruneBlocks(10) twice doesn't cause errors
}
func TestPruneBlocks_TracksProgress(t *testing.T) {
// Test LastPrunedBlockHeightKey is updated correctly
}
func TestPruneBlocks_ErrorHandling(t *testing.T) {
// Test batch creation failure, delete failure, commit failure
}
func TestPruneBlocks_PreservesStateAndMetadata(t *testing.T) {
// Test that /s/{height}, /t, and global metadata are NOT deleted
}
func TestCachedStore_PruneInvalidatesCache(t *testing.T) {
// Test that cache is invalidated after pruning
}Recommendation: Add comprehensive unit tests before merging. Target: >80% coverage for pruning code paths. 6. Documentation & Developer Experience ⭐⭐⭐⭐☆ (4/5)Strengths:
Minor gaps:
Recommendation: Add a section to CLAUDE.md or README.md: ## Block Pruning
Ev-node supports height-based pruning to reduce disk usage while maintaining recent block history.
### Configuration
- `--evnode.node.pruning_enabled`: Enable pruning (default: false)
- `--evnode.node.pruning_keep_recent`: Number of recent blocks to keep (must be > 0)
- `--evnode.node.pruning_interval`: Run pruning every N blocks (must be >= 1)
### Example
Keep last 1000 blocks, prune every 100 blocks:
```bash
--evnode.node.pruning_enabled=true \
--evnode.node.pruning_keep_recent=1000 \
--evnode.node.pruning_interval=100What Gets Pruned
What Is NOT Pruned
Concurrency & Race Conditions ⭐⭐⭐⭐☆ (4/5)Strengths:
Concern: See "Error Handling" section - missing mutex for concurrent pruning protection. Specific Code Commentsstore.go:331-378 (PruneBlocks loop)for h := lastPruned + 1; h <= height; h++ {
// Get header blob to compute the hash index key. If header is already
// missing (e.g. due to previous partial pruning), just skip this height.
headerBlob, err := s.db.Get(ctx, ds.NewKey(getHeaderKey(h)))
if err != nil {
if errors.Is(err, ds.ErrNotFound) {
continue // ✅ Good: gracefully handles missing data
}
return fmt.Errorf("failed to get header at height %d during pruning: %w", h, err)
}
// ... deletion logic
}Issue: The loop continues on
Recommendation: Add retry logic or at least log/metric transient errors separately from permanent errors. submitter.go:369-404 (Pruning trigger)if s.config.Node.PruningEnabled && s.config.Node.PruningKeepRecent > 0 && s.config.Node.PruningInterval > 0 {Good: Triple-check before pruning. if storeHeight <= lastPruned+s.config.Node.PruningInterval {
continue
}Issue: This check means pruning won't run until
But this doesn't account for the current DA included height. If Recommendation: Change condition to: if currentDAIncluded <= lastPruned+s.config.Node.PruningInterval {
continue
}This ensures pruning runs based on DA-included progress, not just store height. cached_store.go:163-171 (Cache invalidation)Already covered in "Error Handling" section - recommend optimizing to invalidate only newly pruned range. store_adapter.go:269-279 (Tail optimization)if meta, err := a.store.GetMetadata(ctx, LastPrunedBlockHeightKey); err == nil && len(meta) == heightLength {
if lastPruned, err := decodeHeight(meta); err == nil {
if candidate := lastPruned + 1; candidate > startHeight {
startHeight = candidate
}
}
}Excellent: This optimization prevents scanning from genesis height on pruned nodes. Well done! Priority RecommendationsHigh Priority (Should Address Before Merge)
Medium Priority (Can Address Post-Merge)
Low Priority (Nice to Have)
ConclusionThis is a solid, well-architected feature with good separation of concerns and proper DA-safety guarantees. The main concern is insufficient test coverage (23%), particularly for the core The "best-effort pruning" design (log-and-continue on errors) is a reasonable trade-off, but operators should be aware that pruning failures could lead to unbounded disk growth. Consider adding metrics/alerts for production deployments. Approval Recommendation: Approve pending addition of tests. The code is production-ready once test coverage reaches >80% for pruning paths. |
3e1e8e0 to
ed66fe6
Compare
26628f5 to
4d65b1e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2984 +/- ##
==========================================
- Coverage 56.38% 56.08% -0.31%
==========================================
Files 118 118
Lines 12036 12134 +98
==========================================
+ Hits 6787 6805 +18
- Misses 4507 4569 +62
- Partials 742 760 +18
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:
|
7aef549 to
4d31269
Compare
3e07144 to
26d0327
Compare
0c346a0 to
94f37d2
Compare
94f37d2 to
ddc064a
Compare
Co-authored-by: julienrbrt <julien@rbrt.fr>
Co-authored-by: julienrbrt <julien@rbrt.fr>
aef0de8 to
20d578b
Compare
There was a problem hiding this comment.
nice work! lgtm, left some nits.
Maybe @chatton knows how to properly test this on CI?
-> advance in heights, prune, verify querying those blocks returns block not found, but we are still advancing heights
I ran the chaos network with different pruning settings (more or less aggressive) and everything went well. Prunning was working as expected. |
Overview
/h/{height},/d/{height},/c/{height},/i/{hash}up to a target height.height → DA heightmapping) for pruned heights./s/{height}(state),/t(current height), and global metadata keys untouched.last-pruned-block-heightmetadata key so pruning is monotonic and idempotent.store.PruneBlocks.DAIncludedHeightKey), so only DA-included blocks are ever pruned, andPruningKeepRecent, pruning is skipped.PruneExecMetato delete per-heightExecMetaentries up to the same target height.ExecMetaPrunerinterface, and the submitter calls it from the DA inclusion pruning hook.StoreAdapter’sTail()now uses thelast-pruned-block-heightmetadata to skip fully pruned ranges instead of linearly scanning from genesis.Config
New node config fields / flags:
--evnode.node.pruning_enabled--evnode.node.pruning_keep_recent--evnode.node.pruning_intervalPruning actually runs only when all three are set to non-trivial values (enabled,
keep_recent > 0,interval > 0).Design trade-offs
Best-effort pruning:
If pruning fails (either store or exec metadata), we log the error but do not halt DA inclusion. This avoids stopping the node but means disk usage can grow silently if pruning keeps failing; operators will need to watch logs/metrics.
Scope:
This PR prunes:
ExecMetaPrunerhook when available.