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	Dec 11, 2025, 10:46 AM

[claude]

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

---

Comprehensive Code Review

I've completed a detailed review of the P2P exchange wrapper implementation. Overall, this is a solid architectural improvement that addresses the missing blocks issue by checking the DA store before making network requests. Below are my findings organized by category:

---

Architecture & Design ⭐

Strengths:

• Smart layering: The wrapper pattern is well-chosen for this use case, providing transparent caching without requiring changes to consumers
• Clear separation of concerns: The exchangeWrapper cleanly separates local store checking from network exchange logic
• Generic implementation: Good use of Go generics to support both SignedHeader and Data types
• Function injection pattern: Using storeGetter and storeGetterByHeight function types provides flexibility and testability

Concerns:

1. Missing metrics and observability (High Priority)

   • No metrics to track cache hit/miss rates for the DA store check
   • No logging to indicate when the wrapper falls back to network vs serving from DA store
   • Recommendation: Add metrics for:
     • DA store hits/misses
     • Network fallback count
     • Latency comparison (store vs network)
   • Add debug-level logging showing which path was taken

2. Interface documentation (pkg/sync/exchange_wrapper.go:13-18)

   • The P2PExchange interface lacks documentation explaining why it extends header.Exchange[H] with lifecycle methods
   • Recommendation: Add godoc explaining the interface purpose and usage

---

Error Handling & Resilience

Strengths:

• Proper fallback behavior when DA store checks fail
• Null-safe checks for daStore and getter functions

Concerns:

1. Silent error suppression (pkg/sync/exchange_wrapper.go:30-32, 42-44)

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

   • Errors from the DA store are completely ignored
   • No distinction between "not found" errors and real failures (network issues, corruption, etc.)
   • Risk: Genuine errors in DA store access could mask underlying problems
   • Recommendation:
     • Log errors at debug or warn level
     • Consider metrics for DA store error rates
     • Differentiate between expected "not found" vs unexpected errors

2. Context cancellation handling

   • No explicit handling of context cancellation during DA store checks
   • If context is cancelled during DA store access, the error is silently ignored and network fetch is attempted
   • Recommendation: Check for context.Canceled and context.DeadlineExceeded errors and propagate them immediately

---

Concurrency & Performance

Strengths:

• No shared mutable state in exchangeWrapper
• Immutable configuration after construction

Concerns:

1. Sequential fallback pattern (pkg/sync/exchange_wrapper.go:27-36)

   • DA store check adds latency even when it fails
   • No timeout on DA store operations separate from the network timeout
   • Recommendation: Consider adding a short timeout for DA store checks to prevent slow DA store access from blocking network requests

2. atomic.Bool usage (pkg/sync/sync_service.go:63)

   • Good use of atomic.Bool for storeInitialized
   • The compare-and-swap logic in WriteToStoreAndBroadcast and initFromP2PWithRetry is correct
   • However, the retry logic in initStore could race if multiple goroutines call it
   • Verification needed: Are there scenarios where WriteToStoreAndBroadcast and initFromP2PWithRetry could be called concurrently?

3. No rate limiting on network fallback

   • If DA store consistently fails, all requests go to network with no backoff
   • Could overwhelm network exchange if DA store has issues
   • Recommendation: Consider circuit breaker pattern if DA store failures are sustained

---

Testing Coverage

Strengths:

• Good unit test coverage for the wrapper with three scenarios per method
• Tests use proper mocking with testify and mockery
• Clear test names and structure

Concerns:

1. Missing test scenarios (pkg/sync/exchange_wrapper_test.go)

   • No test for context cancellation during DA store access
   • No test for IsZero() returning true (this case is treated as miss)
   • No test for error handling when p2pExchange fails
   • No tests for Head(), GetRangeByHeight(), Start(), Stop() methods
   • No integration test showing the wrapper working with real store and exchange
   • Coverage gap: The codecov report shows 70.83% patch coverage with 14 lines missing

2. Test dummy data (pkg/sync/exchange_wrapper_test.go:19)

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

   • Using zero-value structs in tests doesn't validate the IsZero() check behavior
   • Recommendation: Use properly initialized test fixtures

3. Missing error case tests (pkg/sync/sync_service_test.go)

   • No tests for the DA store error scenarios in the integrated sync service
   • New TestHeaderSyncServiceInitFromHigherHeight test doesn't verify the exchange wrapper behavior

---

Code Quality & Go Idioms

Strengths:

• Clean, idiomatic Go code
• Good use of generics
• Proper receiver naming (ew)

Concerns:

1. Exported vs unexported types (pkg/sync/exchange_wrapper.go:20-25)

   • exchangeWrapper is unexported but P2PExchange is exported
   • This asymmetry is intentional but undocumented
   • Recommendation: Add package-level documentation explaining the design

