fix(consensus): bound catch-up block-part resends and restore round timeouts in reactor test

[lklimek]

TL;DR: Fixes a consensus bug where a node helping a peer catch up would flood it with block parts it had already moved past, starving the current round — and fixes the CI test that had been hanging because of it.

Detailed discussion

Two commits. The first is internal test infrastructure with no user-observable behaviour change; the second is a production consensus fix. User story and Scenario are omitted because the user-visible effect is limited to the second commit and is fully described below.

Issue being fixed or feature implemented

Partially addresses #1405. TestReactorValidatorSetChanges intermittently hangs for its full 2-minute budget, most recently on #1417 (a pure Go-toolchain bump): run 32835423456.

It does not close #1405, which also names TestRouter_EvictPeers in internal/p2p — a separate matter (a hardcoded 1-second wait in p2ptest.RequireUpdate over a mocked transport), sharing no mechanism with this test.

Root cause 1 — the test harness disabled consensus recovery

TestReactorValidatorSetChanges drove its 7-node network with newMockTickerFunc(true). mockTicker.ScheduleTimeout forwards only RoundStepNewHeight timeouts, and with onlyOnce=true only the first one; every other scheduled timeout is discarded. The network ran with no timeoutPropose, no timeoutPrevoteWait/timeoutPrecommitWait — so round changes were impossible — and, after the first height, no timeoutNewHeight, so a node that assembled a commit itself never started the next height.

With recovery disabled, any perturbation wedged the network permanently. Both observed CI shapes follow: a proposal delayed past MessageDelay + Precision makes every validator prevote nil and the round change that would recover never fires; separately, a node that applies a commit itself sits at RoundStepNewHeight forever, and when it is the next proposer nobody proposes and nobody times out waiting.

This also explains why #1204 did not fix the flake — it introduced the mock ticker, reverting the deliberate move to a real ticker made by chore(consensus): stabilize consensus algorithm (#284).

Root cause 2 — unbounded catch-up block-part resends

Restoring round changes exposed a second, pre-existing defect. GossipBlockPartsForCatchup never marks catch-up parts as delivered, so prs.ProposalBlockParts.Not().PickRandom() keeps finding the same index missing and the part is re-sent on every gossip tick. The loop ends only when our own view of the peer's height advances — and the traffic it generates delays exactly the NewRoundStep messages that would advance it.

Measured in the reactor test: 1091 height-11 block parts pushed at a node that had already reached height 12, every one discarded, against 9 completed proposal blocks in the same window. The live round cannot assemble its proposal, every validator prevotes nil, and the height churns rounds until the deadline. The same loop drives the part-set-header mismatch branch, which logs at error level every tick without sending anything.

This was not caused by the first commit — before it, the network froze at round 0 with every node on the same height, so no peer was ever "behind" and the catch-up path never engaged.

What was done

1. internal/consensus/reactor_test.go — tickerFun: newTickerFunc() restores real round timeouts, and Timeout.Propose/Timeout.Vote are raised to 5s/2s. Round timeouts only fire when the network stalls, so larger values cost nothing on the healthy path; the previous values made a starved runner abandon rounds it would otherwise have completed.
2. internal/consensus/gossiper.go — catch-up is bounded to one part-set pass per catchupResendInterval (500ms). A pass spends one send per part; once complete, further ticks are skipped until the interval elapses, then a new pass begins.

The bound preserves #1365's guarantee. A peer that silently dropped its parts is still served repeatedly for as long as it stays behind, so the wedge that PR fixed stays fixed; only a peer we believe to be lagging is cheap. Parts are still never marked delivered, which keeps that guarantee independent of PeerState bookkeeping — deliberately, because SetHasProposalBlockPart ignores updates whose round differs from the peer's current round, and catch-up passes the committed block's round.

Rejected on measurement: widening the test's Synchrony params (no benefit), pacing block production via CreateEmptyBlocksInterval (worse, 4/18), and simply lowering the gossip cadence (5ms → 25ms → 100ms moved 2/16 → 7/16 → 5/16, confirming the cure is stopping after a pass, not sending more slowly).

How Has This Been Tested?

Locally, go test -race, 16-core host. N concurrent copies of the test binary on the same host, same bed throughout:

