feat: replace SharedDomainType with SharedForkLifecycle for fork-aware peer selection

[diegomrsantos]

Issue Addressed

Follows up on PR #820 (fork-aware observed_peer_subnets). While #820 correctly stores per-fork bitmaps, all query methods still aggregate across forks with a blind union(). After the Boole grace period ends, peers only subscribed to Alan topics appear useful for Boole subnets — leading to incorrect peer selection.

Proposed Changes

• Introduce ForkLifecycle enum (Normal, WarmUp, GracePeriod) and SharedForkLifecycle shared state in the fork crate, replacing SharedDomainType
• ForkMonitor updates the shared lifecycle state before broadcasting each ForkPhase event
• Peer selection queries in ConnectionManager now consult the lifecycle: in Normal state only the current fork's bitmap is considered; during WarmUp/GracePeriod both forks are aggregated
• All components that previously read SharedDomainType (Discovery, Handshake) now read SharedForkLifecycle::domain_type() instead
• Network::on_fork_phase simplified — no longer sets domain type (monitor handles it), only updates ENR

Additional Info

• Depends on fix: share domain type across network components to fix handshake after fork activation #814, fix: prevent ANSI escape sequences leaking into file log span fields #815, and fix: make observed_peer_subnets fork-aware #820
• SharedForkLifecycle follows the same Arc<RwLock<T>> single-writer pattern as the SharedDomainType it replaces
• The enum makes invalid states unrepresentable — a single value captures fork identity, domain type, and transition phase
• ENR updates remain in Network::on_fork_phase because they require explicit discv5 mutation
• count_observed_peers_for_subnets hoists the lifecycle read outside the per-peer loop to avoid repeated lock acquisition

[diegomrsantos]

@claude review this PR

[claude-code-actions-sigp]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[shane-moore]

What about collapsing WarmUp and GracePeriod variants into something like

Transitioning {
   current,
   domain_type
}

just an idea since consumers don't seem to care whether they're in WarmUp or GracePeriod, just that current and domain_type are correct. and upcoming/previous fields seem unused

[shane-moore]

when fork_map is empty (newly connected peer, no gossipsub yet), Normal returns None (triggering ENR fallback) but WarmUp/GracePeriod returns Some(all-zeros) (skipping ENR fallback). Adding an early if fork_map.is_empty() { return None } would make the behavior consistent

[shane-moore]

thinking of ways we could simplify this further @dknopik. I think one way would be to convert this enum to a flat struct:

pub struct ForkState {
    pub domain_type: DomainType,
    pub current_fork: Fork,
    pub in_transition: bool,
}

since network components seem to just care about what is the current domain type and whether the network is in a fork transition period (warm up/grace period) vs if normal operation

another idea would just be to extend ForkSchedule with a current field:

  pub struct ForkSchedule {
      configs: BTreeMap<Fork, ForkConfig>,
      network_name: String,
      // Runtime: updated by ForkMonitor, read by
  network components
      current: RwLock<ActiveFork>,
  }

  struct ActiveFork {
      fork: Fork,
      in_transition: bool,
  }

benefit of this is killing need for new ForkState module. con is that it mixes config (current ForkSchedule struct) with runtime characteristics (new current field)

or perhaps you wanted to take a look and come up with something. if so, feel free to lmk if you want me to try out whatever you're thinking

[diegomrsantos]

Good call on simplification. I’d keep runtime state separate from ForkSchedule (it’s config-only today), so I don’t think adding mutable current there is the right direction.
I’m fine simplifying the consumer API with an is_transition() helper, but I’d keep ForkLifecycle as an enum for now so we retain WarmUp vs GracePeriod semantics if/when we need them

[dknopik]

I think we can simplify this - I am just not sure how exactly. Here are a few possibilities:

1. Roll back fix: share domain type across network components to fix handshake after fork activation #814. Considering the original motivation was a bugfix for keeping the current domain type updated, this has grown quite complex - hundreds of lines and a lot of code to test. Instead, I think the original approach is way simpler and at this point less error prone.
2. Kind of this approach, but less complex: I think having a RwLock for the current ForkConfig is simple enough. Any parts of the code interested in the lifecycle can use the existing broadcast channel.
3. Finally, avoid the shared state, but instead pass around the ForkSchedule to parts of the code. Or maybe a new wrapper that brings along a SlotClock for easier querying of the current fork.

Right now, this is the rough order of my preference. Happy to discuss all of these approaches!

[diegomrsantos]

@claude review the current PR

@shane-moore it seems claude review stopped working when mentioned

[claude-code-actions-sigp]

Review: 1 issue found (2 related inline comments)

Subnet service lifecycle transitions don't trigger topic subscribe/unsubscribe events — The old code called handle_subnet_changes after every ForkPhase event and explicitly called send_unsubscribes when removing a fork. The new lifecycle_rx.changed() handler only updates ServiceState via on_lifecycle_transition but never emits the actual gossipsub Join/Leave events. Additionally, set_subscribed_forks silently drops removed forks' currently_subscribed data via retain, losing the information needed to unsubscribe from old topics.

[shane-moore]

@claude review the current PR

@shane-moore it seems claude review stopped working when mentioned

interesting! will look into this

[diegomrsantos]

Test coverage gap in subnet subscription flow

The monitor internals (ForkMonitor::new, run) have solid test coverage — the pre-computed transition model is well verified.

However, the subnet subscription flow and network-side ENR update path have zero test coverage. To be fair, these paths weren't tested before the refactor either. But the refactor changed their behavior (see inline comments on subscriptions.rs:100 about the missing handle_subnet_changes call and the retain data loss), which makes the lack of coverage riskier now — there's no safety net to catch the regressions.

At minimum, a test that sends a lifecycle transition through the watch channel and asserts on the TopicEvents coming out of the mpsc receiver would catch both issues. The setup is straightforward since all inputs (db, lifecycle_rx, slot_clock) are already injectable.

[dknopik]

LGTM, thanks!

[mergify]

Merge Queue Status

Rule: default

---

• ✅ Entered queue — 2026-02-27 16:18 UTC
• ✅ Checks passed · on draft merge queue: embarking unstable (0a463f7) and #824 together #852
• ✅ Merged — 2026-02-27 16:29 UTC · at b16350f57db312eaa63cda2759ec8f58652398ef

This pull request spent 11 minutes 17 seconds in the queue, including 9 minutes 28 seconds running CI.

Required conditions to merge

• #approved-reviews-by >= 1 [🛡 GitHub repository ruleset rule unstable and release-v*]
  • feat: replace SharedDomainType with SharedForkLifecycle for fork-aware peer selection #824
• #changes-requested-reviews-by = 0 [🛡 GitHub repository ruleset rule unstable and release-v*]
  • feat: replace SharedDomainType with SharedForkLifecycle for fork-aware peer selection #824
• 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