refactor(network): declarative P2P capability advertisement via IP2PCapabilityResolver

[asdacap]

Changes

Replaces the scattered imperative IProtocolsManager.Add/RemoveSupportedCapability calls with a declarative IP2PCapabilityResolver (void Resolve(ISet<Capability>) + event Action? Changed).

• ProtocolsManager runs all registered resolvers over an empty set and caches a frozen Capability[], invalidating only when a resolver raises Changed. The advertised set changes ~twice per node lifetime (merge transition, snap-sync completion) while Register runs ~100×/session-create, so the per-session path is a single cached-array read — zero allocation.
• Resolvers:
  • DefaultP2PCapabilityResolver — eth/68 base, registered AddFirst so it seeds before chain resolvers.
  • MergeP2PCapabilityResolver — eth/69–71 on IPoSSwitcher.TransitionFinished; registered in MergePluginModule and AuRaMergeModule only (not BaseMergePluginModule, so Optimism/Taiko are unchanged).
  • SnapP2PCapabilityResolver — snap/1 while serving or still snap-syncing; fully replaces the former SnapCapabilitySwitcher (recomputed per session instead of add-on-start / remove-on-Full).
  • XdcP2PCapabilityResolver — removes eth/68, adds 62/63/100.
• P2PProtocolInfoProvider/EthStats derive the default advertised set by running DefaultP2PCapabilityResolver rather than reading a static list.