Load	Before both fixes	Ticker fix only	Both fixes
4 concurrent copies	—	2/16	16/16
3 concurrent copies	10/18	10/18	18/18
unloaded, -count=10	9/10	20/20	10/10

The unloaded baseline reproduced the CI signature exactly — all seven nodes reporting a waitForAndValidateBlock deadline, one with subscription terminated by publisher.

Mechanistic confirmation for commit 1: counting round changes after the last committed block, the old code shows max_round = 0 and one entering new round per node — frozen, and no extra time would help. After it, 16–62 events reaching rounds 2 through 11.

Mechanistic confirmation for commit 2: wasted catch-up parts per batch drop from 164592 to 2845 (a full revert of the resend behaviour scores 2736, so the bound gives up almost nothing), and part-set mismatch errors fall from 73 to 4. The wedge precondition Commit came in before proposal occurs 632 times across those runs with zero wedges.

Unit tests: full TestGossiper and TestPeerGossipWorker suites pass. The two tests added by #1365 were updated rather than removed — they still assert that catch-up never marks parts delivered, and now additionally assert that no resend happens inside the interval and that one does once the clock advances past it. TestGossipBlockPartsForCatchup's table cases now advance the fake clock between cases, since they are independent occasions at the same height.

Breaking Changes

None.

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

Follow-ups worth a look (deliberately out of scope)

• internal/consensus/init_test.go sets deadlock.Opts.DeadlockTimeout = 5s. Under contention that detector fires on a benign lock wait and os.Exit(2)s the entire package test binary.
• newMockTickerFunc(true) is still used by helper_test.go:216, replay_test.go:376 and byzantine_test.go:50. Same trap, unexamined.
• AGENTS.md states CI runs tests with -p 1, but test-group-% in the Makefile does not pass it, so up to GOMAXPROCS package binaries run concurrently. That is the contention source behind this class of flake.
• Within a catch-up pass, indices are still picked at random rather than iterated, so a pass need not cover every index of a multi-part block; successive passes converge. This is fix(consensus): resend catch-up block parts to prevent lagging-peer wedge #1365's own acknowledged tradeoff, unchanged here.

Prior work

• test(consensus): fix flaky TestReactorValidatorSetChanges #1204 — previous attempt at this test; introduced the mock ticker this PR removes.
• test: stabilize four flaky CI tests #1352 — hardened waitForAndValidateBlock's failure path; still in place, and what makes these logs legible enough to diagnose.
• fix(consensus): resend catch-up block parts to prevent lagging-peer wedge #1365 — added the catch-up resend whose unbounded form is fixed here. Its protection is retained, not reverted.

Attribution

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

[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: ff12688c-fd70-40de-9811-6043107f00f1

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

---

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.}

[thepastaclaw]

⛔ Blockers found — Opus deferred (commit 4d92801)
Canonical validated blockers: 2

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The change is confined to test infrastructure and correctly replaces the liveness-disabling one-shot mock ticker with independent production timeout tickers while increasing only test-local timeout parameters. The focused non-race test passed; the race run on a heavily contended host reached active round changes before its wall-clock deadline, consistent with the documented oversubscription limitation rather than the permanent ticker-induced stall this PR fixes. Source: reviewers gpt-5.6-sol (general and tenderdash-consensus-security); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).

Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
• Secondary pass: disabled (temporary_phase2_sonnet_disable)

[thepastaclaw]

Preliminary review — Codex only

