fix(consensus): match full BlockID and require canonical part sets (CON-306) #3845
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| package consensus | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
|
|
||
| "github.com/sei-protocol/sei-chain/sei-tendermint/types" | ||
| ) | ||
|
|
||
| // ErrNonCanonicalProposalParts is returned when assembled proposal parts do not | ||
| // use the default BlockPartSizeBytes chunking of the block's canonical | ||
| // protobuf encoding (i.e. when parts.Header() differs from MakePartSet). | ||
| var ErrNonCanonicalProposalParts = errors.New("non-canonical proposal parts") | ||
|
|
||
| // blockIDMatches reports whether block and parts together match the consensus | ||
| // BlockID (header hash and PartSetHeader). Consensus identity is the full | ||
| // 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.IsComplete() { | ||
| return false | ||
| } | ||
| return block.HashesTo(blockID.Hash) && parts.HasHeader(blockID.PartSetHeader) | ||
| } | ||
|
|
||
| // proposalMatchesLocked reports whether the proposal block and parts equal the | ||
| // locked block identity (header hash and PartSetHeader). | ||
| func proposalMatchesLocked(proposal, locked *types.Block, proposalParts, lockedParts *types.PartSet) bool { | ||
| if locked == nil || lockedParts == nil { | ||
| return false | ||
| } | ||
| return blockIDMatches(proposal, proposalParts, types.BlockID{ | ||
| Hash: locked.Hash(), | ||
| PartSetHeader: lockedParts.Header(), | ||
| }) | ||
| } | ||
|
|
||
| // verifyCanonicalProposalParts ensures ProposalBlockParts match | ||
| // block.MakePartSet(BlockPartSizeBytes). Parts that carry the same logical | ||
| // block bytes under a different chunk size produce a different PartSetHeader; | ||
| // those must be rejected so commit/blocksync (which rebuild with | ||
| // BlockPartSizeBytes) stay consistent. Proposal.BlockID.Hash is not checked | ||
| // 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 { | ||
| parts := cs.roundState.ProposalBlockParts() | ||
|
There was a problem hiding this comment. [nit] Half-and-half signature: the block comes in as a parameter but the parts are read out of Relatedly, the |
||
| if parts == nil { | ||
| return errors.New("nil proposal block parts") | ||
| } | ||
| canonicalParts, err := block.MakePartSet(types.BlockPartSizeBytes) | ||
|
There was a problem hiding this comment. [suggestion] A cheaper equivalent: have the assembly path keep the |
||
| if err != nil { | ||
| return fmt.Errorf("MakePartSet: %w", err) | ||
| } | ||
| if !parts.HasHeader(canonicalParts.Header()) { | ||
| return fmt.Errorf( | ||
| "%w: PartSetHeader got %v, want canonical %v", | ||
| ErrNonCanonicalProposalParts, parts.Header(), canonicalParts.Header(), | ||
| ) | ||
| } | ||
| return nil | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,271 @@ | ||
| package consensus | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "testing" | ||
|
|
||
| "github.com/gogo/protobuf/proto" | ||
|
|
||
| tmconfig "github.com/sei-protocol/sei-chain/sei-tendermint/config" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/crypto" | ||
| tmtime "github.com/sei-protocol/sei-chain/sei-tendermint/libs/time" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/require" | ||
| tmproto "github.com/sei-protocol/sei-chain/sei-tendermint/proto/tendermint/types" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/types" | ||
| ) | ||
|
|
||
| func nonCanonicalPartSet(t *testing.T, block *types.Block) *types.PartSet { | ||
| t.Helper() | ||
| pbb, err := block.ToProto() | ||
| require.NoError(t, err) | ||
| bz, err := proto.Marshal(pbb) | ||
| require.NoError(t, err) | ||
| // Unknown field 999, length-delimited "junk". | ||
| nonCanonical := append(append([]byte{}, bz...), 0xba, 0x3e, 0x04, 'j', 'u', 'n', 'k') | ||
| parts := types.NewPartSetFromData(nonCanonical, types.BlockPartSizeBytes) | ||
| canonical, err := block.MakePartSet(types.BlockPartSizeBytes) | ||
| require.NoError(t, err) | ||
| require.False(t, parts.Header().Equals(canonical.Header())) | ||
| return parts | ||
| } | ||
|
|
||
| func TestRejectNonCanonicalProposalBlockParts(t *testing.T) { | ||
| config := configSetup(t) | ||
| chainID := tmconfig.TestLoadGenesis(config).ChainID | ||
| ctx := t.Context() | ||
|
|
||
| cs1, vss := makeState(ctx, t, makeStateArgs{config: config, validators: 2}) | ||
| height, round := cs1.roundState.Height(), cs1.roundState.Round() | ||
| round++ | ||
| incrementRound(vss[1:]...) | ||
|
|
||
| propBlock, err := cs1.createProposalBlock(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| nonCanonicalParts := nonCanonicalPartSet(t, propBlock) | ||
| blockID := types.BlockID{Hash: propBlock.Hash(), PartSetHeader: nonCanonicalParts.Header()} | ||
| pubKey, err := vss[1].PrivValidator.GetPubKey(ctx) | ||
| require.NoError(t, err) | ||
| proposal := types.NewProposal( | ||
| height, round, -1, blockID, propBlock.Time, | ||
| propBlock.GetTxHashes(), propBlock.Header, propBlock.LastCommit, propBlock.Evidence, pubKey.Address(), | ||
| ) | ||
| p := proposal.ToProto() | ||
| require.NoError(t, vss[1].SignProposal(ctx, chainID, p)) | ||
| proposal.Signature = utils.OrPanic1(crypto.SigFromBytes(p.Signature)) | ||
|
|
||
| cs1.startTestRound(ctx, height, round) | ||
| peerID, err := types.NewNodeID("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA") | ||
| require.NoError(t, err) | ||
|
|
||
| cs1.handleMsg(ctx, msgInfo{&ProposalMessage{proposal}, peerID, tmtime.Now()}, false) | ||
| require.Greater(t, nonCanonicalParts.Total(), uint32(0)) | ||
|
|
||
| // Deliver all but the last part without completing. | ||
| for i := 0; i < int(nonCanonicalParts.Total())-1; i++ { | ||
| cs1.handleMsg(ctx, msgInfo{ | ||
| &BlockPartMessage{Height: height, Round: round, Part: nonCanonicalParts.GetPart(i)}, | ||
| peerID, | ||
| tmtime.Now(), | ||
| }, false) | ||
| } | ||
|
|
||
| cs1.mtx.Lock() | ||
| added, err := cs1.addProposalBlockPart(&BlockPartMessage{ | ||
| Height: height, | ||
| Round: round, | ||
| Part: nonCanonicalParts.GetPart(int(nonCanonicalParts.Total()) - 1), | ||
| }, peerID) | ||
| cs1.mtx.Unlock() | ||
|
|
||
| require.False(t, added) | ||
| require.ErrorIs(t, err, ErrNonCanonicalProposalParts) | ||
| require.Nil(t, cs1.GetRoundState().ProposalBlock, "proposal block must not be accepted from non-canonical parts") | ||
| } | ||
|
|
||
| // Commit/maj23 catch-up can retarget ProposalBlockParts to a certificate | ||
| // PartSetHeader while leaving the original Proposal in place. Assembling | ||
| // canonical parts for that certificate must succeed even when the proposal's | ||
| // BlockID.PartSetHeader differs. | ||
| func TestAcceptCanonicalPartsWhenProposalPartSetHeaderDiffers(t *testing.T) { | ||
| config := configSetup(t) | ||
| chainID := tmconfig.TestLoadGenesis(config).ChainID | ||
| ctx := t.Context() | ||
|
|
||
| cs1, vss := makeState(ctx, t, makeStateArgs{config: config, validators: 2}) | ||
| height, round := cs1.roundState.Height(), cs1.roundState.Round() | ||
| round++ | ||
| incrementRound(vss[1:]...) | ||
|
|
||
| propBlock, err := cs1.createProposalBlock(ctx) | ||
| require.NoError(t, err) | ||
| canonicalParts, err := propBlock.MakePartSet(types.BlockPartSizeBytes) | ||
| require.NoError(t, err) | ||
|
|
||
| staleHeader := nonCanonicalPartSet(t, propBlock).Header() | ||
| require.False(t, staleHeader.Equals(canonicalParts.Header())) | ||
|
|
||
| // Proposal still claims the stale PartSetHeader (as after a mismatched earlier propose). | ||
| blockID := types.BlockID{Hash: propBlock.Hash(), PartSetHeader: staleHeader} | ||
| pubKey, err := vss[1].PrivValidator.GetPubKey(ctx) | ||
| require.NoError(t, err) | ||
| proposal := types.NewProposal( | ||
| height, round, -1, blockID, propBlock.Time, | ||
| propBlock.GetTxHashes(), propBlock.Header, propBlock.LastCommit, propBlock.Evidence, pubKey.Address(), | ||
| ) | ||
| p := proposal.ToProto() | ||
| require.NoError(t, vss[1].SignProposal(ctx, chainID, p)) | ||
| proposal.Signature = utils.OrPanic1(crypto.SigFromBytes(p.Signature)) | ||
|
|
||
| cs1.startTestRound(ctx, height, round) | ||
| peerID, err := types.NewNodeID("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA") | ||
| require.NoError(t, err) | ||
|
|
||
| cs1.handleMsg(ctx, msgInfo{&ProposalMessage{proposal}, peerID, tmtime.Now()}, false) | ||
|
|
||
| // Retarget parts to the certificate/canonical header, as enterCommit does | ||
| // when commit BlockID.PartSetHeader differs from the proposal. | ||
| cs1.mtx.Lock() | ||
| cs1.roundState.SetProposalBlock(nil) | ||
| cs1.roundState.SetProposalBlockParts(types.NewPartSetFromHeader(canonicalParts.Header())) | ||
| cs1.mtx.Unlock() | ||
|
|
||
| for i := 0; i < int(canonicalParts.Total()); i++ { | ||
| cs1.handleMsg(ctx, msgInfo{ | ||
| &BlockPartMessage{Height: height, Round: round, Part: canonicalParts.GetPart(i)}, | ||
| peerID, | ||
| tmtime.Now(), | ||
| }, false) | ||
| } | ||
|
|
||
| rs := cs1.GetRoundState() | ||
| require.NotNil(t, rs.Proposal, "original proposal should remain") | ||
| require.False(t, rs.Proposal.BlockID.PartSetHeader.Equals(canonicalParts.Header())) | ||
| require.NotNil(t, rs.ProposalBlock, "canonical certificate parts must be accepted") | ||
| require.True(t, rs.ProposalBlock.HashesTo(propBlock.Hash())) | ||
| require.True(t, rs.ProposalBlockParts.HasHeader(canonicalParts.Header())) | ||
| } | ||
|
|
||
| func TestEnterPrecommitDoesNotRelockOnPartSetMismatch(t *testing.T) { | ||
| config := configSetup(t) | ||
| chainID := tmconfig.TestLoadGenesis(config).ChainID | ||
| ctx := t.Context() | ||
|
|
||
| cs1, vss := makeState(ctx, t, makeStateArgs{config: config, validators: 4}) | ||
| height, round := cs1.roundState.Height(), cs1.roundState.Round() | ||
| voteCh := cs1.subscribeToVoterBuffered(ctx, t, cs1.address(ctx)) | ||
|
|
||
| propBlock, err := cs1.createProposalBlock(ctx) | ||
| require.NoError(t, err) | ||
| propBlockParts, err := propBlock.MakePartSet(types.BlockPartSizeBytes) | ||
| require.NoError(t, err) | ||
| blockID := types.BlockID{Hash: propBlock.Hash(), PartSetHeader: propBlockParts.Header()} | ||
|
|
||
| pubKey, err := vss[0].PrivValidator.GetPubKey(ctx) | ||
| require.NoError(t, err) | ||
| proposal := types.NewProposal( | ||
| height, round, -1, blockID, propBlock.Time, | ||
| propBlock.GetTxHashes(), propBlock.Header, propBlock.LastCommit, propBlock.Evidence, pubKey.Address(), | ||
| ) | ||
| p := proposal.ToProto() | ||
| require.NoError(t, vss[0].SignProposal(ctx, chainID, p)) | ||
| proposal.Signature = utils.OrPanic1(crypto.SigFromBytes(p.Signature)) | ||
|
|
||
| cs1.startTestRound(ctx, height, round) | ||
| peerID, err := types.NewNodeID("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA") | ||
| require.NoError(t, err) | ||
|
|
||
| cs1.handleMsg(ctx, msgInfo{&ProposalMessage{proposal}, peerID, tmtime.Now()}, false) | ||
| for i := 0; i < int(propBlockParts.Total()); i++ { | ||
| cs1.handleMsg(ctx, msgInfo{ | ||
| &BlockPartMessage{Height: height, Round: round, Part: propBlockParts.GetPart(i)}, | ||
| peerID, | ||
| tmtime.Now(), | ||
| }, false) | ||
| } | ||
| ensurePrevote(t, voteCh, height, round) | ||
|
|
||
| cs1.mtx.Lock() | ||
| cs1.roundState.SetLockedRound(round) | ||
| cs1.roundState.SetLockedBlock(propBlock) | ||
| cs1.roundState.SetLockedBlockParts(propBlockParts) | ||
|
|
||
| mismatchedID := types.BlockID{ | ||
| Hash: propBlock.Hash(), | ||
| PartSetHeader: types.PartSetHeader{ | ||
| Total: propBlockParts.Total(), | ||
| Hash: crypto.CRandBytes(32), | ||
| }, | ||
| } | ||
| require.False(t, blockIDMatches(propBlock, propBlockParts, mismatchedID)) | ||
|
|
||
| // Inject maj23 directly so tryAddVote does not clear ProposalBlock before | ||
| // enterPrecommit; we want the lock/proposal BlockID match path. | ||
| for _, vs := range vss[1:] { | ||
| vote := signVote(ctx, t, vs, tmproto.PrevoteType, chainID, mismatchedID) | ||
| added, err := cs1.roundState.Votes().AddVote(vote, peerID) | ||
| require.NoError(t, err) | ||
| require.True(t, added) | ||
| } | ||
| require.NotNil(t, cs1.roundState.ProposalBlock()) | ||
| cs1.enterPrecommit(ctx, height, round, "test-partset-mismatch") | ||
| cs1.mtx.Unlock() | ||
|
|
||
| // Hash matches lock but PartSetHeader does not → precommit nil, remain locked. | ||
| ensurePrecommitMatch(t, voteCh, height, round, nil) | ||
| cs1.validatePrecommit(ctx, t, round, round, vss[0], nil, propBlock.Hash()) | ||
| } | ||
|
|
||
| // A proposal whose BlockID.Hash lies but whose PartSetHeader matches the | ||
| // canonical part set must still assemble. Rejecting here would leave a | ||
| // complete PartSet with ProposalBlock==nil and block later maj23/commit | ||
| // catch-up that reuses the same header (votes use ProposalBlock.Hash()). | ||
| func TestAssembleDespiteProposalHashMismatch(t *testing.T) { | ||
| config := configSetup(t) | ||
| chainID := tmconfig.TestLoadGenesis(config).ChainID | ||
| ctx := t.Context() | ||
|
|
||
| cs1, vss := makeState(ctx, t, makeStateArgs{config: config, validators: 2}) | ||
| height, round := cs1.roundState.Height(), cs1.roundState.Round() | ||
| round++ | ||
| incrementRound(vss[1:]...) | ||
|
|
||
| propBlock, err := cs1.createProposalBlock(ctx) | ||
| require.NoError(t, err) | ||
| canonicalParts, err := propBlock.MakePartSet(types.BlockPartSizeBytes) | ||
| require.NoError(t, err) | ||
|
|
||
| badHash := crypto.CRandBytes(32) | ||
| require.False(t, propBlock.HashesTo(badHash)) | ||
| blockID := types.BlockID{Hash: badHash, PartSetHeader: canonicalParts.Header()} | ||
| pubKey, err := vss[1].PrivValidator.GetPubKey(ctx) | ||
| require.NoError(t, err) | ||
| proposal := types.NewProposal( | ||
| height, round, -1, blockID, propBlock.Time, | ||
| propBlock.GetTxHashes(), propBlock.Header, propBlock.LastCommit, propBlock.Evidence, pubKey.Address(), | ||
| ) | ||
| p := proposal.ToProto() | ||
| require.NoError(t, vss[1].SignProposal(ctx, chainID, p)) | ||
| proposal.Signature = utils.OrPanic1(crypto.SigFromBytes(p.Signature)) | ||
|
|
||
| cs1.startTestRound(ctx, height, round) | ||
| peerID, err := types.NewNodeID("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA") | ||
| require.NoError(t, err) | ||
|
|
||
| cs1.handleMsg(ctx, msgInfo{&ProposalMessage{proposal}, peerID, tmtime.Now()}, false) | ||
| for i := 0; i < int(canonicalParts.Total()); i++ { | ||
| cs1.handleMsg(ctx, msgInfo{ | ||
| &BlockPartMessage{Height: height, Round: round, Part: canonicalParts.GetPart(i)}, | ||
| peerID, | ||
| tmtime.Now(), | ||
| }, false) | ||
| } | ||
|
|
||
| rs := cs1.GetRoundState() | ||
| require.NotNil(t, rs.Proposal) | ||
| require.NotNil(t, rs.ProposalBlock, "lying proposal hash must not block assembly of canonical parts") | ||
| require.True(t, rs.ProposalBlock.HashesTo(propBlock.Hash())) | ||
| require.False(t, bytes.Equal(rs.Proposal.BlockID.Hash, propBlock.Hash())) | ||
| require.True(t, rs.ProposalBlockParts.HasHeader(canonicalParts.Header())) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] Codex flags this as High:
Proposal.BlockID.Hashis 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
PartSetHeadercryptographically binds the part bytes, every node derives the same B from those bytes, and votes commit toProposalBlock.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
BlockIDhash is only enforced when current parts still belong to that proposal." Nothing in the diff does that, andTestAssembleDespiteProposalHashMismatchenshrines 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'sProposal.Header(which fully determines the block hash) is never checked againstBlockID.Hasheither.Suggestion: implement what the description promises — when
parts.HasHeader(cs.roundState.Proposal().BlockID.PartSetHeader)(i.e. parts still belong to this proposal), also requireblock.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.