Hedge slow requests in SimpleSidecarRetriever

[zilm13]

PR Description

Adds following logic to SimpleSidecarRetriever:

Adds redundant attempts for requests that have been in-flight for too long, re-dispatching them to an alternate custody peer. Bounded by the overlap budget so at most
overlapFraction of the pending requests carry a duplicate attempt at once.

A request carries at most two concurrent attempts; escalation past that is implicit, driven by the RPC first-chunk timeout
(Eth2OutgoingRequestHandler.RESPONSE_CHUNK_ARRIVAL_TIMEOUT, ~10s) freeing a slot to re-hedge a still-stuck request to a fresh peer.

Example (hedgeDelay = 3s, RPC timeout = 10s):

• t=0 — primary attempt → peer A. attemptingPeers = {A}.
• t=3s (hedgeDelay) — size == 1 → hedge → peer B. {A, B}.
• t=6s — size == 2, not eligible; both attempts still outstanding (nothing new).
• t≈10s (RPC timeout) — A times out → removed, {B}; firstInFlightAt not reset (B still in flight), so immediately hedge-eligible again → hedge to fresh peer C. {B, C}.
• t=13s — B times out → hedge to D. {C, D} … rotating a dead peer out ~every RPC timeout until the column arrives or all attempts drain (then primary re-dispatches).

My preliminary testing shows ~2x sync speed improvement

Fixed Issue(s)

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

---

Note

Medium Risk
Changes concurrent DAS retrieval and req/resp batching on the sync path; behavior is tunable and heavily tested but overlap and unsynchronized rounds add extra peer traffic under load.

Overview
Improves Fulu data column sidecar retrieval when peers are slow or columns arrive via gossip before RPC completes.

SimpleSidecarRetriever moves from a single in-flight RPC per column to multiple concurrent attempts per pending column. After a configurable hedge delay (default 3s), it can hedge by requesting the same column from an alternate custody peer, capped by sidecarRetrievalOverlapFraction (default 5% of pending work, floored to at least one hedge when enabled). 0 disables hedging. Completion paths cancel redundant attempts without penalizing peers, and fix races where gossip wins or an attempt finishes before registration so peers are not locked out of request budget.

DataColumnReqRespBatchingImpl.flush() now skips buffered requests whose futures are already done (e.g. caller cancelled because gossip satisfied the need), avoiding useless network RPCs and peer scoring.

Configuration is exposed via P2PConfig, hidden CLI --Xp2p-sidecar-retrieval-overlap-fraction, and passed from BeaconChainController. Tests cover hedging budgets, gossip/cancellation races, and batching behavior.

^{Reviewed by Cursor Bugbot for commit 66b6576. Bugbot is set up for automated code reviews on this repo. Configure here.}

[rolfyone]

claude review

Overview

This PR adds speculative "hedge" requests to SimpleSidecarRetriever: when a column retrieval has been in-flight for longer than hedgeDelay (default 3s), a second attempt is dispatched to an alternate custody peer, bounded by overlapFraction
(default 5%) of pending requests. The winner cancels the loser. The implementation also adds roundInProgress serialization to prevent concurrent round bodies, refactors activeRpcRequest → per-peer attemptingPeers/activeRequests, and splits gossip
vs RPC completion paths.

---

Correctness Issues

1. Narrow leak window in activateMatchedRequest

// 1. Claim the peer slot
match.request.attemptingPeers.add(match.peer.nodeId);

// 2. Fire the RPC (future created here)
final SafeFuture activeRpcRequest = reqRespPromise.handle((sidecar, err) -> {
reqRespCompleted(match.request, match.peer, sidecar);
...
});

// 3. Register the future — happens AFTER firing
match.request.activeRequests.put(match.peer.nodeId, new ActiveRequest(...));

If gossip delivers the column (via reqRespSucceeded) between steps 1 and 3, that method calls request.cancelActiveRequests(), which iterates activeRequests — but the new peer's future isn't there yet. The future is never cancelled and leaks until
the RPC times out. Low severity (the column is already retrieved, the future silently discards its result), but worth documenting or mitigating. The existing comment correctly identifies that the RPC completion callback can't run before step 3
because flush() hasn't been called, but the gossip path is not gated on flush().

2. reqRespSucceeded and the success branch of reqRespCompleted are near-duplicate

// reqRespSucceeded (gossip):
if (pendingRequests.remove(request.columnId) != null) {
request.result.completeAsync(sidecar, asyncRunner);
retrieveCounter.incrementAndGet();
request.cancelActiveRequests();
}

// reqRespCompleted (RPC success branch):
if (maybeResult != null && pendingRequests.remove(request.columnId) != null) {
request.result.completeAsync(maybeResult, asyncRunner);
retrieveCounter.incrementAndGet();
request.cancelActiveRequests();
return;
}

The RPC success branch could just call reqRespSucceeded(request, maybeResult). As-is, a future change to one path (e.g., adding a metric) can silently miss the other.