Restoring the real timeout ticker is sound, but the catch-up limiter does not enforce its intended resend interval for partially populated part sets. Because the pass budget uses the total part-set size, a peer with one missing part can still receive hundreds or thousands of duplicate copies before the 500 ms backoff begins, so changes are required.

Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (tenderdash-consensus-security); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/consensus/gossiper.go`:
- [BLOCKING] internal/consensus/gossiper.go:243: Limit each catch-up pass to the peer's missing parts
  `beginCatchupAttempt` budgets attempts using the total bitmap size, while `PickRandom` at line 194 selects only unset bits and successful catch-up sends deliberately leave those bits unset. A valid partial bitmap containing N-1 set bits therefore causes the sole missing part to be selected and resent N times before the retry interval starts. This state is reachable from an untrusted peer because `NewValidBlockMessage.ValidateBasic` checks the bitmap's size but not its contents, and `ApplyNewValidBlockMessage` installs the supplied bitmap for the matching height. With the production 100 ms gossip cadence, a roughly 337-part default-size block permits about 34 seconds of uninterrupted duplicate sends before a 500 ms pause; the protocol maximum of 1,601 parts permits about 160 seconds. This leaves the flooding behavior that the PR is intended to stop almost unchanged for partial part sets. Count the currently missing bits instead, and add a regression case where every bit except one is set.

[thepastaclaw]

Preliminary review — Codex only

The current head fixes the prior static partial-bitmap amplification, and restoring the real timeout ticker remains appropriate. Two in-scope limiter flaws remain: a peer can extend an active catch-up pass by repeatedly replacing its bitmap, and a malformed header can trigger a maximum-length pass of storage reads and error logs without sending any part.

Source: Codex reviewers gpt-5.6-sol (general and tenderdash-consensus-security); final verifier claude-opus-4-6; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/consensus/gossiper.go`:
- [BLOCKING] internal/consensus/gossiper.go:250-262: Snapshot the missing-part budget for each catch-up pass
  `missing` is recalculated from the latest peer-supplied bitmap on every tick, while `catchupAttempts` and `catchupRetryAt` describe an already active pass. After N-1 attempts against N missing parts, the peer can replace the bitmap with one reporting N-1 missing parts. Because `catchupRetryAt` has not yet been initialized, `catchupAttempts >= missing` immediately resets the counter without waiting. Repeating this before each threshold permits N*(N-1)/2 attempts before the first retry interval; for a matching 1,601-part block that is 1,280,800 uninterrupted part-send attempts. This is reachable because `ApplyNewValidBlockMessage` replaces the bitmap wholesale at the peer's matching height and round. Snapshot the missing count when a pass opens, or track a remaining budget that cannot be increased or reset by bitmap updates until the retry interval expires, and add a regression test that decreases the reported missing count immediately before each threshold.
- [BLOCKING] internal/consensus/gossiper.go:207-210: End the catch-up pass after a failed header check
  `beginCatchupAttempt` consumes only one slot before the header check, so a failure here does not activate the retry interval until every reported missing-part slot has been consumed. A peer can install an otherwise valid `NewValidBlockMessage` at its catch-up height with an all-zero 1,601-bit array and an arbitrary 32-byte part-set hash. The mismatch then causes a block-meta lookup and an Error log on every default 100 ms gossip tick for about 160 seconds, followed by only a 500 ms pause, without sending a part or requiring another inbound message. Multiple peers can amplify this storage, CPU, and log work while contending with live consensus traffic. A metadata, header, or part-load failure should terminate the current pass and set its retry deadline immediately; add a regression test using a maximum-size missing bitmap and mismatched header.

[thepastaclaw]

Preliminary review — Codex only

