fix(consensus): don't propose at historical heights after a bad block-sync handover

[lklimek]

TL;DR: A validator returning to the network delays creating blocks after an incomplete synchronization handover. The delay lasts until it reaches the recorded target or ten minutes elapse. Blocks rejected during application-response validation are not persisted.

User story

As a node operator restoring a validator, I want it to keep catching up before creating new blocks, while retaining supported restart recovery if block processing is interrupted.

Scenario

Base flow

A restored node downloads committed blocks, then switches to consensus to finish catching up and participate in the network.

Actual behavior

An early handover could allow the validator to create blocks at historical heights. During block synchronization, a block could also be stored before the application response had been validated.

Expected behavior

After a handover with a recorded target ahead of the node, local block creation pauses until that target is reached or ten minutes elapse. Voting and following the network continue. The node keeps downloading while usable peers report a large remaining gap. Application-response validation precedes persistence, with the existing recoverable save-before-commit ordering retained.

Detailed discussion

Issue being fixed or feature implemented

Fixes #1413. Related commit-handling work is in #1414. Application-side stale execution context handling belongs to dashpay/platform#4462. BLS verification optimization remains separate in #1427.

What was done?

• catchupTracker gates local proposal building using a fixed committed-height target. WaitForSync retains the highest peer height observed while waiting, even if that peer disappears or reports a lower height. poolRoutine reads applied state after stopping the synchronizer and passes the target to SwitchToConsensus.
• The gate arms only when targetHeight > state.LastBlockHeight. It releases when the target has been applied or an absolute ten-minute deadline expires. Later peer reports cannot clear, extend, or re-arm it; a new handover can re-arm it. Voting and normal consensus transitions remain enabled.
• After a stall exceeding 60 seconds, block sync hands over if no peer can serve the required block. When a servable peer exists, the ten-minute stall backstop applies only within maxCatchupGap (10 blocks) of the highest height reported by a servable peer. Servability requires an overlapping advertised range and an acceptable receive rate; a temporarily full pending-request queue does not disqualify a peer. Heights from other peers remain part of the handover target but do not determine this stall gap. These heights remain peer claims, not verified network-tip evidence.
• The applier uses ProcessProposal(..., true) -> SaveBlock -> FinalizeBlock. Invalid application responses fail before persistence. Saving before finalization preserves the replay-supported crash window where the block store can be one block ahead of application/state. Errors processing an already committed block still panic; this change does not promise recovery from a persistent application failure.

Scope limits: with zero or one initial peer, handover can occur before a useful network target is known. The proposal gate can expire while the node is still behind. The pre-existing absence of a complete join of block-application handlers at synchronizer shutdown remains outside this PR; the post-stop state read does not establish that all handlers have exited.

How Has This Been Tested?

• Committed tests cover application-response validation before saving, save-before-finalize ordering, retained handover targets, proposal suppression and expiry, and near/far stall decisions.
• Added peer-range and receive-rate coverage for the separate servable-height maximum, including busy peers and consistency during concurrent height advancement.
• Full blocksync, consensus, and state package suites with -race -tags=deadlock -p 1 passed at 649659c0. After the servable-height change, the affected blocksync suite passed again with the same flags (16.999s).
• Changed-line golangci-lint v2.13 passed with 0 issues, and git diff --check passed for the follow-up change. See the current commit's CI checks for full-suite and e2e results; no new e2e scenario is introduced.

Breaking Changes

The internal SwitchToConsensus interface gains targetHeight int64, and WaitForSync returns both caught-up status and the retained target. No public API change.

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 (changelog and PR description)
• I have assigned this pull request to a milestone

_{🤖 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: Advanced

Run ID: 967911dd-7d7a-411f-8240-9360910758b8

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]

🕓 Queued for automated review — 12th in line, estimated start in ~2.2 h (commit d3bd400)
Estimated review time once started: ~25 min (two-phase automated review; median of recent runs).

• Request priority review — tick this box and the review moves to the front of the queue.

[thepastaclaw]

Preliminary review — Codex only

The PR improves block-sync handover and application rejection ordering, but three in-scope fail-open paths still permit the historical-height/store-ahead failure modes it is intended to prevent. Proposal suppression is neither armed for the normal one-block-behind handover nor tied to the block-sync high-water mark, and an incoherent accepted ProcessProposal response is still detected only after persistence. Focused tests for the newly added catch-up and block-sync cases pass; the full consensus package run also encountered an unrelated timing-sensitive load test failure.
Source: Codex general and consensus-security reviewers (exact backend model IDs were not present in the supplied evidence); Claude Agent SDK final verifier (exact backend model ID was not exposed to this runtime). openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is 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)

