fix(xdc): relay timeouts even when the local epoch lookup fails

[ak88]

Changes

• Move BroadcastTimeout above the epochSwitchInfo is null check in TimeoutCertificateManager.HandleTimeoutVote, so relaying a current-round timeout no longer depends on this node being able to resolve its own epoch.
• Drop the stale //TODO: Broadcast Timeout in OnReceiveTimeout (see below — relaying received timeouts already happens, and extending it to future rounds is deliberately not done).
• Add regression tests for the relay/pool behaviour by round, and for the epoch-lookup-unavailable case.

Why

_timeouts.Add sits above the epoch lookup but the broadcast sat below it, so a node that cannot resolve its epoch keeps accumulating timeouts locally while going silent towards its peers. Those peers are at the same round and need exactly those timeouts to reach masternodes × CertificateThreshold and build a TC.

That condition is not always transient: GetEpochSwitchInfo returns null when the gap-block snapshot is missing, and TryRecoverSnapshot requires the gap block to be both processed and to have state available, so a freshly synced or state-pruned node can remain in it for a whole epoch. Subnets feel it more sharply — a 3–5 validator set needs 2-of-3 or 3-of-4 for a TC, so one silent node is 20–33% of the quorum rather than statistical noise.

Relay stays restricted to the current round, so the set of messages this node will forward is unchanged. Only the dependency on local epoch resolution is removed.

Why not also relay future-round timeouts

An earlier revision of this PR hoisted the relay all the way into OnReceiveTimeout, which additionally forwarded timeouts for rounds this node has not reached. That has been dropped.

Upstream XDPoSChain does relay those — eth/bft/bft_handler.go does verify → broadcastCh <- timeout → timeoutHandler, and VerifyTimeoutMessage bounds only round < currentRound, with no forward cap — so the wider version was at parity with the Go client. But upstream also drops duplicates at a node-global knownTimeouts LRU before verification (eth/handler.go:869), and this client has no equivalent: Handle(TimeoutMsg) goes straight to OnReceiveTimeout, so every arrival costs a fresh ecrecover in FilterTimeout. Copying upstream's relay breadth without upstream's dedup imports the cost and not the mitigation.

Concretely: FilterTimeout rejects only round < CurrentRound, so any key in snapshot.NextEpochCandidates can sign Timeout(CurrentRound + K, validGap) for arbitrary K. Each K is a distinct Timeout.Hash, so the per-peer _notifiedTimeouts cache dedups repeats but not a varying stream — one inbound message becomes N outbound at every hop, and no node at a lower round can pool those rounds, so there is no liveness gain to offset it. The benefit only exists for peers a few rounds ahead, which the originator's own all-peers broadcast, the per-TimeoutPeriod re-emission, and SyncInfo already cover.

Suggested follow-ups, in order: add a node-global receive-side dedup for votes and timeouts matching upstream's knownTimeouts (a win independent of any of this), then revisit forward-round relay behind a forward bound mirroring VotesManager._maxRoundDistance.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

OnReceiveTimeout_EpochSwitchInfoUnavailable_StillRelaysTimeout covers the fix. OnReceiveTimeout_DifferentRounds_RelaysAndPoolsAsExpected is a guardrail pinning the stale / current / future relay-and-pool matrix, including that a future-round timeout is not relayed — so a future change in that direction has to be deliberate.

Verified the fix is a genuine regression: with TimeoutCertificateManager.cs reset to the branch base, exactly one case fails — OnReceiveTimeout_EpochSwitchInfoUnavailable_StillRelaysTimeout ("Expected to receive exactly 1 call… Actually received no matching calls"). The three round cases pass on the base too, as guardrails should.

Full Nethermind.Xdc.Test suite: 556 passed, 0 failed. dotnet format whitespace --verify-no-changes clean.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[claude]

Claude finished @ak88's task in 5m 38s —— View job

---

Review: relay timeouts for rounds the node has not reached yet

• Read PR diff
• Read TimeoutCertificateManager.cs in full
• Trace call sites (XdcProtocolHandler, SendTimeout, VotesManager.OnReceiveVote)
• Verify the two epoch-info throw paths
• Check gossip dedup / amplification behaviour
• Review tests and README
• Post findings (4 inline comments)

---

