fix(consensus): verify commit vote extensions before a commit is taken

[lklimek]

Issue being fixed or feature implemented

Alternative to #1509. It contains all of #1509 plus commits that replace how the node handles a commit whose vote extensions the application rejects.

A peer can send an authentic commit whose threshold block signature is valid but whose extension vector is stripped, duplicated, replayed from another height, or empty. The block signature does not cover the vector. If a node persisted such a commit, Drive would reject it in FinalizeBlock and the node would crash-loop.

#1509 checked the extensions in ApplyCommit, after the commit was already taken. That required recovery machinery. In one version, a rejected commit made the node move to the next round right away. A peer resending one stripped commit then added a round per rejection, and every round kept vote sets and lengthened timeouts with no limit (review thread).

This PR checks the extensions before a commit is taken. A rejection is then an ordinary verification failure: the commit is not taken, and there is nothing to recover.

What was done?

• Extensions are checked before a commit is taken. TryAddCommit already runs ProcessProposal (ensureProcess) before it takes a commit. It now calls VerifyCommitExtensions right after, at the end of prepareCommitForApply. When the application rejects the vector:
  • the commit is handled like any other verification failure;
  • the extensions_rejected metric is incremented;
  • nothing is persisted, parked or relayed, the round does not change, and the sender is not evicted.