Restoring production timeout tickers is sound, and the current head fixes the active-pass bitmap manipulation by snapshotting both the missing-part budget and retry deadline when a pass opens. One blocking issue remains: storage and validation failures leave that peer-controlled pass active, enabling continuous reads and error logs and allowing an inflated malformed-header budget to carry into repeated valid part sends. Source: Codex reviewers gpt-5.6-sol (general and tenderdash-consensus-security); final verifier claude-opus-4-6; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/consensus/gossiper.go`:
- [BLOCKING] internal/consensus/gossiper.go:202-215: End the catch-up pass after a failed header check
  `beginCatchupAttempt` consumes a peer-controlled budget slot before loading metadata or validating the part-set header, but each failure here returns without ending the active pass. An unauthenticated peer can announce a stored historical height and install a validly encoded 1,601-bit all-zero `NewValidBlockMessage` with an arbitrary valid-length hash; `ValidateBasic` accepts it and `ApplyNewValidBlockMessage` installs it. The default 100 ms gossip loop then performs a metadata read and emits an Error log on every tick for roughly 160 seconds. Because `catchupRetryAt` was set when the pass opened, it has already expired when the budget is exhausted, so another full pass starts immediately without an effective pause. The same state allows send amplification: after opening the 1,601-slot pass with a mismatched header, the peer can replace its same-height state with the stored block's real header and a bitmap reporting one missing part, and the unchanged remaining budget permits approximately 1,600 duplicate sends of that part. Terminate the active pass and establish a fresh retry deadline after metadata, header, or part-load failures, and add regressions for both a persistent maximum-size mismatch and a malformed-to-valid same-height replacement.

[Claudius-Maginificent]

Catch-up pass now ends when it cannot send — fixed in e01f593

Answering the blocking finding carried in the review body of 5031834714 and 5039744991 (internal/consensus/gossiper.go:202-215). It never got an inline thread, so this is a PR-level reply — nothing to resolve.

Verified independently, not taken on faith. Both exploits reproduce. I wrote them as tests against the pushed head da7ddea37 before touching product code, and both went RED — each one saturating at my loop bound of 50 rather than at any budget, which is precisely the claim: the budget was not the limiter.

expected: 1   actual: 50   "a pass that cannot send must cost one attempt per interval, not one per tick"
expected: 0   actual: 50   "a budget opened against an unusable header must not fund sends after the peer swaps it out"

Fix. The send moved into sendCatchupBlockPart, which returns an error whenever no part reached the peer; GossipBlockPartsForCatchup then calls endCatchupPass(), zeroing the remaining budget and arming a fresh deadline from now. I covered one path beyond the three you named — a failing syncProposalBlockPart — on the same reasoning: the invariant worth holding is that only a pass which got as far as sending keeps its budget, not merely one that got as far as loading. The PickRandom miss is covered too, and it is the one honest case: the peer completed its part set between the budget being drawn and the index being picked.

Structuring it as a wrapper plus a helper rather than five calls to endCatchupPass() at five return sites is deliberate — a future early return cannot silently reintroduce this.

On exploit 1, being precise about what the fix is worth: the limiter's contract is one attempt per interval, so wasted reads and error logs drop from one per 100 ms tick to one per 500 ms — 5x, not elimination. That is the ceiling any per-interval bound can offer, and it matches your suggested direction. Exploit 2, the send amplification, is eliminated outright: the inflated budget dies on the first failed attempt, so the fresh pass is budgeted on the single part the peer actually reports.

One residual lever I checked rather than assumed. beginCatchupAttempt still resets its counters when prs.Height changes, which would bypass the new backoff. It is not exploitable: shouldPeerBeCaughtUp confines the catch-up path to blockStoreBase <= prs.Height < rs.Height, and ApplyNewRoundStepMessage drops any message with CompareHRS(...) <= 0, so a peer can walk that range once, upward, for one failed attempt per height — heights it must announce anyway. Left as is.

Verification. Targeted group green under -race (ok github.com/dashpay/tenderdash/internal/consensus 17.697s), go vet and gofmt clean.

The storm bed needed care this time: an unrelated tenant on the measurement host swung an unmodified da7ddea37 binary from 16/16 to 8/16, so a sequential before/after would have been worthless. I re-measured paired instead — both binaries running concurrently at the same tier, three rounds, 24 runs per arm:

	pass	wasted catch-up parts
da7ddea37 (control)	23/24	4013
e01f593b4 (this fix)	23/24	4121

The single failure hit both arms in the same round. No liveness or storm-bound regression.

Separately: both reviews also report _1 additional finding(s) omitted (not in diff)._ — distinct from this one, which was named in the prompt block and counted as blocking. Its text is not published in the review body, the inline comments or the gate comment, so I cannot act on it. Please surface it if it is meant to be actioned.

[thepastaclaw]

Preliminary review — Codex only

The current head fixes the prior failure-path pass leak, but successful passes still permit back-to-back replay and allow a previously opened budget to be concentrated on one remaining part. Both issues are in the limiter introduced by this PR and leave sustained duplicate catch-up traffic possible for large multipart blocks, so changes are still required.
Source: reviewers gpt-5.6-sol (Codex general and TenderDash consensus-security); final verifier claude-opus-4-6. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/consensus/gossiper.go`:
- [BLOCKING] internal/consensus/gossiper.go:280-283: Start the retry interval when the pass finishes
  `catchupRetryAt` is armed when a pass opens, although catch-up sends only one part per data-gossip invocation. At the default 100 ms gossip cadence, a pass with six or more missing parts lasts at least the full 500 ms interval. When `catchupRemaining` reaches zero, the deadline has therefore already expired, and the next tick immediately opens another pass. Successful catch-up sends intentionally do not update the peer bitmap, so a peer that remains at the same height can receive block parts on every tick indefinitely, with no quiet interval between passes. This preserves the traffic-starvation behavior the limiter is intended to stop for multipart blocks. Arm the retry deadline when the final attempt is consumed or completed, and add a test that advances the fake clock by the production gossip cadence between sends and verifies a full retry interval after the pass ends.