The core change is right. Relay belonged in OnReceiveTimeout after FilterTimeout and before the round gate — that mirrors VotesManager.OnReceiveVote:287-291 exactly, the timeout is fully verified (signature, low-S, candidate-set membership) before it is forwarded, and moving the own-vote broadcast into SendTimeout keeps originate and relay separate without double-sending. I also confirmed there is no gossip loop: XdcProtocolHandler.ShouldNotifyTimeout dedups per peer on Timeout.Hash, which is the RLP hash including the signature, so distinct signers for the same round still propagate. Pooling before the epoch lookup is the right order too — nothing is lost when the lookup fails.

Two things should be addressed before merge.

High

Relay is unbounded in the forward round direction — TimeoutCertificateManager.cs:266-271

OnReceiveTimeout bounds only the gap (gapDiff > 3 * epochLength); FilterTimeout rejects only round < CurrentRound. Nothing caps how far ahead timeout.Round may be — the existing test case [TestCase(500UL, 0UL, true, true)] // Far away round but should get filtered in makes that explicit.

That was harmless before: a far-future timeout was verified and then dropped at HandleTimeoutVote's round gate, with no network effect. Now each one is fanned out to every peer. The vote path this PR models itself on does bound it, in both directions (VotesManager.cs:47-48, 279-285, _maxRoundDistance = 7), and discards before BroadcastVote.

So any key in snapshot.NextEpochCandidates can sign Timeout(CurrentRound + K, validGap) with an ever-varying K; the per-peer _notifiedTimeouts cache dedups repeats of the same message but not a stream of distinct ones, so one inbound message becomes N outbound at every hop. No node can pool those rounds either, so the relay carries no liveness benefit to offset it. A forward bound mirroring the vote path preserves the entire fix — a peer a few rounds behind still gets the timeout.

Medium

The exception allowlist does not achieve the invariant its comment states — TimeoutCertificateManager.cs:79-91

I confirmed both cited throw sites are real (BaseEpochSwitchManager.cs:48 and SubnetEpochSwitchManager.cs:38), so the diagnosis is correct. But "neither may escape the P2P message loop" isn't reached, because the same path still throws elsewhere:

• OnReceiveTimeout:251 — _blockTree.Head ?? throw new InvalidOperationException(...), the same exception type, thrown before this catch is reachable;
• GetEpochSwitchInfo dereferences header.Hash on its first line, so a head header that isn't an XdcBlockHeader gives an NullReferenceException, which isn't in the allowlist;
• _specProvider.GetXdcSpec(xdcHeader, …) at line 99, same null header.

A single guard at the boundary (XdcProtocolHandler.Handle(TimeoutMsg), or the top of OnReceiveTimeout) covers all of these, is a smaller diff, and doesn't have to be revisited when IEpochSwitchManager grows another throw. Also note ArgumentException swallows ArgumentNullException/ArgumentOutOfRangeException, downgrading genuine programming errors to a Warn.

Low

• Warn-per-message on a persistent condition (line 89) — the PR's own Remarks say this state lasts a whole epoch on a freshly-synced subnet node, and the line fires once per received timeout. Consider Debug or a per-round throttle, and {e} rather than {e.Message} (ILogger.Warn has no exception overload, and the stack is where the diagnostic value is for the header-gap case).
• Test duplication and the relay assertion (test line 288) — BuildManagerForReceivedTimeouts re-creates setup already in two existing tests; Received(0) reads better as DidNotReceive(); the CurrentRound/Gap consts sit between methods rather than at the top of the type.

The README sentence is accurate and the diagram above it stays consistent with the new flow. Test design is otherwise good — the parameterized stale/current/future table and the two-shape TestCaseSource map cleanly onto the behaviour changed.

Not verified: I could not compile or run the suite — dotnet build needs approval in this environment, so I'm taking the PR's stated 557-passed result at face value.
· branch worktree-xdc-timeout-relay

[claude]

High — relay is now unbounded in the forward round direction.

