perf(blocksync): verify a commit once, not again as the next block's LastCommit

[PastaPastaPasta]

Block sync threshold-verifies each commit three times.

blockApplier.verify verifies the commit for block N against state.Validators. One height later the identical commit arrives again as block N+1's LastCommit, and while that block is applied it is verified twice more against state.LastValidators: once in validateBlock, and once in ValidateBlockWithRoundState. A BLS threshold verification costs about 1.9 ms, so every block pays for two it has already done.

The applier now verifies through Executor.VerifyCommit, which runs the threshold verification itself and, only on success, returns a typed proof — types.VerifiedCommit — of exactly what it verified. ValidateBlock, ValidateBlockWithRoundState, FinalizeBlock, ApplyBlock and ProcessProposal each take the caller's VerifiedCommit as an explicit parameter and skip their own verification only when it matches by content. Block sync performs one threshold verification per block; consensus and the replayer, holding no proof, keep verifying every commit in full.

This PR includes #1456, which was stacked on it and has been merged into this branch, and has since been merged with v1.8-dev to resolve conflicts with #1415, #1416 and #1426.

Why the skip is safe

ValidatorSet.verifyCommit reads exactly: chainID, height (against commit.Height), blockID (against commit.BlockID), vals.QuorumType, vals.QuorumHash, vals.ThresholdPublicKey, and the commit itself — the canonical vote and the quorum signs are both derived from it.

VerifiedCommit's unexported proof pins every one of those, and (*ValidatorSet).VerifyCommitUnlessVerified skips only on a full match: same chain ID, height, block ID, quorum type, quorum hash, threshold public key, and byte-identical signed digests/signatures — checked by content ((*commitProof).checkMatches), never by pointer identity, since block sync's next-height commit is a distinct decoded object from the one that was verified. Anything short of a full match, including a proof-less VerifiedCommit, falls through to a real verification and returns exactly the error VerifyCommit would — internal/statesync depends on that typed-error distinction to decide whether to disconnect a peer.

Marshalling/hashing what the proof covers rather than relying on Commit.Hash() is deliberate: that hash only covers ThresholdBlockSignature, so it would not notice a different Round or different vote extensions.

A VerifiedCommit cannot be forged from outside types. Only VerifyCommitSignatures attaches a proof, and only after the signatures verified; the zero value — the only proof-less VerifiedCommit any other package can construct — always falls through. VerifiedCommit.Commit() returns the commit it was minted with but is documented as not evidence: callers must never validate it in place of the commit they actually received.

Why the round-state check is gated rather than removed

ValidateBlockWithRoundState calls ValidateBlock first, which normally verifies LastCommit. But ValidateBlock short-circuits on blockExec.cache, which is keyed on the block hash alone and carries no promise about the state the earlier validation ran against. Deleting the second verification outright breaks TestStateProposalTime. Gating it on the proof instead makes the skip a property of the data rather than of the cache, so it holds regardless of which path populated the cache. The same reasoning now also covers ProcessProposal's own ValidateBlockWithRoundState call, added while resolving the v1.8-dev merge (see below).

Scope of the proof

The consensus path and the replayer never hold a proof — they always pass the zero VerifiedCommit, so every commit they see is fully re-verified. Only the block applier carries a real proof, one block forward, guarded by its own mutex and reset on UpdateState.

BlockExecutor.Copy does not carry any proof either, so the replayer always verifies fully.

Observability

