feat: DA hints in p2p

[alpe]

Overview

Resolves #2609

The basic idea is to store an additional DAHightHint field within the p2p store.

As SignedHeader and Data are used in other places, too. I added an an DAHeightHintContainer type to wrap the tuple for the store only.
The DAHeight Hint is added by the da_submitter and read in the syncer to fetch the missing DA header/data for the most recent block as required.

Please note: this is a breaking change to the p2p network and store

[claude]

Claude finished @alpe's task in 2m 48s —— View job

---

Comprehensive Code Review

I've completed a thorough review of the DA hints implementation. Here are my findings organized by priority and category:

---

🔴 Critical Issues

1. Resource Leak: AsyncDARetriever Not Stopped

Location: block/internal/syncing/syncer.go:239-276

The AsyncDARetriever is started in Syncer.Start() (line 208) but never stopped in Syncer.Stop(). This causes goroutine leaks.

Impact: Each syncer restart leaks 5 worker goroutines indefinitely.

Fix Required:

// In Syncer.Stop(), before s.wg.Wait():
if s.asyncDARetriever != nil {
    s.asyncDARetriever.Stop()
}

Fix this →

---

2. Channel Not Closed on Async Retriever Stop

Location: block/internal/syncing/async_da_retriever.go:50-57

The workCh channel is never closed when the retriever stops, which could cause goroutines to hang if they're trying to send to it.

Current Code:

func (r *AsyncDARetriever) Stop() {
    if r.cancel != nil {
        r.cancel()
    }
    r.wg.Wait()
    // workCh is never closed!
}

Fix Required: Close workCh after all workers have exited to prevent potential hangs.

Fix this →

---

3. Unbounded Retry Loop in SetProcessedHeight

Location: block/internal/syncing/p2p_handler.go:62-72

The retry loop has a hardcoded limit of 1000 iterations with no backoff or error handling. This is a code smell that could hide CAS contention issues.

Current Code:

func (h *P2PHandler) SetProcessedHeight(height uint64) {
    for range 1_000 {  // Magic number, no error handling
        current := h.processedHeight.Load()
        if height <= current {
            return
        }
        if h.processedHeight.CompareAndSwap(current, height) {
            return
        }
    }
    // Silently fails after 1000 attempts!
}

Issues:

• No logging if all 1000 attempts fail
• No runtime.Gosched() to yield CPU during contention
• Magic number without justification

Recommendation: Add logging if the loop exhausts, or consider if this pattern is necessary at all (CAS should succeed quickly in normal conditions).

---

🟡 High Priority Issues

4. Missing Test Coverage for Binary Compatibility Edge Cases

Location: types/p2p_envelope_test.go

The backward compatibility tests (TestSignedHeaderBinaryCompatibility and TestDataBinaryCompatibility) are excellent, but they don't test:

• What happens when an old node (using SignedHeader) receives data from a new node (using P2PSignedHeader with DA hints)?
• What happens when da_height_hint is 0 vs unset (both will be 0 in Go)?
• Forward compatibility: Can new code handle legacy data?

Coverage Gap: The tests verify that P2P types can unmarshal old formats, but don't verify all edge cases around the optional field semantics.

Recommendation: Add tests for:

// Test: Old format → New format → Old format round-trip
// Test: What happens with da_height_hint = 0 (valid DA height)
// Test: Partial compatibility scenarios

---

5. Potential Race in AsyncDARetriever.RequestRetrieval

Location: block/internal/syncing/async_da_retriever.go:61-76

The pattern of checking inFlight and adding to it is racy with the cleanup in processRetrieval:

// RequestRetrieval - adds to inFlight under lock
func (r *AsyncDARetriever) RequestRetrieval(height uint64) {
    r.mu.Lock()
    defer r.mu.Unlock()
    if _, exists := r.inFlight[height]; exists {
        return
    }
    // ... send to workCh
    r.inFlight[height] = struct{}{}
}

// processRetrieval - removes from inFlight under lock
defer func() {
    r.mu.Lock()
    delete(r.inFlight, height)
    r.mu.Unlock()
}()

Scenario: If a request completes and deletes from inFlight, then a duplicate request comes in before the channel is drained, it will be queued again.

Assessment: The current implementation handles this correctly because workCh acts as a buffer, but the intent could be clearer. Consider documenting this behavior or adding a test case.

---

6. Error Handling: Silent Failures in AppendDAHint

Location: block/internal/submitting/da_submitter.go:213-216

if err := s.headerDAHintAppender.AppendDAHint(ctx, res.Height, hashes...); err != nil {
    s.logger.Error().Err(err).Msg("failed to append da height hint in header p2p store")
    // ignoring error here, since we don't want to block the block submission'
}

Concern: While the comment explains the rationale, this means DA hints can be silently lost without any retry mechanism or visibility into how often this happens.

Recommendation:

• Add a metric to track AppendDAHint failures
• Consider a background retry mechanism for failed hint updates
• Document the implications: nodes will fall back to scanning DA without hints

