fix: Dedup Vote Extensions in ValidateVoteExtensions (backport #18895…

…) (#18900) Co-authored-by: David Terpay <[email protected]> Co-authored-by: Facundo <[email protected]>
cosmos · Dec 27, 2023 · 96a33dd · 96a33dd
1 parent f08c8cc
commit 96a33dd
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Bug Fixes
 
 * (baseapp) [#18609](https://github.com/cosmos/cosmos-sdk/issues/18609) Fixed accounting in the block gas meter after BeginBlock and before DeliverTx, ensuring transaction processing always starts with the expected zeroed out block gas meter.
+* (baseapp) [#18895](https://github.com/cosmos/cosmos-sdk/pull/18895) Fix de-duplicating vote extensions during validation in ValidateVoteExtensions.
 
 ## [v0.50.2](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.2) - 2023-12-11
 

diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go
@@ -2055,15 +2055,25 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	valStore := mock.NewMockValidatorStore(ctrl)
 
-	// for brevity and simplicity, all validators have the same key
-	privKey := secp256k1.GenPrivKey()
-	pubKey := privKey.PubKey()
-	tmPk := cmtprotocrypto.PublicKey{
-		Sum: &cmtprotocrypto.PublicKey_Secp256K1{
-			Secp256K1: pubKey.Bytes(),
-		},
+	// 10 good vote extensions, 2 bad ones from 12 total validators
+	numVals := 12
+	privKeys := make([]secp256k1.PrivKey, numVals)
+	vals := make([]sdk.ConsAddress, numVals)
+	for i := 0; i < numVals; i++ {
+		privKey := secp256k1.GenPrivKey()
+		privKeys[i] = privKey
+
+		pubKey := privKey.PubKey()
+		val := sdk.ConsAddress(pubKey.Bytes())
+		vals[i] = val
+
+		tmPk := cmtprotocrypto.PublicKey{
+			Sum: &cmtprotocrypto.PublicKey_Secp256K1{
+				Secp256K1: pubKey.Bytes(),
+			},
+		}
+		valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), val).Return(tmPk, nil)
 	}
-	valStore.EXPECT().GetPubKeyByConsAddr(gomock.Any(), gomock.Any()).Return(tmPk, nil).AnyTimes()
 
 	baseappOpts := func(app *baseapp.BaseApp) {
 		app.SetExtendVoteHandler(func(sdk.Context, *abci.RequestExtendVote) (*abci.ResponseExtendVote, error) {
@@ -2229,7 +2239,7 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
 
 	// Now onto the second block, this time we process vote extensions from the
 	// previous block (which we sign now)
-	for _, ve := range allVEs {
+	for i, ve := range allVEs {
 		cve := cmtproto.CanonicalVoteExtension{
 			Extension: ve,
 			Height:    1,
@@ -2240,13 +2250,17 @@ func TestBaseApp_VoteExtensions(t *testing.T) {
 		bz, err := marshalDelimitedFn(&cve)
 		require.NoError(t, err)
 
+		privKey := privKeys[i]
 		extSig, err := privKey.Sign(bz)
 		require.NoError(t, err)
 
 		prepPropReq.LocalLastCommit.Votes = append(prepPropReq.LocalLastCommit.Votes, abci.ExtendedVoteInfo{
 			VoteExtension:      ve,
 			BlockIdFlag:        cmtproto.BlockIDFlagCommit,
 			ExtensionSignature: extSig,
+			Validator: abci.Validator{
+				Address: vals[i].Bytes(),
+			},
 		})
 	}
 

diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go
@@ -71,6 +71,7 @@ func ValidateVoteExtensions(
 		sumVP int64
 	)
 
+	cache := make(map[string]struct{})
 	for _, vote := range extCommit.Votes {
 		totalVP += vote.Validator.Power
 
@@ -95,7 +96,13 @@ func ValidateVoteExtensions(
 			return fmt.Errorf("vote extensions enabled; received empty vote extension signature at height %d", currentHeight)
 		}
 
+		// Ensure that the validator has not already submitted a vote extension.
 		valConsAddr := sdk.ConsAddress(vote.Validator.Address)
+		if _, ok := cache[valConsAddr.String()]; ok {
+			return fmt.Errorf("duplicate validator; validator %s has already submitted a vote extension", valConsAddr.String())
+		}
+		cache[valConsAddr.String()] = struct{}{}
+
 		pubKeyProto, err := valStore.GetPubKeyByConsAddr(ctx, valConsAddr)
 		if err != nil {
 			return fmt.Errorf("failed to get validator %X public key: %w", valConsAddr, err)

diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go
@@ -195,6 +195,40 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsSingleVoteAbsent() {
 	s.Require().NoError(baseapp.ValidateVoteExtensions(s.ctx, s.valStore, 3, chainID, llc))
 }
 
+// check ValidateVoteExtensions works with duplicate votes
+func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() {
+	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)
+
+	ve := abci.ExtendedVoteInfo{
+		Validator:          s.vals[0].toValidator(333),
+		VoteExtension:      ext,
+		ExtensionSignature: extSig0,
+		BlockIdFlag:        cmtproto.BlockIDFlagCommit,
+	}
+
+	llc := abci.ExtendedCommitInfo{
+		Round: 0,
+		Votes: []abci.ExtendedVoteInfo{
+			ve,
+			ve,
+		},
+	}
+	// expect fail (duplicate votes)
+	s.Require().Error(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")