feat: p2p exchange wrapper

[tac0turtle]

Overview

This pr was an idea from @renaynay in which we create an exchange wrapper so that the exchange can check our store before making public requests for data. This would help the edge case i ran into today where the p2p store was missing 40 blocks randomly

[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 12, 2026, 3:47 PM

[claude]

Claude finished @tac0turtle's task in 2m 34s —— View job

---

Code Review Complete

I've performed a comprehensive code review of the p2p exchange wrapper implementation. Below is my detailed analysis organized by focus areas.

---

1. Code Quality & Architecture

Critical Issues

❌ Incomplete Implementation of GetRangeByHeight (pkg/sync/exchange_wrapper.go:55)

The GetRangeByHeight method currently passes through directly to p2p without checking the DA store first. This is the core issue that @renaynay identified in their review.

Problem: This method is called by the syncer to request ranges of blocks, which is exactly where checking the DA store would provide the most benefit. The current implementation defeats the purpose of the exchange wrapper.

Expected behavior:

1. Check DA store for the requested range
2. Identify which blocks are available locally
3. Only request missing blocks from the network
4. Merge local + network blocks and return the complete range

Impact: High - This is the primary use case for the wrapper and it's not implemented.

Fix GetRangeByHeight implementation →%20Query%20DA%20store%20for%20blocks%20from%20height%20to%20'to'%202)%20Find%20contiguous%20range%203)%20Request%20missing%20blocks%20from%20p2p%204)%20Merge%20and%20return%20complete%20range.%20Reference%20lines%2055-57&repo=evstack/ev-node&branch=marko/exchange)

⚠️ Function Parameter Design (pkg/sync/exchange_wrapper.go:10-11, pkg/sync/sync_service.go:61-62)

Storing storeGetter and storeGetterByHeight function types on the struct is unnecessary complexity. These functions are essentially adapters around store methods.

Problem:

• Increases coupling between exchangeWrapper and the specific store accessor functions
• Makes the code harder to follow (as noted in review comments)
• The functions are passed in but could be simplified

Better approach (as suggested by @renaynay):

// Option 1: Pass just the methods needed, not generic functions
type exchangeWrapper[H header.Header[H]] struct {
    p2pExchange P2PExchange[H]
    daStore     store.Store
}

func (ew *exchangeWrapper[H]) Get(ctx context.Context, hash header.Hash) (H, error) {
    if ew.daStore != nil {
        // Call store methods directly based on H type
        // For *types.SignedHeader: h, _, err := ew.daStore.GetBlockByHash(ctx, hash)
        // For *types.Data: _, d, err := ew.daStore.GetBlockByHash(ctx, hash)
    }
    return ew.p2pExchange.Get(ctx, hash)
}

Alternative: Use type assertions or interface methods instead of function parameters.

⚠️ Naming Confusion (pkg/sync/sync_service.go:61-62, 81-108)

Variables getter and getterByHeight in sync_service.go are confusingly named alongside similar patterns in exchange_wrapper.go.

Suggestion: Use more descriptive names like:

• daStoreHeaderGetter / daStoreDataGetter
• headerByHashFromDA / dataByHashFromDA

Architectural Observations

✅ Good: Separation of Concerns

The wrapper pattern is clean and follows the decorator pattern appropriately. It wraps the p2p exchange without modifying it.

✅ Good: Generics Usage

Good use of Go generics to support both *types.SignedHeader and *types.Data through a single implementation.

⚠️ Potential Issue: DA Store Lifecycle

The exchangeWrapper.Start() method (line 59) only starts the p2p exchange, but doesn't ensure the DA store is ready. As @renaynay noted, you should verify the DA datastore is started before allowing Get/GetByHeight calls, otherwise they may fail.

Recommendation: Add a check or initialization in the Start method to ensure daStore is accessible.

---

2. Error Handling & Resilience

⚠️ Silent Error Swallowing (pkg/sync/exchange_wrapper.go:30, 42)

if h, err := ew.getter(ctx, ew.daStore, hash); err == nil && !h.IsZero() {
    return h, nil
}