---

7. Protobuf Field Numbering Concern

Location: proto/evnode/v1/evnode.proto:110-123

The P2PSignedHeader and P2PData messages use optional fields for da_height_hint:

message P2PSignedHeader {
  Header header = 1;
  bytes signature = 2;
  Signer signer = 3;
  optional uint64 da_height_hint = 4;  // Field 4 was reserved in SignedHeader
}

Issue: The use of field number 4, which was reserved in SignedHeader, is intentional for backward compatibility. However, the PR description states this is a "breaking change to the p2p network."

Question: If fields 1-3 are identical and field 4 uses a reserved slot, shouldn't this be backward compatible on the wire format? The tests suggest it is. This contradiction needs clarification.

Recommendation: Update the PR description to accurately reflect the compatibility story, or explain what specifically is breaking.

---

🟢 Medium Priority Issues

8. Inconsistent Error Logging Levels

Location: block/internal/syncing/async_da_retriever.go:100

r.logger.Debug().Err(err).Uint64("height", height).Msg("async DA retrieval failed")

DA retrieval failures are logged at Debug level, which may make production troubleshooting difficult. Consider whether some failures should be Warn or Error level, or add a metric.

---

9. Magic Number: Worker Pool Size

Location: block/internal/syncing/async_da_retriever.go:42-43

for i := 0; i < 5; i++ {  // Hardcoded worker count
    r.wg.Add(1)
    go r.worker()
}

Issue: The number 5 is hardcoded without configuration option or justification.

Recommendation: Make this configurable or document why 5 workers is the right number.

---

10. Buffer Size Selection

Location: block/internal/syncing/async_da_retriever.go:33

workCh: make(chan uint64, 100), // Buffer size 100

Question: What's the rationale for a buffer of 100? This should be documented or made configurable based on expected concurrency.

---

11. Missing Context Propagation

Location: pkg/sync/sync_service.go:184-198

The AppendDAHint method uses the context passed in, but there's no timeout or cancellation handling specific to this operation. If the context is cancelled mid-operation, the store could be left in an inconsistent state.

Current Code:

func (s *SyncService[V]) AppendDAHint(ctx context.Context, daHeight uint64, hashes ...types.Hash) error {
    entries := make([]V, 0, len(hashes))
    for _, h := range hashes {
        v, err := s.store.Get(ctx, h)  // Could be cancelled here
        // ...
    }
    return s.store.Append(ctx, entries...)  // Or here
}

Recommendation: Consider using a child context with timeout for critical sections, or document that this must be called with a non-cancellable context.

---

🔵 Code Quality & Best Practices

12. Excellent Binary Compatibility Strategy

Location: types/p2p_envelope.go

The implementation of binary compatibility between SignedHeader/Data and P2PSignedHeader/P2PData is well-designed:

✅ Uses proto field compatibility (fields 1-3 match exactly)
✅ Comprehensive tests for both directions
✅ Handles optional fields correctly

Recommendation: Document this pattern in comments as it's a clever solution to the upgrade problem.

---

13. Well-Structured Async Retriever

Location: block/internal/syncing/async_da_retriever.go

The worker pool pattern is well-implemented:

• Proper use of sync.WaitGroup
• Idempotent request handling via inFlight map
• Non-blocking request submission with overflow protection

Minor improvement: Add metrics for queue depth and worker utilization.

---

14. Generic Interface Design

Location: pkg/sync/sync_service.go:39-43

type EntityWithDAHint[H any] interface {
    header.Header[H]
    SetDAHint(daHeight uint64)
    DAHint() uint64
}

Clean use of Go generics to extend the header interface with DA hint capabilities. Well done.

---

🟣 Testing & Coverage

15. Test Coverage Analysis

Codecov Report: 70.17% patch coverage with 68 lines missing

Key gaps:

• types/p2p_envelope.go: 53.42% (29 missing, 5 partial)
• block/internal/syncing/syncer.go: 58.62% (12 missing)
• pkg/sync/sync_service.go: 69.44% (7 missing, 4 partial)

Critical uncovered paths:

1. Error handling in MarshalBinary/UnmarshalBinary for unsupported types
2. AsyncDARetriever worker pool saturation
3. DA hint append failures and retry logic

Recommendation: Add tests for error paths and edge cases, particularly around the async retriever behavior under high load.

---

16. Good Test Quality

Location: block/internal/syncing/async_da_retriever_test.go

The tests demonstrate:

• ✅ Idempotency verification
• ✅ Worker pool limits
• ✅ Concurrent request handling

These are well-written and cover the important behavioral aspects.

---

🟤 Documentation & Maintenance

17. Missing Migration Documentation

Location: PR description and code comments

The PR mentions "breaking change to the p2p network and store" but:

• No migration guide for existing networks
• No version compatibility matrix
• No rollback strategy

