fix: prevent sync deadlock when no peer can serve the requested header range - #2
fix: prevent sync deadlock when no peer can serve the requested header range#2nonsense wants to merge 3 commits into
Conversation
An empty response to a headers or bodies request is the protocol-correct answer of a peer that does not have the requested data, but the downloaders reported it as a bad message (ReputationChangeKind::BadMessage, -16384): four honest answers cross the ban threshold and disconnect the peer for 12 hours. When no peer can serve the requested range - e.g. a fork choice target ahead of every currently connected peer - the node bans its entire honest peer set within seconds, guaranteeing the download it is waiting on can never complete. This wedged OP Stack nodes in production (Ink Mainnet 2026-07-16, unichain 2026-08-05), where a >32 block gap after a sequencer stall sent all replicas into backfill with no peer able to serve the range. Empty responses are no longer reported; the failed request is resubmitted as before, and the fetcher already deprioritizes peers whose last response was unsatisfactory when picking a peer for the next request. Malformed responses (wrong start block, non-contiguous headers, invalid seals, too many bodies) keep the full penalty. The engine's single-block and block-range fetch paths already treat empty responses as non-reportable. Also promotes the "Penalizing peer" log from trace to debug so reputation destruction is visible closer to default verbosity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The Headers stage propagated the downloader's Pending forever: a fork choice target that no connected peer can serve - e.g. after a sequencer stall leaves every replica equally behind - parked the pipeline at stage 1 indefinitely, with no retry, no error and no log line. Combined with fork choice updates only being tracked during backfill, this wedged OP Stack nodes in production until an operator restart (Ink Mainnet 2026-07-16, unichain 2026-08-05). Two bounds, both progress-aware so a slow but healthy sync is never interrupted: - `HeaderStage` now arms a stall deadline (default 30s, configurable via `stages.headers.stall_timeout`) while the downloader is pending and resets it whenever headers arrive. On expiry the stage returns a new recoverable `StageError::Stalled` instead of pending forever. The ETL and downloader state are kept so a retry with an unchanged sync gap resumes the download; the ETL collectors are instead cleared whenever the sync gap changes, which also fixes stale collector entries leaking into a later run with a different target. A timeout wrapped around the `execute_ready` future itself would not work here: the headers stage legitimately downloads the entire gap - potentially for hours during initial sync - before it resolves. - The pipeline now gives a stage a readiness budget (default 5m, `PipelineBuilder::with_execute_ready_timeout`) armed on the first `execute_ready` failure and cleared once the stage becomes ready. When a stage keeps failing readiness beyond the budget, the pipeline emits `PipelineEvent::TimedOut` and gives up on the stage for the current run with `ControlFlow::NoProgress`, so control returns to the caller (the engine re-evaluates the backfill target) instead of retrying a stale target forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A finished backfill only returned the network to `SyncState::Idle` when the debug-only `--debug.startup-sync-state-idle` flag was set (the unconditional transition was removed in paradigmxyz#16742 and re-added behind the flag in paradigmxyz#19429). A backfill that ends without reaching its target - now reachable via the stage stall handling - therefore left the node marked syncing indefinitely, suppressing transaction gossip, until live sync happened to advance. This contributed to wedged OP Stack nodes in production (Ink Mainnet 2026-07-16, unichain 2026-08-05). Make the idle transition on `BackfillSyncFinished` unconditional again. If the node is still behind, `on_backfill_sync_finished` re-checks the distance against the latest tracked fork choice state and re-triggers backfill (emitting `BackfillSyncStarted`, which marks the network as syncing again) or issues the remaining downloads, so the idle window between consecutive runs is brief. This also means the initial-sync latch flips after the first backfill run instead of at the first live-sync block, matching pre-paradigmxyz#16742 behavior; a brief syncing/idle flap between runs is preferable to a node that stays "syncing" forever while wedged. The flag keeps its startup semantics. Also warn when the connected-peer count reaches zero while the node is syncing - a sync that has no peers cannot make progress, and this state was previously invisible at default log levels. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // An empty response is the protocol-correct answer of a peer that does not have the | ||
| // requested bodies and must not accrue ban-worthy reputation; the request is simply | ||
| // resubmitted. | ||
| if let Some(peer_id) = peer_id && |
There was a problem hiding this comment.
I think this case should be handled before even calling the on_error function.
| // Penalize the peer for bad response | ||
| if let Some(peer_id) = peer_id { | ||
| trace!(target: "downloaders::headers", ?peer_id, %error, "Penalizing peer"); | ||
| if matches!(error, DownloadError::EmptyResponse) { |
There was a problem hiding this comment.
Same here. Instead of adding an exception inside the penalize_peer function, it should never even be called in this case.
|
|
||
| /// Default total time budget a stage is given to keep failing [`Stage::execute_ready`] before | ||
| /// the pipeline gives up the current run with [`ControlFlow::NoProgress`]. | ||
| pub const DEFAULT_EXECUTE_READY_TIMEOUT: Duration = Duration::from_secs(5 * 60); |
There was a problem hiding this comment.
| pub const DEFAULT_EXECUTE_READY_TIMEOUT: Duration = Duration::from_secs(5 * 60); | |
| pub const DEFAULT_EXECUTE_READY_TIMEOUT: Duration = Duration::from_mins(5); |
einar-oplabs
left a comment
einar-oplabs
left a comment
There was a problem hiding this comment.
I would suggest splitting in to three PRs and fixing the download problem before upstreaming.
| /// This bounds how long the pipeline retries a stage that cannot become ready, e.g. the | ||
| /// headers stage waiting on a download no peer can serve, so control returns to the caller | ||
| /// instead of retrying a stale target forever. It only counts consecutive readiness | ||
| /// failures, so slow but progressing stages are not affected. |
There was a problem hiding this comment.
Fable 5 (high): should-fix: this claim does not hold. ready_deadline is armed on the first readiness failure and cleared only when execute_ready returns Ok — i.e. when the entire header gap has been collected. Header batches arriving in between never reset it. So: one 30s stall early in a run (peer churn at startup is common), then healthy progress past the 5-minute mark, then one more transient 30s stall → the whole run aborts, despite continuous progress in between.
Suggest making progress reset the budget — e.g. count consecutive stall windows in the stage itself (it already resets its own stall_deadline on every yielded batch, so it knows) and only surface a budget-exhausted error when there was genuinely zero progress. At minimum, correct this comment. See the companion comments in headers.rs for why a spurious abort is not free (ETL discard on a moved target).
| "Stage kept failing to become ready within the timeout, giving up on it for this pipeline run" | ||
| ); | ||
| self.event_sender.notify(PipelineEvent::TimedOut { stage_id }); | ||
| return Ok(ControlFlow::NoProgress { |
There was a problem hiding this comment.
Fable 5 (high): question: in the common case this does not actually end the run as NoProgress. With a prior checkpoint this returns NoProgress { Some(N) }, then run_loop does progress.update(N), executes the remaining stages (one cheap no-op pass each), and PipelineProgress::next_ctrl converts a Some block number into ControlFlow::Continue { block_number: N }. NoProgress { None } only reaches the engine from a checkpoint-less DB (the added test's setup). Functionally this is fine — on_backfill_sync_finished treats Continue { N } and NoProgress { Some(N) } identically — but the PR description and the StageError::Stalled doc promise a NoProgress result; worth rewording (e.g. "the run completes without the stage having progressed") so upstream reviewers don't trace an apparent mismatch.
| match self.downloader.poll_next_unpin(cx) { | ||
| Poll::Ready(Some(Ok(headers))) => { | ||
| // the downloader is making progress, so reset the stall deadline | ||
| self.stall_deadline = None; |
There was a problem hiding this comment.
Fable 5 (high): should-fix: "progress" here is one yielded stream batch — stream_batch_size contiguous validated headers, which ReverseHeadersDownloaderBuilder::new sets to commit_threshold (default 10,000). Two healthy-network cases trip the 30s timer:
- Sustained throughput below ~333 headers/s (10k/30s) means every inter-batch gap exceeds 30s →
Stalledfires perpetually on a slow-but-working connection. - Validation is strictly in-order from the tip down, so one slow/timed-out request at the head of the range blocks yielding while up to 99 buffered responses wait — fetcher request timeouts alone can approach 30s.
Each stall is individually recoverable, but it arms the pipeline readiness budget (see pipeline/mod.rs). Consider resetting the deadline on downloader-level progress (e.g. growth of validated/buffered headers), or moving stall detection into the downloader, which sees every response.
| if self.sync_gap != Some(gap.clone()) { | ||
| // discard any headers collected for a previous gap, e.g. by an attempt that stalled, | ||
| // so ranges of different targets are never mixed in the ETL collectors | ||
| self.clear_etl_state(); |
There was a problem hiding this comment.
Fable 5 (high): should-fix: the clear itself is necessary and correct (mixing two targets' ranges in the collectors could write stale-chain headers, and mid-collection interruption is newly possible with Stalled), but note the systemic cost: after a budget-expiry abort, the next run's target has usually moved — on OP Stack the backfill target is the FCU head (backfill_target_hash), which moves every block; on ETH it is finalized, which moves ≈ every 6.4 min, i.e. faster than the 5m budget. So nearly every abort lands here and throws away everything collected, up to the whole run's download. A chronically slow node then aborts every ~5min → discards → restarts → initial header sync livelocks where the old code was slow-but-certain. Fixing the budget-reset issue (see pipeline/mod.rs) resolves this.
Also: this new discard-on-changed-gap path is untested — worth a test regardless of how the budget question is resolved.
| // peers guarantees the download can never complete. The fetcher already | ||
| // deprioritizes peers whose last response was unsatisfactory when picking a peer | ||
| // for the resubmitted request. | ||
| debug!(target: "downloaders::headers", ?peer_id, %error, "Peer unable to serve requested headers"); |
There was a problem hiding this comment.
Fable 5 (high): should-fix: with the penalty gone, nothing rate-limits the retry loop. on_headers_error resubmits the request at Priority::High immediately, and when no peer has the range (the incident shape) every request returns empty at RTT speed and is instantly resubmitted — continuously, for the entire stall window of every backfill cycle, indefinitely. Previously this self-limited by banning the peer set (which was the bug). Suggest a small growing delay after consecutive empty responses for the same range, or temporarily marking the range unserveable. Upstream reviewers are likely to raise this.
| // Penalize the peer for bad response | ||
| if let Some(peer_id) = peer_id { | ||
| trace!(target: "downloaders::headers", ?peer_id, %error, "Penalizing peer"); | ||
| if matches!(error, DownloadError::EmptyResponse) { |
There was a problem hiding this comment.
Fable 5 (high): question: worth disclosing explicitly in the upstream PR text: an always-empty peer is now completely penalty-free — it can hold a peer slot forever, with only soft deprioritization (the fetcher's last_response_likely_bad), and only when a better idle peer exists. Combined with the new stall timeout the node can no longer be wedged by such a peer, and geth tolerates empty responses too, so the trade-off looks acceptable — but the removal of any escalation path should be an explicit, argued choice rather than implicit. (Timeout penalties for non-responding peers are unchanged.)
| /// pipeline run with no progress once the stage keeps stalling beyond the pipeline's | ||
| /// readiness timeout, returning control to the caller for re-targeting. | ||
| #[error("stage stalled while waiting to become ready: {0}")] | ||
| Stalled(String), |
There was a problem hiding this comment.
Fable 5 (high): nit: stringly-typed payload, and the call site allocates a formatted string every 30s in the retry loop. Stalled { timeout: Duration } (or another structured form) would match the sibling variants better. Also worth a note in the upstream PR: StageError and PipelineEvent are pub enums without #[non_exhaustive], so the added variants are technically semver-breaking for downstream exhaustive matches.
| /// Set the total time budget a stage is given to keep failing [`Stage::execute_ready`] | ||
| /// before the pipeline gives up the current run with | ||
| /// [`ControlFlow::NoProgress`](crate::ControlFlow::NoProgress). | ||
| pub const fn with_execute_ready_timeout(mut self, timeout: Duration) -> Self { |
There was a problem hiding this comment.
Fable 5 (high): nit: asymmetric with stall_timeout — this 5m budget is builder-API-only, with no reth.toml/CLI plumbing (the default does reach the production pipeline via PipelineBuilder::default()). Fine as-is, but consider a config knob, or state why not: operators tuning stall_timeout will look for its counterpart.
| // requested bodies and must not accrue ban-worthy reputation; the request is simply | ||
| // resubmitted. | ||
| if let Some(peer_id) = peer_id && | ||
| !matches!(error, DownloadError::EmptyResponse) |
There was a problem hiding this comment.
Fable 5 (high): question: only the Headers stage gets stall detection — bodies (and era) keep the wait-forever behavior, so the same "no peer can serve" shape can still deadlock one stage later. In practice headers gate bodies for this incident, and peers that served the headers virtually always have the bodies — which is presumably the justification — but state it in the PR, or upstream will ask why the fix is asymmetric.
2 participants
When a fork choice target is >32 blocks ahead and no connected peer can serve the header
range — e.g. a fleet of equally-behind replicas resuming after a sequencer stall — the node
bans its entire honest peer set (4 empty responses × BadMessage = 12h ban), the Headers
stage pends forever with no error or log line, and the network stays marked syncing until
an operator restart. Three production incidents on 1s-blocktime OP Stack chains hit this
exact shape; on 12s chains the same path is reachable after ~6.4 min of drift.
EmptyResponseas a bad message — it is the protocol-correct answer of apeer that lacks the data, and the fetcher already deprioritizes such peers for the next
request (sibling fix to fix(net): recover partial header responses paradigmxyz/reth#26482)
StageError::Stalledwhen the download makesno progress for
stages.headers.stall_timeout(default 30s), keeping ETL/downloaderstate so a same-gap retry resumes; end the pipeline run with
ControlFlow::NoProgressonce readiness failures exceed a budget (default 5m), so the engine re-targets from the
latest fork choice state instead of retrying a stale target forever
SyncState::Idleunconditionally onBackfillSyncFinished(reverts the fix: small networking fixes paradigmxyz/reth#16742 gate — sticky "syncing" after an unproductive backfill suppresses tx
gossip indefinitely; the debug flag keeps its startup semantics)