Issues:

1. Errors from the DA store are silently ignored - you can't distinguish between "not found" vs "store failure"
2. No logging when DA store fails
3. No metrics to track DA store hit/miss rate

Recommendation:

if ew.daStore != nil && ew.getter != nil {
    h, err := ew.getter(ctx, ew.daStore, hash)
    if err == nil && !h.IsZero() {
        // Consider: metrics for cache hit
        return h, nil
    }
    // Log non-nil errors at debug level for visibility
    if err != nil && !errors.Is(err, store.ErrNotFound) {
        // log.Debug().Err(err).Msg("DA store lookup failed, falling back to p2p")
    }
    // metrics for cache miss
}

⚠️ Nil Check Pattern (pkg/sync/exchange_wrapper.go:29, 41)

The double nil check if ew.daStore != nil && ew.getter != nil is defensive but indicates a design issue. Per @renaynay's comment, if you always pass a daStore to the constructor for all node types, you don't need these checks.

Recommendation: Simplify by requiring daStore in constructor, or document when it's legitimately nil.

---

3. Security

✅ No Critical Security Issues

• No credential exposure
• No injection vulnerabilities
• Context properly propagated for cancellation

ℹ️ Observation: Input Validation

The wrapper trusts both the DA store and p2p exchange for input validation. This is acceptable since these are internal dependencies, but ensure:

• Hash validation happens at the store/exchange layer
• Headers returned from both sources are properly validated before use

---

4. Performance & Resource Efficiency

⚠️ Missing Performance Benefits

The primary performance optimization (reducing network calls) is only partially implemented:

• ✅ Get and GetByHeight check DA store first
• ❌ GetRangeByHeight doesn't check DA store (the most important one!)
• ❌ No metrics to measure effectiveness

⚠️ Optimization Suggestion: Use GetHeader Instead of Full Block

@julienrbrt's review comment is excellent: In sync_service.go:101-103, for HeaderSyncService, you're calling GetBlockByHash which retrieves both header and data:

getter := func(ctx context.Context, s store.Store, hash header.Hash) (*types.SignedHeader, error) {
    h, _, err := s.GetBlockByHash(ctx, hash)  // ← Wasteful, ignores Data
    return h, err
}

Impact: This fetches unnecessary data from disk and discards it.

Better approach:

getter := func(ctx context.Context, s store.Store, hash header.Hash) (*types.SignedHeader, error) {
    height, err := s.getHeightByHash(ctx, hash)
    if err != nil {
        return nil, err
    }
    return s.GetHeader(ctx, height)  // Only fetch header
}

Note: getHeightByHash is currently private in store.go:83. Consider exposing it or adding a GetHeaderByHash(hash) method to the Store interface.

Optimize header retrieval →

Missing: Metrics and Observability

No metrics for:

• DA store hit rate
• DA store error rate
• Network call reduction percentage
• Latency comparison (DA vs p2p)

Recommendation: Add metrics to measure the effectiveness of this optimization.

---

5. Testing & Reliability

✅ Good Test Coverage for Implemented Methods

The tests in exchange_wrapper_test.go adequately cover:

• Store hits
• Store misses
• Store not configured scenarios

❌ Missing Tests

