fix(consensus): match full BlockID and require canonical part sets (CON-306) - #3845
fix(consensus): match full BlockID and require canonical part sets (CON-306)#3845wen-coding wants to merge 4 commits into
Conversation
Use header hash plus PartSetHeader for lock/POL/valid/finalize identity checks, and reject assembled gossip parts that are not MakePartSet of the decoded block so LastBlockID stays consistent across validators. Co-authored-by: Cursor <[email protected]>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3845 +/- ##
==========================================
- Coverage 61.52% 60.67% -0.86%
==========================================
Files 2360 2269 -91
Lines 199370 188925 -10445
==========================================
- Hits 122661 114625 -8036
+ Misses 65749 64192 -1557
+ Partials 10960 10108 -852
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sound, well-scoped tightening of consensus identity checks: all hash-only comparisons in state.go are consistently converted to full-BlockID matching, the canonical-MakePartSet gate correctly carves out the maj23/commit part-set retarget case, and the honest path is preserved (I traced enterNewRound's per-round reset, BlockFromProto→ValidateBasic rejecting omitted header hash fields, and protoutils.Scan tolerating unknown fields so the new check is genuinely what rejects the test's junk bytes). No blockers; findings are observability, a terminal-stall edge case after enterCommit, hot-path cost, and test-coverage gaps.
Findings: 0 blocking | 12 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test gap — round-trip canonicality is only exercised on tx-less blocks. Both new tests use
createProposalBlock/testBlock, neither of which carries txs, evidence, or a multi-signatureLastCommit. The new gate makesproto.Marshal(ToProto(BlockFromProto(bz))) == bza hard liveness requirement for every block, so a single non-round-trip-stable field (e.g. anEvidenceListvariant, or aCommitSigwithBlockIDFlagAbsentand zeroTimestamp) would make every node reject every block. Please add a table test that builds a block with several txs, at least one evidence item, and aLastCommitmixingBlockIDFlagCommit/BlockIDFlagAbsentsigs, and assertsverifyCanonicalProposalPartsacceptsMakePartSet(block)for each shape. - Test gap — the safety change in
state.gois only covered by unit tests of the helpers. There is no state-machine test asserting the new negative behavior: thatenterPrecommitdoes not relock,enterCommitdoes not moveLocked*→Proposal*, andtryFinalizeCommitdoes not finalize when the header hash matches but thePartSetHeaderdiffers. That divergence (two distinctBlockIDs sharing a header hash) is the exact scenario the PR exists to fix, and it is currently unverified end-to-end. - Rollout: worth stating the plan in the PR description.
non-app-hash-breakingis accurate for ABCI/AppHash, but this does change consensus voting behavior — an upgraded validator will prevote nil (and decline to setValidBlock/relock) in cases where a non-upgraded one prevotes the proposal. There is no version gate, so during a rolling validator upgrade the two binaries can vote differently at the same height. In practice this only manifests if a proposer emits non-canonical parts (honest proposers always useMakePartSet), so the exposure is small — but it is a consensus-behavior change, not a purely local one, and reviewers should know whether this ships coordinated or rolling. - Test hygiene: the junk-append construction (
0xba, 0x3e, 0x04, 'j','u','n','k') is duplicated betweennonCanonicalPartSetinblock_id_match_state_test.goand the inline block inTestNonCanonicalPartSetSameHeaderHash. Worth sharing one helper so the wire-encoding trick is documented and maintained in a single place. - Cursor's second-opinion pass produced no output —
cursor-review.mdis empty (Codex'scodex-review.mdreports no material issues). Treat this review and Codex's as the only completed passes. - I could not execute
go test ./internal/consensus/...orgofmt/goimportsin this environment (the sandbox declined those commands), so the PR's test-plan results and formatting compliance are unverified here and rest on CI. Static reading of the new files shows conventional grouping (stdlib / third-party / sei-tendermint) consistent withgoimports. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| logger.Error("Encountered error building block from parts", "block parts", cs.roundState.ProposalBlockParts()) | ||
| return false, err | ||
| } | ||
| if err := cs.verifyCanonicalProposalParts(block); err != nil { |
There was a problem hiding this comment.
[suggestion] Rejection here is silent apart from this Error log, and in one case it is terminal. Before enterCommit, recovery is fine — enterNewRound clears ProposalBlock/ProposalBlockParts for round > 0, so the node prevotes nil on timeout and moves on. But after enterCommit has run, enterNewRound is gated by RoundStepCommit <= step, so nothing ever resets the part set: ProposalBlockParts stays complete (no missing parts left to fetch), ProposalBlock stays nil, tryCreateProposalBlock re-derives the same failure every call, and there is no consensus→blocksync fallback (SwitchToBlockSync is only wired from state sync in node/node.go). The node stalls at that height permanently, and a restart re-fetches the same bytes. That state is only reachable if the network committed a non-canonical PartSetHeader (i.e. a mixed-version validator set), so it is not a blocker — but please add a dedicated counter/metric alongside the existing MarkBlockGossipComplete bookkeeping so the condition is alertable rather than showing up only as a log line on a wedged node. Also note the node keeps gossiping the rejected parts to peers while in this state.
| if parts == nil { | ||
| return fmt.Errorf("nil proposal block parts") | ||
| } | ||
| canonical, err := block.MakePartSet(types.BlockPartSizeBytes) |
There was a problem hiding this comment.
[suggestion] This re-marshals the whole block and rebuilds the full part set (split + Merkle root over every part) on the consensus hot path, once per completed block, purely to compare a root hash. getBlockFromBlockParts has already read the complete byte stream via io.ReadAll(parts.GetReader()). Threading those bytes out and doing bytes.Equal(bz, marshaled) against proto.Marshal(block.ToProto()) is the same check (a canonical part-set root is exactly a commitment to the byte stream) without the second Merkle tree, and it also yields a more actionable error. For a multi-MB block the saving is real, and this runs while cs.mtx is held.
| if !block.HashesTo(blockID.Hash) { | ||
| panic("cannot finalize commit; proposal block does not hash to commit hash") | ||
| if !blockIDMatches(block, blockParts, blockID) { | ||
| panic("cannot finalize commit; proposal block/parts do not match commit BlockID") |
There was a problem hiding this comment.
[suggestion] Merging the two panics loses post-mortem detail on a node-halting path: the old messages distinguished "parts header is not the commit header" from "block does not hash to the commit hash", and this one carries neither got nor want. Since tryFinalizeCommit now guards with the same predicate this panic should be unreachable, which makes it precisely the kind of panic you only ever read once, in a chain-halt incident. Suggest including the values, e.g. panic(fmt.Sprintf("cannot finalize commit; block/parts do not match commit BlockID: block=%X parts=%v commit=%v", block.Hash(), blockParts.Header(), blockID)).
|
|
||
| rs := cs1.GetRoundState() | ||
| require.NotNil(t, rs.Proposal, "proposal should still be accepted") | ||
| require.Nil(t, rs.ProposalBlock, "proposal block must not be accepted from non-canonical parts") |
There was a problem hiding this comment.
[suggestion] This asserts the outcome but not the cause, so the test would still pass if the parts were rejected earlier by AddPart's proof check, by protoutils.Scan, or by BlockFromProto's ValidateBasic — i.e. it can silently stop exercising verifyCanonicalProposalParts without failing. (I confirmed it does reach the new check today: runtime.Schema.scan skips fields with no rule, so unknown field 999 passes, and the junk does not perturb any hashed content.) Consider calling cs1.addProposalBlockPart directly for the final part and asserting the returned error mentions the canonical-MakePartSet mismatch, or asserting on verifyCanonicalProposalParts itself, so the test is pinned to the new behavior.
| // BlockID; comparing only the header hash would treat different part-set | ||
| // encodings as the same value. | ||
| func blockIDMatches(block *types.Block, parts *types.PartSet, blockID types.BlockID) bool { | ||
| if block == nil || parts == nil || blockID.IsNil() { |
There was a problem hiding this comment.
[nit] blockID.IsNil() only rejects a fully-zero BlockID (len(Hash) == 0 && PartSetHeader.IsZero()), so a half-populated one — hash set with a zero PartSetHeader, or vice versa — falls through to the comparison. That is harmless (the empty side fails HashesTo/HasHeader), but IsComplete() states the intended precondition directly: a 32-byte hash plus Total > 0 and a 32-byte part-set hash.
| if proposal == nil || locked == nil || proposalParts == nil || lockedParts == nil { | ||
| return false | ||
| } | ||
| return proposal.HashesTo(locked.Hash()) && proposalParts.HasHeader(lockedParts.Header()) |
There was a problem hiding this comment.
[nit] This duplicates blockIDMatches with the locked block's identity spelled out inline. It could delegate — blockIDMatches(proposal, proposalParts, types.BlockID{Hash: locked.Hash(), PartSetHeader: lockedParts.Header()}) — which keeps the two-place definition of "same consensus identity" in one place and preserves the nil guards. (Also a micro-nit on line 38: fmt.Errorf with no format directives is better as errors.New.)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I checked whether the nil-checks in blockIDMatches/proposalMatchesLocked (block_id_match.go) are dead/defensive code — they are not: LockedBlock()/LockedBlockParts()/ProposalBlock()/ProposalBlockParts() are plain field accessors on SafeRoundState that return nil pointers whenever a block/parts hasn't been locked or assembled yet (e.g. pre-lock rounds), so these checks guard real, reachable nil states rather than being redundant.
Extended reasoning...
This PR tightens consensus BlockID/part-set matching in Tendermint core (lock, POL, valid-block, and finalize-commit paths) — genuinely consensus-critical code where even a nit-level gap deserves human sign-off. One inline finding shows the new canonical-parts check was added to two of the three ProposalBlock-construction paths but omitted from the default (GossipTransactionKeyOnly) path, which is a real inconsistency in the fix even though it does not appear to break safety. Given the criticality of this code path and that a review already flagged a non-trivial gap, this should get a human look rather than a shadow approval.
wen-coding
changed the title
Address PR feedback with a sentinel reject error, marshaled-bytes check, reject metric, and stronger coverage of hash/PartSetHeader mismatch paths. Co-authored-by: Cursor <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ce36b46. Configure here.
There was a problem hiding this comment.
The change correctly generalizes the consensus identity checks from header-hash-only to full BlockID, and the state.go call-site conversions all look right, but the new canonical-parts check only compares protobuf bytes and therefore still accepts alternate chunk boundaries — so a malicious proposer can commit a BlockID whose PartSetHeader no longer matches MakePartSet, which permanently breaks block sync and light-client verification at that height. Confirming Codex's finding as a blocker; Cursor produced no output.
Findings: 2 blocking | 7 non-blocking | 3 posted inline
Blockers
cursor-review.mdis empty — the Cursor second-opinion pass produced no output, so this review reflects only the Codex pass plus my own analysis.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Test coverage gap: every new test builds a tiny block, so with
BlockPartSizeBytes = 1MBall part sets haveTotal() == 1.TestRejectNonCanonicalProposalBlockParts'sfor i := 0; i < Total()-1loop never iterates. Add at least one case with a >1MB block so multi-part assembly (GetReaderconcatenation across parts) is actually exercised by the canonical check, and one case with alternate chunk boundaries (see the blocker). - Byte-exact
Marshal(ToProto(BlockFromProto(bz))) == bzis now load-bearing on the consensus hot path. ReadingBlock.ToProto/BlockFromProto, the only nullable field isLastCommitand it round-trips symmetrically, so this looks safe — but a false positive here is unrecoverable (see the state.go:2181 comment), so it's worth empirically replaying a mainnet block range throughgetBlockFromBlockParts+verifyCanonicalProposalPartsbefore shipping. - This tightens consensus rules (prevote / relock / valid-block / finalize acceptance) with no version or height gate. Nodes on mixed versions can make different prevote/lock decisions for the same proposal. That's normal for sei-tendermint changes that land at a release boundary, but the PR body only claims "non-app-hash-breaking" — it's worth stating explicitly that this requires a coordinated validator upgrade.
- Perf:
verifyCanonicalProposalPartsadds a fullToProto+proto.Marshalof the block (up toMaxBlockSizeBytes) on every complete proposal assembly, plus retainingpartsBytes. Acceptable once per block, but note the recommendedMakePartSetfix would add Merkle hashing on top — the part-length check suggested inline avoids that. - No test covers the post-commit rejection path (parts retargeted by
enterCommit, assembly rejected), which is the case the newNonCanonicalProposalPartsmetric comment specifically calls out as alertable. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if err != nil { | ||
| return fmt.Errorf("proto.Marshal: %w", err) | ||
| } | ||
| if !bytes.Equal(partsBytes, canonical) { |
There was a problem hiding this comment.
[blocker] Confirming Codex's finding — this byte comparison does not make the part set canonical, so the PR's stated goal ("require canonical part sets") isn't met, and the comment on line 43-44 claiming this is "equivalent to matching MakePartSet's PartSetHeader, without rebuilding the Merkle tree" is false.
PartSet.AddPart (types/part_set.go:288) verifies only the Merkle proof against the header hash; Part.ValidateBasic rejects only parts larger than BlockPartSizeBytes. So the same canonical bytes can be split into any chunking up to MaxBlockPartsCount (= 101) parts, producing a different Merkle root and a different PartSetHeader, and this check passes.
Concrete failure: a malicious proposer signs BlockID{H, PSH'} where PSH' chunks the canonical bytes into e.g. 512KB parts. Every honest node builds NewPartSetFromHeader(PSH'), all proofs verify, the assembled bytes are canonical, this check passes → the block is prevoted, locked and committed under BlockID{H, PSH'}. Any node that later block-syncs that height recomputes first.MakePartSet(types.BlockPartSizeBytes) and verifies the next block's LastCommit against {H, PSH_canonical} (internal/blocksync/reactor.go:538-545) — the signatures were made over PSH', so VerifyCommitLight fails and new nodes can never sync past that height. Light clients break the same way.
Fix: compare against the real canonical header, e.g. canonicalParts, err := block.MakePartSet(types.BlockPartSizeBytes) then parts.HasHeader(canonicalParts.Header()). If you want to avoid re-hashing, the cheap equivalent is to keep the byte comparison and assert the chunking: parts.Total() == ceil(len(canonical)/BlockPartSizeBytes) and len(parts.GetPart(i).Bytes) == BlockPartSizeBytes for all but the last part. Same reasoning invalidates the comment on lines 69-70 ("only the header hash can still disagree"). Please add a test that splits canonical bytes on non-default boundaries.
| logger.Error("Encountered error building block from parts", "block parts", cs.roundState.ProposalBlockParts()) | ||
| return false, err | ||
| } | ||
| if err := cs.verifyCanonicalProposalParts(block, partsBytes); err != nil { |
There was a problem hiding this comment.
[suggestion] When this rejects, the part was already added and the part set is now complete, so AddPart returns false for everything afterwards and there is no path to re-request parts — the node is stuck for this height until the round advances.
That's fine in the propose/prevote case (round times out, prevote nil), but consider the commit catch-up case: enterCommit sets ProposalBlockParts = NewPartSetFromHeader(commitBlockID.PartSetHeader), the parts arrive and complete, and if this check fails the node can never finalize that height — and a restart re-downloads the same parts and fails identically. That's a permanent, restart-proof stall, which is presumably why the metric is labelled by step. Worth either resetting ProposalBlockParts to NewPartSetFromHeader(...) on rejection so a retry is possible, or documenting that a post-commit rejection is intentionally a halt.
Also a nit: added is true here but you return false, which skips cs.eventMsg(mi) peer bookkeeping at state.go:821. Other error paths in this function (e.g. the ByteSize check) return added, err.
| // Install canonical parts before verifyCanonical so proposal PartSetHeader | ||
| // gating and the bytes check both see the MakePartSet encoding. | ||
| cs.roundState.SetProposalBlockParts(newParts) | ||
| pbb, err := block.ToProto() |
There was a problem hiding this comment.
[suggestion] In this tx-key reconstruction path the canonical-bytes half of verifyCanonicalProposalParts is a tautology: partsBytes is proto.Marshal(block.ToProto()) and verifyCanonicalProposalParts immediately recomputes exactly that from the same block, so bytes.Equal always holds. The only check that does any work is block.HashesTo(proposal.BlockID.Hash) — and it's reachable only because line 2315 was moved up to install newParts. Consider replacing lines 2313-2333 with a direct if !block.HashesTo(proposal.BlockID.Hash) { ... return false }; that drops two full-block marshals from this path and makes the intent obvious.
Separately, if you keep the current shape, do the check before SetProposalBlockParts(newParts) on line 2315: on rejection the node is left holding a complete part set with ProposalBlock == nil, which the reactor will gossip to peers (harmless but wasted bandwidth) and which is the same unrecoverable-complete-part-set state noted at line 2181. TestTxKeyReconstructionRejectsProposalHashMismatch asserts ProposalBlock is nil but doesn't assert anything about the leftover parts.
There was a problem hiding this comment.
No new issues found this run. The two concerns I previously raised on this PR — the tx-key reconstruction path skipping verifyCanonicalProposalParts, and the removed "optimistic" comment — have both been addressed in the follow-up commit (state.go:2286, 2313-2334). This PR still touches core BFT safety logic (BlockID identity matching, panic conditions in finalizeCommit), so it's worth a human confirming the consensus semantics before merge.
Extended reasoning...
Overview
This PR tightens Tendermint consensus BlockID matching to compare the full BlockID (header hash + PartSetHeader) instead of header hash alone, across the lock/POL/valid/finalize-commit paths, and adds verifyCanonicalProposalParts to enforce that assembled proposal parts are the canonical protobuf encoding of the block. All three ProposalBlock-construction sites (gossip-complete, addProposalBlockPart, and tx-key reconstruction) now run this check after the latest commit.
Security risks
This is consensus safety-critical code. An incorrect BlockID/canonical-parts check could cause validators to diverge on which block they accept, is the primary risk category here (not memory-safety/injection). The new checks are additive restrictions (reject more, not accept more), which is the safer failure direction for a validator, and the PR's own reasoning about honest-node handling of non-canonical parts is currently backed by tests including one covering the tx-key path.
Level of scrutiny
High — this is core BFT consensus logic (prevote/precommit/finalize-commit paths and a node-halting panic), not a peripheral or config change. Even with no active bugs found by this run's bug hunt, changes of this nature (identity/equality semantics in the hot commit path) benefit from a human reviewer familiar with Tendermint consensus invariants confirming the edge cases (e.g., maj23/commit catch-up retargeting parts, the post-enterCommit stall scenario raised by an earlier reviewer) are acceptable for this release.
Other factors
The PR has good test coverage added for the new behavior (canonical/non-canonical part sets, stale PartSetHeader retargeting, tx-key hash-mismatch rejection), and prior review feedback (including my own) was incorporated in a follow-up commit. No outstanding review comments appear unaddressed at this point.
… gate Byte equality alone allowed alternate chunking of the same block bytes; compare against MakePartSet(BlockPartSizeBytes) instead, and do not reject assembly when Proposal.BlockID.Hash lies but parts are canonical. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
The consensus tightening itself looks correct — every changed call site's fall-through path is sound and all the new helpers are nil-safe — but the one test that is supposed to prove this PR's core safety assumption (decode → re-encode is byte-stable, so honest blocks still assemble) is tautological and asserts nothing, and the newly widened getBlockFromBlockParts signature returns bytes that both call sites discard.
Findings: 2 blocking | 6 non-blocking | 4 posted inline
Blockers
cursor-review.mdis empty — the Cursor second-opinion pass produced no output, so that review lane contributed nothing to this synthesis. Codex reported no material issues.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Safety/liveness trade-off is ungated and worth stating explicitly in the PR description: if ≥2/3 of validators ever precommit a
BlockIDwith a non-canonicalPartSetHeader(e.g. a mixed-binary window during rollout where the old majority accepts it),enterCommitretargetsProposalBlockPartsto that header,addProposalBlockPartrejects the assembly,ProposalBlockstays nil, and the upgraded node stalls at that height with no in-consensus recovery path — blocksync also rebuilds withMakePartSize, so it can't rescue it either. Fail-stop is almost certainly the right choice over divergence here, but since this is an ungated consensus-rule tightening (noapp/tagsupgrade gate), it deserves a sentence in the description so operators know what the new stall signature means. The newnon_canonical_proposal_parts{step}metric is the right hook for that alert — good addition. - The junk-suffix construction
append(append([]byte{}, bz...), 0xba, 0x3e, 0x04, 'j','u','n','k')is duplicated three times (block_id_match_test.go:103,block_id_match_test.go:173,block_id_match_state_test.go:25). ThenonCanonicalPartSethelper already exists inblock_id_match_state_test.go— bothblock_id_match_test.gosites could use it (or a sharedwithUnknownField(bz)helper). - Minor style:
verifyCanonicalProposalParts(block *types.Block)takes the block as an argument but reads the parts out ofcs.roundStateinternally. Both callers already hold the parts in scope; taking(block, parts)would make the function pure and directly unit-testable without aState. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| cs.roundState.SetProposal(nil) | ||
| cs.roundState.SetProposalBlockParts(parts) | ||
| require.NoError(t, cs.verifyCanonicalProposalParts(tc.block)) |
There was a problem hiding this comment.
[blocker] This assertion is a tautology, so TestCanonicalPartBytesRoundTripShapes does not test what its name claims.
parts on line 164 is tc.block.MakePartSet(types.BlockPartSizeBytes), and verifyCanonicalProposalParts internally computes block.MakePartSet(types.BlockPartSizeBytes) on the same tc.block and compares the two headers. Same deterministic function, same input — this can never fail, for any of the three shapes in the table.
The invariant the production code actually depends on is round-trip byte stability: the proposer marshals block B into parts P; the receiver assembles P, decodes to B' via getBlockFromBlockParts, and MakePartSet(B') must reproduce P.Header(). If ToProto(BlockFromProto(bz)) is ever not byte-identical to bz for some shape (nil-vs-empty repeated fields, NewCommitSigAbsent() zero timestamps, vote extensions, evidence), every node rejects an honest proposal and the chain stalls — exactly the failure the metric added in this PR is meant to alert on.
So the table should decode first:
bz, err := io.ReadAll(parts.GetReader())
require.NoError(t, err)
var pbb tmproto.Block
require.NoError(t, proto.Unmarshal(bz, &pbb))
decoded, err := types.BlockFromProto(&pbb)
require.NoError(t, err)
cs.roundState.SetProposalBlockParts(parts)
require.NoError(t, cs.verifyCanonicalProposalParts(decoded))That makes the with_txs and mixed_last_commit cases meaningful (the mixed_last_commit case with NewCommitSigAbsent() is the interesting one). Note the two state-level tests do exercise the real round trip end-to-end on a createProposalBlock() block, so the basic honest path is covered — it's the shape matrix that currently isn't.
| require.NoError(t, err) | ||
| junk := append(append([]byte{}, bz...), 0xba, 0x3e, 0x04, 'j', 'u', 'n', 'k') | ||
| cs.roundState.SetProposalBlockParts(types.NewPartSetFromData(junk, types.BlockPartSizeBytes)) | ||
| err = cs.verifyCanonicalProposalParts(tc.block) |
There was a problem hiding this comment.
[suggestion] Same table, negative case: this one does exercise a real difference (junk bytes vs. tc.block), so it passes for the right reason. But note that after the fix suggested above it would read more naturally as "decoded block vs. junk parts", matching the production shape.
| } | ||
|
|
||
| func (cs *State) getBlockFromBlockParts() (*types.Block, error) { | ||
| func (cs *State) getBlockFromBlockParts() (*types.Block, []byte, error) { |
There was a problem hiding this comment.
[suggestion] The new []byte return value is discarded at both call sites (2176 and 2248 both use block, _, err :=), and there are no other callers. As written this is signature churn with no consumer — either drop it, or use it.
Using it is probably the better move, because verifyCanonicalProposalParts currently re-marshals the entire block and rebuilds the full merkle tree over all parts (MakePartSet) on every complete assembly. That lands on the latency-sensitive path right before prevote, and for blocks near ConsensusParams.Block.MaxBytes it's a non-trivial allocation + hashing spike. Since you already have the assembled bz in hand, the same property can be established more cheaply:
canonicalBz, err := proto.Marshal(pbb) // or block.ToProto() + Marshal
if !bytes.Equal(bz, canonicalBz) { ... }
// plus a part-count/chunk-size check to reject non-default chunkingBytes-equal + default chunk size fully determines the PartSetHeader, so this is equivalent to the header comparison without re-merkling. It also yields a much better error message (byte offset of first divergence) than "got header X, want Y". If you'd rather keep MakePartSet for clarity, that's a defensible call — but then please drop the unused return value.
|
|
||
| // NonCanonicalProposalParts counts complete proposal assemblies rejected | ||
| // because the part bytes were not the canonical protobuf encoding of the | ||
| // decoded block (or the proposal BlockID hash mismatched). Labeled by the |
There was a problem hiding this comment.
[nit] This doc comment contradicts the implementation on two points:
- "or the proposal BlockID hash mismatched" —
verifyCanonicalProposalPartsdeliberately does not checkProposal.BlockID.Hash(its own doc comment says so, andTestAssembleDespiteProposalHashMismatchasserts the opposite behaviour). This counter can never be incremented for a hash mismatch. - "the part bytes were not the canonical protobuf encoding" — the check also fires for canonical bytes chunked at a non-default part size (
TestRejectNonDefaultPartChunking), which this wording excludes.
Suggest: "counts complete proposal assemblies rejected because the assembled PartSetHeader did not equal MakePartSet(block, BlockPartSizeBytes) — non-canonical encoding or non-default chunking."
…bytes Exercise MakePartSet round-trip on the decoded block, align the reject metric comment with MakePartSet semantics, and simplify getBlockFromBlockParts. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
sei-tendermint/internal/consensus/state.go:2173-2194— When commit-catch-up parts complete but fail the newverifyCanonicalProposalPartscheck (a +2/3 commit certificate for a non-canonicalPartSetHeader),ProposalBlockPartsis left complete andProposalBlockstays nil forever — no more parts are requested and, once a height reachesRoundStepCommit, no code path resets the part set again, so the node stalls at that height permanently and a restart re-fails identically. This only occurs during a mixed pre/post-fix rollout with a Byzantine or straggling supermajority (a fully-upgraded honest network can't produce such a certificate), so it isn't a merge blocker, but consider resettingProposalBlockPartson rejection (or explicitly documenting the halt as intentional) to give operators a recovery path.Extended reasoning...
verifyCanonicalProposalParts(block_id_match.go:46-62) is now called fromaddProposalBlockPart(state.go:2173-2194) and from theIsComplete()branch oftryCreateProposalBlock(state.go:2245-2267) whenever a proposal's assembled parts complete. If the completed part set is not the canonicalMakePartSet(BlockPartSizeBytes)chunking of the decoded block, it returnsErrNonCanonicalProposalParts, and both call sites bail out withProposalBlockleftnilwhileProposalBlockPartsis left in place, already marked complete.The path that makes this permanent is
enterCommit(state.go:1751-1783): when the node lacks the committed block, it setsProposalBlockParts = NewPartSetFromHeader(commitBlockID.PartSetHeader)and clearsProposalBlock, then transitions toRoundStepCommit. If the gossiped parts that later complete that exact header fail the canonical check,ProposalBlocknever gets set. BecauseAddPartalready reported the set complete, no further parts are ever requested (nothing is missing), andenterNewRoundonly resetsProposalBlock/ProposalBlockPartsforround > 0(state.go:1027-1037) — but a height that has enteredRoundStepCommitnever re-enters a round for that height; only the height itself can advance.tryFinalizeCommit(state.go:1800) then permanently failsblockIDMatchesagainst a nilProposalBlock. There is no consensus→blocksync fallback in this path, so the node halts at that height indefinitely, and because the peer keeps serving the identical non-canonical bytes, even a full process restart re-downloads them and fails the same way.This is a genuine regression relative to pre-PR behavior, which had no canonical-chunking check and would have finalized the block. I want to be fair to the strongest refutation here: reaching this state requires a +2/3 commit certificate for a non-canonical
PartSetHeader, which a fully-upgraded, honest post-PR network cannot produce, because every post-PR validator only setsProposalBlock(and therefore only precommits non-nil) afterverifyCanonicalProposalPartspasses. So the trigger is narrowed to a rolling-upgrade window where a supermajority is still on pre-fix binaries (which don't reject non-canonical parts) combined with a Byzantine or non-canonical-chunking proposer. In that scenario, arguably a fail-closed halt is the correct safety response — accepting the non-canonical block would just reintroduce the blocksync/light-client verification break this PR exists to prevent, and retrying with the same committed header would re-download identical bytes and fail identically regardless of any reset. That is a real, well-reasoned argument for why this may be intentional rather than a defect.Concrete walkthrough of the stall: (1) mixed-version network reaches a +2/3 precommit/commit certificate for
BlockID{H, PSH'}wherePSH'is a non-canonical chunking of the block's bytes (Byzantine proposer, or pre-fix validators that don't reject it); (2) a post-PR node that lacks the block entersenterCommit, setsProposalBlockParts = NewPartSetFromHeader(PSH'),ProposalBlock = nil, moves toRoundStepCommit; (3) peers gossip parts matchingPSH', the set completes,addProposalBlockPartcallsverifyCanonicalProposalParts, which rejects because the assembled bytes don't matchMakePartSet(BlockPartSizeBytes); (4)ProposalBlockstays nil,AddPartwill never accept another part for this already-complete set,enterNewRoundcannot fire again for this height/round, andtryFinalizeCommitfails on every subsequent call; (5) restarting the process re-syncs the samePSH'header and re-downloads the same non-canonical bytes, hitting the identical rejection.All three independent verifiers reproduced this exact mechanism against the code and converged on
nit(not blocking), and the PR author already added theNonCanonicalProposalPartsstep-labeled metric specifically so this condition is alertable — an explicit, accepted mitigation for exactly this scenario, as also requested by seidroid's inline review. Given the narrow, adversarial/mixed-version trigger, the plausible case that fail-closed is the intended safety property, and the existing alerting mitigation, this is worth a small follow-up (resetProposalBlockPartstoNewPartSetFromHeader(...)on rejection so a retry is at least possible, or explicitly document the halt as by-design) but should not block this PR.
There was a problem hiding this comment.
Well-motivated and internally coherent fix: comparing the full BlockID plus requiring canonical (MakePartSet(BlockPartSizeBytes)) part sets brings consensus acceptance in line with what blocksync already demands (internal/blocksync/reactor.go:538 builds the commit BlockID from MakePartSet), closing a real "commit a BlockID nobody can ever re-derive" hole. No blocking correctness or security issues found; remaining notes are hardening, a PR-description/code mismatch, one perf/repeated-work concern, and test-matrix gaps.
Findings: 0 blocking | 12 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only Claude's findings with Codex's single High finding. - PR description claims "proposal
BlockIDhash is only enforced when current parts still belong to that proposal" — no such enforcement exists in the diff, andTestAssembleDespiteProposalHashMismatchasserts the opposite. Please align the description with the code (or add the check). See inline note onblock_id_match.go:46. - New invariant worth documenting:
verifyCanonicalProposalPartsnow requiresMakePartSet(BlockFromProto(proto.Unmarshal(bytes)))to reproduce the sender'sPartSetHeaderbyte-for-byte. Thetmprotostructs have noXXX_unrecognizedfield, so unknown fields are dropped on decode — after this change, any future field added totmproto.Block/Header/Data/Commit/Evidencemakes older-binary nodes reject every proposal from a newer proposer (prevote nil, round stalls) rather than silently tolerate it. Blocksync already depended on this round-trip (blocksync/reactor.go:538), so it isn't a new requirement, but it now fails much earlier and more visibly. A line insei-tendermint/AGENTS.md(and/or a proto-compat test) would keep the next proto change from being an accidental consensus break. - Not gated on an upgrade/version tag, which looks right here (the honest
MakePartSetpath is unchanged, so patched and unpatched nodes only diverge when the round's proposer is Byzantine) — but it does mean patched nodes prevote nil in a round where unpatched nodes would prevote a non-canonical block. Worth an explicit note in the PR body since it is a change to vote-acceptance rules rather than a pure bug fix. - Consistency check came out clean:
grep HashesTo(ininternal/consensusnow only matches insideblockIDMatches, so no identity comparison in the package was left on header-hash-only. - Minor duplication: the "append unknown field 999" byte sequence is inlined in
block_id_match_test.goas well as in thenonCanonicalPartSethelper inblock_id_match_state_test.go; a shared helper returning both the raw bytes and the part set would avoid the copy. - I could not compile or run the consensus package in this environment (sandboxed), so the test assertions and helper signatures were verified by reading only; CI's consensus shard is the real gate. Note the PR's own test plan still has the CI consensus shard unchecked.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // here: a mismatched proposal hash must not block later maj23/commit catch-up | ||
| // that retargets the same PartSetHeader, and votes already commit to | ||
| // ProposalBlock.Hash() + parts.Header(). | ||
| func (cs *State) verifyCanonicalProposalParts(block *types.Block) error { |
There was a problem hiding this comment.
[suggestion] Codex flags this as High: Proposal.BlockID.Hash is never validated against the assembled block, so a proposer can sign {Hash: A, PartSetHeader: P_B} and honest nodes will assemble and prevote block B.
I don't think it's a safety bug: the signed PartSetHeader cryptographically binds the part bytes, every node derives the same B from those bytes, and votes commit to ProposalBlock.Hash() + parts.Header(), so there is no divergence path — B is the block the proposal actually committed to.
But it is a real gap given this PR's premise ("consensus identity is the full BlockID"), and the PR description explicitly claims the check exists: "proposal BlockID hash is only enforced when current parts still belong to that proposal." Nothing in the diff does that, and TestAssembleDespiteProposalHashMismatch enshrines the opposite. Two consequences of the unbound hash today: defaultSetProposal's commit-catch-up filter (!proposal.BlockID.Equals(blockID), state.go ~2078) silently drops a proposal whose parts are correct but whose hash lies, and the fork's Proposal.Header (which fully determines the block hash) is never checked against BlockID.Hash either.
Suggestion: implement what the description promises — when parts.HasHeader(cs.roundState.Proposal().BlockID.PartSetHeader) (i.e. parts still belong to this proposal), also require block.HashesTo(Proposal.BlockID.Hash). That keeps the maj23/commit retarget path unaffected while making a lying proposer's round fail fast. If you'd rather keep current behavior, please fix the PR description and this doc comment instead.
| logger.Error("Encountered error building block from parts", "block parts", cs.roundState.ProposalBlockParts()) | ||
| return false, err | ||
| } | ||
| if err := cs.verifyCanonicalProposalParts(block); err != nil { |
There was a problem hiding this comment.
[suggestion] Perf on the critical path: verifyCanonicalProposalParts re-marshals the whole block (block.ToProto() + proto.Marshal) and then SHA-256s every part plus the Merkle root, immediately before prevote, for every completed assembly. For Sei's block sizes and sub-second targets that's a non-trivial addition to the prevote latency you already track.
getBlockFromBlockParts has the assembled bz in hand a few lines up. Threading it through and comparing proto.Marshal(block.ToProto()) against bz (plus parts.Total() == expected chunk count for BlockPartSizeBytes, to still reject the non-default-chunking case in TestRejectNonDefaultPartChunking) gets the same guarantee without the hashing pass. Optional, but cheap to do while the code is fresh.
| logger.Error("Encountered error building block from parts", "block parts", cs.roundState.ProposalBlockParts()) | ||
| return false | ||
| } | ||
| if err := cs.verifyCanonicalProposalParts(block); err != nil { |
There was a problem hiding this comment.
[suggestion] Repeated-work amplification: on rejection, ProposalBlockParts stays complete while ProposalBlock stays nil. defaultSetProposal returns nil (not an error) for an already-set proposal, so handleMsg calls tryCreateProposalBlock again for every duplicate ProposalMessage a peer sends for that round — each one redoing a full proto.Unmarshal in getBlockFromBlockParts plus a full MakePartSet, and each one emitting another logger.Error and another metric increment. Before this change the first assembly set ProposalBlock and all later duplicates short-circuited at the ProposalBlock() != nil guard.
It takes a Byzantine proposer to enter this state, and an analogous loop already exists when getBlockFromBlockParts itself fails, so this isn't new in kind — but it's now reachable with well-formed, decodable parts. Caching the rejected PartSetHeader for the round and short-circuiting on it would bound the work (and make the metric count distinct rejected part sets rather than rejection events, which is probably what you want for alerting).
| }, | ||
| } | ||
|
|
||
| cases := []struct { |
There was a problem hiding this comment.
[suggestion] The round-trip matrix is the most valuable test here — it's what guards against MakePartSet(decode(bytes)) drifting from the proposer's header and making every node prevote nil. Two gaps worth closing:
- No case with
Evidence.EvidenceList→ToProto/FromProtois the most normalization-prone path in the block (e.g.ValidatorSet.ToProtodeliberately zeroesTotalVotingPower"to be consistent with cached data", validator_set.go:859), and after this PR a lossy round-trip there stalls consensus at the first evidence-bearing block instead of failing later. ADuplicateVoteEvidence(and ideally aLightClientAttackEvidence) case would pin it. - No positive multi-part case.
TestRejectNonDefaultPartChunkingcovers multi-part rejection, but every accepted case here fits in a singleBlockPartSizeBytespart; a block with enough txs to span >1 canonical part would exercise the concatenation/reader path you actually depend on.
| } | ||
| if err := cs.verifyCanonicalProposalParts(block); err != nil { | ||
| Global.NonCanonicalProposalPartsAt(cs.roundState.Step().String()).Add(1) | ||
| logger.Error( |
There was a problem hiding this comment.
[nit] logger.Error here is peer-triggerable, and handleMsg logs a second "failed to process message" error for the same event (state.go:867). Consider Warn for one of the two to keep a Byzantine proposer from generating paired ERROR lines per block-part message; the new NonCanonicalProposalParts metric already gives operators the alertable signal.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
No new bugs found this round, but this PR rewrites core consensus safety checks (BlockID matching, part-set canonicalization, and finalizeCommit's panic conditions), so it warrants a human review rather than auto-approval.
What was reviewed this round:
verifyCanonicalProposalParts: confirmed it rebuilds the real Merkle tree viablock.MakePartSet, not a byte comparison — resolves the earlier blocker where same-bytes-different-chunking could produce a divergent PartSetHeader.- Confirmed neither the gossip path nor the tx-key-reconstruction path in
tryCreateProposalBlockchecksproposal.BlockID.Hashanymore (the hash gate was intentionally dropped), so the two paths behave consistently, and votes are still signed against the real reconstructedProposalBlock.Hash(), not the proposer's claimed hash. - Confirmed the
NonCanonicalProposalPartsmetric doc comment no longer contains the stale "(or the proposal BlockID hash mismatched)" wording flagged in an earlier comment.
Extended reasoning...
Overview
This PR changes core Tendermint consensus identity/acceptance logic in sei-tendermint/internal/consensus: blockIDMatches/proposalMatchesLocked now compare full BlockID (hash + PartSetHeader) instead of header hash alone across the lock/POL/valid/finalize paths in state.go, and a new verifyCanonicalProposalParts check rejects assembled proposal parts whose PartSetHeader doesn't match block.MakePartSet(BlockPartSizeBytes). finalizeCommit's panic conditions were also consolidated. New metrics and tests were added for the rejection path.
Security risks
No injection/auth-style risk, but this is exactly the kind of change where a subtle logic error causes a chain halt or a validator-set split (some nodes accept a block/commit that others reject). The PR's own review history bears this out: across its revisions, reviewers (cursor bugbot, seidroid, and a prior run of this same reviewer) found and the author fixed a high-severity commit-catch-up stall, a blocker where non-canonical chunking could slip through a byte-only comparison, and a tautological test. The current diff reflects fixes for all of these, but the density of real findings in a single PR is itself a signal this deserves a careful human pass, not just automated sign-off.
Level of scrutiny
High. This is consensus safety/liveness code (BlockID matching, part-set canonicality, and the panic conditions in finalizeCommit), not a peripheral or config-only change. Per the repo's own review guidance this isn't a case of an unregistered-upgrade-tag false positive — the concerns raised here were concrete logic issues in the gate itself, several of which were confirmed and fixed.
Other factors
The PR has already been through multiple rounds of bot and prior-agent review with blocker/high findings, all of which appear addressed in the current diff (verified by reading the current block_id_match.go and state.go directly rather than relying on the timeline). New unit/state tests were added for the new rejection paths, and reviewers also flagged residual test-quality nits (e.g., a still-somewhat-indirect assertion style) that are minor and non-blocking. Given the criticality of the code path and the volume of substantive fixes already made mid-review, this is a good candidate for a final human read-through before merge rather than auto-approval.
1 participant
Summary
BlockID(header hash andPartSetHeader), not header hash alone.MakePartSet(block, BlockPartSizeBytes).Header()— same encoding and default chunking that blocksync remakes. A mismatchedProposal.BlockID.Hashdoes not block assembly when parts are otherwise canonical, so maj23/commit catch-up that reuses the samePartSetHeadercan still finalize; votes continue to bind toProposalBlock.Hash()+ parts header.PartSetHeader, and assembly whenProposal.BlockID.Hashdiffers from the assembled block hash.non-app-hash-breaking: Does not change ABCI execution or
AppHashcomputation. Logical block contents for theMakePartSetpath are unchanged; this only tightens Tendermint BlockID / part-set acceptance rules.Test plan
GOWORK=off go test ./internal/consensus/ -run 'TestBlockIDMatches|TestProposalMatchesLocked|TestNonCanonicalPartSet|TestCanonicalPartBytes|TestRejectNonDefault|TestRejectNonCanonical|TestAcceptCanonicalParts|TestEnterPrecommitDoesNotRelock|TestAssembleDespite'Made with Cursor