🔴 3 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/blocksync/reactor.go`:
- [BLOCKING] internal/blocksync/reactor.go:311: Arm proposal suppression whenever the observed peer tip is above persisted state
  `Synchronizer.height` is the next unapplied height, while `state.LastBlockHeight` is the last applied height. Therefore, when a peer advertises `state.LastBlockHeight+1`, `IsCaughtUp` returns true even though that peer's tip block has not been applied, and the `!caughtUp` condition forces `behind=false`. An ordinary peer can advertise exactly that range, cause the handover, and delay its consensus height/commit traffic until the validator enters propose at that already-committed historical height. This bypasses the new suppression gate despite positive evidence that the peer is ahead. Whether block sync considers the remaining gap suitable for consensus catch-up and whether proposals must remain suppressed are separate predicates; arm the tracker whenever the observed block-sync tip exceeds the persisted state height.

In `internal/consensus/catchup.go`:
- [BLOCKING] internal/consensus/catchup.go:77-81: Do not discard the block-sync high-water mark based on mutable peer state
  The tracker retains only a callback to current consensus `PeerState` heights, not the block-sync tip that proved the node was behind. Those heights are untrusted and do not exclusively represent an advertised tip: `handleVoteMessage` calls `SetHasCommit`, whose `setHasCommit` raises `PRS.Height` to the catch-up commit's own historical height, and an unsigned `NewRoundStep` can establish a similarly low initial height. If that value is at or below the local proposal height, this code permanently clears the tracker—even before any block is committed—and a later report of the peer's actual far-higher tip cannot re-arm it. Retain the block-sync target height as a monotonic high-water mark and suppress proposals until committed local progress reaches that target; lower claims, inferred commit heights, and peer removal must not reduce or clear it.

In `internal/blocksync/applier.go`:
- [BLOCKING] internal/blocksync/applier.go:96-103: Validate ProcessProposal coherence before saving the block
  With `verify=false`, `ProcessProposal` checks the application's status, response structure, transaction-result shape, and app-hash length, but it does not verify that the returned app hash, results hash, validator updates, or consensus parameters match the committed block. `SaveBlock` persists the block before `FinalizeBlock` performs that coherence check through `ValidateBlockWithRoundState`. If the application returns ACCEPT with a stale but structurally valid response, finalization returns `ErrInvalidBlock` and the applier panics after advancing the block store while neither Tenderdash state nor the application was committed. Handshake replay deterministically encounters the same mismatch, preserving the restart loop this PR is intended to close. Perform the coherence validation before `SaveBlock`; if duplicate last-commit signature verification is too expensive, split that verification from the response/header coherence checks rather than delaying the first coherence gate until after persistence.

[thepastaclaw]

Final validation — Phase 2 only (queue backlog)

Four blocking issues remain: unsigned peer heights can indefinitely suppress proposals, the handover loses a post-stop state synchronization barrier, and two paths bypass the intended historical-proposal protection. The consensus and blocksync package suites pass with the race detector and deadlock tag, but their existing tests do not cover these failure scenarios. The worktree is unchanged.

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

Review provenance

• Triage: critical by gpt-6-astra (effort low) — The changes affect consensus proposal eligibility, block-sync handover, and block persistence ordering, where incorrect height tracking or recovery behavior could stall validators, produce invalid historical proposals, or leave nodes in restart-proof crash loops.
• Phase 1 reviewers: not run (skipped for throughput: 55 PRs queued, above the 10 limit)
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

🔴 4 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/catchup.go`:
- [BLOCKING] internal/consensus/catchup.go:77-79: Do not give unsigned height claims an indefinite veto over proposals
  A peer without validator keys can both arm and indefinitely hold this gate, even when the node is at the real tip. During a multi-validator restart at committed height H, the peer can advertise StatusResponse{Base: H + 2, Height: 1 << 60}; this passes blocksync validation. Honest peers hold blocks only through H, so nobody advertises a usable block at H+1. After syncTimeout, stopNothingToFetch hands over with behind=true. An unsigned NewRoundStep advertising height 1<<60 also passes consensus validation and makes this condition reject every proposal attempt. Honest peer reports and blockCommitted do not override that maximum, and there is no expiry while the attacker remains connected. The does-not-rearm protection does not help because the attacker controls the evidence used for initial arming. Repeating this against restarting validators can leave every proposer suppressed without any Byzantine voting power. Require verified or corroborated catch-up evidence, or a bounded mechanism for discarding unproven ahead-height claims, rather than granting one peer a permanent proposal veto.
