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 4m 39s —— View job

---

Comprehensive Code Review ✅

I've completed a thorough review of this PR, which introduces DA height hints to optimize block syncing by providing hints about where headers/data can be found in the DA layer. Here's my detailed analysis:

---

1. Architecture & Design

✅ Strengths

Clean Generic Design: The P2PEnvelope[H] generic wrapper in types/p2p_envelope.go:14-48 is well-designed, allowing flexible wrapping of different message types while maintaining type safety and satisfying the header.Header interface.

Binary Compatibility Strategy: The use of P2PSignedHeader and P2PData protobuf messages with optional DA hint fields (proto/evnode/v1/evnode.proto:111-123) achieves backward compatibility. Old nodes can decode new messages (ignoring the optional field), and new nodes can decode old messages (DA hint defaults to 0).

Async DA Retrieval: The new AsyncDARetriever (block/internal/syncing/async_da_retriever.go:11-112) with worker pool pattern is excellent for non-blocking DA fetches. The deduplication via inFlight map prevents redundant DA queries.

⚠️ Concerns

Incomplete Migration: Line 661 in block/internal/syncing/syncer.go has a TODO comment stating "To be reverted once DA Hints are merged" - but this PR IS the DA hints PR. This code writes to P2P stores WITHOUT DA hints when events come from DA. This creates an inconsistency:

• DA submitter adds hints via AppendDAHint (lines 213, 258)
• But syncer writes to P2P stores without hints when processing DA events (lines 666, 671)

Recommendation: Remove the TODO and update the code to properly set DA hints when writing to P2P stores from DA events. The DA height is available in the event (event.DaHeight).

// Instead of:
s.headerStore.WriteToStoreAndBroadcast(ctx, &types.P2PSignedHeader{Message: event.Header}, ...)

// Should be:
s.headerStore.WriteToStoreAndBroadcast(ctx, &types.P2PSignedHeader{
    Message: event.Header, 
    DAHeightHint: event.DaHeight,
}, ...)

DA Hint Source of Truth: There are now multiple ways DA hints get set:

1. DA submitter sets hints after successful submission (da_submitter.go:213, 258)
2. DA retriever provides hints when fetching from DA store (sync_service.go:102-104, 164-166)
3. P2P network propagates hints in envelopes

This complexity could lead to inconsistencies. Consider documenting the precedence and lifecycle of DA hints more explicitly.

---

2. Error Handling & Resilience

✅ Strengths

Graceful Error Handling: DA hint appending errors are logged but don't block operations (da_submitter.go:213-216, 258-261). This is correct since hints are optimizations, not critical for correctness.

Context Cancellation: AsyncDARetriever properly handles context cancellation in both worker loop (async_da_retriever.go:83) and result channel sending (async_da_retriever.go:107).

⚠️ Concerns

Silent DA Hint Failures: When AppendDAHint fails in da_submitter.go:213-216, the error is only logged. While this prevents blocking submissions, there's no metric or alert to track DA hint append failures. This makes it hard to detect if the optimization is degraded.

Recommendation: Add a metric for DA hint append failures to track this optimization's health:

s.metrics.DAHintAppendFailures.Add(1)

Unbounded Retry in SetProcessedHeight: p2p_handler.go:63-72 has a 1,000 iteration loop for compare-and-swap. While unlikely to fail repeatedly, there's no timeout or error return if all attempts fail.

Recommendation: Consider logging if retries exceed a threshold (e.g., 10) to detect potential contention issues.

---

3. Concurrency & Thread Safety

✅ Strengths

Proper Synchronization:

• AsyncDARetriever uses mutex for inFlight map (async_da_retriever.go:62-67)
• Atomic operations for processedHeight in P2PHandler (p2p_handler.go:63-72)
• Proper worker pool lifecycle management with WaitGroup (async_da_retriever.go:51-56)

Deadlock Prevention: Channel sends in AsyncDARetriever.RequestRetrieval use non-blocking select with default case (async_da_retriever.go:69-75), preventing blocking when worker pool is full.

⚠️ Concerns

Worker Pool Size: Hardcoded 5 workers in AsyncDARetriever.Start (async_da_retriever.go:43). For high-throughput scenarios with many concurrent sync operations, this might become a bottleneck.

Recommendation: Make worker pool size configurable, potentially based on the expected DA query latency and block production rate.

Channel Buffer Sizes:

• workCh buffer size is 100 (async_da_retriever.go:33)
• resultCh buffer varies by caller

These magic numbers should be documented or made configurable. If 100 requests queue up, subsequent requests are silently dropped (line 74).

---

4. Security

✅ Strengths

Proposer Validation: P2PHandler.assertExpectedProposer (p2p_handler.go:132-138) validates proposer addresses, preventing malicious blocks from unauthorized proposers.

