From 19dd690995957b2ec033281bcf5e87544e591292 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Tue, 26 Dec 2023 13:17:12 -0600 Subject: [PATCH 1/5] cache validators as ves are processed --- baseapp/abci_utils.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 4cf9fa8e2282..5f488960a41d 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -72,6 +72,7 @@ func ValidateVoteExtensions( sumVP int64 ) + cache := make(map[string]struct{}) for _, vote := range extCommit.Votes { totalVP += vote.Validator.Power @@ -96,7 +97,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 { + continue + } + 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) From 1977b78d0b7ea7b8c7acd370605d779eb8c60c46 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Tue, 26 Dec 2023 13:21:53 -0600 Subject: [PATCH 2/5] test duplicates --- baseapp/abci_utils_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index bf3748101e17..b21a71db3d6a 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -196,6 +196,41 @@ 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, + 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") From 5015e54e260174d4ccfbf57729ac726de732a154 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Tue, 26 Dec 2023 13:31:08 -0600 Subject: [PATCH 3/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 925a03e88b9c..0a420fb9b005 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (simulation) [#17911](https://github.com/cosmos/cosmos-sdk/pull/17911) Fix all problems with executing command `make test-sim-custom-genesis-fast` for simulation test. * (simulation) [#18196](https://github.com/cosmos/cosmos-sdk/pull/18196) Fix the problem of `validator set is empty after InitGenesis` in simulation test. * (baseapp) [#18551](https://github.com/cosmos/cosmos-sdk/pull/18551) Fix SelectTxForProposal the calculation method of tx bytes size is inconsistent with CometBFT +* (baseapp) [#18895](https://github.com/cosmos/cosmos-sdk/pull/18895) Fix de-duplicating vote extensions during validation in ValidateVoteExtensions. ### API Breaking Changes From bbb408657dbae93d5ab751f9d6549704bc924531 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Tue, 26 Dec 2023 13:38:11 -0600 Subject: [PATCH 4/5] error on dup --- baseapp/abci_utils.go | 2 +- baseapp/abci_utils_test.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 5f488960a41d..b09a22b33ecf 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -100,7 +100,7 @@ func ValidateVoteExtensions( // Ensure that the validator has not already submitted a vote extension. valConsAddr := sdk.ConsAddress(vote.Validator.Address) if _, ok := cache[valConsAddr.String()]; ok { - continue + return fmt.Errorf("duplicate validator; validator %s has already submitted a vote extension", valConsAddr.String()) } cache[valConsAddr.String()] = struct{}{} diff --git a/baseapp/abci_utils_test.go b/baseapp/abci_utils_test.go index b21a71db3d6a..ac72b90e8e26 100644 --- a/baseapp/abci_utils_test.go +++ b/baseapp/abci_utils_test.go @@ -224,7 +224,6 @@ func (s *ABCIUtilsTestSuite) TestValidateVoteExtensionsDuplicateVotes() { Votes: []abci.ExtendedVoteInfo{ ve, ve, - ve, }, } // expect fail (duplicate votes) From e230a6d5c9a5ce24ffedfe20b5232bbf4a45732f Mon Sep 17 00:00:00 2001 From: David Terpay Date: Tue, 26 Dec 2023 14:31:42 -0600 Subject: [PATCH 5/5] base app test fix --- baseapp/abci_test.go | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 48682b974eb0..efca5b74079b 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -2082,15 +2082,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) { @@ -2256,7 +2266,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, @@ -2267,6 +2277,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) @@ -2274,6 +2285,9 @@ func TestBaseApp_VoteExtensions(t *testing.T) { VoteExtension: ve, BlockIdFlag: cmtproto.BlockIDFlagCommit, ExtensionSignature: extSig, + Validator: abci.Validator{ + Address: vals[i].Bytes(), + }, }) }