- [BLOCKING] internal/consensus/gossiper.go:276-283: Tighten an active pass when the missing bitmap shrinks
  The pass snapshots its initial missing count, but `sendCatchupBlockPart` selects from the peer's latest bitmap on every tick. An untrusted peer can open a valid N-part pass reporting every part missing, allow one send to succeed, and then send another same-height `NewValidBlockMessage` with the same stored part-set header but only one unset bit. `ValidateBasic` accepts that replacement and `ApplyNewValidBlockMessage` installs it wholesale, while the unchanged `catchupRemaining` funds N-1 successful duplicate sends of the sole missing part. For a 1,601-part block, this permits roughly 160 seconds of uninterrupted duplicates at the default cadence, and `endCatchupPass` never runs because every send succeeds. Consume a snapshot of distinct missing indices or monotonically reduce the active remaining budget when the current missing count decreases; never increase or reopen the active pass from a replacement bitmap.

[Copilot]

Pull request overview

This PR addresses a consensus gossip liveness issue where catch-up block-part resends can become effectively unbounded (starving other consensus messages), and it stabilizes TestReactorValidatorSetChanges by restoring real consensus round timeouts in the test network harness.

Changes:

• Restore real timeout ticker usage in TestReactorValidatorSetChanges and increase propose/vote timeouts to better tolerate race-detector and single-process contention.
• Add per-peer catch-up pass accounting to bound catch-up block-part resends to at most one part-set “pass” per catchupResendInterval.
• Update and extend gossiper unit tests to validate the bounded resend behavior and pass-budget invariants; wire a real clock into the peer gossiper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
internal/consensus/reactor_test.go	Switches the flaky reactor test back to a real timeout ticker and increases round timeouts to allow recovery under load.
internal/consensus/gossiper.go	Implements bounded catch-up block-part resend passes using a clock and per-peer pass bookkeeping.
internal/consensus/gossiper_test.go	Updates existing catch-up resend tests and adds new cases for pass budgeting, retry interval behavior, and malformed/mismatched peer state.
internal/consensus/gossip_peer_worker.go	Initializes the new gossiper clock with a real clock in production wiring.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

[thepastaclaw]

Preliminary review — Codex only

