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, 9:17 AM

[claude]

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

---

Code Review Complete

I've completed a comprehensive code review of the P2P exchange wrapper implementation. Here are my findings:

---

Critical Issues

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

The GetRangeByHeight method currently just delegates to the p2p exchange without checking the DA store first. This is the most critical issue raised by @renaynay in review comments.

Current code:

func (ew *exchangeWrapper[H]) GetRangeByHeight(ctx context.Context, from H, to uint64) ([]H, error) {
    return ew.p2pExchange.GetRangeByHeight(ctx, from, to)
}

Problem: This defeats the entire purpose of the wrapper - the syncer calls GetRangeByHeight to fetch blocks, and if this doesn't check the DA store first, you'll still have the missing blocks issue you're trying to solve.

Required fix: Implement a hybrid approach:

1. Check DA store for contiguous range starting from from.Height()
2. Request any remaining blocks from the p2p network
3. Combine results

Fix this →

---

High Priority Issues

2. Use GetHeader Instead of GetBlockByHash (pkg/sync/sync_service.go:101-103)

@julienrbrt correctly identified that for the header sync service, you should use GetHeader instead of GetBlockByHash.

Current code:

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

Problem: GetBlockByHash is more expensive as it retrieves both header and data. For header sync, you only need the header.

Fix: Change to:

getter := func(ctx context.Context, s store.Store, hash header.Hash) (*types.SignedHeader, error) {
    return s.GetHeaderByHash(ctx, hash)
}

Fix this →

3. Confusing Function Naming (pkg/sync/sync_service.go:61-62, 81-88)

The naming of getter and getterByHeight stored in the sync service, alongside similar function parameters in constructors, creates confusion about which is which.

Problem: Hard to follow which getter function is being used where, especially when both the struct and constructor parameters have similar names.

Recommendation: Rename for clarity:

• getter → daStoreGetByHash
• getterByHeight → daStoreGetByHeight

This makes it immediately clear these are DA store accessors.

Fix this →

4. Architecture: Pass Functions Instead of Store (pkg/sync/exchange_wrapper.go:22, 29, 41)

@renaynay suggested passing the getter functions directly instead of storing the DA datastore reference.

Current approach: Store daStore + getter + getterByHeight on the wrapper.

Suggested approach: Don't pass daStore at all - just pass pre-configured functions that have the store bound to them.

Benefits:

• Cleaner separation of concerns
• The wrapper doesn't need to know about the store
• Less nil checking needed
• More functional approach

