fix(blocksync): survive transient peer failures and stalls

[QuantumExplorer]

Stacked on #1394, which is stacked on #1393. GitHub will retarget as those
merge. Review only the last commit.

Issue being fixed or feature implemented

Two ways block sync gives up far more easily than it should. Neither is a
percentage problem — both make a node stop syncing — so they outweigh any of the
throughput work in the rest of this stack.

1. A single failed block request dropped the peer

consumeJobResult called RemovePeer and reported a p2p.PeerError for any
errBlockFetch, including a plain 15s request timeout. PeerManager.Errored
evicts on the first report:

func (m *PeerManager) Errored(peerID types.NodeID, err error) {
	...
	if m.isConnected(peerID) {
		m.evict[peerID] = true
	}

Peers serve block requests one at a time, so a busy peer times out long before it
is unhealthy. Worse, dropping it also fails the up to 20 requests already in
flight to it (maxPendingRequestsPerPeer), and each of those failures dropped
another peer in turn.

2. The p2p client reported the peer itself, behind the caller's back

newPromise sent a PeerError on every request timeout, so no caller-side
policy could soften it. Statesync's backfill already documents that it does not
want this:

} else if errors.Is(err, context.DeadlineExceeded) {
	// we don't punish the peer as it might just have not responded in time

3. A 60s stall handed over to consensus even when far behind

WaitForSync returned on time.Since(lastAdvance) > syncTimeout and
poolRoutine then switched to consensus unconditionally. That is a one-way door
— only the state sync path ever calls SwitchToBlockSync — so a node that
stalled for 60s while thousands of blocks behind was permanently demoted to
consensus catch-up, which is far slower than block sync.

What was done?

• Count consecutive failures per peer. Drop a peer only after
  maxConsecutiveFailures (5) failures in a row; the count resets on the first
  successful response. A failed request also stops being counted as pending,
  which previously only happened as a side effect of deleting the peer.
• Let the caller decide what a timeout is worth. newPromise now rejects
  without reporting. GetBlock is the only production caller of any
  promise-returning client method — GetChunk/GetParams/GetLightBlock exist
  on the interface but statesync uses direct channel sends and its own dispatcher
  — so this is confined to the block sync path. A genuinely dead connection is
  still caught by the transport's ping/pong (60s interval, 90s timeout).
• A stall alone no longer ends block sync. It ends only when no peer reports
  a height above ours, or when the stall outlasts maxSyncStall (10 min) so a
  wedged synchronizer can still hand over rather than blocking forever. The
  decision is factored into stallVerdictFor so it is testable without waiting
  on the 1s ticker.
• WaitForSync returns whether the node actually caught up, and poolRoutine
  uses that rather than a second IsCaughtUp() call racing the synchronizer it
  just stopped.

How Has This Been Tested?

New tests:

• TestStallVerdictFor — table over the stall decision: progressing, stalled
  with nothing left to fetch, stalled while ahead of every peer, stalled with
  peers still holding blocks, and either side of the wedge limit.
• TestConsumeJobResultKeepsPeerOnTransientFailure — failures below the
  threshold keep the peer; the client mock has no Send expectation, so
  reporting it would fail the test. The next failure crosses the threshold and
  does report and remove.
• TestAddFailure — threshold behaviour, that a success clears the run, and that
  an unknown peer is never reported (so the remaining in-flight failures of an
  already-removed peer do not report it again).
• TestAddFailureClearsPending — a failed request stops being pending.

Two cases in the existing TestConsumeJobResult encoded the old
one-failure-one-eviction policy. Rather than delete them, the second now seeds
the peer one failure short of the threshold so it still exercises removal and the
re-request of that peer's pending blocks.

• go build ./internal/... ./types/... ./node/... ./cmd/...
• go test -race -tags=deadlock -p 1 over blocksync, p2p/..., statesync,
  consensus, state, store — all pass
• golangci-lint: 69 issues, identical to the base branch

Built and tested against DOCKER/Dockerfile's deps stage (Go 1.26.5).

Breaking Changes

None to the wire protocol or on-disk format. Behavioural: peers are no longer
evicted on a single failed block request or a single request timeout. That is the
point of the change, but it does mean a bad peer is tolerated for up to 5
requests before removal.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7408b5b3-f58b-4b74-9b35-7d7680e6f2d0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/blocksync-peer-resilience

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[QuantumExplorer]

CI follow-up

golangci-lint — fixed

unused-parameter: parameter 'pool' on a test closure this PR emptied out. Now
_. Worth recording how it slipped past my local check: CI runs golangci-lint
with only-new-issues: true, which flags findings on changed lines even when
the same finding already exists elsewhere in the file, and pins v2.12. I had
been comparing aggregate issue counts against the base with v2.8. Re-checked by
extracting the exact lines this branch adds or changes and intersecting them with
a full v2.12 run: zero findings on any of them.

tests (01) — flaky, not from this PR. Re-running.

--- FAIL: TestReactorValidatorSetChanges (120.08s) — a timeout, not an
assertion.

This PR cannot reach it:

• internal/consensus imports neither internal/blocksync nor
  internal/p2p/client, in production or test code, so nothing changed here is
  reachable from that test.
• The test has prior flakiness fixes on record (test(consensus): fix flaky TestReactorValidatorSetChanges #1204, and earlier upstream
  "reduce size of validator set changes test").

Ran it 5 times in a row locally under -race: all pass, ~12s each against the
120s timeout it hit in CI.

govulncheck — waiting on #1395

The Go stdlib advisory (GO-2026-5856). #1395 bumps to 1.26.5 and is fully
green; this PR clears once that merges and CI is re-run.

🤖 Addressed by Claude Code

[lklimek]

Audit Summary — PR #1396 fix(blocksync): survive transient peer failures and stalls

Reviewed by: Claude Code with a 3-agent team:

• security-engineer-smythe (opus) — OWASP-classified security review, concurrency and DoS surface
• project-reviewer-adams (opus) — cross-artifact consistency (code ↔ docs ↔ tests ↔ config), code quality
• qa-engineer-marvin (sonnet) — adversarial correctness, execution-verified claims

Every one of the 18 findings was then independently re-validated by a 4th adversarial pass (all returned valid). The three blocking findings were not merely read — they were reproduced by throwaway probe tests compiled and run against this worktree at 09530ab:

• numFailures 4 → 0 after a single StatusResponse (AddPeer → Put wholesale-replaces PeerData)
• numPending driven to -2 by three failures, after which FindTimedoutPeers() returns empty — the failing peer escapes the slow-peer eviction check entirely
• eviction verdict returned with 15 requests still pending — five concurrent in-flight timeouts, not five sequential retries

Overall assessment

The PR's stated goal is to survive transient peer failures and stalls without punishing peers unnecessarily. The bounded stall/handover mechanism (maxSyncStall) and the TOCTOU fix threading caughtUp out of WaitForSync are both genuine, correctly-scoped improvements, and integration with the two PRs beneath it in the stack (#1393's RemovePeer, #1394's UpdateMonitor) is clean.

But the core new mechanism — count consecutive failures, evict at a threshold — has three independent logic defects, each of which separately defeats the PR's own stated purpose:

1. The failure counter is reset by any StatusResponse — which every peer sends every ~10s, unsolicited and unauthenticated, and which the peer store accepts unconditionally. A single failed request costs the full 15s peerTimeout, so a peer failing requests serially can never reach 5 consecutive failures. The eviction threshold is practically unreachable for exactly the deliberate-misbehavior case it exists to catch.
2. The counter's semantics don't match its name or doc comment — block requests are pipelined (up to 20 concurrent per peer), so one transient outage window times out 5+ requests at once and evicts the peer on the first window. That is precisely the transient-failure case the PR title promises to survive.
3. AddFailure's unfloored numPending-- drives the count negative — which both exempts the failing peer from the slow-peer eviction check (FindTimedoutPeers requires numPending > 0) and keeps it eligible for selection when honest peers' concurrency caps bind. Failure is rewarded with more traffic, on the exact height that just failed.

Everything else found — a ~540-line Error-level log flood per stall episode, an unauthenticated peer height gating a 10× longer lockout window, an undocumented behavior change on three shared client APIs, structural/idiom nits, and missing coverage on the new instrumentation and the TOCTOU fix itself — is real and worth fixing, but none of it independently breaks the PR's stated contract the way the three blocking items do.

Verdict: request changes. Fix the failure-counting mechanism on three fronts before merge — make the counter resistant to a StatusResponse reset, make it reflect actual failed request rounds rather than concurrent in-flight timeouts, and floor numPending at zero. The stall-handling and TOCTOU-fix portions are sound and can stay as-is; the log-flood, stall-window-trust, and undocumented-API-contract findings are good fast-follow candidates but need not gate this PR.

Findings

18 findings (0 CRITICAL / 0 HIGH / 8 MEDIUM / 10 LOW) — 22 raw, deduplicated to 18 across 3 reviewers (36% redundancy; two findings were independently raised by all three agents).

Class	Sev	OWASP / CWE	Location	Finding
🚫 blocking	MEDIUM	A04 Insecure Design, A05 Security Misconfiguration; CWE-841, CWE-799, CWE-400	internal/blocksync/peer_store.go:130-144, synchronizer.go:239-263, p2p_msg_handler.go:56-64	Consecutive-failure counter is reset by any peer-sent StatusResponse, so the new eviction threshold is unreachable for a selectively-failing peer
🚫 blocking	MEDIUM	A04 Insecure Design; CWE-191 Integer Underflow, CWE-770	internal/blocksync/peer_store.go:136-144	AddFailure decrements numPending on a path that previously deleted the peer — failing makes a peer invisible to the slow-peer check and keeps it selectable
🚫 blocking	MEDIUM	logic, doc-accuracy, concurrency	internal/blocksync/synchronizer.go:40-44	The "consecutive" failure counter actually counts concurrent in-flight timeouts — the documented intent is not delivered
non-blocking	MEDIUM	A04 Insecure Design; CWE-807 Untrusted Input in Security Decision, CWE-400	internal/blocksync/synchronizer.go:322-404, reactor.go:33-38	maxSyncStall extends a peer-controlled block-sync lock-out from 60s to 10 minutes, keyed off an unauthenticated peer-reported height
non-blocking	MEDIUM	A04 Insecure Design, A09 Logging & Monitoring Failures; CWE-390, CWE-400	internal/p2p/client/client.go:397-427,167-227	newPromise's timeout no longer reports the peer on any of its 4 shared APIs; only blocksync compensates, and the new caller-owns-punishment contract is undocumented and untested
non-blocking	MEDIUM	logging, observability (execution-verified)	internal/blocksync/synchronizer.go:352-362	Peer-triggerable Error-level log flood: up to ~540 identical lines per stall episode, proven by execution
non-blocking	MEDIUM	test-coverage, concurrency	internal/blocksync/synchronizer.go:322-370, reactor.go:293-300	WaitForSync's actual loop and its effect on SwitchToConsensus's skipWAL argument are untested — only the extracted pure function is covered
non-blocking	LOW	observability, operator-ux	internal/blocksync/synchronizer.go:339-341	Block sync gives up silently on the stopNothingToFetch path — no log line at all
non-blocking	LOW	observability, metrics	internal/blocksync/synchronizer.go:244-263	New failure/stall states are observable only by grepping logs; no metric, despite existing consensus.Metrics wiring
non-blocking	LOW	idiom, encapsulation, DRY	internal/blocksync/peer_store.go:136-144	AddFailure breaks the peer store's UpdateFunc idiom, bypasses the wrapper that maintains maxHeight, and hides a pending-count mutation
non-blocking	LOW	DRY, readability	internal/blocksync/synchronizer.go:339-362	The syncTimeout threshold is evaluated twice — once inside stallVerdictFor, once again in its caller
non-blocking	LOW	concurrency, consistency	internal/blocksync/synchronizer.go:330-338	The stall decision is assembled from four separately-locked reads that never coexisted
non-blocking	LOW	logging, operator-ux	internal/blocksync/synchronizer.go:249-254	"keeping peer" is logged for peers that are already gone — AddFailure's bool conflates "kept" with "not found"
non-blocking	LOW	dead-branch, naming, test-accuracy	internal/blocksync/synchronizer.go:379-380	stopNothingToFetch as documented is unreachable, and one test case exercises a state production cannot produce

Pre-existing / outside-diff (follow-up, not this PR)

Four findings are real but sit outside this diff's responsibility. They are reported here only, with no inline comments:

• MEDIUM — internal/p2p/client/ — A pre-existing issue was identified in this area during review; this PR marginally widens its exposure window. Given its nature, it is being handled through a private security-disclosure channel rather than described in this public thread.
• LOW — internal/blocksync/reactor.go:32-38 — stall constants live in reactor.go but are used only by synchronizer.go.
• LOW — docs/tendermint-core/block-sync/reactor.md:240-247 — block-sync spec doc still describes the old switch-to-consensus condition.
• LOW — internal/blocksync/peer_store.go:236-237 — new godoc sentences omit the trailing period required by STYLE_GUIDE.md.

Positive observations

• The TOCTOU fix is genuinely correct: threading caughtUp out of WaitForSync into SwitchToConsensus(ctx, state, caughtUp || stateSynced) removes a real race against a synchronizer that was just stopped, exactly as the PR's own comment claims.
• The bounded stall/handover mechanism is well-shaped: replacing an indefinite wedge with an explicit maxSyncStall ceiling is the right structural move, and extracting stallVerdictFor as a pure, table-tested decision function is good practice — TestStallVerdictFor covers the decision table thoroughly.
• Stack integration is clean: interaction with fix(blocksync): don't punish peers for duplicate block responses #1393's RemovePeer and perf(blocksync): remove redundant work from the block apply path #1394's UpdateMonitor was checked and holds up; no conflicts or double-eviction paths.
• Reviewer effort was verification-first: all three blocking findings were reproduced with executable probes rather than asserted from reading the diff.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}