---

Design / Concurrency

3. reqRespCompleted races with the round body

roundInProgress serializes concurrent round invocations, but RPC completion callbacks and gossip completions run outside that guard. The result is that during a round:

• reqRespCompleted can concurrently remove peers from attemptingPeers/activeRequests and reset firstInFlightAtMillis
• The round's hedge-eligibility filter (attemptingPeers.size() == 1, hasBeenInFlightFor(...)) can observe a stale snapshot

This is acceptable for a best-effort heuristic, but it means the overlap budget accounting is approximate: a request that transitions from 2 → 1 attempts mid-round could be counted toward currentOverlap and excluded from hedging, even though one
attempt already freed. The comment should note this explicitly.

4. No validation on overlapFraction input

overlapFraction > 1.0 allows more hedged requests than pending requests. overlapFraction < 0.0 is clamped by the budget <= 0 check, but still an unexpected configuration. A guard at construction time (checkArgument(overlapFraction >= 0.0 &&
overlapFraction <= 1.0, ...)) would make the contract explicit.

5. cancelActiveRequests() is not atomic

void cancelActiveRequests() {
activeRequests.values().forEach(activeRequest -> activeRequest.promise().cancel(true));
activeRequests.clear();
attemptingPeers.clear();
clearInFlightStart();
}

If two threads call this concurrently (possible: gossip path + dispose-completed-requests path on the next round), the forEach → clear → clear sequence is not linearized. Double-cancellation is harmless (cancel on an already-done future is a
no-op), but attemptingPeers.clear() after activeRequests.clear() could leave a window where attemptingPeers is non-empty but activeRequests is empty. The next round would incorrectly treat this request as "has one attempt" (for hedge eligibility)
while activeRequests disagrees. Low probability in practice.

---

Minor / Style

6. Builder.sidecarRetrievalOverlapFraction accepts any Double silently

Other timeout parameters in P2PConfig.Builder are validated (or at least documented). The overlap fraction should be validated at build() time or in the setter — a value of 5.0 (a common misinterpretation of "5%") would schedule 5× the pending
requests as hedges per round.

7. hedgeDelay not exposed as a CLI option

The default 3s is well-reasoned but tied to a specific knowledge of RPC timeout internals. Operators tuning --Xp2p-sidecar-retrieval-overlap-fraction would likely also want to tune hedgeDelay. Consider adding
--Xp2p-sidecar-retrieval-hedge-delay-ms as a hidden option alongside the fraction.

8. Missing test: timeout-triggered re-hedge

The PR description explicitly documents the important scenario where the primary RPC times out while a hedged attempt is still in flight, freeing the slot to re-hedge to a third peer. This is the path that enables the rotating-hedge behaviour
described in the javadoc. There's no test exercising it.

9. sanityTest change disconnects the sidecar from custodyPeerHavingData

Before:
custodyPeerHavingData.addSidecar(sidecar0); // sidecar added before connecting
testPeerManager.connectPeer(custodyPeerHavingData);
After:
// createSidecarAndAddToAllPeers adds sidecar to all peers (including custodyPeerHavingData)
testPeerManager.connectPeer(custodyPeerHavingData);

The test name is sanityTest and checks simpleSidecarRetriever (hedging disabled). This change is fine, but the helper createSidecarAndAddToAllPeers is called throughout the test file without appearing in the diff — it must already exist in the
file. Worth confirming it adds the sidecar to all connected peers (including both custody peers) before or at connection time.

---

What's Good

• The two-attempt cap (enforced by size() == 1 for hedge eligibility) is simple, correct, and avoids combinatorial explosion.
• roundInProgress guard correctly solves the race described in its comment — without it, two concurrent rounds could double-spend the budget and double-dispatch primaries.
• Overlap budget scales with outstanding work rather than peer limits — this is the right choice and is well-explained in the comment.
• Tests cover the three important cases: hedge success, disabled hedging, and budget limit. Readable and not over-mocked.
• DEFAULT_HEDGE_DELAY and DEFAULT_ROUND_PERIOD are now named constants instead of magic literals, which improves testability and readability.
• The javadoc on addHedgeMatches with the step-by-step timeline is excellent.

---

Summary

