fix(ve-validation): account for BlockID flag in vote-extensions (#17394)

Co-authored-by: Aleksandr Bezobchuk <[email protected]>
(cherry picked from commit 104ebe6)

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
 

baseapp/abci_test.go

@@ -1866,6 +1866,7 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) {
 					},
 					VoteExtension:      ext,
 					ExtensionSignature: extSig,
+					BlockIdFlag:        cmtproto.BlockIDFlagCommit,
 				},
 			},
 		},

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)

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
+}