The state_last_commit_verification_skipped counter is incremented on every skip. During block sync it tracks three skips per block after the first (ValidateBlock, ProcessProposal's check of the app response, and FinalizeBlock); in consensus it stays flat.

Detailed discussion of the merge with v1.8-dev

v1.8-dev moved forward with #1415, #1416 and #1426 while this branch was stacked, conflicting on types/validator_set.go and three files in internal/blocksync. Resolving it mechanically would have reintroduced two regressions that no conflict marker flagged:

• A silent extra BLS verification. fix(consensus): don't propose at historical heights after a bad block-sync handover #1416 split the applier's block-apply path into ProcessProposal(verify=true) → SaveBlock → FinalizeBlock. Taking either side of the conflict as-is leaves ProcessProposal's verify branch calling ValidateBlockWithRoundState with a hard-coded zero VerifiedCommit, so every synced block would threshold-verify its LastCommit once more even though the applier already held the proof — defeating the point of this PR. TestBlockApplierSkipsTheLastCommitItVerified could not have caught this: it counts skips, not full verifications, and the skip count it asserted (2, 4) stayed correct either way. Fixed by adding a lastCommit types.VerifiedCommit parameter to Executor.ProcessProposal itself, forwarded from the applier; the skip count is now 3 per block after the first, and the regression is pinned by first widening that test's expectation and watching it fail against the merge-only commit, then implementing the fix.
• A dropped typed error. fix(consensus): recover committed blocks across stale proposals and round changes #1415 added ErrInvalidCommitQuorumHash, returned from inside the very function the conflict marker straddled. Taking this branch's side of the marked hunk as-is silently drops it — consensus classifies peers by that error's type (state_try_add_commit.go, msg_handlers.go). Restored in commitSignData, outside the conflict marker, and pinned with an ErrorAs check in TestVerifiedCommitKeepsTheVerifyCommitErrors.

The merge is committed in three steps for reviewability: the textual merge resolution alone (still carrying the extra-verification regression, on purpose, for the next commit's test to catch), the ProcessProposal proof-threading fix, and a small trim of tests made redundant by this branch's own VerifiedCommit type (a duplicate skip-counter helper, and a subtest pinning "rejected commit yields no verification" that the type's zero value already guarantees by construction).

Diff-simplification was assessed deliberately (per request) rather than assumed: no leftover memo, CommitVerification or NewUnverifiedCommit scaffolding remains anywhere in the branch (confirmed by grep across the merged tree). The trims above are the only redundancy found; test coverage that looks like duplication on the surface (e.g. types/verified_commit_test.go pinning proof semantics input-by-input vs. internal/state/execution_test.go pinning the wiring) was deliberately kept, since each pins something the other doesn't.

Measurements

Replaying mainnet history from a local peer, 3,000-block window at height 190k, v1.7-dev against this branch, back to back:

	ms/block	verify stage	fb_validate
v1.7-dev	12.81	3.911 ms	1.977 ms
this branch	8.36	2.003 ms	0.010 ms

−34.7% off the block: one threshold verification per block instead of three.

Numbers taken with tenderdash's per-block fsync removed, because Go's File.Sync() on macOS issues F_FULLFSYNC (6.78 ms here versus 0.23 ms for a plain fsync) and would otherwise swamp everything else. Linux nodes do not pay that. They predate the memo being replaced by the typed VerifiedCommit and the v1.8-dev merge; the verification work per block is unchanged by either.

Tests

• types/verified_commit_test.go: the proof matches only an exact commit/chain/height/blockID/quorum-type/quorum-hash/key, rejects/falls through on distinct kinds of mismatch (now including a dedicated wrong-quorum-hash case asserting ErrInvalidCommitQuorumHash via ErrorAs), survives mutation of its inputs after construction, a proof-less VerifiedCommit never skips even when it holds the exact commit being verified, and the fall-through path returns exactly the same error VerifyCommit would.
• internal/blocksync/applier_test.go: TestBlockApplierSkipsTheLastCommitItVerified drives three real blocks through sm.BlockExecutor and asserts LastCommitVerificationSkipped reaches 0/3/6 after heights 1/2/3 (three sites per block after the first: ValidateBlock, ProcessProposal's app-response check, FinalizeBlock) — mutation-tested by temporarily widening the expectation and watching it fail against the merge-only commit before the ProcessProposal fix landed, then pass after. TestBlockApplierOffersTheVerifiedCommitForward pins that the forwarded proof actually reaches ProcessProposal (none on the first height, the real proof on the next).
• internal/state/execution_test.go: TestApplyBlockSkipsVerifiedLastCommit covers the ApplyBlock flow specifically (stays at 2 skips — ApplyBlock always calls ProcessProposal(verify=false)), documented as such now that block sync no longer calls ApplyBlock at all.
• Mocks (internal/state/mocks/executor.go) regenerated via mockery (v3.7.4, go:generate), not hand-edited.
• go build ./..., gofmt -l and golangci-lint run --new-from-rev=origin/v1.8-dev: clean. go test -race -count=1 ./types/... ./internal/state/... ./internal/blocksync/... ./dash/quorum/... ./node/...: all green. go test -count=1 ./internal/consensus/... (full, unfiltered): green, 67.4s.

Breaking Changes

None externally. state.Executor (internal interface): ProcessProposal gains a lastCommit types.VerifiedCommit parameter — internal only, all implementations/mocks updated in this PR.

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: Advanced

Run ID: 7f9f8549-ccea-4260-b7b1-0aa860714e88

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 ~1.6 h (commit c3c078f)
Estimated review time once started: ~20 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]

Final validation — Sol-only technical fallback

The focused state and blocksync test suites pass, and the memo comparison pins the inputs used by commit verification. However, the central verify-once invariant is not achieved because the round-state path still performs an unconditional threshold verification, and the new memo behavior has no direct tests.

Source: reviewer 1: gpt-5.6-sol (agent: sol-fallback-reviewer, role: general); reviewer 2: gpt-5.6-sol (agent: sol-fallback-reviewer, role: tenderdash-consensus-security); final verifier: gpt-5.6-sol (agent: sol-verifier, role: final-verifier)

One or more required Phase-1 GLM Flash lanes remained technically unusable after the bounded exact-model retry. Their evidence was discarded as authoritative, and the complete selected role cohort was rerun fresh on exact gpt-5.6-sol before this fresh Sol verifier produced the final decision. No additional Phase-2 reviewer pass ran.

Review provenance

• Phase 1 GLM evidence: technically unusable after bounded retry; discarded from the decision
• GLM failure attempts: codex-general-b08d2fd95a904ff9acd5767ef234613b (failed), codex-general-2d5f72ff201a4dd38564c3248fe2a37a (failed), codex-tenderdash-consensus-security-8cbd3a5f5ded45549161c9033cd7ce02 (failed), codex-tenderdash-consensus-security-631883b3da9a49b6a32f7ec6e45fb4cb (failed)
• Sol-only fallback reasons: launch_transport_or_nonzero_exit, launch_transport_or_nonzero_exit
• Sol-only fallback reviewers: gpt-5.6-sol — general (completed); agent sol-fallback-reviewer, gpt-5.6-sol — tenderdash-consensus-security (completed); agent sol-fallback-reviewer
• Fresh verifier (Sol): gpt-5.6-sol — final-verifier; agent sol-verifier
• Additional Phase 2 pass: not run; the Sol-only fallback is final

🔴 1 blocking | 🟡 1 suggestion(s)

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/state/execution.go`:
- [BLOCKING] internal/state/execution.go:530-534: Round-state validation still repeats the threshold verification
  This unconditional `VerifyCommit` prevents the stated verify-once behavior. When block N is applied, `blockApplier.verify` verifies commit N. While applying block N+1, the memo skips commit N in the first `ValidateBlock` call, but `ApplyBlock` then reaches `FinalizeBlock` and `ValidateBlockWithRoundState`; its `ValidateBlock` call returns from the block cache, after which these lines threshold-verify commit N again. Each commit is therefore still verified twice over its lifecycle. Make this check honor the same full-match memo while preserving the real verification when no memo exists, so the consensus path remains unchanged.
- [SUGGESTION] internal/state/execution.go:448-499: Add behavioral coverage for the verified-commit memo
  No changed test directly exercises `NoteVerifiedCommit` or `lastCommitAlreadyVerified`; the blocksync changes only assert that the new method is called. Those mock expectations still pass while the round-state path performs the redundant verification, so they do not establish the PR's central performance invariant. Add tests for an exact memo match, mismatches in each pinned verification input, and an executor/applier flow proving that an already verified commit is not threshold-verified again while any mismatch falls back to real verification and rejects an invalid signature.

[PastaPastaPasta]

@thepastaclaw

• [BLOCKING] internal/state/execution.go:530-534: Round-state validation still repeats the threshold verification
  This unconditional VerifyCommit prevents the stated verify-once behavior. When block N is applied, blockApplier.verify verifies commit N. While applying block N+1, the memo skips commit N in the first ValidateBlock call, but ApplyBlock then reaches FinalizeBlock and ValidateBlockWithRoundState; its ValidateBlock call returns from the block cache, after which these lines threshold-verify commit N again. Each commit is therefore still verified twice over its lifecycle. Make this check honor the same full-match memo while preserving the real verification when no memo exists, so the consensus path remains unchanged.

This seems not blocking to me; and I believe there was a reason; and that handling this case was harder for some reason; or potentially unsafe.

[thepastaclaw]

You're right about the underlying reason. I traced both paths and the stacked follow-up:

• The ValidateBlockWithRoundState check is intentional/load-bearing, not something we can simply delete. Its leading ValidateBlock may return from blockExec.cache, and that cache is keyed only by block hash—not by the state/validator inputs against which the block was previously validated. Keeping a real VerifyCommit fallback here is necessary.
• perf(state): skip the second LastCommit verification when the memo proves it ran #1428 is the deliberately separate stacked change that gates this check on the new full-match memo. So the automated finding is correct that perf(blocksync): verify a commit once, not again as the next block's LastCommit #1427 alone still performs one repeat verification, but I agree it should not block perf(blocksync): verify a commit once, not again as the next block's LastCommit #1427 if we preserve the stacked scope.
• There is also a real safety issue with perf(state): skip the second LastCommit verification when the memo proves it ran #1428's current implementation: NoteVerifiedCommit snapshots only after ApplyBlock, while ProcessProposal/FinalizeBlock expose shallow aliases of the commit's byte slices to an in-process ABCI app. Those bytes can therefore change after blockApplier.verify succeeds but before the memo is recorded. perf(blocksync): verify a commit once, not again as the next block's LastCommit #1427 remains safe specifically because this second VerifyCommit is still present; perf(state): skip the second LastCommit verification when the memo proves it ran #1428 needs to capture an immutable snapshot immediately after successful verification before using the memo to bypass this fallback.

My recommendation is to downgrade this finding on #1427 to a scoped follow-up, keep the second check unchanged here, and fix the snapshot timing plus direct exact-match/mismatch tests in #1428. We should also reword #1427's title/body or explicitly link #1428, because “verify once” currently overstates what #1427 alone delivers (it reduces three verifications per block to two; the pair targets one).

[Claudius-Maginificent]

Six findings from a full grumpy-review pass, posted individually — see summary comment for the complete report and the two still-open items not posted here (test coverage gap, and an unresolved cross-PR disagreement on ABCI buffer aliasing with an external reviewer's finding on #1428).

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

[Claudius-Maginificent]

PR's "consensus is unaffected" claim is false — the memo is read by the consensus path at every blocksync-to-consensus handover.

The PR states "Consensus path never calls NoteVerifiedCommit, so this only affects blocksync." That's true for the write side only. The read side is reachable from consensus: internal/consensus/block_executor.go:103 -> ValidateBlockWithRoundState -> ValidateBlock -> this line. node/node.go constructs exactly one BlockExecutor and hands the same pointer to both consensus.NewState and blocksync.NewReactor, so a memo written by the block-sync applier survives the block-sync -> consensus handover and is read by the consensus path at height H+1, where H is the last block-synced height. This fires on the happy path at every handover, not a corner case.

No signature-verification bypass exists today — every input ValidatorSet.verifyCommit reads is pinned by the memo, so a match genuinely implies an identical prior verification. But the documented security boundary doesn't match the real one, and reviewers/maintainers/the stacked #1428 will reason from the wrong one.

Recommendation: narrow it structurally rather than just re-documenting it — clear the memo in blocksync.Reactor.poolRoutine immediately before SwitchToConsensus (internal/blocksync/reactor.go:296-301), so the memo cannot outlive the reactor that produced it. Smaller and more auditable than accepting the wider scope and documenting around it.

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

[PastaPastaPasta]

You are right that the read side is reachable from consensus for one height after the handover, and the PR description overstated it. I have rewritten that section rather than adding the clear-before-switch.

I pushed back on the structural fix because I do not think it buys anything. The skip fires only on a full match: same chain ID, height, block ID, quorum type, quorum hash, threshold key and a byte-identical marshalled commit, all captured as owned copies at the moment VerifyCommit succeeded. When consensus validates the first post-handover block and the entry matches, that is exactly the verification block sync already did, on exactly the same inputs. A skip that fires in consensus is no less safe than one that fires in block sync, so clearing the memo would cost one real verification per handover for no change in what is trusted. The description now says plainly that consensus can read the memo for that one height, and why that is fine.

Clearing it would also need a new interface method or a type assertion in poolRoutine, which is more surface than the property it protects.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[Claudius-Maginificent]

Not ready for review yet:

1. 1 unresolved bot-flagged review thread(s) [HIGH].

Please address and re-request review when ready.

[PastaPastaPasta]

Pushed 1deb0da, which reworks the memo in response to the reviews above:

• NoteVerifiedCommit is replaced by Executor.VerifyCommit, which performs the threshold verification itself and memoises only on success. The block sync applier verifies through it. Nothing else can write the memo.
• The memo holds the two most recent verified commits under a mutex, deep-copies every slice-backed input, and is consulted in validateBlock only after ValidateBasic.
• New state_last_commit_verification_skipped counter; memo encode failures are logged.
• Tests rewritten to drive the real executor with genuine BLS commits and assert on the skip counter.

Each inline thread has a reply. Six are resolved; the handover thread stays open with my reasoning for not clearing the memo before SwitchToConsensus. The PR description is updated to match the new design.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

Final validation — Phase 2 only (queue backlog)

Verified the supplied Phase-2 claims against the exact head and found no actionable in-scope defects: the memo owns its verification inputs, is populated only after successful verification, and both LastCommit checks retain real verification on a mismatch, including after consensus handover. Independently reran state and blocksync tests with the race detector and deadlock tag, plus consensus, node, and types tests with the deadlock tag; all passed using a revision-compatible local BLS build. The worktree remains unchanged; lint was not independently validated.

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) — This change skips BLS commit signature verification in block sync and consensus validation based on a shared memo, so errors in input matching, ownership, concurrency, or validator-set transitions could allow invalid commits to bypass consensus-critical checks.
• Phase 1 reviewers: not run (skipped for throughput: 39 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

[thepastaclaw]

Final validation — Phase 2 only (queue backlog)

The verified-commit memo records only successful verification, owns all slice-backed inputs, and requires a complete match before skipping either LastCommit verification; its reuse at consensus handover preserves that invariant. No in-scope defects were confirmed. State and blocksync tests passed with and without the race detector, and types, consensus, and node tests passed with the deadlock tag, using native BLS artifacts matching the pinned dependency.

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) — This change skips cryptographic commit verification in block sync and consensus validation based on a shared memo, so errors in input matching, ownership, concurrency, or cache interactions could admit invalid commits and compromise consensus safety.
• Phase 1 reviewers: not run (skipped for throughput: 18 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