feat(syncer): verify force inclusion for p2p blocks

[julienrbrt]

Blocked on #2891 being merged.

ref: #2906

TODO: uncomment https://github.com/evstack/ev-node/pull/3005/changes#diff-a167327bc3b61102868a57e748c72401fd8fb4c3f9ab61101ee34e981d2c5a39R403 before rebase. Done.

[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	Feb 3, 2026, 1:09 PM

[claude]

Claude finished @julienrbrt's task in 1m 47s —— View job

---

Code Review: P2P Block Force Inclusion Verification

Tasks

• Read and analyze all changed files
• Review code quality and architecture
• Assess error handling and resilience
• Check security considerations
• Evaluate performance and resource efficiency
• Review testing coverage
• Provide comprehensive feedback

---

Summary

This PR extends forced inclusion verification to P2P blocks by using DA height hints. The implementation is generally sound, but there are critical architectural concerns that need addressing based on the author's recent comment indicating this "is actually not the right solution for P2P."

Critical Issues

1. Fundamental Approach Concerns ⚠️

Location: syncer.go:739-751

The author's comment (Feb 3, 2026) indicates that using P2P hints for forced inclusion verification is not the right solution:

Knowing #2891 (comment), this is actually not the right solution for P2P.

Issue: The core mechanism stores P2P DA height hints and uses them for verification:

// Line 739-744
currentDaHeight, ok := s.p2pHeightHints[nextHeight]
if !ok {
    currentDaHeight = currentState.DAHeight
} else {
    delete(s.p2pHeightHints, nextHeight)
}

Recommendation: Before merging, clarify the intended approach. The current implementation may need significant revision.

---

2. Silent Error Handling 🔴

Location: syncer.go:745-751

if err := s.VerifyForcedInclusionTxs(ctx, currentDaHeight, data); err != nil {
    s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("forced inclusion verification failed")
    if errors.Is(err, errMaliciousProposer) {
        s.cache.RemoveHeaderDAIncluded(headerHash)
        return err
    }
}

Critical Issue: Non-malicious errors (DA connection failures, timeout errors, etc.) are logged but swallowed, allowing potentially invalid blocks to be synced.

Risk: A node experiencing transient DA connectivity issues would accept blocks without verifying forced inclusion, defeating the censorship resistance mechanism.

From Review Discussion: The author stated the current behavior is intentional to prevent crashes, but this creates a security vulnerability.

Recommended Fix:

if err := s.VerifyForcedInclusionTxs(ctx, currentDaHeight, data); err != nil {
    s.logger.Error().Err(err).Uint64("height", nextHeight).Msg("forced inclusion verification failed")
    if errors.Is(err, errMaliciousProposer) {
        s.cache.RemoveHeaderDAIncluded(headerHash)
    }
    // Always return the error - let caller decide retry strategy
    return fmt.Errorf("forced inclusion verification failed: %w", err)
}

Alternative: Implement retry logic at the processHeightEvent level to distinguish between transient and persistent failures.

---

3. Race Condition in p2pHeightHints Map 🟡

Location: syncer.go:117, 659, 739-743

type Syncer struct {
    // ...
    p2pHeightHints map[uint64]uint64 // map[height]daHeight - NOT thread-safe
}

Issue: The p2pHeightHints map is accessed from multiple goroutines without synchronization:

• Written in processHeightEvent (line 659)
• Read and deleted in TrySyncNextBlock (lines 739-743)
• Both are called from different goroutines (p2pWorkerLoop, processLoop)

Risk: Concurrent map access can cause panics or data corruption.

Recommended Fix: Use sync.Map or add mutex protection:

type Syncer struct {
    p2pHeightHints    sync.Map // map[uint64]uint64
    // or:
    p2pHeightHintsMu  sync.RWMutex
    p2pHeightHints    map[uint64]uint64
}

---

4. Unbounded Memory Growth 🟡

Location: syncer.go:659, p2pHeightHints map

s.p2pHeightHints[height] = daHeightHint

Issue: If blocks are delayed or stuck, the p2pHeightHints map can grow indefinitely without cleanup.

Recommended Fix: Add periodic cleanup:

// Clean up old hints (heights that are far behind current height)
currentHeight, _ := s.store.Height(s.ctx)
for height := range s.p2pHeightHints {
    if height < currentHeight-1000 { // Keep only recent hints
        delete(s.p2pHeightHints, height)
    }
}

---

Architecture & Code Quality

Strengths ✅

1. Good separation of concerns: DA height hint logic is well-isolated
2. Clear intent: The P2P hint mechanism is straightforward
3. Comprehensive testing: Excellent test coverage with multiple scenarios
4. Proper tracing: Updated tracing to pass daHeight parameter

Concerns

1. Missing P2P Hint Validation

Location: syncer.go:619-663

P2P hints are trusted without validation. A malicious peer could provide incorrect DA heights.

Recommendation: Add sanity checks:

// Validate hint is not too far in the past or future
if daHeightHint < s.genesis.DAStartHeight || daHeightHint > s.daRetrieverHeight.Load()+1000 {
    s.logger.Warn().
        Uint64("hint", daHeightHint).
        Uint64("current_da_height", s.daRetrieverHeight.Load()).
        Msg("Ignoring suspicious DA height hint from P2P")
    continue
}

2. Interface Signature Change

Location: block_syncer.go:24

The VerifyForcedInclusionTxs interface now requires daHeight parameter. This is a breaking change.

Impact: Any external implementations of BlockSyncer will break.

Recommendation: Document this breaking change in release notes.

---

Testing & Reliability

Excellent Test Coverage ✅

• Comprehensive unit tests in syncer_forced_inclusion_test.go
• E2E tests covering P2P scenarios
• Tests for malicious sequencer detection
• Dynamic grace period tests

Test Quality Observations

1. Good: Tests cover both success and failure paths
2. Good: Tests validate grace period logic extensively
3. Good: Malicious sequencer E2E test is comprehensive

Minor Test Issue

Location: syncer_test.go:850

The test TestProcessHeightEvent_TriggersAsyncDARetrieval has confusing comments:

// Wait, the logic for DA retrieval is BEFORE the "next block" check?
// Let's check syncer.go...

Recommendation: Clean up or remove the debugging comments.

---

Performance Considerations

1. Extra DA Retrievals

P2P hints trigger targeted DA retrieval, which is good for catching up, but could cause redundant fetches if the DA worker is already processing that height.

Current mitigation: Height check at line 649:

if daHeightHint < s.daRetrieverHeight.Load() {
    continue
}

This is adequate.

2. Map Lookups on Every Block

Every block sync does a map lookup for P2P hints. This is O(1) and acceptable.

---

Security Analysis

Strengths ✅

1. Censorship resistance: P2P blocks are now verified for forced inclusion
2. Malicious sequencer detection: Properly detects and handles malicious proposers
3. Grace period mechanism: Provides tolerance for legitimate block congestion

Vulnerabilities

1. Silent Failure Mode 🔴 (Already discussed above)

The most critical security issue - transient errors allow unverified blocks.

2. P2P Hint Trust 🟡 (Already discussed above)

Untrusted P2P hints could be exploited.

3. No Rate Limiting on Priority DA Fetches

A malicious peer could flood with bogus hints, causing excessive DA requests.

Recommendation: Add rate limiting:

const maxPriorityHeightsPerSecond = 10
// Track and limit priority queue additions

---

Documentation & Maintainability

Strengths ✅

1. Code comments are clear and explain the intent
2. Function signatures are well-documented
3. Test names are descriptive

Improvements Needed

1. Missing Architecture Documentation

The relationship between P2P hints, DA retrieval, and verification is complex but not documented.

Recommendation: Add package-level or README documentation explaining:

• When P2P hints are used vs. state DA height
• How the priority queue interacts with sequential DA fetching
• The complete flow from P2P event to verification

2. Error Handling Strategy Not Documented

The decision to swallow non-malicious errors is not explained in code.

---

Specific Code Issues

1. Inconsistent Error Handling in VerifyForcedInclusionTxs

Location: syncer.go:978-996

forcedIncludedTxsEvent, err := s.fiRetriever.RetrieveForcedIncludedTxs(ctx, daHeight)
if err != nil {
    if errors.Is(err, da.ErrForceInclusionNotConfigured) {
        s.logger.Debug().Msg("forced inclusion namespace not configured, skipping verification")
        return nil
    }
    return fmt.Errorf("failed to retrieve forced included txs from DA: %w", err)
}

Issue: Only one specific error type is silently ignored. Other errors (connection failures, timeouts) are returned. But as discussed, these returned errors are then swallowed by the caller.

Consistency Problem: The multi-layer error handling makes the control flow hard to follow.

2. Potential Nil Dereference

Location: syncer.go:659

s.p2pHeightHints[height] = daHeightHint

While p2pHeightHints is initialized in NewSyncer, it's worth adding a defensive check in case of future refactoring.

---

Comparison with DA-Source Blocks

The PR correctly handles the difference between DA and P2P sources:

• DA events have authoritative DaHeight values (line 791-801)
• P2P events use hints that are validated

However, the comment at line 745 notes this approach may be fundamentally wrong, so this analysis may need revision.

---

Recommendations Summary

Must Fix Before Merge 🔴

1. Clarify the architectural approach - Author indicates current solution is wrong
2. Fix silent error handling - Either return all errors or implement proper retry logic
3. Add thread safety to p2pHeightHints - Prevent race conditions
4. Add P2P hint validation - Don't trust hints from peers blindly

Should Fix 🟡

1. Add cleanup for p2pHeightHints map - Prevent memory leaks
2. Document error handling strategy - Make intent clear
3. Add rate limiting for priority DA fetches - Prevent DoS

Nice to Have 🔵

1. Add architecture documentation - Explain the complex flow
2. Clean up test comments - Remove debugging artifacts
3. Add defensive nil checks - Future-proof against refactoring

---

Conclusion

The implementation shows good engineering practices with comprehensive testing and clear code structure. However, there are critical issues that must be addressed:

1. The author's own comment suggests the fundamental approach may be incorrect
2. Silent error handling creates a security vulnerability
3. Race conditions in the hint map need fixing

Given the author's Feb 3 comment about the approach being wrong, I recommend:

1. Clarify the intended design before further review
2. Address the error handling as it's a security issue regardless of approach
3. Fix the race condition as it can cause crashes

The test coverage is excellent, which will help validate any architectural changes.

---

[gemini-code-assist]

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a mechanism to utilize Data Availability (DA) height hints from P2P events to enhance the verification process for forced inclusion transactions. By storing and applying these specific DA height hints, the system can more accurately and efficiently validate transactions, ensuring that the verification logic aligns with the most relevant DA information available, without introducing performance bottlenecks due to a related retriever improvement.

Highlights

• P2P DA Height Hint Storage: A new map, p2pHeightHints, has been added to the Syncer struct to store Data Availability (DA) height hints associated with specific block heights received via P2P events.
• Dynamic DA Height for Verification: The trySyncNextBlock function now prioritizes using a stored P2P DA height hint for verifying forced inclusion transactions. If a hint exists for the next block height, it is used; otherwise, it falls back to the current state's DA height.
• Refactored Forced Inclusion Verification: The verifyForcedInclusionTxs function has been refactored to directly accept a daHeight parameter, simplifying its interface and allowing for more flexible DA height input during verification.
• Test Updates: All relevant test cases in syncer_forced_inclusion_test.go have been updated to reflect the new signature of the verifyForcedInclusionTxs function, ensuring continued correctness.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[julienrbrt]

Knowing #2891 (comment), this is actually not the right solution for P2P.