From comments: @tac0turtle asked "how would an existing network handle using this feature? its breaking so is it safe to upgrade?" and @alpe responded about binary compatibility, but this needs formal documentation.

Recommendation: Add a migration guide covering:

• Upgrade sequence for live networks
• Data migration for p2p stores
• Backward compatibility windows
• Rollback procedures

---

18. Inline Comments for Complex Logic

Location: block/internal/syncing/syncer.go:600-622

The DA hint extraction logic in processHeightEvent is complex but lacks inline comments:

if event.Source == common.SourceP2P {
    var daHeightHints []uint64
    if event.DaHeightHints[0] > 0 && event.DaHeightHints[1] > 0 {
        // Why this logic? Comment needed
        if _, exists := s.cache.GetHeaderDAIncluded(event.Header.Hash().String()); !exists {
            daHeightHints = []uint64{event.DaHeightHints[0]}
        }
        // ... more complex logic
    }
}

Recommendation: Add comments explaining the deduplication logic and why we check cache for DA inclusion.

---

📊 Summary

Blocking Issues (Must Fix Before Merge)

1. ✅ Goroutine leak in AsyncDARetriever (not stopped)
2. ✅ workCh channel not closed on Stop
3. ⚠️ Clarify breaking change story vs. binary compatibility

Recommended Fixes

• Add metrics for DA hint append failures
• Document worker pool sizing decisions
• Improve test coverage for error paths
• Add migration documentation

Positive Highlights

• Binary compatibility strategy is excellent
• AsyncDARetriever design is solid
• Generic interface design is clean
• Comprehensive compatibility tests

Overall Assessment

The implementation is well-designed with a clever approach to backward compatibility. The main concerns are resource management (goroutine leak) and operational visibility (metrics, logging). Once the critical issues are addressed, this will be a solid addition.

Recommendation: Address the resource leak and clarify the breaking change documentation before merging. The CI issues mentioned by @tac0turtle should also be investigated.

---

[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 20, 2026, 1:38 PM

[codecov]

Codecov Report

❌ Patch coverage is 70.17544% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.71%. Comparing base (9ad4016) to head (6e4554d).

Files with missing lines	Patch %	Lines
types/p2p_envelope.go	53.42%	29 Missing and 5 partials ⚠️
block/internal/syncing/syncer.go	58.62%	12 Missing ⚠️
pkg/sync/sync_service.go	69.44%	7 Missing and 4 partials ⚠️
block/internal/syncing/async_da_retriever.go	86.53%	6 Missing and 1 partial ⚠️
block/internal/submitting/da_submitter.go	80.95%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2891      +/-   ##
==========================================
+ Coverage   59.53%   59.71%   +0.17%     
==========================================
  Files         107      109       +2     
  Lines       10075    10261     +186     
==========================================
+ Hits         5998     6127     +129     
- Misses       3447     3497      +50     
- Partials      630      637       +7     

Flag	Coverage Δ
combined	59.71% <70.17%> (+0.17%)	⬆️

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.

[alpe]

This is where the DA height is passed to the sync service to update the p2p store

[alpe]

This is where the "fetch from DA" is triggered for the current block event height

[alpe]

This is a data container to persist the DA hint together with the block header or data.
types.SignedHeader and types.Data are used all over the place so I did not modify them but added introduced this type for the p2p store and transfer only.

It may make sense to do make this a Proto type. WDYT?

[alpe]

Stores the DA height hints

[tac0turtle]

if da hint is not in the proto how do other nodes get knowledge of the hint?

also how would an existing network handle using this feature? its breaking so is it safe to upgrade?

[julienrbrt]

nit: gci linter

[julienrbrt]

Nice! It really makes sense.

I share the same concern as @tac0turtle however about the upgrade strategy given it is p2p breaking.

[alpe]

if da hint is not in the proto how do other nodes get knowledge of the hint?

The sync_service wraps the header/data payload in a DAHeightHintContainer object that is passed upstream to the p2p layer. When the DA height is known, the store is updated.

also how would an existing network handle using this feature? its breaking so is it safe to upgrade?

It is a breaking change. Instead of signed header or data types, the p2p network exchanges DAHeightHintContainer. This would be incompatible. Also the existing p2p stores would need migration to work.

[julienrbrt]

Could we broadcast both until every networks are updated? Then for final we can basically discard the previous one.

[alpe]

fyi: This PR is missing a migration strategy for the p2p store ( and ideally network)

[alpe]

I have added 2 new types for the p2p store that are binary compatible to the types.Data and SignedHeader. With this, we should be able to roll this out without breaking the in-flight p2p data and store.

[julienrbrt]

lgtm! I can see how useful the async retriever will be for force inclusion verification as well. We should have @auricom verify if p2p still works with Eden.

[julienrbrt]

This is going to be really useful for force inclusion checks as well.

[tac0turtle]

ci seems to be having some issues, can these be fixed.

Also was this tested on an existing network? If not, please do that before merging