feat(cache): cache pruning

[julienrbrt]

Overview

Supersed #2748
Except it is fully contained in the cache.

We delete all the cache for the height that just been marked as included.

[claude]

Claude finished @julienrbrt's task —— View job

---

Code Review Complete

I've performed a comprehensive review of the cache pruning implementation. Overall, the approach is sound and well-tested, but there are several important concerns and suggestions for improvement.

---

Critical Issues

1. Potential Memory Leak in hashByHeight Map ⚠️

Location: block/internal/cache/generic_cache.go:76-78, 91-93

The new hashByHeight map can accumulate entries for the same height from multiple sources:

• setSeen(hash, height) stores height -> hash
• setDAIncluded(hash, daHeight, blockHeight) also stores blockHeight -> hash

Problem: If a header and data have different hashes but the same block height, the second call will overwrite the first entry in hashByHeight. This means deleteAllForHeight() will only clean up the last hash stored for that height, leaving the other hash orphaned in the hashes map.

// Example scenario:
cm.SetHeaderSeen(headerHash, 100)     // hashByHeight[100] = headerHash
cm.SetDataSeen(dataHash, 100)         // hashByHeight[100] = dataHash (overwrites!)
cm.DeleteHeight(100)                  // Only deletes dataHash from hashes map
// headerHash remains in hashes map forever!

Recommendation: Use sync.Map to store a slice/set of hashes per height, or use separate tracking maps for headers vs data (e.g., headerHashByHeight and dataHashByHeight).

---

2. Intentionally Preserving daIncluded Status Without Clear Rationale

Location: block/internal/cache/generic_cache.go:108

// c.daIncluded.Delete(hash) // we actually do not want to delete the DA-included status here

Concerns:

• The comment explains what but not why DA-included status must be preserved
• This creates unbounded growth of the daIncluded map over the lifetime of the node
• No pruning mechanism is visible for this map in the codebase
• Without understanding the reason, this appears to be a memory leak

Questions for clarification:

1. Why must DA-included status persist after height pruning?
2. Is this needed for future lookups (e.g., by syncing nodes)?
3. What is the expected upper bound on daIncluded map size?

Recommendation:

• Add comprehensive documentation explaining the retention policy
• Consider implementing eventual cleanup (e.g., retain only last N heights of DA inclusion data)
• Add monitoring/metrics for daIncluded map size

---

3. Race Condition: Height Parameter Ambiguity

Location: Throughout cache manager interface

Issue: The term "height" is ambiguous—it could mean block height or DA height. The reviewer (tac0turtle) already flagged this. While the code works, the lack of clarity creates maintenance risk.

Current naming:

SetHeaderDAIncluded(hash string, daHeight uint64, blockHeight uint64)
DeleteHeight(blockHeight uint64)

Recommendation: Be explicit everywhere:

• Rename height parameters to either blockHeight or daHeight consistently
• Update function/variable names: deleteAllForHeight → deleteAllForBlockHeight
• Add godoc clarifying which height system each parameter refers to

---

Architecture & Design

4. Good: Separation of Concerns

The pruning logic is well-encapsulated in the cache layer. The submitter correctly calls DeleteHeight() only after persistence, preventing data loss.

5. Good: Test Coverage for Critical Path

TestSubmitter_CacheClearedOnHeightInclusion (lines 412-516) is excellent—it validates:

• Heights 1-2 are pruned after DA inclusion
• Height 3 remains intact (not yet included)
• DA inclusion status is preserved (addressing concern Peer discovery #2 above)

This test gives confidence the core logic works as intended.

---

Concurrency Safety

6. Adequate but Not Optimal: sync.Map Usage

sync.Map is appropriate for read-heavy workloads with disjoint key sets. However:

Concerns:

• The deleteAllForHeight() function performs multiple operations that aren't atomic as a group:

  c.itemsByHeight.Delete(height)
  hash, ok := c.hashByHeight.Load(height)
  if ok {
      c.hashes.Delete(hash)
      c.hashByHeight.Delete(height)
  }

• If another goroutine calls setSeen() with the same height between Load and Delete, you could have inconsistent state

Recommendation:

• Document the concurrency model: "Callers must ensure no concurrent writes to the same height during deletion"
• Or add mutex protection around multi-step operations affecting the same height

---

7. Potential Panic: Type Assertion Without Safety

Location: block/internal/cache/generic_cache.go:72, 87

return seen.(bool)  // Panics if not bool
return daIncluded.(uint64)  // Panics if not uint64

While sync.Map usage here should be type-safe, a defensive check would prevent hard-to-debug panics:

seen, ok := seen.(bool)
if !ok {
    return false  // or log error
}
return seen

---

Error Handling

8. Silent Failure in Submitter

Location: block/internal/submitting/submitter.go:235-236

if err := s.store.SetMetadata(s.ctx, store.DAIncludedHeightKey, bz); err != nil {
    s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("failed to persist DA included height")
}
// Cache deletion happens regardless of persistence success
s.cache.DeleteHeight(nextHeight)

Issue: If metadata persistence fails but cache deletion succeeds, you lose the cache data permanently. On restart, you'll need to re-sync from DA.

Recommendation:

• Only call DeleteHeight() if persistence succeeds
• Or make the entire operation idempotent by re-fetching from DA if cache is missing

---

Performance

9. Missing Persistence for hashByHeight

Location: block/internal/cache/generic_cache.go:153-201 (SaveToDisk/LoadFromDisk)

The hashByHeight map is never saved to or loaded from disk. This means:

• On restart, pruning won't work correctly until new entries are added
• Could lead to stale cache entries accumulating after restart

Recommendation: Add persistence for hashByHeight similar to other maps, or document why it's safe to skip.

---

Code Quality

10. Good: Removal of Dead Code

Removing the unused rangeByHeight function is excellent housekeeping.

11. Missing Test: Error Paths in deleteAllForHeight

The new deletion logic isn't covered by unit tests in generic_cache_test.go. Consider adding:

• Test deletion when hash exists vs doesn't exist
• Test deletion of specific heights while others remain
• Test concurrent deletion and insertion

---

Documentation

12. Insufficient Godoc for DeleteHeight

Location: block/internal/cache/manager.go:170-176

// DeleteHeight removes from all caches the given height.
// This can be done when a height has been da included.
func (m *implementation) DeleteHeight(blockHeight uint64) {

Recommendation: Expand documentation:

// DeleteHeight removes all cache entries associated with the given block height.
// This should be called after a block height has been DA-included and persisted to store.
// 
// Pruning behavior:
// - Removes items, seen hashes, and pending events for the height
// - Preserves DA-included status for future reference (intentional)
// - Must only be called after successful persistence to avoid data loss
//
// Parameters:
//   blockHeight: The ev-node block height (NOT DA height) to prune

---

Recommendations Summary

Must Fix:

1. ⚠️ Address the hashByHeight overwrite issue (critical memory leak)
2. Document why daIncluded status is preserved and implement eventual cleanup
3. Only call DeleteHeight() after successful persistence

Should Fix:
4. Rename parameters for clarity (blockHeight vs daHeight)
5. Add persistence for hashByHeight map
6. Add defensive type assertions to prevent panics

Nice to Have:
7. Document concurrency guarantees
8. Add unit tests for deleteAllForHeight edge cases
9. Improve godoc for DeleteHeight method

---

Final Assessment

Strengths:

• Well-contained implementation in the cache layer
• Excellent test coverage for the integration path
• Proper ordering: persistence before pruning
• Clean removal of unused code

Weaknesses:

• Potential memory leak from hashByHeight overwrite
• Unbounded growth of daIncluded map
• Missing persistence for new data structure
• Insufficient error handling around persistence failures

Verdict: The core concept is sound, but the implementation needs refinement to prevent memory leaks and ensure production reliability. I recommend addressing the critical issues (1-3) before merging.

---

[github-actions]

The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Oct 17, 2025, 12:50 PM

[codecov]

Codecov Report

❌ Patch coverage is 63.63636% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.34%. Comparing base (1890a37) to head (e3fc018).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
block/internal/cache/generic_cache.go	38.46%	8 Missing ⚠️
block/internal/cache/manager.go	69.23%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2761      +/-   ##
==========================================
- Coverage   61.42%   61.34%   -0.08%     
==========================================
  Files          81       81              
  Lines        8622     8628       +6     
==========================================
- Hits         5296     5293       -3     
- Misses       2828     2838      +10     
+ Partials      498      497       -1     

Flag	Coverage Δ
combined	61.34% <63.63%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tac0turtle]

utACK