feat: p2p exchange wrapper #2855
Conversation
tac0turtle
changed the title
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @tac0turtle's task —— View job Comprehensive Code ReviewI've completed a thorough review of the P2P exchange wrapper implementation. This is a good optimization that addresses the edge case of missing blocks in the P2P store by checking the DA store first. Here's my detailed analysis: 🟢 StrengthsArchitecture & Design
Integration
🟡 Issues & Recommendations1. CRITICAL: Performance Issue - Unnecessary Data FetchingLocation: The getter functions retrieve full block data when only headers are needed: // HeaderSyncService getter - fetches FULL BLOCK
getter := func(ctx context.Context, s store.Store, hash header.Hash) (*types.SignedHeader, error) {
h, _, err := s.GetBlockByHash(ctx, hash) // ❌ Fetches block data unnecessarily
return h, err
}As @julienrbrt correctly noted in the review comment, Impact:
Recommendation: // For hash-based lookup
getter := func(ctx context.Context, s store.Store, hash header.Hash) (*types.SignedHeader, error) {
// Need to map hash to height first, then use GetHeader
// OR add a new Store method: GetHeaderByHash(ctx, hash)
h, _, err := s.GetBlockByHash(ctx, hash)
return h, err
}
// For height-based lookup - ALREADY CORRECT ✓
getterByHeight := func(ctx context.Context, s store.Store, height uint64) (*types.SignedHeader, error) {
return s.GetHeader(ctx, height) // ✓ Efficient
}Action Required:
2. Error Handling: Silent Failure on Store ErrorsLocation: if h, err := ew.getter(ctx, ew.daStore, hash); err == nil && !h.IsZero() {
return h, nil
}
// Falls through silently on errorIssue: All DA store errors (network failures, disk errors, corruption) are silently ignored and fall through to P2P. Recommendation: if h, err := ew.getter(ctx, ew.daStore, hash); err != nil {
// Log the error but continue to P2P fallback
// Consider metrics for monitoring store health
if !errors.Is(err, store.ErrNotFound) {
// log.Debug().Err(err).Msg("DA store lookup failed, falling back to P2P")
}
} else if !h.IsZero() {
return h, nil
}Severity: Medium - Could mask serious issues like disk corruption or database failures. 3. Concurrency Safety: Data Race PotentialLocation: The Current State:
Recommendation: // Add comment to document initialization safety contract
type SyncService[H header.Header[H]] struct {
// ... existing fields ...
// daStore, getter, getterByHeight are set once during initialization
// and must not be modified after Start() is called
daStore store.Store
getter storeGetter[H]
getterByHeight storeGetterByHeight[H]
}Severity: Low - Current implementation is safe, but better documentation would help. 4. Test Coverage: Missing Edge CasesLocation: Missing Test Cases:
Recommendation: t.Run("Zero Header Returned", func(t *testing.T) {
mockEx := extmocks.NewMockExchange[*types.SignedHeader](t)
mockEx.On("Get", ctx, hash).Return(expectedHeader, nil)
getter := func(ctx context.Context, s store.Store, h header.Hash) (*types.SignedHeader, error) {
return &types.SignedHeader{}, nil // Zero header
}
ew := &exchangeWrapper[*types.SignedHeader]{
Exchange: mockEx,
daStore: mocks.NewMockStore(t),
getter: getter,
}
h, err := ew.Get(ctx, hash)
assert.NoError(t, err)
assert.Equal(t, expectedHeader, h) // Should fallback to P2P
})
t.Run("Context Cancellation", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel() // Cancel immediately
getter := func(ctx context.Context, s store.Store, h header.Hash) (*types.SignedHeader, error) {
return nil, ctx.Err() // Should respect context
}
mockEx := extmocks.NewMockExchange[*types.SignedHeader](t)
mockEx.On("Get", ctx, hash).Return(nil, context.Canceled)
ew := &exchangeWrapper[*types.SignedHeader]{
Exchange: mockEx,
daStore: mocks.NewMockStore(t),
getter: getter,
}
_, err := ew.Get(ctx, hash)
assert.Error(t, err)
})Severity: Medium - Current tests validate happy path but miss important edge cases. 5. Documentation: Missing Rationale and BehaviorLocation: The
Recommendation: // exchangeWrapper wraps a header.Exchange to check the local DA store before
// making network requests. This addresses the edge case where the P2P store
// may be missing blocks that exist in the DA store.
//
// Behavior:
// - On Get/GetByHeight, first checks DA store using provided getter functions
// - If found and non-zero, returns immediately (fast path)
// - Otherwise, falls back to network exchange (slow path)
//
// Trade-offs:
// - Adds store lookup latency on cache miss (~1-5ms typically)
// - Reduces unnecessary P2P traffic and handles store inconsistencies
// - Silently ignores store errors (falls back to P2P)
type exchangeWrapper[H header.Header[H]] struct {
header.Exchange[H]
daStore store.Store
getter storeGetter[H]
getterByHeight storeGetterByHeight[H]
}Severity: Low - Code is readable but context would help future maintainers. 6. Metrics & Observability: No InstrumentationLocation: There are no metrics to track:
Recommendation: // Add prometheus metrics
var (
storeHitCounter = prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "sync_exchange_store_hits_total",
Help: "Number of successful DA store lookups",
},
[]string{"type"}, // "hash" or "height"
)
storeMissCounter = prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "sync_exchange_store_misses_total",
Help: "Number of DA store misses requiring P2P fallback",
},
[]string{"type"},
)
)
func (ew *exchangeWrapper[H]) Get(ctx context.Context, hash header.Hash) (H, error) {
if ew.daStore != nil && ew.getter != nil {
if h, err := ew.getter(ctx, ew.daStore, hash); err == nil && !h.IsZero() {
storeHitCounter.WithLabelValues("hash").Inc()
return h, nil
}
storeMissCounter.WithLabelValues("hash").Inc()
}
return ew.Exchange.Get(ctx, hash)
}Severity: Medium - Critical for production debugging and performance tuning. 🔒 Security Analysis✅ No Critical Security Issues Identified
Minor Consideration:
⚡ Performance AnalysisPositive Impacts:
Negative Impacts:
Optimization Opportunities:
📊 Test Coverage AnalysisCurrent Coverage: 70.83% (14 lines missing coverage) Lines Missing Coverage (per Codecov):
Test Quality Assessment:
🎯 Priority RecommendationsMust Fix Before Merge:
Should Fix:
Nice to Have:
✅ SummaryThis PR implements a pragmatic solution to the missing blocks edge case. The wrapper pattern is clean and the integration is minimal. The main concerns are:
The implementation is fundamentally sound and safe, but would benefit from the optimizations noted above before merge. Overall Assessment: ✅ Approve with Changes |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2855 +/- ##
==========================================
+ Coverage 64.49% 64.58% +0.08%
==========================================
Files 81 82 +1
Lines 7374 7403 +29
==========================================
+ Hits 4756 4781 +25
- Misses 2076 2078 +2
- Partials 542 544 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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