diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b56dc969fce..8146b36ac403 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/bank) [#16795](https://github.com/cosmos/cosmos-sdk/pull/16852) Add `DenomMetadataByQueryString` query in bank module to support metadata query by query string. * (baseapp) [#16239](https://github.com/cosmos/cosmos-sdk/pull/16239) Add Gas Limits to allow node operators to resource bound queries. +* (baseapp) [#17393](https://github.com/cosmos/cosmos-sdk/pull/17394) Check BlockID Flag on Votes in `ValidateVoteExtensions` ### Improvements diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 8feca8fd8fd5..b6027102d98b 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1866,6 +1866,7 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) { }, VoteExtension: ext, ExtensionSignature: extSig, + BlockIdFlag: cmtproto.BlockIDFlagCommit, }, }, }, diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 82ea3d544e91..9b5fe2e63328 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -64,6 +64,12 @@ func ValidateVoteExtensions( sumVP := math.NewInt(0) for _, vote := range extCommit.Votes { + // Only check + include power if the vote is a commit vote. There must be super-majority, otherwise the + // previous block (the block vote is for) could not have been committed. + if vote.BlockIdFlag != cmtproto.BlockIDFlagCommit { + continue + } + if !extsEnabled { if len(vote.VoteExtension) > 0 { return fmt.Errorf("vote extensions disabled; received non-empty vote extension at height %d", currentHeight) diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go new file mode 100644 index 000000000000..58d8770ad05a --- /dev/null +++ b/baseapp/abci_utils_test.go @@ -0,0 +1,286 @@ +package baseapp_test + +import ( + "bytes" + "testing" + + abci "github.com/cometbft/cometbft/abci/types" + "github.com/cometbft/cometbft/crypto/secp256k1" + cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" + cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" + protoio "github.com/cosmos/gogoproto/io" + "github.com/cosmos/gogoproto/proto" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/suite" + + "cosmossdk.io/math" + + "github.com/cosmos/cosmos-sdk/baseapp" + "github.com/cosmos/cosmos-sdk/baseapp/testutil/mock" + sdk "github.com/cosmos/cosmos-sdk/types" +) + +const ( + chainID = "chain-id" +) + +type testValidator struct { + consAddr sdk.ConsAddress + tmPk cmtprotocrypto.PublicKey + privKey secp256k1.PrivKey +} + +func newTestValidator() testValidator { + privkey := secp256k1.GenPrivKey() + pubkey := privkey.PubKey() + tmPk := cmtprotocrypto.PublicKey{ + Sum: &cmtprotocrypto.PublicKey_Secp256K1{ + Secp256K1: pubkey.Bytes(), + }, + } + + return testValidator{ + consAddr: sdk.ConsAddress(pubkey.Address()), + tmPk: tmPk, + privKey: privkey, + } +} + +func (t testValidator) toValidator() abci.Validator { + return abci.Validator{ + Address: t.consAddr.Bytes(), + Power: 0, // ignored for now + } +} + +type ABCIUtilsTestSuite struct { + suite.Suite + + valStore *mock.MockValidatorStore + vals [3]testValidator + ctx sdk.Context +} + +func NewABCIUtilsTestSuite(t *testing.T) *ABCIUtilsTestSuite { + t.Helper() + // create 3 validators + s := &ABCIUtilsTestSuite{ + vals: [3]testValidator{ + newTestValidator(), + newTestValidator(), + newTestValidator(), + }, + } + + // create mock + ctrl := gomock.NewController(t) + valStore := mock.NewMockValidatorStore(ctrl) + s.valStore = valStore + + // set up mock + s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[0].consAddr.Bytes()).Return(math.NewInt(333), s.vals[0].tmPk, nil).AnyTimes() + s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[1].consAddr.Bytes()).Return(math.NewInt(333), s.vals[1].tmPk, nil).AnyTimes() + s.valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), s.vals[2].consAddr.Bytes()).Return(math.NewInt(334), s.vals[2].tmPk, nil).AnyTimes() + s.valStore.EXPECT().TotalBondedTokens(gomock.Any()).Return(math.NewInt(1000), nil).AnyTimes() + + // create context + s.ctx = sdk.Context{}.WithConsensusParams(cmtproto.ConsensusParams{ + Abci: &cmtproto.ABCIParams{ + VoteExtensionsEnableHeight: 2, + }, + }) + return s +} + +func TestACITUtilsTestSuite(t *testing.T) { + suite.Run(t, NewABCIUtilsTestSuite(t)) +} + +// check ValidateVoteExtensions works when all nodes have CommitBlockID votes +func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsHappyPath() { + ext := []byte("vote-extension") + cve := cmtproto.CanonicalVoteExtension{ + Extension: ext, + Height: 2, + Round: int64(0), + ChainId: chainID, + } + + bz, err := marshalDelimitedFn(&cve) + s.Require().NoError(err) + + extSig0, err := s.vals[0].privKey.Sign(bz) + s.Require().NoError(err) + + extSig1, err := s.vals[1].privKey.Sign(bz) + s.Require().NoError(err) + + extSig2, err := s.vals[2].privKey.Sign(bz) + s.Require().NoError(err) + + llc := abci.ExtendedCommitInfo{ + Round: 0, + Votes: []abci.ExtendedVoteInfo{ + { + Validator: s.vals[0].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig0, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + { + Validator: s.vals[1].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig1, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + { + Validator: s.vals[2].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig2, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + }, + } + // expect-pass (votes of height 2 are included in next block) + s.Require().NoError(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) +} + +// check ValidateVoteExtensions works when a single node has submitted a BlockID_Absent +func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsSingleVoteAbsent() { + ext := []byte("vote-extension") + cve := cmtproto.CanonicalVoteExtension{ + Extension: ext, + Height: 2, + Round: int64(0), + ChainId: chainID, + } + + bz, err := marshalDelimitedFn(&cve) + s.Require().NoError(err) + + extSig0, err := s.vals[0].privKey.Sign(bz) + s.Require().NoError(err) + + extSig2, err := s.vals[2].privKey.Sign(bz) + s.Require().NoError(err) + + llc := abci.ExtendedCommitInfo{ + Round: 0, + Votes: []abci.ExtendedVoteInfo{ + { + Validator: s.vals[0].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig0, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + // validator of power >1/3 is missing, so commit-info shld still be valid + { + Validator: s.vals[1].toValidator(), + BlockIdFlag: cmtproto.BlockIDFlagAbsent, + }, + { + Validator: s.vals[2].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig2, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + }, + } + // expect-pass (votes of height 2 are included in next block) + s.Require().NoError(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) +} + +// check ValidateVoteExtensions works when a single node has submitted a BlockID_Nil +func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsSingleVoteNil() { + ext := []byte("vote-extension") + cve := cmtproto.CanonicalVoteExtension{ + Extension: ext, + Height: 2, + Round: int64(0), + ChainId: chainID, + } + + bz, err := marshalDelimitedFn(&cve) + s.Require().NoError(err) + + extSig0, err := s.vals[0].privKey.Sign(bz) + s.Require().NoError(err) + + extSig2, err := s.vals[2].privKey.Sign(bz) + s.Require().NoError(err) + + llc := abci.ExtendedCommitInfo{ + Round: 0, + Votes: []abci.ExtendedVoteInfo{ + { + Validator: s.vals[0].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig0, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + // validator of power <1/3 is missing, so commit-info shld still be valid + { + Validator: s.vals[1].toValidator(), + BlockIdFlag: cmtproto.BlockIDFlagNil, + }, + { + Validator: s.vals[2].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig2, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + }, + } + // expect-pass (votes of height 2 are included in next block) + s.Require().NoError(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) +} + +// check ValidateVoteExtensions works when two nodes have submitted a BlockID_Nil / BlockID_Absent +func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsTwoVotesNilAbsent() { + ext := []byte("vote-extension") + cve := cmtproto.CanonicalVoteExtension{ + Extension: ext, + Height: 2, + Round: int64(0), + ChainId: chainID, + } + + bz, err := marshalDelimitedFn(&cve) + s.Require().NoError(err) + + extSig2, err := s.vals[2].privKey.Sign(bz) + s.Require().NoError(err) + + llc := abci.ExtendedCommitInfo{ + Round: 0, + Votes: []abci.ExtendedVoteInfo{ + // validator of power >2/3 is missing, so commit-info shld still be valid + { + Validator: s.vals[0].toValidator(), + BlockIdFlag: cmtproto.BlockIDFlagCommit, + }, + { + Validator: s.vals[1].toValidator(), + BlockIdFlag: cmtproto.BlockIDFlagNil, + }, + { + Validator: s.vals[2].toValidator(), + VoteExtension: ext, + ExtensionSignature: extSig2, + BlockIdFlag: cmtproto.BlockIDFlagAbsent, + }, + }, + } + + // expect-pass (votes of height 2 are included in next block) + s.Require().Error(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc)) +} + +func marshalDelimitedFn(msg proto.Message) ([]byte, error) { + var buf bytes.Buffer + if err := protoio.NewDelimitedWriter(&buf).WriteMsg(msg); err != nil { + return nil, err + } + + return buf.Bytes(), nil +}