The current pass accounting fixes both prior resend-budget defects, but panic unwinding still bypasses failure settlement, allowing an unreadable historical record to trigger a long burst of per-tick storage panics and full-stack logs. The focused race command reached the consensus package suite and failed only in the unrelated existing TestLoadHeightProgressUnderSustainedFlood contention assertion; the catch-up panic path has no regression coverage.
Source: reviewers gpt-5.6-sol (Codex general and TenderDash consensus-security); final verifier claude-opus-4-6; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/consensus/gossiper.go`:
- [BLOCKING] internal/consensus/gossiper.go:220-225: End the catch-up pass when block-store access panics
  `beginCatchupAttempt` consumes a pass slot before `sendCatchupBlockPart` calls `LoadBlockMeta` or `LoadBlockPart`. Those block-store methods intentionally panic on database read, decode, and validation failures, while `peerGossipWorker.runGossipHandler` recovers the panic, logs a full stack, and continues ticking. Because `endCatchupPass` is reached only when `sendCatchupBlockPart` returns normally with `false`, panic unwinding leaves the remaining budget active. A peer that selects an unreadable historical height and reports a maximum-size 1,601-part missing bitmap can therefore cause a storage panic and full-stack Error log every default 100 ms tick for roughly 160 seconds before the budget expires, then repeat after the backoff. This defeats the PR's explicit invariant that an attempt which cannot send must end its pass. Settle unsuccessful attempts in a defer so panic unwinding also closes the pass, and add a regression using a block-store mock that panics.

[thepastaclaw]

Preliminary review — Codex only

The prior panic-path blocker is fixed by deferred pass settlement and focused regression coverage, but two in-scope throttle defects remain: successful final sends can consume their retry interval while blocked, and peer-asserted height advances can clear the interval entirely. The focused consensus package test reached completion but failed only in the known unrelated TestLoadHeightProgressUnderSustainedFlood contention assertion.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (tenderdash-consensus-security); final verifier claude-opus-4-6. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/consensus/gossiper.go`:
- [BLOCKING] internal/consensus/gossiper.go:423-430: Start the retry gap after the final send completes
  `beginCatchupAttempt` consumes the final slot and arms `catchupRetryAt` from `now` before `sendCatchupBlockPart` performs the corresponding block-store reads and outbound send. The latter reaches `Channel.Send`, which explicitly blocks until the envelope is enqueued or the context ends (`internal/p2p/channel.go:217-224`). If storage or outbound congestion delays that final successful send by at least 500 ms, the deadline has already expired when the pass actually finishes. After the worker's normal sleep, the next invocation can immediately open another pass, eliminating the promised quiet gap under exactly the congestion where repeated catch-up traffic can interfere with current-round data gossip. Settle successful pass completion and arm the deadline using `g.clock.Now()` after the final send returns; add a regression whose mocked send advances the fake clock by the interval before returning and verify that a full additional interval is still required.
- [BLOCKING] internal/consensus/gossiper.go:330-359: Preserve the retry gap across unverified height advances
  The default height-change branch clears every non-sticky retry deadline, including one armed when a one-attempt pass cleanly exhausted its budget. An untrusted connected peer can therefore claim historical height H with `NewRoundStep`, install the real stored `PartSetHeader` and a bitmap with one unset bit through `NewValidBlock`, receive that part, and then claim H+1 before the next gossip tick. `ApplyNewRoundStepMessage` verifies only monotonic HRS progression, while `ApplyNewValidBlockMessage` accepts the replacement at the matching height and round; neither proves that the peer processed the prior part. Repeating this over retained heights yields one historical block part on every gossip tick instead of one pass per 500 ms interval, and reconnecting permits replay of the retained range. This restores sustained data-channel traffic and block-store reads that the PR is intended to suppress. Preserve the pending deadline across peer-asserted height changes, or grant immediate service only after progress the local node can verify.

[thepastaclaw]

🔴 Blocking: Start the retry gap after the final send completes

beginCatchupAttempt consumes the final slot and arms catchupRetryAt from now before sendCatchupBlockPart performs the corresponding block-store reads and outbound send. The latter reaches Channel.Send, which explicitly blocks until the envelope is enqueued or the context ends (internal/p2p/channel.go:217-224). If storage or outbound congestion delays that final successful send by at least 500 ms, the deadline has already expired when the pass actually finishes. After the worker's normal sleep, the next invocation can immediately open another pass, eliminating the promised quiet gap under exactly the congestion where repeated catch-up traffic can interfere with current-round data gossip. Settle successful pass completion and arm the deadline using g.clock.Now() after the final send returns; add a regression whose mocked send advances the fake clock by the interval before returning and verify that a full additional interval is still required.

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Preserve the retry gap across unverified height advances

The default height-change branch clears every non-sticky retry deadline, including one armed when a one-attempt pass cleanly exhausted its budget. An untrusted connected peer can therefore claim historical height H with NewRoundStep, install the real stored PartSetHeader and a bitmap with one unset bit through NewValidBlock, receive that part, and then claim H+1 before the next gossip tick. ApplyNewRoundStepMessage verifies only monotonic HRS progression, while ApplyNewValidBlockMessage accepts the replacement at the matching height and round; neither proves that the peer processed the prior part. Repeating this over retained heights yields one historical block part on every gossip tick instead of one pass per 500 ms interval, and reconnecting permits replay of the retained range. This restores sustained data-channel traffic and block-store reads that the PR is intended to suppress. Preserve the pending deadline across peer-asserted height changes, or grant immediate service only after progress the local node can verify.

_{source: ['codex']}