The core mechanism is sound. The main things to address before merging: validate overlapFraction at construction (#4), document or mitigate the gossip-vs-activation window (#1), and add the re-hedge-after-timeout test (#8). Items #2 and #5 are
low-risk cleanups worth doing but not blockers.

[zilm13]

@rolfyone addressed feedback
Only skipped:

• (7) It will lead to complex correctness checks, it depends on non-configurable retrieval timeout of 10s etc. We'd better keep it hardcoded I think
• (9) The helper is in place otherwise code couldn't be compiled. Looks like a LLM bug.

[zilm13]

I've decided not trying to avoid race completely, we are ok with eventual consistency here, we'd better remove over-complication. We should be sure that requests are eventually cleared and that's all. It's verified with the new test.

[rolfyone]

codex has a concern

P1 SimpleSidecarRetriever.java#L330-L334: activeRequests.put(...) happens after the request is handed to reqResp. Because nextRound() is explicitly unsynchronized and production uses a 4-thread das runner, another round can flush() the buffered request before this put. On failure, reqRespCompleted removes the peer from attemptingPeers/activeRequests, then this code re-inserts a completed ActiveRequest. That stale entry is then counted by createFromCurrentPendingRequests; with a peer limit of 1 it can reserve the peer forever for a still-pending column. The existing PR comment says this clears on timeout, but in this interleaving the future has already completed, so there is no later timeout to clean it up.
Open Question
SimpleSidecarRetriever.java#L252: is it intentional that the default 0.05 overlap floors to zero for fewer than 20 pending requests? Mainnet Fulu SAMPLES_PER_SLOT is 8, so single-block sampling will not hedge by default.

Still running another round from claude...

[rolfyone]

claude had a run at it too, seems mostly very minor things but included here.

PR #11011 Review — Hedge slow requests in SimpleSidecarRetriever

Defect 1 — Spurious RPC still fires after gossip-race cancellation

In activateMatchedRequest, when gossip resolves a request between attemptingPeers.add() and activeRequests.put(), the code calls activeRpcRequest.cancel(true). But this cancels the handle-wrapper future, not reqRespPromise — the future already
buffered in DataColumnReqRespBatchingImpl.bufferedRequests. flush() drains that buffer unconditionally with no isDone() guard, so the real libp2p RPC goes to the network anyway. When the response arrives, the reqRespCompleted callback doesn't run
(the dependent stage was already cancelled), so countSidecarRequest charges the peer but countSidecarReceived never credits it — degrading peer score for a self-inflicted request.

Fix: Cancel reqRespPromise directly before cancelling activeRpcRequest, or guard flush() with !promise().isDone() before adding to the node batch.

---

Defect 2 — "At most two concurrent attempts" Javadoc invariant is wrong

addHedgeMatches documents a strict two-attempt ceiling. But concurrent nextRound() calls (scheduler thread vs. reqResp thread) can both observe attemptingPeers.size() == 1 for the same request and both succeed in activateMatchedRequest with
different peers, producing three concurrent attempts. The code in nextRound() already correctly says this is best-effort — the Javadoc in addHedgeMatches contradicts that and will mislead future maintainers.

Fix: Change the Javadoc to "typically at most two; concurrent rounds may transiently produce three."

---

Defect 3 — hedgeCounter overcounts unactivated hedge matches

hedgeCounter.incrementAndGet() fires when a hedge RequestMatch is added to matches, before activateMatchedRequest runs. If a concurrent round already added that peer to attemptingPeers, activation returns false and no RPC fires — but the counter
was already incremented. The single-threaded test setup never triggers this, so all exact getHedgeCount() assertions pass despite the count being wrong in production.

Fix: Move the increment inside activateMatchedRequest, after the guard succeeds and the attempt is confirmed to be a hedge.

---

Design concern — Default 5% fraction silently disables hedging below 20 pending requests

final int maxOverlap = (int) Math.floor(overlapFraction * pendingRequests.size());

With overlapFraction = 0.05 (the default): Math.floor(0.05 × N) = 0 for N < 20. The primary scenario described in the PR — one column stuck on a slow peer — is exactly the case where hedging is disabled. Math.max(1, (int) Math.floor(...)) gated on
overlapFraction > 0 would allow one hedge from the first stuck request while preserving the ≤5% cap at scale.

---

Test concern — Re-hedge exclusion mechanism doesn't match production

shouldReHedgeToThirdPeerWhenPrimaryFailsWhileHedgeInFlight excludes the "column-not-available" peer via currentRequestLimit(1) — after one failure, completedRequests increments and available = 0, excluding the peer from re-selection. In
production, DataColumnReqRespBatchingImpl.getCurrentRequestLimit is a concurrent-window limit that frees the slot after a failed response, so the peer becomes selectable again immediately. The high-score primary peer would likely be re-selected
rather than thirdPeer. The test validates intended behavior but through a stub mechanism that doesn't exist in production.

---

Test coverage gap — concurrent-round races are untestable

StubAsyncRunner is single-threaded, so concurrent nextRound() invocations (Defect 2) and the gossip-race in activateMatchedRequest (Defect 1) have no test coverage. Worth noting as known uncovered territory.

---

Overall

The core hedging state machine is correct — the ConcurrentHashMap-per-peer approach, activation guard, failure cleanup, and overlap budget are sound. The three defects above are real but not catastrophic: one leaks an occasional spurious RPC, one
is a counter inaccuracy, one is a Javadoc lie. The design concern about the floor rounding is worth intentional discussion before merging.

[zilm13]

@rolfyone
Good feedback, thank you!
Everything was addressed

[rolfyone]

LGTM

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 71a2e60. Configure here.}

[StefanBratanov]

LGTM