diff --git a/CHANGELOG.md b/CHANGELOG.md index d25ab2a7f81..a44429c8dba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/group, x/gov) [#17109](https://github.com/cosmos/cosmos-sdk/pull/17109) Let proposal summary be 40x longer than metadata limit. * (all) [#16537](https://github.com/cosmos/cosmos-sdk/pull/16537) Properly propagated `fmt.Errorf` errors and using `errors.New` where appropriate. * (version) [#17096](https://github.com/cosmos/cosmos-sdk/pull/17096) Improve `getSDKVersion()` to handle module replacements +* (x/staking) [#17164](https://github.com/cosmos/cosmos-sdk/pull/17164) Add `BondedTokensAndPubKeyByConsAddr` to the keeper to enable vote extension verification. ### Bug Fixes diff --git a/baseapp/abci_test.go b/baseapp/abci_test.go index 4f9207924f1..3e571274c86 100644 --- a/baseapp/abci_test.go +++ b/baseapp/abci_test.go @@ -1790,13 +1790,9 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) { }, } - val1 := mock.NewMockValidator(ctrl) - val1.EXPECT().BondedTokens().Return(math.NewInt(667)) - val1.EXPECT().CmtConsPublicKey().Return(tmPk, nil).AnyTimes() - consAddr := sdk.ConsAddress(addr.String()) - valStore.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr.Bytes()).Return(val1, nil).AnyTimes() - valStore.EXPECT().TotalBondedTokens(gomock.Any()).Return(math.NewInt(1000)).AnyTimes() + valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(math.NewInt(667), tmPk, nil) + valStore.EXPECT().TotalBondedTokens(gomock.Any()).Return(math.NewInt(1000), nil).AnyTimes() // set up baseapp prepareOpt := func(bapp *baseapp.BaseApp) { @@ -1884,7 +1880,7 @@ func TestABCI_PrepareProposal_VoteExtensions(t *testing.T) { require.Equal(t, 1, len(resPrepareProposal.Txs)) // now vote extensions but our sole voter doesn't reach majority - val1.EXPECT().BondedTokens().Return(math.NewInt(666)) + valStore.EXPECT().BondedTokensAndPubKeyByConsAddr(gomock.Any(), consAddr.Bytes()).Return(math.NewInt(666), tmPk, nil) resPrepareProposal, err = suite.baseApp.PrepareProposal(&reqPrepareProposal) require.NoError(t, err) require.Equal(t, 0, len(resPrepareProposal.Txs)) diff --git a/baseapp/abci_utils.go b/baseapp/abci_utils.go index 6dacc5407d8..39bb4e93809 100644 --- a/baseapp/abci_utils.go +++ b/baseapp/abci_utils.go @@ -2,11 +2,11 @@ package baseapp import ( "bytes" + "context" "fmt" "github.com/cockroachdb/errors" abci "github.com/cometbft/cometbft/abci/types" - cmtcrypto "github.com/cometbft/cometbft/crypto" cryptoenc "github.com/cometbft/cometbft/crypto/encoding" cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" @@ -15,7 +15,6 @@ import ( "cosmossdk.io/math" - cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/mempool" ) @@ -26,20 +25,12 @@ import ( var VoteExtensionThreshold = math.LegacyNewDecWithPrec(667, 3) type ( - // Validator defines the interface contract require for verifying vote extension - // signatures. Typically, this will be implemented by the x/staking module, - // which has knowledge of the CometBFT public key. - Validator interface { - CmtConsPublicKey() (cmtprotocrypto.PublicKey, error) - BondedTokens() math.Int - } - // ValidatorStore defines the interface contract require for verifying vote // extension signatures. Typically, this will be implemented by the x/staking // module, which has knowledge of the CometBFT public key. ValidatorStore interface { - GetValidatorByConsAddr(sdk.Context, cryptotypes.Address) (Validator, error) - TotalBondedTokens(ctx sdk.Context) math.Int + TotalBondedTokens(ctx context.Context) (math.Int, error) + BondedTokensAndPubKeyByConsAddr(context.Context, sdk.ConsAddress) (math.Int, cmtprotocrypto.PublicKey, error) } // GasTx defines the contract that a transaction with a gas limit must implement. @@ -62,7 +53,6 @@ func ValidateVoteExtensions( ) error { cp := ctx.ConsensusParams() extsEnabled := cp.Abci != nil && currentHeight >= cp.Abci.VoteExtensionsEnableHeight && cp.Abci.VoteExtensionsEnableHeight != 0 - marshalDelimitedFn := func(msg proto.Message) ([]byte, error) { var buf bytes.Buffer if err := protoio.NewDelimitedWriter(&buf).WriteMsg(msg); err != nil { @@ -89,19 +79,10 @@ func ValidateVoteExtensions( return fmt.Errorf("vote extensions enabled; received empty vote extension signature at height %d", currentHeight) } - valConsAddr := cmtcrypto.Address(vote.Validator.Address) - - validator, err := valStore.GetValidatorByConsAddr(ctx, valConsAddr) - if err != nil { - return fmt.Errorf("failed to get validator %X: %w", valConsAddr, err) - } - if validator == nil { - return fmt.Errorf("validator %X not found", valConsAddr) - } - - cmtPubKeyProto, err := validator.CmtConsPublicKey() + valConsAddr := sdk.ConsAddress(vote.Validator.Address) + bondedTokens, cmtPubKeyProto, err := valStore.BondedTokensAndPubKeyByConsAddr(ctx, valConsAddr) if err != nil { - return fmt.Errorf("failed to get validator %X public key: %w", valConsAddr, err) + return fmt.Errorf("failed to get validator %X info (bonded tokens and public key): %w", valConsAddr, err) } cmtPubKey, err := cryptoenc.PubKeyFromProto(cmtPubKeyProto) @@ -125,12 +106,16 @@ func ValidateVoteExtensions( return fmt.Errorf("failed to verify validator %X vote extension signature", valConsAddr) } - sumVP = sumVP.Add(validator.BondedTokens()) + sumVP = sumVP.Add(bondedTokens) } // Ensure we have at least 2/3 voting power that submitted valid vote // extensions. - totalVP := valStore.TotalBondedTokens(ctx) + totalVP, err := valStore.TotalBondedTokens(ctx) + if err != nil { + return fmt.Errorf("failed to get total bonded tokens: %w", err) + } + percentSubmitted := math.LegacyNewDecFromInt(sumVP).Quo(math.LegacyNewDecFromInt(totalVP)) if percentSubmitted.LT(VoteExtensionThreshold) { return fmt.Errorf("insufficient cumulative voting power received to verify vote extensions; got: %s, expected: >=%s", percentSubmitted, VoteExtensionThreshold) diff --git a/baseapp/testutil/mock/mocks.go b/baseapp/testutil/mock/mocks.go index 437ed68f485..482e8f59dd1 100644 --- a/baseapp/testutil/mock/mocks.go +++ b/baseapp/testutil/mock/mocks.go @@ -5,68 +5,15 @@ package mock import ( + context "context" reflect "reflect" math "cosmossdk.io/math" crypto "github.com/cometbft/cometbft/proto/tendermint/crypto" - baseapp "github.com/cosmos/cosmos-sdk/baseapp" - types "github.com/cosmos/cosmos-sdk/crypto/types" - types0 "github.com/cosmos/cosmos-sdk/types" + types "github.com/cosmos/cosmos-sdk/types" gomock "github.com/golang/mock/gomock" ) -// MockValidator is a mock of Validator interface. -type MockValidator struct { - ctrl *gomock.Controller - recorder *MockValidatorMockRecorder -} - -// MockValidatorMockRecorder is the mock recorder for MockValidator. -type MockValidatorMockRecorder struct { - mock *MockValidator -} - -// NewMockValidator creates a new mock instance. -func NewMockValidator(ctrl *gomock.Controller) *MockValidator { - mock := &MockValidator{ctrl: ctrl} - mock.recorder = &MockValidatorMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockValidator) EXPECT() *MockValidatorMockRecorder { - return m.recorder -} - -// BondedTokens mocks base method. -func (m *MockValidator) BondedTokens() math.Int { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "BondedTokens") - ret0, _ := ret[0].(math.Int) - return ret0 -} - -// BondedTokens indicates an expected call of BondedTokens. -func (mr *MockValidatorMockRecorder) BondedTokens() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BondedTokens", reflect.TypeOf((*MockValidator)(nil).BondedTokens)) -} - -// CmtConsPublicKey mocks base method. -func (m *MockValidator) CmtConsPublicKey() (crypto.PublicKey, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CmtConsPublicKey") - ret0, _ := ret[0].(crypto.PublicKey) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// CmtConsPublicKey indicates an expected call of CmtConsPublicKey. -func (mr *MockValidatorMockRecorder) CmtConsPublicKey() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CmtConsPublicKey", reflect.TypeOf((*MockValidator)(nil).CmtConsPublicKey)) -} - // MockValidatorStore is a mock of ValidatorStore interface. type MockValidatorStore struct { ctrl *gomock.Controller @@ -90,27 +37,29 @@ func (m *MockValidatorStore) EXPECT() *MockValidatorStoreMockRecorder { return m.recorder } -// GetValidatorByConsAddr mocks base method. -func (m *MockValidatorStore) GetValidatorByConsAddr(arg0 types0.Context, arg1 types.Address) (baseapp.Validator, error) { +// BondedTokensAndPubKeyByConsAddr mocks base method. +func (m *MockValidatorStore) BondedTokensAndPubKeyByConsAddr(arg0 context.Context, arg1 types.ConsAddress) (math.Int, crypto.PublicKey, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetValidatorByConsAddr", arg0, arg1) - ret0, _ := ret[0].(baseapp.Validator) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret := m.ctrl.Call(m, "BondedTokensAndPubKeyByConsAddr", arg0, arg1) + ret0, _ := ret[0].(math.Int) + ret1, _ := ret[1].(crypto.PublicKey) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 } -// GetValidatorByConsAddr indicates an expected call of GetValidatorByConsAddr. -func (mr *MockValidatorStoreMockRecorder) GetValidatorByConsAddr(arg0, arg1 interface{}) *gomock.Call { +// BondedTokensAndPubKeyByConsAddr indicates an expected call of BondedTokensAndPubKeyByConsAddr. +func (mr *MockValidatorStoreMockRecorder) BondedTokensAndPubKeyByConsAddr(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetValidatorByConsAddr", reflect.TypeOf((*MockValidatorStore)(nil).GetValidatorByConsAddr), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BondedTokensAndPubKeyByConsAddr", reflect.TypeOf((*MockValidatorStore)(nil).BondedTokensAndPubKeyByConsAddr), arg0, arg1) } // TotalBondedTokens mocks base method. -func (m *MockValidatorStore) TotalBondedTokens(ctx types0.Context) math.Int { +func (m *MockValidatorStore) TotalBondedTokens(ctx context.Context) (math.Int, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "TotalBondedTokens", ctx) ret0, _ := ret[0].(math.Int) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // TotalBondedTokens indicates an expected call of TotalBondedTokens. @@ -180,7 +129,7 @@ func (m *MockProposalTxVerifier) EXPECT() *MockProposalTxVerifierMockRecorder { } // PrepareProposalVerifyTx mocks base method. -func (m *MockProposalTxVerifier) PrepareProposalVerifyTx(tx types0.Tx) ([]byte, error) { +func (m *MockProposalTxVerifier) PrepareProposalVerifyTx(tx types.Tx) ([]byte, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "PrepareProposalVerifyTx", tx) ret0, _ := ret[0].([]byte) @@ -195,10 +144,10 @@ func (mr *MockProposalTxVerifierMockRecorder) PrepareProposalVerifyTx(tx interfa } // ProcessProposalVerifyTx mocks base method. -func (m *MockProposalTxVerifier) ProcessProposalVerifyTx(txBz []byte) (types0.Tx, error) { +func (m *MockProposalTxVerifier) ProcessProposalVerifyTx(txBz []byte) (types.Tx, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ProcessProposalVerifyTx", txBz) - ret0, _ := ret[0].(types0.Tx) + ret0, _ := ret[0].(types.Tx) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/docs/architecture/adr-064-abci-2.0.md b/docs/architecture/adr-064-abci-2.0.md index f392ccae1e8..f4665c3de7d 100644 --- a/docs/architecture/adr-064-abci-2.0.md +++ b/docs/architecture/adr-064-abci-2.0.md @@ -219,7 +219,8 @@ a default signature verification method which applications can use: ```go type ValidatorStore interface { - GetValidatorByConsAddr(sdk.Context, cryptotypes.Address) (cryptotypes.PubKey, error) + TotalBondedTokens(ctx context.Context) (math.Int, error) + BondedTokensAndPubKeyByConsAddr(context.Context, sdk.ConsAddress) (math.Int, cmtprotocrypto.PublicKey, error) } // ValidateVoteExtensions is a function that an application can execute in @@ -230,16 +231,10 @@ func (app *BaseApp) ValidateVoteExtensions(ctx sdk.Context, currentHeight int64, continue } - valConsAddr := cmtcrypto.Address(vote.Validator.Address) - - validator, err := app.validatorStore.GetValidatorByConsAddr(ctx, valConsAddr) - if err != nil { - return fmt.Errorf("failed to get validator %s for vote extension", valConsAddr) - } - - cmtPubKey, err := validator.CmtConsPublicKey() + valConsAddr := sdk.ConsAddress(vote.Validator.Address) + bondedTokens, cmtPubKey, err := valStore.BondedTokensAndPubKeyByConsAddr(ctx, valConsAddr) if err != nil { - return fmt.Errorf("failed to convert public key: %w", err) + return fmt.Errorf("failed to get bonded tokens and public key for validator %s: %w", valConsAddr, err) } if len(vote.ExtensionSignature) == 0 { diff --git a/tests/integration/staking/keeper/vote_extensions_test.go b/tests/integration/staking/keeper/vote_extensions_test.go new file mode 100644 index 00000000000..5947373c1c3 --- /dev/null +++ b/tests/integration/staking/keeper/vote_extensions_test.go @@ -0,0 +1,96 @@ +package keeper_test + +import ( + "bytes" + "testing" + + abci "github.com/cometbft/cometbft/abci/types" + cmtproto "github.com/cometbft/cometbft/proto/tendermint/types" + protoio "github.com/cosmos/gogoproto/io" + "github.com/cosmos/gogoproto/proto" + "gotest.tools/v3/assert" + + "cosmossdk.io/math" + + "github.com/cosmos/cosmos-sdk/baseapp" + ed25519 "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/staking/testutil" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" +) + +func TestValidateVoteExtensions(t *testing.T) { + t.Parallel() + f := initFixture(t) + + // enable vote extensions + cp := simtestutil.DefaultConsensusParams + cp.Abci = &cmtproto.ABCIParams{VoteExtensionsEnableHeight: 1} + f.sdkCtx = f.sdkCtx.WithConsensusParams(*cp).WithBlockHeight(2) + + // setup the validators + numVals := 3 + privKeys := []cryptotypes.PrivKey{} + for i := 0; i < numVals; i++ { + privKeys = append(privKeys, ed25519.GenPrivKey()) + } + + vals := []stakingtypes.Validator{} + for _, v := range privKeys { + valAddr := sdk.ValAddress(v.PubKey().Address()) + simtestutil.AddTestAddrsFromPubKeys(f.bankKeeper, f.stakingKeeper, f.sdkCtx, []cryptotypes.PubKey{v.PubKey()}, math.NewInt(100000000000)) + vals = append(vals, testutil.NewValidator(t, valAddr, v.PubKey())) + } + + votes := []abci.ExtendedVoteInfo{} + + for i, v := range vals { + v.Tokens = math.NewInt(1000000) + v.Status = stakingtypes.Bonded + assert.NilError(t, f.stakingKeeper.SetValidator(f.sdkCtx, v)) + assert.NilError(t, f.stakingKeeper.SetValidatorByConsAddr(f.sdkCtx, v)) + assert.NilError(t, f.stakingKeeper.SetNewValidatorByPowerIndex(f.sdkCtx, v)) + _, err := f.stakingKeeper.Delegate(f.sdkCtx, sdk.AccAddress(privKeys[i].PubKey().Address()), v.Tokens, stakingtypes.Unbonded, v, true) + assert.NilError(t, err) + + // each val produces a vote + voteExt := []byte("something" + v.OperatorAddress) + cve := cmtproto.CanonicalVoteExtension{ + Extension: voteExt, + Height: f.sdkCtx.BlockHeight() - 1, // the vote extension was signed in the previous height + Round: 0, + ChainId: "chain-id-123", + } + + extSignBytes, err := mashalVoteExt(&cve) + assert.NilError(t, err) + + sig, err := privKeys[i].Sign(extSignBytes) + assert.NilError(t, err) + + ve := abci.ExtendedVoteInfo{ + Validator: abci.Validator{ + Address: v.GetOperator(), + Power: v.ConsensusPower(sdk.DefaultPowerReduction), + }, + VoteExtension: voteExt, + ExtensionSignature: sig, + BlockIdFlag: cmtproto.BlockIDFlagCommit, + } + votes = append(votes, ve) + } + + err := baseapp.ValidateVoteExtensions(f.sdkCtx, f.stakingKeeper, f.sdkCtx.BlockHeight(), "chain-id-123", abci.ExtendedCommitInfo{Round: 0, Votes: votes}) + assert.NilError(t, err) +} + +func mashalVoteExt(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 +} diff --git a/x/staking/keeper/validator.go b/x/staking/keeper/validator.go index e5dd3c85513..2c812bb5d17 100644 --- a/x/staking/keeper/validator.go +++ b/x/staking/keeper/validator.go @@ -7,6 +7,7 @@ import ( "fmt" "time" + cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" gogotypes "github.com/cosmos/gogoproto/types" "cosmossdk.io/collections" @@ -601,3 +602,18 @@ func (k Keeper) IsValidatorJailed(ctx context.Context, addr sdk.ConsAddress) (bo return v.Jailed, nil } + +// BondedTokensAndPubKeyByConsAddr returns the consensus public key and bonded tokens by consensus address +func (k Keeper) BondedTokensAndPubKeyByConsAddr(ctx context.Context, addr sdk.ConsAddress) (math.Int, cmtprotocrypto.PublicKey, error) { + v, err := k.GetValidatorByConsAddr(ctx, addr) + if err != nil { + return math.ZeroInt(), cmtprotocrypto.PublicKey{}, err + } + + pubkey, err := v.CmtConsPublicKey() + if err != nil { + return math.ZeroInt(), cmtprotocrypto.PublicKey{}, err + } + + return v.BondedTokens(), pubkey, nil +} diff --git a/x/staking/testutil/expected_keepers_mocks.go b/x/staking/testutil/expected_keepers_mocks.go index e40bbd84d63..da70c5f1ffd 100644 --- a/x/staking/testutil/expected_keepers_mocks.go +++ b/x/staking/testutil/expected_keepers_mocks.go @@ -10,6 +10,7 @@ import ( address "cosmossdk.io/core/address" math "cosmossdk.io/math" + crypto "github.com/cometbft/cometbft/proto/tendermint/crypto" types "github.com/cosmos/cosmos-sdk/types" types0 "github.com/cosmos/cosmos-sdk/x/staking/types" gomock "github.com/golang/mock/gomock" @@ -290,6 +291,22 @@ func (m *MockValidatorSet) EXPECT() *MockValidatorSetMockRecorder { return m.recorder } +// BondedTokensAndPubKeyByConsAddr mocks base method. +func (m *MockValidatorSet) BondedTokensAndPubKeyByConsAddr(arg0 context.Context, arg1 types.ConsAddress) (math.Int, crypto.PublicKey, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "BondedTokensAndPubKeyByConsAddr", arg0, arg1) + ret0, _ := ret[0].(math.Int) + ret1, _ := ret[1].(crypto.PublicKey) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// BondedTokensAndPubKeyByConsAddr indicates an expected call of BondedTokensAndPubKeyByConsAddr. +func (mr *MockValidatorSetMockRecorder) BondedTokensAndPubKeyByConsAddr(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BondedTokensAndPubKeyByConsAddr", reflect.TypeOf((*MockValidatorSet)(nil).BondedTokensAndPubKeyByConsAddr), arg0, arg1) +} + // Delegation mocks base method. func (m *MockValidatorSet) Delegation(arg0 context.Context, arg1 types.AccAddress, arg2 types.ValAddress) (types0.DelegationI, error) { m.ctrl.T.Helper() diff --git a/x/staking/types/expected_keepers.go b/x/staking/types/expected_keepers.go index 5e45733d0af..697cab3594e 100644 --- a/x/staking/types/expected_keepers.go +++ b/x/staking/types/expected_keepers.go @@ -3,6 +3,8 @@ package types import ( context "context" + cmtprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" + "cosmossdk.io/core/address" "cosmossdk.io/math" @@ -70,6 +72,10 @@ type ValidatorSet interface { // MaxValidators returns the maximum amount of bonded validators MaxValidators(context.Context) (uint32, error) + + // BondedTokensAndPubKeyByConsAddr returns the bonded tokens and consensus public key for a validator. + // Used in vote extension validation. + BondedTokensAndPubKeyByConsAddr(context.Context, sdk.ConsAddress) (math.Int, cmtprotocrypto.PublicKey, error) } // DelegationSet expected properties for the set of all delegations for a particular (noalias)