1. GetRangeByHeight - No tests at all (because it's not implemented)
2. Start/Stop lifecycle - Not tested
3. Concurrent access - No concurrency tests
4. DA store errors - Tests only cover "not found", not store failures
5. Integration tests - No tests verifying the wrapper works correctly in the full sync flow

⚠️ Test Quality Issues

Mock Usage (exchange_wrapper_test.go:31)

daStore: mocks.NewMockStore(t),

The mock store is created but never configured with expectations. The test passes because the getter function is provided directly, bypassing the store. This doesn't test the real integration.

Better approach: Either mock the store properly or use a real in-memory store implementation.

Dummy Data (exchange_wrapper_test.go:19)

expectedHeader := &types.SignedHeader{} // Just a dummy

Using zero-value headers in tests masks potential issues. Use realistic test data with proper values.

---

6. Documentation & Developer Experience

⚠️ Missing Documentation

• No package-level documentation for exchange_wrapper.go
• No godoc comments for exported types (even though they're private, internal docs are valuable)
• No explanation of when/why to use DA store vs p2p
• No documentation of the lifecycle (when is DA store available?)

Suggested Documentation

// Package sync provides P2P synchronization services with an intelligent
// exchange wrapper that checks local DA storage before making network requests.
//
// The exchangeWrapper implements a cache-first strategy:
// 1. Check local DA store for requested data
// 2. Fall back to P2P exchange if not found locally
// 3. This reduces network load and improves sync performance

// exchangeWrapper wraps a P2P exchange with DA store-aware caching.
// It checks the local DA store before making network requests for headers/data.
// This is particularly useful for recovering from sync gaps where blocks may be
// missing from the P2P store but available in the DA store.
type exchangeWrapper[H header.Header[H]] struct { ... }

⚠️ Review Comment: Questions Unanswered

@julienrbrt asked (sync_service.go:75): "What is this used for now?"

The variable getter is used to construct the exchangeWrapper. This should be documented in the code.

---

Summary of Required Changes

High Priority (Blocking)

1. ❌ Implement GetRangeByHeight with DA store checking and p2p fallback
2. ⚠️ Add lifecycle check to ensure DA store is started before use
3. ⚠️ Add tests for GetRangeByHeight

Medium Priority (Should Fix)

4. ⚠️ Optimize header retrieval to use GetHeader instead of GetBlockByHash
5. ⚠️ Improve error handling - log DA store errors instead of silent ignore
6. ⚠️ Simplify architecture - reconsider the function parameter pattern
7. ⚠️ Improve test quality - use realistic data, test error cases
8. ⚠️ Add observability metrics

Low Priority (Nice to Have)

9. ℹ️ Improve naming clarity for getter functions
10. ℹ️ Add documentation
11. ℹ️ Add concurrency tests

---

Positive Aspects

✅ Clean decorator pattern implementation
✅ Good use of generics for type safety
✅ Addresses a real production issue (missing blocks)
✅ Follows existing codebase patterns
✅ Basic test coverage for simple paths

---

Final Recommendation

Status: ⚠️ Changes Requested

This PR introduces a valuable optimization but has one critical gap: GetRangeByHeight is not implemented. Since this method is likely called most frequently by the syncer, the PR won't provide the expected benefits until it's fixed.

Before merging:

1. Implement GetRangeByHeight with DA store fallback logic
2. Add tests for the new implementation
3. Consider optimizing the header retrieval path
4. Address the review feedback from @renaynay and @julienrbrt

[codecov]

Codecov Report

❌ Patch coverage is 69.09091% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.76%. Comparing base (6d132bf) to head (1f1ed3e).

Files with missing lines	Patch %	Lines
pkg/sync/sync_service.go	58.06%	10 Missing and 3 partials ⚠️
pkg/sync/exchange_wrapper.go	77.77%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2855      +/-   ##
==========================================
- Coverage   57.87%   57.76%   -0.11%     
==========================================
  Files          97       98       +1     
  Lines        9303     9341      +38     
==========================================
+ Hits         5384     5396      +12     
- Misses       3315     3339      +24     
- Partials      604      606       +2     

Flag	Coverage Δ
combined	57.76% <69.09%> (-0.11%)	⬇️

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.

[renaynay]

don't you by default pass daStore to constructor of exchangeWrapper for all node types? If so, you can assume it exists and don't need to nest this call

Although exchangeWrapper start should ensure that DA datastore is started (so that get calls won't fail)

[renaynay]

why don't you just define these as functions that don't take the store, and just pass the function itself into the constructor so u don't need to store DA ds on the exchangeWrapper?

[renaynay]

You need to actually implement this method w/ the p2p as a fallback system bc this is the method called by syncer.

So here, check da store + pull out whatever contiguous range it has + request remainder from network (if there is remainder)

[renaynay]

confusing naming here, hard to follow which is which

[renaynay]

exactly, just pass in these functions (pref named better) to the constructor and no need to pass the DA ds