- [BLOCKING] internal/consensus/catchup.go:77-81: Preserve the known handover target before accepting lower peer heights
  The handover transfers only a boolean and discards the blocksync height that established the node was behind. This gate then permanently disarms on any nonzero consensus maximum at or below the local proposal height. For example, a node can hand over after height 99 knowing a blocksync peer advertises height 5000, while that peer has not announced a consensus height; peers still syncing defer their consensus gossip until ready. If a seed reports consensus height 1, mayPropose(100) immediately clears the tracker despite the known gap. A later announcement of 5000 cannot re-arm it. A historical catch-up commit can similarly populate PeerState.Height through SetHasCommit without reporting the sender's actual tip. Preserve the handover target until catch-up evidence establishes that it has been reached or legitimately superseded; a lower consensus height from the currently reporting peers is not sufficient proof.

In `internal/blocksync/reactor.go`:
- [BLOCKING] internal/blocksync/reactor.go:306: Restore the post-stop applier-state read before consensus handover
  Caching executor.State() before Stop introduces a stale-state window and removes an existing synchronization barrier. State() and Apply() use the same applier mutex. Previously, the post-Stop State() call waited for an Apply already holding that mutex; now a valid H+1 application can start after this read and consensus receives H without waiting for it. If application finalization has completed but the state-store write has not, State.OnStart can reload H and resume ABCI processing at a height already processed by the application. There is also a completed-apply failure: if H+1 finishes during Stop and requests RetainHeight=H+1, LoadSeenCommit contains H+1 and LoadBlockCommit(H) has been pruned. SwitchToConsensus then panics loading the stale H commit before its startup reload can help. Capture peer-height evidence before shutdown if needed, but read the state passed to consensus after Stop. This restores the barrier removed by this PR without requiring a fix for the separate, pre-existing lack of an apply-loop join.
- [BLOCKING] internal/blocksync/reactor.go:311: Determine proposal suppression independently of the sync verdict
  IsCaughtUp compares the synchronizer's next block height against peers' committed block heights, so caughtUp=true does not establish that the actual handover state has applied their tip. With LastBlockHeight=99, synchronizer.height=100, and a peer advertising committed height 100, WaitForSync returns true and this expression forces behind=false. Consensus starts at height 100 without arming proposal suppression, although the network has already committed that height. A selected proposer can therefore still build the historical proposal this PR is intended to prevent. The same short-circuit ignores a higher peer learned between WaitForSync and this read. Keep caughtUp for the existing skipWAL decision, but determine behind independently by comparing peer-height evidence with the actual post-stop handover state's LastBlockHeight.

[lklimek]

LGTM

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The reviewed changes correctly address the historical-height proposal bug and the block-sync persistence-order issue. Handover targets are retained monotonically, proposal suppression is independent of the synchronization verdict and bounded by ten minutes, final applier state is read after synchronizer shutdown, and application responses are validated before block persistence. No in-scope blocking issues or actionable suggestions remain.

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: tenderdash-consensus-security); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

Review provenance

• Triage: critical by gpt-6-astra (effort low) — The large, intricate diff changes consensus proposal eligibility and block-sync handover behavior in internal/consensus/state_enter_propose.go and related consensus/blocksync state transitions, directly affecting a critical consensus surface.
• Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — tenderdash-consensus-security (completed, effort high); agent phase1-reviewer
• Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 85% left, 5h 100% left
• Fresh verifier: gpt-6-astra — final-verifier; agent astra-verifier
• Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Synchronizer shutdown does not explicitly join block-application routines — Synchronizer.Stop() does not formally wait for all in-flight block-application handlers to exit. The post-stop executor.State() read serializes with active Apply() calls, but a handler may still continue after shutdown returns, so explicit lifecycle joining would improve the handover boundary. This is a pre-existing concurrency concern explicitly excluded from this PR's scope and does not block approval.
  • Follow-up: Track block-application handlers with a wait group and make synchronizer shutdown return only after all handlers have exited.