Merged latest master (incl. #12069 decoupling InitializeNetwork from INethermindApi); the conflicting SnapCapabilitySwitcher wiring is dropped in favour of SnapP2PCapabilityResolver, which preserves both the snap-serving and snap-sync-until-synced behaviours.

Types of changes

• Refactoring

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

New unit tests for each resolver and for ProtocolsManager capability caching/invalidation; E2ESyncTests exercises post-merge capability negotiation. Full solution build is green (0 warnings, 0 errors).

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

[claude]

Claude finished @asdacap's task in 5m 56s —— View job

---

PR Review: declarative P2P capability advertisement via IP2PCapabilityResolver

• Gather context and understand the request
• Read relevant rules files (coding-style, di-patterns, robustness)
• Read changed source files
• Analyze for correctness, edge cases, regressions, security, and performance
• Post inline comments for specific findings
• Final summary

---

Summary

Clean refactor overall. The declarative resolver model is a clear improvement over the previous imperative Add/RemoveSupportedCapability pattern, and the double-checked locking in GetAdvertisedCapabilities() is correctly implemented. The DI wiring (AddFirst/AddLast ordering, per-plugin module registration) is correct. One medium issue needs addressing before merge; the rest are low-priority but worth fixing.

---

Findings

Medium

SnapP2PCapabilityResolver fires Changed on every sync-mode transition (inline comment)

OnSyncModeChanged unconditionally fires Changed on every ISyncModeSelector.Changed event. During initial sync, SyncMode transitions frequently (Headers → Bodies → Receipts → StateNodes etc.). Each one invalidates the cache and triggers a full rebuild, even when the snap capability flag would be identical before and after the transition. The PR description says capabilities "change ~twice per node lifetime" but the snap resolver alone fires on every step of sync.

The old SnapCapabilitySwitcher only acted on the Full mode bit — the one transition that actually changes snap's contribution. A simple fix:

private void OnSyncModeChanged(object? sender, SyncModeChangedEventArgs e)
{
    bool wasSnap = _syncConfig.SnapSync && (e.Previous & SyncMode.Full) == 0;
    bool isSnap  = _syncConfig.SnapSync && (e.Current  & SyncMode.Full) == 0;
    if (wasSnap != isSnap)
        Changed?.Invoke();
}

Low

Nullability gap in GetAdvertisedCapabilities() (inline comment)

_cachedCapabilities is Capability[]? but the method returns Capability[]. The lock semantics guarantee non-null at the return site, but the annotation gap means a compiler warning is suppressed or missed. Restructure to return resolved; inside the if-null branch, or use return _cachedCapabilities!;.

Inconsistent null guarding for array constructor parameters (inline comment)

factories and capabilityResolvers are not null-checked in the ProtocolsManager constructor, unlike all other parameters. Not a practical issue (Autofac injects [], not null), but the inconsistency is worth aligning.

Observability regression: snap capability transitions are no longer logged (inline comment)

SnapCapabilitySwitcher logged at info/debug when snap was enabled and when sync completed and snap was disabled. The new SnapP2PCapabilityResolver is silent. Consider adding a debug log in OnSyncModeChanged or inside Resolve() when the snap state transitions.

---

Correctness / design notes (no action required)

• Thread safety: The Volatile.Read + double-checked lock pattern in GetAdvertisedCapabilities() is correctly implemented. InvalidateCapabilities() also uses the same lock, so there is no race between invalidation and rebuild.
• DefaultP2PCapabilityResolver.Changed empty accessors ({ add { } remove { } }) correctly discards subscriptions for a static resolver. ProtocolsManager subscriptesto it harmlessly.
• XdcP2PCapabilityResolver: Relies on DefaultP2PCapabilityResolver running first (registered AddFirst) to seed eth/68 before removing it. This is guaranteed by the AddFirst/AddLast ordering. Correct.
• MergeP2PCapabilityResolver fires Changed on TerminalBlockReached, which is raised exactly when TransitionFinished becomes true. The sequence is correct.
• SnapP2PCapabilityResolver.Dispose() properly unsubscribes from _syncModeSelector.Changed; Autofac will call Dispose() on all IDisposable singletons at container shutdown.
• E2E test migration: PostMergeCapabilitiesResolver replaces the previous EnablePostMergeEthCapabilities helper cleanly. The lambda factory form (_ => new PostMergeCapabilitiesResolver()) is acceptable since the type has no constructor dependencies.
• Module scoping (Optimism/Taiko via BaseMergePluginModule not getting MergeP2PCapabilityResolver) is intentional and verified.

[claude]

Claude finished @asdacap's task in 7m 1s —— View job

---

PR Review: declarative P2P capability advertisement via IP2PCapabilityResolver

• Gather context and understand the request
• Load tool schemas
• Read relevant rules files (coding-style, di-patterns, robustness)
• Read changed source files
• Analyze for correctness, edge cases, regressions, security, and performance
• Post inline comments for specific findings
• Final summary

---

Summary

The refactor is clean and well-reasoned. The declarative IP2PCapabilityResolver pattern is a meaningful improvement over scattered imperative Add/RemoveSupportedCapability calls, and the DCL caching in ProtocolsManager.GetAdvertisedCapabilities() is correctly implemented (volatile read for fast path, lock + volatile write for rebuild, InvalidateCapabilities takes the same lock so no TOCTOU window).

Correctness verified:

• Double-checked locking in GetAdvertisedCapabilities() is safe: Volatile.Read for the fast path, the lock guards both invalidation and rebuild, and Volatile.Write ensures visibility to subsequent readers.
• MergeP2PCapabilityResolver correctly handles the post-merge-at-startup case: when the node restarts after the merge, TransitionFinished is already true and the first lazy cache build includes eth/69–71 without needing Changed to fire.
• SnapP2PCapabilityResolver semantically preserves the old SnapCapabilitySwitcher behavior: snap is advertised while serving or while snap-syncing; new sessions after Full sync simply don't include it.
• XdcP2PCapabilityResolver.Remove(eth/68) is safe even if the default resolver hasn't run (no-op on missing element).
• IDisposable resolvers (Snap, Merge) are registered as singletons; Autofac disposes them when the container scope ends.
• Resolver order (AddFirst for default, AddLast for overrides) is respected by the ordered-component DSL; XdcP2PCapabilityResolver correctly runs after DefaultP2PCapabilityResolver.
• MergeP2PCapabilityResolver is registered only in MergePluginModule and AuRaMergeModule (not BaseMergePluginModule), so Optimism/Taiko are correctly excluded.

No critical or high findings. Three low-severity issues noted as inline comments:

Severity	Location	Issue
Low	ProtocolsManager.cs:394	Missing ! null-forgiving on return _cachedCapabilities (project is Nullable=annotations so no warning, but ! documents the invariant)
Low	E2ESyncTests.cs:310	Lambda factory form (.AddLast<IP2PCapabilityResolver>(_ => new ...)) is the DI anti-pattern; prefer type-based .AddLast<IP2PCapabilityResolver, PostMergeCapabilitiesResolver>()
Low	SnapP2PCapabilityResolver.cs:47	Changed fires on every sync-mode transition, not only when snap contribution actually flips — unnecessary cache rebuilds (cheap and rare, so harmless; see inline for precise guard)

Additional note (not in diff, no inline possible): The comment at InitializeNetwork.cs:263–265 says "Capabilities must be finalized before the RLPx listener accepts peers" — the word "finalized" made sense for the old imperative model. Now the real invariant is "all IP2PCapabilityResolver singletons must be instantiated before sessions start". Worth a small rewording in a follow-up.

Overall, the implementation is solid. Tests are comprehensive (unit tests per resolver + caching/invalidation test in ProtocolsManagerTests + E2ESyncTests for post-merge negotiation).

[claude]

Medium – Fires Changed on every sync-mode transition, not just snap-relevant ones

private void OnSyncModeChanged(object? sender, SyncModeChangedEventArgs e) => Changed?.Invoke();

This unconditionally fires Changed on every ISyncModeSelector.Changed event. During initial sync, SyncMode transitions frequently (e.g. Headers → Bodies → Receipts → StateNodes). Each transition invalidates ProtocolsManager's cache and triggers a full rebuild, even when neither SnapServingEnabled nor the Full-mode check would yield a different result.

The PR description states capabilities "change ~twice per node lifetime," but this resolver alone fires during every sync-mode step. SnapCapabilitySwitcher only triggered on Full — the single moment snap should be removed.

Consider restricting invalidation to the transitions that actually change the snap capability:

private void OnSyncModeChanged(object? sender, SyncModeChangedEventArgs e)
{
    bool wasSnap = _syncConfig.SnapSync && (e.Previous & SyncMode.Full) == 0;
    bool isSnap  = _syncConfig.SnapSync && (e.Current  & SyncMode.Full) == 0;
    if (wasSnap != isSnap)
        Changed?.Invoke();
}

[claude]

Low – Nullability gap: returning Capability[]? as Capability[]

_cachedCapabilities is declared Capability[]?, but this method returns Capability[]. The return here is logically non-null (the lock prevents InvalidateCapabilities from racing), but the compiler doesn't see that flow and should emit CS8603. If the project has <Nullable>enable</Nullable>, this is a suppressed or missed warning.

Either restructure to use a local:

lock (_capabilitiesLock)
{
    if (_cachedCapabilities is null)
    {
        ...
        Volatile.Write(ref _cachedCapabilities, resolved);
        return resolved;
    }

    return _cachedCapabilities;
}

Or suppress explicitly with return _cachedCapabilities!;.

[claude]

Low – Inconsistent null guarding for array parameters

factories and capabilityResolvers are not null-checked, unlike the other constructor parameters. If either is null, the foreach on line 70 throws NullReferenceException rather than the ArgumentNullException that other parameters produce.

Since Autofac injects [] for empty array registrations (not null), this is not a practical issue, but the inconsistency is worth aligning:

ArgumentNullException.ThrowIfNull(factories);
ArgumentNullException.ThrowIfNull(capabilityResolvers);

[claude]

Low – Observability regression: snap transition is no longer logged

SnapCapabilitySwitcher logged at info/debug when snap was enabled and when sync completed and snap was disabled:

"Enabled snap capability"
"State sync finished. Disabled snap capability."

The new resolver is silent. Adding a debug log in Resolve() (or a conditional log in OnSyncModeChanged) when the snap state transitions would preserve that visibility for node operators and devs debugging sync issues.

[claude]

Nit – SnapServingEnabled null-check is reversed from intent

_syncConfig.SnapServingEnabled == true is correct (handles null safely), but it is slightly inconsistent with how SnapSync is checked on the next line (_syncConfig.SnapSync — a plain bool). Fine as-is; just noting the asymmetry for reviewers.

[claude]

Low: _cachedCapabilities is declared Capability[]? and the project uses <Nullable>annotations</Nullable> so no warning fires, but the null-forgiving operator would document the invariant that this field cannot be null here (it was either already non-null when we entered the lock, or we just wrote it via Volatile.Write).

Suggested change

	return _cachedCapabilities;
	return _cachedCapabilities!;

[claude]

Low: Per the DI rules in .agents/rules/di-patterns.md, the lambda factory form is an anti-pattern. Since PostMergeCapabilitiesResolver has no dependencies, prefer the type-based overload — even for a private nested type:

Suggested change

	.AddLast<IP2PCapabilityResolver>(_ => new PostMergeCapabilitiesResolver())
	.AddLast<IP2PCapabilityResolver, PostMergeCapabilitiesResolver>()

[claude]

Low: Changed fires on every sync-mode transition, not only when the snap contribution actually flips. If the mode goes from StateNodes → FastBlocks both sides are "not Full", so the cache is rebuilt unnecessarily. In practice, mode changes are rare and rebuilds are cheap (one HashSet allocation per resolver), so this is harmless. But if you want to be precise:

private void OnSyncModeChanged(object? sender, SyncModeChangedEventArgs e)
{
    bool wasFull = (e.Previous & SyncMode.Full) != 0;
    bool isFull  = (e.Current  & SyncMode.Full) != 0;
    if (wasFull != isFull)
        Changed?.Invoke();
}

[LukaszRozmej]

Can this be done by deregistering DefaultCapabilityResolver?

[asdacap]

I guess, the Add* method can be modified to remove previous registration.

[asdacap]

Done in 6c28605 — added a RemoveOrderedComponents<T, TImpl>() DSL (next to ClearOrderedComponents, backed by OrderedComponents.RemoveAll), and XdcModule now calls .RemoveOrderedComponents<IP2PCapabilityResolver, DefaultP2PCapabilityResolver>() right after registering the XDC resolver. So the default eth/68 resolver is deregistered instead of XdcP2PCapabilityResolver removing eth/68 at resolve time — the XDC resolver now only contributes eth/62, eth/63 and eth/100. Covered by OrderedComponentsTests.TestRemove and the updated Xdc resolver test.

Posted by Claude (Opus 4.8) on behalf of @asdacap.

[AnkushinDaniil]

Only tests the resolver in isolation - nothing asserts the DI composition actually drops eth/68 from XDC's set. Worth a container-level check?

[asdacap]

Added in 37dbf86 — a container-level test (Di_composition_drops_default_eth68_resolver) that mirrors NetworkModule (AddFirst default) + XdcModule (AddLast xdc, then RemoveOrderedComponents default), resolves IP2PCapabilityResolver[], and asserts the composed capability set excludes eth/68 (and that no DefaultP2PCapabilityResolver remains in the array) — not just the resolver in isolation.

Posted by Claude (Opus 4.8) on behalf of @asdacap.

[AnkushinDaniil]

Gates on TransitionFinished but only subscribes to TerminalBlockReached, which fires while TransitionFinished is still false. it flips true later in ForkchoiceUpdated, no event. On a live TTD merge, eth/69-71 never get advertised. Should this also check _poSSwitcher.HasEverReachedTerminalBlock()?

[asdacap]

Good catch — confirmed regression for the live TTD path. The pre-resolver code added eth/69-71 unconditionally when TerminalBlockReached fired; my resolver re-gated Resolve on TransitionFinished, which is still false at that point (it only flips true later in PoSSwitcher.ForkchoiceUpdated, which raises no event), so the cache rebuild dropped them and they were never advertised until a restart.

Fixed in 5ab5f31: Resolve now gates on TransitionFinished || HasEverReachedTerminalBlock(). HasEverReachedTerminalBlock() is set right after TerminalBlockReached fires, and ProtocolsManager's cache invalidation is lazy (nulls the cache, rebuilds on next read), so the rebuild sees it true. Added a parameterized case for the live-merge sequence (terminal reached before transition finished).

Posted by Claude (Opus 4.8) on behalf of @asdacap.