From 96a33ddd1ffedc219c0196f249a5d1fc5ce3038f Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 27 Dec 2023 09:51:58 -0800 Subject: [PATCH] fix: Dedup Vote Extensions in `ValidateVoteExtensions` (backport #18895) (#18900) Co-authored-by: David Terpay <35130517+davidterpay@users.noreply.github.com> Co-authored-by: Facundo --- CHANGELOG.md | 1 + baseapp/abci_test.go | 32 +++++++++++++++++++++++--------- baseapp/abci_utils.go | 7 +++++++ baseapp/abci_utils_test.go | 34 ++++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 169f1d438101..f9c6e3cafb1c 100644 --- 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 index d368b9e81d29..ab75512f06bd 100644 --- 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,6 +2250,7 @@ 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) @@ -2247,6 +2258,9 @@ func TestBaseApp_VoteExtensions(t *testing.T) { 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 index 0b570ab52a69..db5b2f3cd8ea 100644 --- 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 index 8919ee81ba8c..9624156c21fa 100644 --- 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")