fix: make observed_peer_subnets fork-aware

[diegomrsantos]

Issue Addressed

Closes #818

Proposed Changes

During fork transitions (e.g., Alan → Boole), peers may be subscribed to the same subnet via topics from different forks simultaneously — for example, ssv.v2.42 (Alan) and /ssv/mainnet/boole/42 (Boole). The peer manager's observed_peer_subnets previously stored a single flat bitmap per peer, so unsubscribing from one fork's topic would incorrectly clear the subnet bit even though the peer was still subscribed via the other fork.

This PR makes the subscription tracking fork-aware by maintaining a separate bitmap per fork per peer. Query methods (used for peer counting, disconnect decisions, and priority dialing) aggregate across forks transparently, so the rest of the peer management logic is unaffected.

Additional Info

• The gossipsub subscription/unsubscription event handlers now use topic::parse_topic() (which returns fork info) instead of topic::parse_subnet_id() (which discarded it), threading the fork identity through to the connection manager.
• This also lays groundwork for future fork-aware peer selection, e.g., preferring peers that are on the current fork's topics after a grace period ends.

[claude-code-actions-sigp]

Claude finished @diegomrsantos's task in 2m 6s —— View job

---

Code Review Complete ✅

Summary

This PR successfully addresses issue #818 by making peer subnet tracking fork-aware. The implementation correctly handles fork transitions where peers may be subscribed to the same subnet via different fork-specific topics simultaneously (e.g., ssv.v2.42 and /ssv/mainnet/boole/42). The nested HashMap<PeerId, HashMap<Fork, Bitfield>> structure properly models this scenario.

---

Strengths

1. Correct Problem Modeling

The core data structure change from HashMap<PeerId, Bitfield> to HashMap<PeerId, HashMap<Fork, Bitfield>> correctly addresses the root cause: during fork transitions, a peer subscribed to both ssv.v2.42 (Alan) and /ssv/mainnet/boole/42 (Boole) should not have subnet 42 cleared when unsubscribing from one fork.

2. Transparent Aggregation

The aggregate_fork_bitmaps() helper (connection.rs:298-306) uses bitwise union to merge per-fork bitmaps, making query methods fork-agnostic. This is a clean separation - the rest of the peer management logic doesn't need to change.

fn aggregate_fork_bitmaps(fork_map: &HashMap<Fork, Bitfield<Fixed<U128>>>) -> Bitfield<Fixed<U128>> {
    let mut result = Bitfield::default();
    for bitfield in fork_map.values() {
        result = result.union(bitfield);
    }
    result
}

3. Proper Memory Management

The cleanup logic (connection.rs:150-157) prevents memory bloat by removing empty entries:

if !subscribed {
    if bitfield.is_zero() {
        fork_map.remove(&fork);
    }
    if fork_map.is_empty() {
        self.observed_peer_subnets.remove(&peer);
    }
}

4. Excellent Test Coverage

The test suite comprehensively validates the fix:

• Single-fork subscribe/unsubscribe (baseline behavior)
• Multi-fork retention test (lines 607-624) - regression test for Make observed_peer_subnets Fork aware #818
• Aggregation across forks
• Cleanup behavior verification
• Peer counting with multi-fork scenarios
• Idempotency test (lines 829-845)

All tests follow AAA pattern with descriptive names.

5. Out-of-Bounds Protection

Commit 19f675c adds proper bounds checking with a warning (connection.rs:142-147):

if idx < bitfield.len() {
    let _ = bitfield.set(idx, subscribed);
} else {
    tracing::warn!(
        subnet = idx,
        max = bitfield.len(),
        "Subnet ID exceeds bitfield capacity"
    );
}

This prevents silent failures when invalid subnet IDs are passed.

---

Areas for Improvement

1. Performance Consideration (Minor)

The aggregate_fork_bitmaps() function creates a new bitfield and unions all per-fork bitmaps on every query. During steady-state with multiple forks active, this could be called frequently from:

• count_observed_peers_for_subnets() (line 220)
• peer_offers_needed_subnets_observed_only() (line 245)
• qualifies_for_priority_connection() (line 191)

Observation: This is likely not a bottleneck given:

• Small number of forks (typically 2-3 during transitions)
• Bitwise OR is fast (128-bit bitmap)
• Queries happen during connection decisions, not hot path

Recommendation: Monitor in production. If profiling shows this as hot, consider caching the aggregated result and invalidating on subscription changes.

Verdict: No action needed now.

2. Type Safety Consideration (Very Minor)

The Fork enum is a closed set (Alan, Boole), so invalid values aren't possible in safe Rust. The implementation correctly relies on this.

---

Concurrency & Thread Safety

The ConnectionManager follows standard Rust ownership semantics. All mutable access goes through &mut self, ensuring no data races. The nested HashMap structure is safe for concurrent reads (when not mutating) via standard borrowing rules.

Verdict: Thread-safe ✅

---

Security Assessment

No security vulnerabilities identified:

• No sensitive data exposure
• Proper bounds validation (lines 139-147)
• Safe Rust only (no unsafe blocks)
• Memory cleanup prevents leaks

Verdict: Secure ✅

---

Documentation Quality

• Clear comments explaining fork-aware tracking (connection.rs:50-53)
• Issue reference (Make observed_peer_subnets Fork aware #818) in code comments
• Comprehensive PR description linking fork transition scenario
• Well-documented test cases

Verdict: Documentation excellent ✅

---

Edge Cases Covered

1. ✅ Subscribe/unsubscribe same fork+subnet (idempotency test, lines 829-845)
2. ✅ Multi-fork retention (regression test for Make observed_peer_subnets Fork aware #818, lines 607-624)
3. ✅ Full unsubscribe cleanup (lines 678-695)
4. ✅ Partial unsubscribe retention (lines 698-713)
5. ✅ Disconnected peers not counted (lines 747-767)
6. ✅ Out-of-bounds subnet IDs (warning added in commit 19f675c)

---

Code Quality

• Readability: Helper functions with clear names
• Modularity: Clean separation between tracking and aggregation
• Error Handling: Proper bounds checking with logging
• Testing: Comprehensive edge case coverage
• Consistency: Follows existing Anchor patterns

---

Final Verdict

✅ Approve

This PR correctly solves the fork transition bug with a clean, well-tested implementation. The solution is minimal, focused, and production-ready. The minor performance observation is not a blocker - the implementation is already optimized for the common case (small number of forks, fast bitwise operations).

The addition of the out-of-bounds warning in the second commit demonstrates attention to robustness. The comprehensive test suite ensures the fix works correctly and won't regress.

---

Completed Tasks

• Read all changed files
• Analyze fork-aware bitmap implementation
• Review security and thread safety
• Check error handling and edge cases
• Assess test coverage
• Verify documentation quality
• Provide comprehensive feedback

[dknopik]

LGTM, thanks!

[mergify]

Merge Queue Status

Rule: default

---

• ✅ Entered queue — 2026-02-11 15:57 UTC
• ✅ Checks passed · on draft merge queue: embarking unstable (5bad119) and #820 together #825
• ✅ Merged — 2026-02-11 16:07 UTC · at c79569c347b37506e5f81eccb039fde1b39dc698

This pull request spent 10 minutes 32 seconds in the queue, including 8 minutes 56 seconds running CI.

Required conditions to merge

• #approved-reviews-by >= 1 [🛡 GitHub repository ruleset rule unstable and release-v*]
  • fix: make observed_peer_subnets fork-aware #820
• #changes-requested-reviews-by = 0 [🛡 GitHub repository ruleset rule unstable and release-v*]
  • fix: make observed_peer_subnets fork-aware #820
• check-success=test-suite-success
• any of [🛡 GitHub repository ruleset rule unstable and release-v*]:
  • check-success = license/cla
  • check-neutral = license/cla
  • check-skipped = license/cla