• A future-round commit whose block we already hold is checked before the round is entered, so a rejection never moves the round. Without the block, the commit's round (authenticated by its threshold signature) is entered once, as in v1.8-dev, and the check runs when the block arrives. When the commit is accepted and the node is that round's proposer, it does not propose: a proposal would replace the checked ProcessProposal result and force a second one for the same round.
• Commits that arrive while another commit waits for its block:
  • each one is authenticated when it arrives (ValidateBasic and a BLS check, charged to the sender's budget in its turn), and a forged one gets its sender evicted;
  • valid ones are queued, one slot per connected peer, and the slot is freed when the peer disconnects. Many peer identities cannot push out an honest commit;
  • a commit equal to the waiting one, or to the one its sender already queued (same height, round, BlockID and extension vector), is dropped before its BLS check and takes no slot. A copy of another peer's queued commit is verified (charged to its sender) and takes the sender's own slot, so the first sender cannot make the authentic commit disappear by replacing its slot or disconnecting; a commit that would need its block processed again is refused by the expectation (see below);
  • when the block completes, the candidates are tried a round at a time: the round already processed first, then each other round in order of first appearance, arrival order within a round. The block is processed (ProcessProposal) once per round, then the application checks each commit's vector. The first one accepted is applied;
  • if every one is rejected, the block continues on the normal path, where the node's own +2/3 precommits can finish the height.
• A commit built from the node's own +2/3 precommits is checked before ApplyCommit. If the application rejects it, the node logs an error and persists nothing. It waits for a peer's commit instead of panicking, because a panic would loop: WAL replay rebuilds the same commit.
• Foreign-round commits cannot force re-execution. A commit's extension list is authenticated only entry by entry: order, count, duplication, a THRESHOLD_RECOVER entry's sign request ID, and app-supplied (RAW) sign request IDs bound to no height or round all vary under one genuine block signature, so no cache of verdicts on vectors can bound the work. After the application rejects a commit at a (block, round), Tenderdash asks its ExtendVote for the vector expected there and keeps it for the height. A later commit that would need that block processed again for its round is refused before ProcessProposal and without any ABCI call when its vector differs from the expected one, or equals it and was already rejected. Commits of the round processed last still go to VerifyVoteExtension. Forced ProcessProposal calls per height are ≤ 2·G plus the finalizing one, where G is the number of foreign rounds with a signature-valid commit for the held block, whatever the number of peers or vectors.
• Own round restored after a rejected foreign-round commit. Before verifying a peer precommit's extensions, or extending on relock, the node reprocesses its own round if a rejected foreign-round commit left the application at another round (Drive fails ExtendVote on a round mismatch). Rejections of peer commits log at debug (peer-floodable); rejection of the node's own commit stays at Error.
• Invariant documented at ensureProcess, in the spec and in UPGRADING.md: CurrentRoundState must match the application's latest ProcessProposal; a result from another round is never reused without a new ProcessProposal. A failed ProcessProposal clears CurrentRoundState, since the application may have dropped the context it held (Drive does), so a later commit of the round processed before is not judged, and ExtendVote is not asked, on a context the application no longer holds; the failed own-round restore is not retried per resent precommit. Drive keeps one execution context per height and finalizes only the round processed last, so consensus logic keeps no per-round result map and restores nothing.
• ABCI contract for repeated ProcessProposal (proto comment on RequestProcessProposal, api.md, ABCI++ methods spec, UPGRADING.md): Tenderdash may call ProcessProposal more than once for the same height and round with the same block (e.g. after a commit-extension rejection, or when block sync hands over to consensus, which starts without block sync's result), and again for a round after processing another round. The application must treat it as re-execution replacing its execution context; FinalizeBlock refers to the round processed last. This matches Drive v4.1.1 (process_proposal.rs: same round + same hash, non-proposer → drop context and re-execute; only same round + different hash errors).
• Block sync reuses the ProcessProposal result by header and round (CurrentRoundState Source + MatchesBlock, the same rule as consensus' ensureProcess). A commit rejected at the round processed last is retried without processing the block again; a commit of another round processes it again.
• Reference kvstore / e2e app: duplicate detection now mirrors Drive. ProcessProposal errors only when called again for the round processed last with a different (or empty) block hash. A repeat of that round with the same block hash re-executes it (e.g. block sync processes the block, the commit's extensions are rejected, and consensus processes the same block and round again after the handover). Re-processing a round after another one (e.g. commits rejected at r0 then r1, then the node's own r0 quorum; or block sync r1 → r2 → r1) is allowed too. In both cases FinalizeBlock's round-keyed state lookup still finds the state, and the replaced round state is closed. Before this, those sequences made the authentic commit fail (consensus) or blockApplier.Apply (block sync) panic under kvstore/e2e networks.
• Recovery machinery removed. state_apply_commit.go, state_add_commit.go, state_data.go, state_enter_commit.go and state_enter_propose.go are back to their v1.8-dev versions. The following are gone:
  • round changes on rejection, and the recovery loop;
  • KeepProcessedBlock and SkipProposalCreation;
  • Prepaid and keeping commits for which the budget ran out.
• Docs updated: UPGRADING.md (including "before you upgrade" ordering, the consensus_commit_verify_failures{reason="extensions_rejected"} metric and the own-commit log line), the ABCI++ spec, the proto comment (with types.pb.go and api.md regenerated), and docs/nodes/metrics.md. UNKNOWN is documented as REJECT; handshake catch-up is named apart from WAL replay. The metric help text covers own and block-sync commits, block sync labels it through consensus.CommitVerifyFailureReason, and both handshake catch-up paths give the same rollback hint.
• Ride-alongs, unrelated to commit extensions but shipped here:
  • PEX reactor: calculateNextRequestTime no longer holds r.mtx while calling peerManager.PeerRatio() (lock-order deadlock), with reactor_deadlock_test.go;
  • CI: proto-lint.yml passes github_token to buf-setup-action;
  • consensus blockExecutor: create/ensureProcess read the committed state through the RWMutex accessor getCommittedState();
  • e2e app: VerifyVoteExtension enforces the expected commit vector (empty validator_pro_tx_hash).

How Has This Been Tested?

• The three consensus test files are merged into commit_extensions_test.go.
  • TestCommitExtensionsRejectedBeforeSave covers 5 paths (held, parked, future, future-held, replay) × 4 vectors (stripped, duplicated, cross-height replay, empty). Each case asserts that the rejection happens before SaveBlock, nothing is persisted or relayed, the metric is incremented, nobody is evicted, and an honest commit then finishes the height without processing the block again.
  • Other tests cover:
    • 50 resends at round 0 plus 50 at round 2: the round, vote sets and step are unchanged;
    • 41 peer identities ahead of an honest peer (copies of the waiting commit are dropped, so only 2 application checks run);
    • candidates tried round by round (TestParkedCommitsAreTriedRoundByRound);
    • TestOwnQuorumAfterCommitsRejectedInTwoRounds: waiting r0 commit rejected, queued r1 commit rejected, then the own r0 quorum finalizes with a kvstore-like executor (ProcessProposal rounds [0 1 0], no panic);
    • TestQueuedCommitCopies: an attacker parks a stripped commit and queues the authentic one; an honest peer's copy of the authentic commit is kept, so after the attacker replaces its slot with another bad vector, or disconnects, the authentic commit is still applied; the same sender's repeat and a copy of the parked commit cost no BLS check and no slot;
    • TestSameRoundReprocessedAfterHandover: the block processed for round 0 as block sync would (extensions rejected), then consensus, without that result, processes the same block and round again for the authentic commit, which is applied with a kvstore-like executor. A full block sync → consensus reactor handover test was judged disproportionate: the blocksync package imports consensus and the consensus test fixture is internal;
    • TestForeignRoundCommitsCannotForceReprocessing: 116 signature-valid vectors (subsets, permutation, duplicates, junk THRESHOLD_RECOVER sign request IDs, foreign-height RAW) alternated over two rounds, with a resent peer precommit after each commit making the node return to its own round: ProcessProposal rounds exactly [1 0 2 0] (2·G), ExtendVote ×2, no application call for refused commits; the genuine commit still finalizes;
    • TestFailedProcessProposalForgetsProcessedRound (a failed ProcessProposal for a foreign commit or for the own-round restore, then a rejected commit for the round processed before: the block is processed again or the commit is refused by the expectation, and ExtendVote is asked only in that context), TestFailedOwnRoundRestoreIsNotRetried (11 resends of a precommit after a failed restore: one ProcessProposal), TestExpectationLearnedOnlyForProcessedRound;
    • TestRejectedExpectedVectorIsRemembered (an app rejecting its own expected vector is not asked again);
    • TestOwnRoundRestoredAfterForeignCommitRejected (peer precommit and relock, with a Drive-like round-checking executor);
    • TestCommitExtensionRejectionLogsAtDebug, TestSameVoteExtensionShape, TestCommitExpectations, TestCommitKey;
    • forged queued commits;
    • the budget charged in the sender's turn;
    • WAL replay;
    • the own-precommit fallback;
    • rejection of the node's own commit, then the same vector from a peer is judged again by the application without processing the block again;
    • both handshake catch-up paths return the rollback hint.
• Block sync: TestBlockApplierRejectsCommitExtensionsAndRetries, TestBlockApplierInvalidatesRejectedProposal (block/round/state change forces a new ProcessProposal), and TestBlockApplierReprocessesRoundAfterAnother (r1, r1, r2 rejected, then r1 accepted: rounds [1 2 1], no panic).
• kvstore: TestProcessProposalRoundReprocessing (r0, r0 same hash → ok, r0 other hash → error, r0 no hash → error, r1, r0 → ok, r0 same hash → ok, then FinalizeBlock at r0 succeeds).
• New regression tests were confirmed failing without the fix (old kvstore duplicate rules, dedup removed, cross-peer dedup, expected-vector prefilter removed, own-round restore removed, relock ensureProcess removed, floodable classification removed, replay hint removed, stale CurrentRoundState kept after a failed ProcessProposal, ExtendVote asked outside the processed context, ensureOwnRound disabled or re-processing on every precommit).
• TestParkedCommitChecksStateIDBeforeSavingBlock is adjusted. A commit with a mismatched state ID is now dropped rather than returned as an error, and the test still asserts that nothing is persisted.
• Mutation check: all 9 mutations of the key logic are caught.
• go test -tags=deadlock -count=1 ./internal/consensus ./internal/state ./internal/blocksync ./types ./abci/... ./test/e2e/app passes (packages run one after another).
• -race on the new and changed tests passes.
• make lint LINT_BASE=origin/v1.8-dev reports 0 issues.

Breaking Changes

Same as #1509, which changes the ABCI VerifyVoteExtension contract for applications. The spec now also states that Tenderdash may call ProcessProposal more than once for the same height and round with the same block (re-execution that replaces the execution context), and again for a round after processing another round at the same height, and that FinalizeBlock refers to the round processed last. Drive v4.1.1 already behaves this way; the reference kvstore/e2e app is updated to match. An application that errors on a repeated same-block ProcessProposal for a round must be changed to re-execute instead. ExtendVote contract: Tenderdash may call ExtendVote on any node, non-validators included, after a commit-level VerifyVoteExtension rejection that follows ProcessProposal of that block and round, to learn the vector a commit of that block and round must carry. ExtendVote must be free of side effects, and its threshold-recoverable subset must equal exactly what the commit-level VerifyVoteExtension accepts; an ABCI error or an invalid response is fatal. Extensions that are not threshold-recoverable may still be non-deterministic. Drive v4.1.1 and the e2e app comply. No other breaking 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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · 483f7e2

• Bots — thepastaclaw ✓
• Self-review — posted; again after any push
• Within your 5 open PRs
• Build green
• Approvals
  • github (.github/workflows/proto-lint.yml) — ktechmidas or shumkov
  • files with no dedicated owner — you own it

When every box is checked the PR Hygiene check passes and this can merge.

[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: Repository: dashpay/tenderdash/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 810c89d7-b132-4648-bb8a-acf009d11b3b

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]

✅ Final review complete — no blockers (commit 483f7e2)

[github-actions]

Claudius here. Reviewed via sequential producers (QA first, per CI ordering); its own MEDIUM/BLOCKING self-assessment tripped the early-stop gate, so the security, project-consistency, and docs passes didn't run this time around — they'll pick it up on the next push. One MEDIUM, non-blocking finding posted inline: selectParkedCommit can drive an uncapped number of full ABCI ProcessProposal re-executions (one per distinct candidate round among connected peers) on a single block-completion event, with no budget analogous to the authentication step's verificationBudget. It's bounded by peer count and recoverable via block sync, not a safety break — but it sits in the exact commit-recovery code this PR adds, so worth a look. Full report: review-report/report.html.
📊 View full HTML review report

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[github-actions]

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

At the exact reviewed head, all six prior findings are fixed, and commit-extension verification precedes persistence across consensus, block sync, and handshake catch-up. One non-blocking replay scalability suggestion remains; the claimed injection of unsigned DEFAULT extensions is prevented by existing signature-count validation. Uncached Makefile-driven tests passed for the seven affected core packages, and focused regression and concurrency tests passed with the race detector and deadlock tag.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: tenderdash-consensus-security); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: tenderdash-consensus-security); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: critical by gpt-6.1-sol (effort low) — The large, intricate diff changes consensus commit acceptance in internal/consensus/state_try_add_commit.go and prepareCommitForApply in internal/consensus/block_executor.go, including application verification of vote extensions, authenticated peer commit queuing, and round-transition behavior before persistence.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — tenderdash-consensus-security (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 13% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — tenderdash-consensus-security (completed, effort xhigh); agent phase2-reviewer

🤖 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/commit_candidates.go`:
- [SUGGESTION] internal/consensus/commit_candidates.go:79-84: Avoid quadratic candidate insertion during WAL replay
  During WAL replay, retainable preserves every fromReplay entry regardless of whether its sender remains connected. Distinct historical peer IDs therefore accumulate at one unfinished height, and each insertion scans the accumulated slice in holds, DeleteFunc, and the replacement lookup, making reconstruction O(N²). Identical copies of an authentic replacement still receive separate sender-owned slots, so connection churn can trigger this without producing distinct commit vectors. The live connection limit does not bound replay's historical sender count, and the default WAL limit is 1 GB. A candidate-only replay benchmark measured approximately 19 ms, 62 ms, and 263 ms for 2,000, 4,000, and 8,000 senders, respectively. Preserve arrival order and per-peer ownership while indexing slots by peer ID, and avoid repeated disconnected-peer pruning scans during replay. Add a replay/churn benchmark to protect that behavior.

[github-actions]

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

[lklimek]

/self-reviewed

[github-actions]

Ready for review — needs ktechmidas or shumkov.
Full checklist in the description.

[github-actions]

Ready for review — needs ktechmidas or shumkov.
Full checklist in the description.