Data Consistency Checks: P2PHandler.ProcessHeight validates that header's data hash matches actual data commitment (p2p_handler.go:104-108), preventing header/data mismatch attacks.

⚠️ Concerns

No DA Hint Validation: DA hints are accepted from P2P peers without validation. A malicious peer could provide incorrect hints, causing unnecessary DA queries to wrong heights. While this doesn't compromise correctness (actual DA data is validated), it could be a DoS vector.

Recommendation: Consider adding validation or rate limiting:

1. Track hint accuracy per peer
2. Penalize peers providing consistently wrong hints
3. Add metrics for hint miss rates

Optional Field Handling: The protobuf optional fields (DaHeightHint) default to 0 when not set. This is semantically ambiguous - does 0 mean "no hint" or "DA height 0"? For a DA starting at height 1, this works, but it's fragile.

Recommendation: Use a sentinel value (e.g., math.MaxUint64) to explicitly represent "no hint available" rather than relying on 0.

---

5. Performance & Resource Efficiency

✅ Strengths

Optimization Impact: This PR's main goal is performance optimization - avoiding full DA scans when syncing by using hints. The async retrieval pattern means DA queries don't block the sync path.

Efficient Marshaling: Binary compatibility approach means existing SignedHeader and Data types don't need changes, avoiding widespread refactoring.

⚠️ Concerns

Memory Overhead: Every P2P store entry now carries an additional uint64 (8 bytes) for the DA hint. For nodes storing millions of blocks, this adds up. However, the optimization likely outweighs the cost.

DA Store Queries: The sync_service.go getters query DA store for state (GetStateAtHeight) for EVERY P2P store read to populate DA hints (sync_service.go:97-104, 159-166, 188-195). This could be expensive if:

1. State queries are not cached
2. High read volume on P2P stores

Recommendation:

1. Consider caching state-to-DA-height mappings
2. Add metrics to track DA store query latency during P2P reads
3. Evaluate whether hints should be populated lazily vs. eagerly

Hint Propagation Delay: DA hints are added AFTER DA submission succeeds (da_submitter.go:213, 258). There's a window where:

1. Block is submitted to DA
2. Block is written to P2P store (without hint)
3. DA submission returns
4. Hint is appended to P2P store

During this window, peers might fetch the block without the hint. While not incorrect, it reduces optimization effectiveness.

---

6. Testing

✅ Strengths

Comprehensive Unit Tests:

• Binary compatibility verified in types/p2p_envelope_test.go:92-153
• Async retriever deduplication tested in async_da_retriever_test.go:39-85
• Worker pool limits tested in async_da_retriever_test.go:87-141

Test Coverage: 66.30% patch coverage per Codecov report is decent for new functionality.

⚠️ Concerns

93 Lines Missing Coverage: Key areas lacking coverage according to Codecov:

• pkg/sync/sync_service.go: 57.83% (21 missing, 14 partials)
• types/p2p_envelope.go: 53.42% (29 missing, 5 partials)
• block/internal/syncing/syncer.go: 58.62% (12 missing)

No Integration Test for Full Flow: Tests verify individual components but not the end-to-end flow:

1. Block produced and submitted to DA
2. DA height hint added to P2P store
3. Peer syncs and retrieves block with hint
4. Peer uses hint to fetch from correct DA height

Recommendation: Add an integration test in test/integration/ that verifies the full sync optimization path using DA hints.

Missing Edge Cases:

• What happens when DA hint points to wrong height?
• What happens when DA height not yet available (hint points to future)?
• Concurrent writes to same P2P store entry with different hints?

---

7. Documentation & Migration

⚠️ Major Concerns

No Migration Documentation: The PR description states "this is a breaking change to the p2p network and store" but provides no migration guide. Critical questions unanswered:

1. Network Upgrade: How should existing networks upgrade without partitioning?
2. Store Migration: How do nodes migrate existing P2P stores to include hints?
3. Rollback Strategy: Can nodes safely rollback if issues arise?
4. Compatibility Window: Should there be a version that supports both formats during transition?

Review Comments Reference Solution: @alpe mentioned (Dec 15) adding types "binary compatible" with old types. The code implements this, but it's not documented in the PR description or migration guide.

Recommendation: Add to PR description:

## Migration Strategy

### Network Compatibility
- New protobuf messages use optional fields for backward compatibility
- Old nodes can receive new messages (ignore DA hint field)
- New nodes can receive old messages (DA hint defaults to 0)
- Recommend coordinated upgrade across network

### Store Migration
- Existing P2P stores work without migration
- DA hints will be populated for new blocks
- Old blocks will have hint = 0 (no optimization, but functional)
- Optional: Background migration to populate hints for historical blocks

### Rollback
- Rolling back requires wiping P2P stores OR accepting that hints won't be read
- Not recommended after network majority upgrades

Missing Code Documentation:

• P2PEnvelope generic type lacks doc comment explaining its purpose
• DaHeightHints field in DAHeightEvent (common/event.go:25) has comment but doesn't explain why it's an array of 2
• EntityWithDAHint interface (sync_service.go:40-44) lacks explanation of its role

---

8. Critical Issues Found

🔴 Issue 1: Incomplete DA Hint Propagation (High Priority)

File: block/internal/syncing/syncer.go:661-676

The TODO comment indicates this code path was meant to be updated but wasn't. When events from DA are written to P2P stores, DA hints are not included.

Impact: Reduces effectiveness of the optimization - blocks synced from DA won't have hints in P2P store, so subsequent peers won't benefit.

Fix: Fix incomplete DA hint propagation →

🟡 Issue 2: Ambiguous Zero Value (Medium Priority)

Files: proto/evnode/v1/evnode.proto:115, 122 and usage throughout

Using optional uint64 where 0 means "no hint" is ambiguous. If DA actually starts at height 0, there's confusion.

Impact: Could cause incorrect optimization behavior if DA uses height 0.

Fix: Document clearly that 0 means "no hint" or use sentinel value.

🟡 Issue 3: Missing Test for Network Upgrade (Medium Priority)

No test verifies that old message format can be decoded by new code and vice versa under realistic network conditions.

Impact: Risk of network partition during upgrade if compatibility breaks.

Fix: Add backward compatibility integration test →

🟢 Issue 4: Network Testing Required (Low Priority - Process Issue)

Per @tac0turtle's comment (Jan 12), this needs testing on an existing network. The PR shows no evidence of this testing.

Impact: Unknown real-world behavior, potential for production issues.

Recommendation: Deploy to testnet and provide results before merging.

---

9. Code Quality & Go Conventions

✅ Strengths

• Consistent error wrapping with context
• Proper use of zerolog for structured logging
• Good separation of concerns (submitter, syncer, p2p handler)
• Effective use of Go generics in P2PEnvelope

Minor Issues

Magic Numbers: Several hardcoded values without constants:

• Worker pool size: 5 (async_da_retriever.go:43)
• Work channel buffer: 100 (async_da_retriever.go:33)
• CAS retry limit: 1000 (p2p_handler.go:63)

Inconsistent Naming:

• DaHeightHints (camelCase in field) vs DAHeightHint (PascalCase in proto)
• Should standardize on DA or Da for consistency

---

10. CI Status

Current Status: Most checks passing, one failure:

• ❌ Docs Deploy Preview (failing)
• ✅ Semantic PR, Docs Build, Rust Tests, Rust Lint (passing)
• ⏳ CI, PR checks (in progress)

The docs deployment failure appears unrelated to this PR's core functionality.

---

11. Summary & Recommendations

Must Fix Before Merge

1. Fix DA hint propagation in syncer.go:661-676 - Critical for feature to work correctly
2. Add migration documentation - Required for safe network upgrade
3. Test on existing network - As requested by maintainer (@tac0turtle)

Should Fix Before Merge

4. Add metrics for DA hint append failures
5. Improve test coverage for edge cases (wrong hints, concurrent writes)
6. Document the zero-value ambiguity for DA hints
7. Add integration test for backward compatibility

Consider for Follow-up

8. Make worker pool size configurable
9. Add hint accuracy tracking and peer reputation
10. Optimize DA store queries with caching
11. Background migration for historical blocks

---

Conclusion

This is a solid optimization PR with a thoughtful design for backward compatibility. The async DA retrieval pattern and binary-compatible protobuf approach are well-executed. However, there are critical gaps:

1. Incomplete implementation (syncer.go TODO)
2. Missing migration strategy documentation
3. No network testing evidence

These must be addressed before merge. The code quality is generally good, with comprehensive unit tests, but integration testing and documentation need strengthening.

Estimated Merge Readiness: 75% - Core functionality is sound, but critical gaps in completeness and testing remain.

---

[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 22, 2026, 10:32 AM

[codecov]

Codecov Report

❌ Patch coverage is 66.30435% with 93 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.97%. Comparing base (9348732) to head (94fe911).

Files with missing lines	Patch %	Lines
pkg/sync/sync_service.go	57.83%	21 Missing and 14 partials ⚠️
types/p2p_envelope.go	53.42%	29 Missing and 5 partials ⚠️
block/internal/syncing/syncer.go	58.62%	12 Missing ⚠️
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 ⚠️
pkg/store/store.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2891      +/-   ##
==========================================
+ Coverage   58.63%   58.97%   +0.33%     
==========================================
  Files         111      113       +2     
  Lines       10405    10634     +229     
==========================================
+ Hits         6101     6271     +170     
- Misses       3659     3699      +40     
- Partials      645      664      +19     

Flag	Coverage Δ
combined	58.97% <66.30%> (+0.33%)	⬆️

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