refactor(store): replace go-header store by ev-node store

[julienrbrt]

Overview

Attempt to replace go-header default store by ev-node store.
This avoids duplication of blocks.

[github-actions]

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

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Jan 30, 2026, 4:54 PM

[claude]

Claude finished @julienrbrt's task in 3m 2s —— View job

---

Comprehensive Code Review

I've completed a thorough review of this PR's store refactoring. This is an excellent architectural improvement that successfully consolidates the storage layer. Below is my detailed analysis across multiple dimensions.

---

🎯 Overview

This PR successfully refactors the block storage architecture by introducing store adapters that wrap the ev-node store to satisfy go-header's header.Store[H] interface. This eliminates data duplication between go-header's store and ev-node's store while maintaining compatibility with go-header's P2P infrastructure.

Key Changes:

• New StoreAdapter[H] generic implementation with HeaderStoreAdapter and DataStoreAdapter type aliases
• Simplified rollback commands (removed explicit go-header store operations)
• Refactored sync service to use store adapters
• Removed exchange_wrapper.go and related duplicate store management code

---

✅ Strengths

Architecture & Design

1. Excellent use of Go generics - The StoreAdapter[H] design is clean and type-safe
2. Single source of truth - Eliminates data duplication by using ev-node store as the authoritative source
3. Clean adapter pattern - StoreGetter interface provides good abstraction boundary
4. Smart pending cache - LRU cache (1000 items) prevents unbounded memory growth for P2P-received items
5. Well-documented - Code comments clearly explain the adapter's role and behavior

Concurrency & Synchronization

1. Proper synchronization primitives - Good use of atomic.Uint64 for height tracking and mutexes for critical sections
2. Double-checked locking in heightSub.Wait() (lines 67-76) minimizes lock contention
3. Blocking GetByHeight - Correctly implements go-header's expected blocking behavior via heightSub

Testing

1. Comprehensive test coverage - 51 test functions across both adapters
2. Good test organization - Tests cover initialization, retrieval, ranges, tail, rollback scenarios
3. Parallel test execution - All tests properly use t.Parallel()

---

⚠️ Issues & Concerns

🔴 Critical Issues

1. Context.Background() Usage in Constructors (pkg/store/store_adapter.go:142, 400)

Severity: Medium-High

// Line 142 in NewStoreAdapter
if h, err := getter.Height(context.Background()); err == nil {
    storeHeight = h
}