However: The current approach is acceptable if you ensure the DA datastore lifecycle is properly managed (see issue #5).

---

Medium Priority Issues

5. Missing DA Store Lifecycle Management (pkg/sync/exchange_wrapper.go:59-61)

@renaynay's comment about ensuring "DA datastore is started" is important but not addressed.

Problem: The exchange wrapper's Start() method doesn't verify the DA store is ready. If DA store isn't started, the Get/GetByHeight calls might fail or return incomplete results.

Current code:

func (ew *exchangeWrapper[H]) Start(ctx context.Context) error {
    return ew.p2pExchange.Start(ctx)
}

Recommendation: Add a check or comment documenting the assumption that DA store is already started before the exchange wrapper is used.

6. Error Handling: Silent Failures on DA Store Lookups (pkg/sync/exchange_wrapper.go:30, 42)

The current implementation silently ignores all errors from DA store lookups and falls back to p2p.

Current pattern:

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

Problem: This swallows all errors, including:

• Context cancellation
• Database corruption
• Access permission issues
• Legitimate "not found" vs. system errors

Recommendation: Distinguish between "not found" errors (fallback to p2p) and system errors (return error). Example:

if ew.daStore != nil && ew.getter != nil {
    h, err := ew.getter(ctx, ew.daStore, hash)
    if err == nil && !h.IsZero() {
        return h, nil
    }
    // Check for non-"not found" errors
    if err != nil && !errors.Is(err, store.ErrNotFound) {
        return h, fmt.Errorf("DA store lookup failed: %w", err)
    }
}

Fix this →

7. Unclear Purpose of daStore Field in SyncService (pkg/sync/sync_service.go:56)

@julienrbrt asked "What is this used for now?" about the daStore field in SyncService.

Current usage: Passed to exchangeWrapper constructor but not used directly by SyncService.

Recommendation: Add a comment explaining:

// daStore is the DA layer store, passed to exchangeWrapper to enable
// checking local DA storage before making p2p requests
daStore store.Store

---

Code Quality & Architecture

Positive Observations:

✅ Generic implementation using header.Header[H] is clean and reusable
✅ Proper separation of concerns with the wrapper pattern
✅ Follows existing project patterns (similar to other exchange implementations)
✅ Good use of function types for dependency injection

Areas for Improvement:

8. Interface Definition (pkg/sync/exchange_wrapper.go:13-18)

Good addition of the P2PExchange interface for testing. However, consider:

• Document why this interface is needed (for mocking/testing)
• The interface duplicates most of header.Exchange[H] - consider embedding it:

// P2PExchange extends header.Exchange with lifecycle management.
// This interface exists to enable mocking of the underlying p2p exchange.
type P2PExchange[H header.Header[H]] interface {
    header.Exchange[H]
    Start(context.Context) error
    Stop(context.Context) error
}

9. Zero Value Checks (pkg/sync/exchange_wrapper.go:30, 42)

The !h.IsZero() checks are good defensive programming, but it's unclear when a header would be returned with err == nil but be zero-valued. Consider:

• Document when this can happen
• Or remove if it's redundant with error checking

---

Testing & Reliability

Test Coverage Issues:

10. Missing Test Cases (pkg/sync/exchange_wrapper_test.go)

Good basic tests, but missing critical scenarios:

• ❌ No test for GetRangeByHeight (the unimplemented method!)
• ❌ No test for context cancellation during DA store lookup
• ❌ No test for DA store returning errors vs. not found
• ❌ No test for Head() method
• ❌ No test for Start()/Stop() lifecycle

11. Test Quality

The existing tests have some issues:

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

Using zero-valued headers in tests doesn't validate the actual data flow. Consider using proper test fixtures.

12. Mock Verification

In "Hit in Store" test (line 21-38), the mock p2pExchange is created but not verified that it's NOT called. Add:

mockEx.AssertNotCalled(t, "Get")

---

Performance & Resource Efficiency

13. Potential Optimization: Caching

Consider adding metrics or logging to track DA store hit rate. This would help validate if the wrapper is actually solving the missing blocks problem.

// Add to exchangeWrapper
type exchangeWrapperMetrics struct {
    daStoreHits   atomic.Int64
    daStoreMisses atomic.Int64
    p2pRequests   atomic.Int64
}

14. Concurrency Safety

The wrapper looks thread-safe as it has no mutable state. Good! Consider documenting this:

// exchangeWrapper is safe for concurrent use.
type exchangeWrapper[H header.Header[H]] struct {

---

Documentation & Developer Experience

15. Missing Package/Type Documentation

• No package-level comment explaining the exchange wrapper concept
• exchangeWrapper type has no doc comment
• Function type definitions lack explanation

Recommended additions:

// exchangeWrapper wraps a P2P exchange with local DA store checks.
// It attempts to fulfill requests from the DA store before falling back
// to network requests, reducing network traffic and improving resilience
// when blocks are randomly missing from the p2p store.
type exchangeWrapper[H header.Header[H]] struct {

16. Error Messages Need Context

If implementing error handling improvements (issue #6), ensure error messages include context:

return h, fmt.Errorf("failed to query DA store for hash %x: %w", hash, err)

---

Security Considerations

✅ No apparent security issues
✅ No credential exposure
✅ Proper context propagation for cancellation
✅ No unchecked external input

---

Summary

Must Fix Before Merge:

1. ❗ Implement GetRangeByHeight with DA store checking - This is the core functionality
2. ❗ Use GetHeader instead of GetBlockByHash for header sync - Performance issue

Should Fix:
3. Improve error handling to distinguish system errors from not-found
4. Add comprehensive tests for GetRangeByHeight
5. Clarify naming (getter functions)
6. Add documentation for types and methods

Nice to Have:
7. Add metrics for DA store hit rate
8. Document concurrency safety
9. Implement suggested architecture change (pass functions instead of store)

---

Overall Assessment:
The PR is a good start on an important resilience feature, but the GetRangeByHeight implementation is incomplete and defeats the purpose of the wrapper since that's what the syncer actually calls. The existing review comments from @renaynay are spot-on and need to be addressed before merging.

Code Coverage:
The 69% patch coverage is concerning given that the critical GetRangeByHeight method is not implemented and not tested.

---

[codecov]

Codecov Report

❌ Patch coverage is 69.09091% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.78%. Comparing base (aaae087) to head (f5333c3).

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.88%   57.78%   -0.11%     
==========================================
  Files          97       98       +1     
  Lines        9306     9344      +38     
==========================================
+ Hits         5387     5399      +12     
- Misses       3315     3339      +24     
- Partials      604      606       +2     

Flag	Coverage Δ
combined	57.78% <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