2. Function type aliases (pkg/sync/exchange_wrapper.go:10-11)

   • storeGetter and storeGetterByHeight are clean but lack documentation
   • Recommendation: Add godoc comments explaining their purpose

3. Removed functionality (pkg/sync/sync_service.go)

   • Removed TrustedHash configuration support - was this intentional?
   • Removed getChainID() method - verify this isn't used elsewhere
   • Changed initialization logic significantly - needs migration docs if this is breaking

---

Security Considerations

Strengths:

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

Concerns:

1. Trust boundary (pkg/sync/exchange_wrapper.go:27-36)

   • DA store data is trusted implicitly without validation
   • No verification that data from DA store matches network data
   • Risk: If DA store is compromised or corrupted, invalid data could be served
   • Recommendation: Consider adding optional validation or checksums

2. No data freshness checks

   • DA store could serve stale data if not properly synchronized
   • No mechanism to verify DA store data is current
   • Recommendation: Add freshness validation or timestamp checks

---

Documentation & Developer Experience

Concerns:

1. Missing PR description details

   • PR description mentions "missing 40 blocks randomly" but doesn't explain the root cause
   • No explanation of why the exchange wrapper solves this vs fixing the underlying issue
   • No performance impact analysis (does DA store checking add latency?)

2. Missing architecture docs

   • No diagram or documentation showing data flow: DA store → wrapper → network
   • No guidance on when DA store is expected to have data

3. Breaking changes (node/full.go:160, 177)

   • Constructor signatures changed to add daStore parameter
   • This is a breaking change for anyone constructing these services manually
   • Recommendation: Add migration notes or deprecation warnings

---

Response to Review Comments

Addressing julienrbrt's review comments:

1. "GetHeader is less expensive" (pkg/sync/sync_service.go:106)

   getterByHeight := func(ctx context.Context, s store.Store, height uint64) (*types.SignedHeader, error) {
       return s.GetHeader(ctx, height)
   }

   • ✅ This is correctly using GetHeader - the comment appears to be acknowledging good practice

2. "What is this used for now?" (location unclear)

   • Needs response from PR author explaining the removed trusted hash feature

---

Specific File Issues

pkg/sync/sync_service.go:164-175

• Complex atomic boolean logic with CompareAndSwap
• The pattern is correct but could benefit from comments explaining the race condition it prevents

pkg/sync/sync_service.go:319-325

• Creating the wrapper inline makes it harder to test
• Recommendation: Extract to a factory function for better testability

pkg/sync/sync_service.go:356-387

• The initFromP2PWithRetry refactor removes trusted hash support
• This appears to be a significant behavioral change
• Verify: Is removal of trusted hash support intentional? Any migration path needed?

---

Recommendations Summary

Must Fix (High Priority):

1. Add error logging for DA store failures (currently silent)
2. Handle context cancellation properly (don't fallback to network if context is cancelled)
3. Increase test coverage to at least 85% (currently 70.83%)
4. Document the breaking changes (constructor signatures, removed trusted hash support)

Should Fix (Medium Priority):
5. Add observability (metrics and logging) for cache hits/misses
6. Add godoc comments for exported types and functions
7. Add integration tests with real store and exchange
8. Consider adding timeout/circuit breaker for DA store access

Nice to Have (Low Priority):
9. Add architecture documentation with diagrams
10. Extract wrapper creation to factory function
11. Add validation for DA store data freshness

---

Verdict

This PR provides a solid architectural improvement with a clean wrapper pattern. The main concerns are:

• Lack of observability: No metrics or logging to understand wrapper behavior in production
• Silent error handling: DA store errors are completely ignored, which could mask issues
• Test coverage gaps: Missing edge cases and error scenarios
• Breaking changes: Constructor changes need documentation

The implementation is production-ready after addressing the error handling and observability concerns. The test coverage gaps are concerning given codecov shows 14 uncovered lines.

Suggested Action: Address the high-priority issues (error logging, context handling, tests) before merging, and file follow-up issues for the medium-priority improvements.

---

[codecov]

Codecov Report

❌ Patch coverage is 69.09091% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.27%. Comparing base (09b7efd) to head (3233285).
⚠️ Report is 2 commits behind head on main.

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   65.94%   63.27%   -2.68%     
==========================================
  Files          87       91       +4     
  Lines        7990     8544     +554     
==========================================
+ Hits         5269     5406     +137     
- Misses       2147     2553     +406     
- Partials      574      585      +11     

Flag	Coverage Δ
combined	63.27% <69.09%> (-2.68%)	⬇️

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