// Line 400 in Height()
if h, err := a.getter.Height(context.Background()); err == nil && h > 0 {

Problem: Using context.Background() bypasses cancellation and timeout controls. While Height() is likely fast, this violates Go best practices for context propagation.

Impact: If the underlying store operation blocks or takes longer than expected, there's no way to cancel it. This could cause initialization delays or prevent graceful shutdown.

Recommendation:

• For constructors, consider accepting a context parameter: NewStoreAdapter(ctx context.Context, getter StoreGetter[H], gen genesis.Genesis)
• For the Height() method (which returns uint64 without error per interface), consider caching the height more aggressively or accepting that brief blocking is acceptable
• Document why context.Background() is safe here if it's truly necessary

Fix context usage →

---

2. Inefficient Tail() Implementation with Linear Scan (pkg/store/store_adapter.go:258-267)

Severity: Medium

// Walk up from genesisInitialHeight to find the first available item (pruning case)
for h := a.genesisInitialHeight + 1; h <= height; h++ {
    item, err = a.getter.GetByHeight(ctx, h)
    if err == nil {
        return item, nil
    }
    if pendingItem, ok := a.pending.Peek(h); ok {
        return pendingItem, nil
    }
}

Problem: As noted in the TODO comment and Gemini review, this performs O(n) operations when pruning has created gaps. If the first valid block is at height 10000 after pruning, this loops 9999 times.

Impact:

• Performance degradation with large height gaps
• Increased latency on Tail() calls after pruning
• Potential timeout issues in P2P sync operations

Recommendation:
Store the minimum available height as metadata in the ev-node store:

// Add to Store interface methods:
GetMinHeight(ctx context.Context) (uint64, error)
SetMinHeight(ctx context.Context, height uint64) error

// Then Tail() becomes O(1):
minHeight, err := a.store.GetMinHeight(ctx)
if err == nil {
    return a.getter.GetByHeight(ctx, minHeight)
}

The TODO comment acknowledges this; recommend prioritizing this optimization before enabling pruning.

Fix Tail() performance →

---

🟡 Medium Priority Issues

3. Potential Race Condition in pendingHead() (pkg/store/store_adapter.go:188-198)

Severity: Medium

func (a *StoreAdapter[H]) pendingHead() (H, uint64) {
    var maxHeight uint64
    var head H
    for _, h := range a.pending.Keys() {
        if item, ok := a.pending.Peek(h); ok && h > maxHeight {
            maxHeight = h
            head = item
        }
    }
    return head, maxHeight
}

Problem: The LRU cache is not protected by a lock during iteration. While hashicorp/golang-lru/v2 has internal locking for individual operations, iterating over Keys() and then calling Peek() is not atomic. An eviction could occur between Keys() iteration and Peek().

Impact:

• Peek() might return ok=false if the item was evicted between calls
• Unlikely in practice due to 1000-item cache size, but theoretically possible under heavy load

Recommendation:
Either:

1. Document that this is acceptable (returning stale head is fine for best-effort scenarios)
2. Add error handling for the case where Peek() fails
3. Consider using GetOldest() from the LRU API if available

---

4. Missing Error Propagation in DeleteRange() (pkg/store/store_adapter.go:494-512)

Severity: Low-Medium

func (a *StoreAdapter[H]) DeleteRange(ctx context.Context, from, to uint64) error {
    for height := from; height < to; height++ {
        a.pending.Remove(height)
        
        if a.onDeleteFn != nil {
            if err := a.onDeleteFn(ctx, height); err != nil {
                return err  // ❌ Returns immediately, doesn't clean up remaining heights
            }
        }
    }
    // ...
}

Problem: If onDeleteFn fails at height N, heights N+1 to to are not removed from the pending cache, leaving partial state.

Impact: After a failed rollback, the pending cache may contain items that should have been deleted, causing inconsistency.

Recommendation:

var errs []error
for height := from; height < to; height++ {
    a.pending.Remove(height)
    if a.onDeleteFn != nil {
        if err := a.onDeleteFn(ctx, height); err != nil {
            errs = append(errs, fmt.Errorf("height %d: %w", height, err))
        }
    }
}
return errors.Join(errs...)

---

5. Inconsistent Height Updates in Head() (pkg/store/store_adapter.go:201-228)

Severity: Low

The Head() method updates heightSub as a side effect during reads:

func (a *StoreAdapter[H]) Head(ctx context.Context, _ ...header.HeadOption[H]) (H, error) {
    // ...
    if pendingHeight > storeHeight {
        a.heightSub.SetHeight(pendingHeight)  // Side effect during read
        return pendingHead, nil
    }
    // ...
}

Problem: Read operations shouldn't typically have side effects. This makes reasoning about state changes harder and could lead to unexpected notifications to waiters.

Recommendation: Consider updating height only in write operations (Append, Init). If this is intentional for lazy height updates, document it clearly.

---

🟢 Minor Issues & Improvements

6. Magic Number for LRU Cache Size

const defaultPendingCacheSize = 1000

Observation: The 1000-item cache size is reasonable but not justified. Under what workload could this be exceeded?

Recommendation: Add a comment explaining the sizing rationale:

// defaultPendingCacheSize limits memory usage for pending items received via P2P.
// At ~10KB per item, 1000 items = ~10MB memory overhead.
// This is sized to handle burst P2P traffic during initial sync.
const defaultPendingCacheSize = 1000

---

7. GetRange Partial Success Behavior (pkg/store/store_adapter.go:353-363)

for height := from; height < to; height++ {
    item, err := a.GetByHeight(ctx, height)
    if err != nil {
        // Return what we have so far
        if len(items) > 0 {
            return items, nil  // ⚠️ Silently returns partial range
        }
        return nil, header.ErrNotFound
    }
    items = append(items, item)
}

Observation: Returning partial results without indicating the range is incomplete could be confusing.

Recommendation: This might be intentional for go-header compatibility. If so, document it:

// Returns partial range if some heights are unavailable, which is acceptable
// during sync operations when the full range may not yet be available.

---

🔒 Security Analysis

✅ No Critical Security Issues Found

1. Input Validation: Height parameters are properly validated
2. No SQL Injection Risk: Uses key-value store, not SQL
3. No Credential Exposure: No secrets in logs or errors
4. Context Cancellation: Properly respects context cancellation in Wait() and other blocking operations

⚠️ Minor Concerns

1. Unbounded Hash Input: GetByHash() and Has() accept arbitrary byte slices. While the underlying store handles this, consider validating hash length to prevent potential DoS via large inputs.

---

🚀 Performance Analysis

✅ Strengths

1. LRU Cache: Prevents memory bloat for pending items
2. Atomic Operations: atomic.Uint64 for height is lock-free for reads
3. Double-Checked Locking: Minimizes contention in Wait()

⚠️ Concerns

1. Tail() Linear Scan: O(n) with pruning (addressed above)
2. Get() Linear Scan of Pending Cache:

   for _, h := range a.pending.Keys() {
       if pendingItem, ok := a.pending.Peek(h); ok && !pendingItem.IsZero() && bytes.Equal(pendingItem.Hash(), hash) {

   This is O(n) where n = cache size (max 1000). Consider maintaining a hash → height index if Get() is hot path.

---

📊 Test Coverage Assessment

✅ Good Coverage

• 51 test functions across both adapters
• Tests cover: initialization, append, retrieval, ranges, tail, rollback, concurrent access
• Good use of t.Parallel() for performance

⚠️ Missing Tests

1. Concurrent Access Tests: No tests for concurrent Append() + GetByHeight() + DeleteRange()
2. LRU Eviction Scenarios: What happens when cache exceeds 1000 items?
3. Context Cancellation in GetByHeight: Does Wait() properly handle canceled contexts?
4. Edge Case: What happens if onDeleteFn is set after adapter is in use?

Recommendation: Add a few race detector tests:

go test -race ./pkg/store/...

---

📝 Code Quality & Go Best Practices

✅ Excellent

1. Idiomatic Go: Clean interfaces, good use of generics
2. Error Handling: Consistent wrapping with context
3. Naming: Clear, descriptive names throughout
4. Documentation: Good inline comments

🟡 Could Improve

1. Exported Error Variables: errElapsedHeight is unexported but might be useful for callers to detect
2. Interface Segregation: StoreGetter is clean, but consider splitting Reader interface in the main store to make testing easier

---

🎓 Architectural Observations

✅ Smart Design Decisions

1. Adapter Pattern: Clean separation of concerns between ev-node store and go-header requirements
2. Pending Cache Strategy: Clever solution to handle P2P-received items before validation
3. Generic Implementation: StoreAdapter[H] avoids code duplication

💡 Future Considerations

1. Pruning Support: The TODO comments indicate pruning awareness; ensure the min-height metadata solution is implemented before enabling pruning
2. Metrics: Consider adding Prometheus metrics for:
   • Pending cache hit/miss rate
   • Tail() scan depth
   • heightSub wait times
3. Observability: Add structured logging for adapter operations to aid debugging

---

📋 Summary & Recommendation

Overall Assessment: STRONG APPROVAL with Minor Fixes Required

This PR represents a significant architectural improvement. The code is well-written, properly tested, and achieves its goal of eliminating data duplication.

Required Before Merge:

1. ✅ Fix or document context.Background() usage (Critical)
2. ⚠️ Document the Tail() performance limitation and pruning implications (Medium)
3. ⚠️ Fix DeleteRange() error handling to avoid partial state (Medium)

Recommended for Follow-up PRs:

1. Optimize Tail() with min-height metadata
2. Add concurrent access tests with race detector
3. Consider hash → height index for Get() if it's a hot path
4. Add observability metrics

Test Coverage Note:

Codecov shows 79.73% patch coverage with 46 missing lines. The uncovered lines appear to be mostly error paths and edge cases, which is acceptable for this initial implementation.

---

Excellent work on this refactoring! The unified store architecture is a clear improvement over the previous dual-store approach. The adapter pattern is well-executed and maintains compatibility with go-header while eliminating duplication.

---

[gemini-code-assist]

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! 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 the node's data storage strategy by centralizing all block-related data within the ev-node store. This change aims to prevent data duplication and simplify the overall architecture. By introducing adapter layers, the existing go-header P2P synchronization mechanisms can now operate directly on the unified ev-node store, leading to a more coherent and maintainable system. The modifications also streamline rollback procedures and reduce redundant code paths related to data persistence.

Highlights

• Unified Store Architecture: The pull request refactors the storage mechanism to use the ev-node store as the single source of truth for both headers and data, eliminating the need for a separate go-header store and reducing data duplication.
• Introduction of Store Adapters: New HeaderStoreAdapter and DataStoreAdapter implementations are introduced. These adapters wrap the ev-node store, allowing it to satisfy the header.Store interface from the go-header library, thus integrating seamlessly with go-header's P2P infrastructure.
• Simplified Rollback Logic: The rollback commands (apps/evm/cmd/rollback.go and apps/testapp/cmd/rollback.go) are simplified. They now primarily rely on the ev-node store's Rollback method, removing explicit go-header store operations for headers and data, and improving error handling with errors.Join.
• Refactored Sync Service Initialization: The pkg/sync/sync_service.go file is updated to use the new store adapters. NewDataSyncService and NewHeaderSyncService now directly accept the ev-node store and wrap it with the appropriate adapter, streamlining the setup of synchronization services.
• Dependency Clean-up: Imports related to go-header/store and ds.Batching are removed from various files, reflecting the consolidated storage approach and reducing unnecessary dependencies.

🧠 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.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request successfully refactors the store integration for go-header by introducing DataStoreAdapter and HeaderStoreAdapter. This change eliminates data duplication by allowing the go-header P2P infrastructure to directly utilize the ev-node store. The rollback commands have been updated to reflect this unified store approach, and comprehensive tests have been added for the new adapter implementations. This is a significant improvement in architecture and efficiency.

[codecov]

Codecov Report

❌ Patch coverage is 78.73134% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.88%. Comparing base (455b6c1) to head (dfe1071).

Files with missing lines	Patch %	Lines
pkg/store/store_adapter.go	78.83%	37 Missing and 14 partials ⚠️
pkg/sync/sync_service.go	70.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3036      +/-   ##
==========================================
+ Coverage   55.33%   55.88%   +0.54%     
==========================================
  Files         117      117              
  Lines       11685    11848     +163     
==========================================
+ Hits         6466     6621     +155     
- Misses       4494     4495       +1     
- Partials      725      732       +7     

Flag	Coverage Δ
combined	55.88% <78.73%> (+0.54%)	⬆️

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.

[julienrbrt]

It is O(1) for non pruned node, and O(n) for pruned nodes, so we should improve this by saving the pruned tail in state and #2984

[julienrbrt]

This should leave enough header/data for p2p syncing nodes before they executes the block.
We can think of making it bigger otherwise.