OnReceiveTimeout bounds only the gap distance (gapDiff > 3 * epochLength), and FilterTimeout rejects only timeout.Round < CurrentRound. There is no upper bound on timeout.Round, and the existing test at line 245 of the test file ([TestCase(500UL, 0UL, true, true)] // Far away round but should get filtered in) confirms that's deliberate for the filter.

Before this change that didn't matter: a far-future timeout was verified and then dropped at HandleTimeoutVote's round gate, with no network effect. Now every such message is fanned out to every peer.

The vote path — cited in the PR description as the model — does bound this, in both directions (VotesManager.cs:47-48, 279-285):

private const int _maxRoundDistance = 7; // Maximum allowed distance from the current round
...
if (blockDiff > _maxBlockDistance ||
    Math.Abs((long)vote.ProposedBlockInfo.Round - (long)_ctx.CurrentRound) > _maxRoundDistance)
    return Task.CompletedTask; // discarded, before BroadcastVote

Consequence: any key in snapshot.NextEpochCandidates can sign Timeout(CurrentRound + K, validGap) for arbitrary, ever-varying K. Each distinct message is relayed once to every peer by every node that receives it (ShouldNotifyTimeout's _notifiedTimeouts cache dedups repeats of the same message, but not a stream of distinct ones), so one inbound message becomes N outbound, network-wide. No node can ever pool those rounds either, so the relay is pure cost with no liveness benefit.

Bounding the forward distance the same way the vote path does keeps the entire benefit of this fix — a peer one or a few rounds behind still gets the timeout — while removing the amplification surface:

if (gapDiff > 3 * epochLength ||
    timeout.Round > _consensusContext.CurrentRound + MaxRoundDistance)
{
    // Discarded propagated timeout, too far away
    return Task.CompletedTask;
}

(Stale rounds are already dropped by FilterTimeout, so only the forward bound is needed here.)

[claude]

Medium — the exception allowlist does not achieve the invariant it states.

The comment says "Timeouts are handled on the P2P message loop, so neither may escape", but on that same path several throws remain unguarded, including one of the very same type:

• OnReceiveTimeout line 251: _blockTree.Head ?? throw new InvalidOperationException("Failed to get current block") — thrown synchronously into XdcProtocolHandler.Handle(TimeoutMsg) before this catch is ever reached.
• GetEpochSwitchInfo dereferences header.Hash on its first line (BaseEpochSwitchManager.cs:32). xdcHeader here is _blockTree.Head?.Header as XdcBlockHeader, which is null whenever the head header isn't an XdcBlockHeader — that's a NullReferenceException, not in the allowlist.
• _specProvider.GetXdcSpec(xdcHeader, ...) on line 99 with the same possibly-null header.

Enumerating exception types at one call site is brittle: it also has to be revisited every time IEpochSwitchManager grows a new throw (SubnetEpochSwitchManager.GetEpochSwitchInfoBetween already throws NotImplementedException, SendTimeout throws DataExtractionException). A single guard at the boundary — Handle(TimeoutMsg)/Handle(VoteMsg) in XdcProtocolHandler, or the top of OnReceiveTimeout — covers all of them, is a smaller diff, and doesn't need the list kept in sync. If you prefer to keep the catch here, please at least drop the null-header hole and the line-251 throw, otherwise the P2P loop is still reachable by an exception.

Separately: ArgumentException also catches ArgumentNullException / ArgumentOutOfRangeException, so a genuine programming error downstream is now downgraded to a Warn line.

[claude]

Low — Warn-level log on a condition the PR itself describes as persistent.

Per the PR's own Remarks, a missing gap-block snapshot makes this path fail "persistently for a whole epoch", and this line runs once per received timeout. On a freshly-synced subnet node that is a Warn per inbound timeout message from every peer for EpochLength - Gap blocks — enough to bury anything else in the log. Consider IsDebug/Debug here, or throttling to once per round (the _tcBuildStartedByRound-style pattern already in this class).

Also, {e.Message} discards the stack trace, which for the header-store-gap case is where the diagnostic value is. ILogger.Warn has no exception overload, so {e} is the way to keep it.

[claude]

Low (test quality). Two small things:

1. syncPeerPool.Received(0).AllPeers reads better as DidNotReceive(), but more importantly the assertion is a proxy: it verifies BroadcastTimeout was entered, not that a timeout was handed to a peer. That's a reasonable compromise given XdcProtocolHandler is an internal class that can't be substituted, but the test will silently start passing/failing if any other AllPeers access enters this code path (e.g. the SendSyncInfo loop in OnCountdownTimer). A one-line comment saying that's the intent would save the next reader the trip.
2. BuildManagerForReceivedTimeouts largely re-creates the setup already present in HandleTimeoutVote_ThresholdReached_RetainsRecentTimeouts (lines 200-228) and FilterTimeout_DifferentCases_ReturnsExpected (lines 251-269), and sits alongside the existing BuildTimeoutCertificateManager helper. AGENTS.md asks for shared setup to be factored rather than copied — folding those call sites onto the new helper (or giving the existing one optional substitutes) would shrink the file instead of growing it.

Nit: the CurrentRound/Gap consts are declared between test methods; convention in this file (and .editorconfig) puts fields at the top of the type.