From 8955fcb22b1c13d68d1a9ad016038206c0aa847d Mon Sep 17 00:00:00 2001 From: Philip Offtermatt <57488781+p-offtermatt@users.noreply.github.com> Date: Wed, 12 Jun 2024 09:10:28 +0200 Subject: [PATCH] refactor: use IterateLastValidatorPowers instead of GetLastValidators (#1953) * Add skeleton for GetLastValidators wrapper * Fix unit tests * Correct comment * Log error messages if validators are not found * Change AnyTimes to more specific Times(1) * Instantiate slices with their max length and truncate * Remove GetLastValidators from expectation * Remove GetLastValidators call in consumer * Move GetLastBondedValidators to validator_set_updates * Add comment on iteration loop --- tests/integration/unbonding.go | 11 ++--- testutil/ibc_testing/generic_setup.go | 2 +- testutil/keeper/expectations.go | 33 +++++++++++++++ testutil/keeper/mocks.go | 42 ------------------- testutil/keeper/unit_test_helpers.go | 4 +- x/ccv/consumer/keeper/changeover_test.go | 16 ++++--- x/ccv/consumer/keeper/keeper.go | 8 +++- x/ccv/consumer/keeper/keeper_test.go | 11 +++-- x/ccv/provider/keeper/grpc_query.go | 6 +-- x/ccv/provider/keeper/grpc_query_test.go | 12 +++--- x/ccv/provider/keeper/partial_set_security.go | 3 +- .../keeper/partial_set_security_test.go | 2 +- x/ccv/provider/keeper/proposal.go | 2 +- x/ccv/provider/keeper/proposal_test.go | 16 +++---- x/ccv/provider/keeper/relay.go | 2 +- x/ccv/provider/keeper/relay_test.go | 8 ++-- x/ccv/provider/keeper/validator_set_update.go | 7 ++++ x/ccv/provider/proposal_handler_test.go | 5 ++- x/ccv/types/expected_keepers.go | 1 - x/ccv/types/utils.go | 38 +++++++++++++++++ 20 files changed, 143 insertions(+), 86 deletions(-) diff --git a/tests/integration/unbonding.go b/tests/integration/unbonding.go index a8398843aa..e7af063d7b 100644 --- a/tests/integration/unbonding.go +++ b/tests/integration/unbonding.go @@ -467,13 +467,14 @@ func (s *CCVTestSuite) TestRedelegationProviderFirst() { // during the staking module EndBlock. func (s *CCVTestSuite) TestTooManyLastValidators() { sk := s.providerApp.GetTestStakingKeeper() + pk := s.providerApp.GetProviderKeeper() // get current staking params p := sk.GetParams(s.providerCtx()) // get validators, which are all active at the moment vals := sk.GetAllValidators(s.providerCtx()) - s.Require().Equal(len(vals), len(sk.GetLastValidators(s.providerCtx()))) + s.Require().Equal(len(vals), len(pk.GetLastBondedValidators(s.providerCtx()))) // jail a validator val := vals[0] @@ -482,17 +483,17 @@ func (s *CCVTestSuite) TestTooManyLastValidators() { sk.Jail(s.providerCtx(), consAddr) // save the current number of bonded vals - lastVals := sk.GetLastValidators(s.providerCtx()) + lastVals := pk.GetLastBondedValidators(s.providerCtx()) // pass one block to apply the validator set changes // (calls ApplyAndReturnValidatorSetUpdates in the the staking module EndBlock) s.providerChain.NextBlock() // verify that the number of bonded validators is decreased by one - s.Require().Equal(len(lastVals)-1, len(sk.GetLastValidators(s.providerCtx()))) + s.Require().Equal(len(lastVals)-1, len(pk.GetLastBondedValidators(s.providerCtx()))) // update maximum validator to equal the number of bonded validators - p.MaxValidators = uint32(len(sk.GetLastValidators(s.providerCtx()))) + p.MaxValidators = uint32(len(pk.GetLastBondedValidators(s.providerCtx()))) sk.SetParams(s.providerCtx(), p) // pass one block to apply validator set changes @@ -508,5 +509,5 @@ func (s *CCVTestSuite) TestTooManyLastValidators() { // ApplyAndReturnValidatorSetUpdates where the staking module has a inconsistent state s.Require().NotPanics(s.providerChain.NextBlock) s.Require().NotPanics(func() { sk.ApplyAndReturnValidatorSetUpdates(s.providerCtx()) }) - s.Require().NotPanics(func() { sk.GetLastValidators(s.providerCtx()) }) + s.Require().NotPanics(func() { pk.GetLastBondedValidators(s.providerCtx()) }) } diff --git a/testutil/ibc_testing/generic_setup.go b/testutil/ibc_testing/generic_setup.go index 09dfa62140..21e9d1326f 100644 --- a/testutil/ibc_testing/generic_setup.go +++ b/testutil/ibc_testing/generic_setup.go @@ -143,7 +143,7 @@ func AddConsumer[Tp testutil.ProviderApp, Tc testutil.ConsumerApp]( prop.Top_N = consumerTopNParams[index] // isn't used in CreateConsumerClient // opt-in all validators - for _, v := range providerApp.GetTestStakingKeeper().GetLastValidators(providerChain.GetContext()) { + for _, v := range providerKeeper.GetLastBondedValidators(providerChain.GetContext()) { consAddr, _ := v.GetConsAddr() providerKeeper.SetOptedIn(providerChain.GetContext(), chainID, providertypes.NewProviderConsAddress(consAddr)) } diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 8a010ce405..0c6db62cf0 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -217,3 +217,36 @@ func GetMocksForSlashValidator( Times(1), } } + +// SetupMocksForLastBondedValidatorsExpectation sets up the expectation for the `IterateLastValidatorPowers` `MaxValidators`, and `GetValidator` methods of the `mockStakingKeeper` object. +// These are needed in particular when calling `GetLastBondedValidators` from the provider keeper. +// Times is the number of times the expectation should be called. Provide -1 for `AnyTimes“. +func SetupMocksForLastBondedValidatorsExpectation(mockStakingKeeper *MockStakingKeeper, maxValidators uint32, vals []stakingtypes.Validator, powers []int64, times int) { + iteratorCall := mockStakingKeeper.EXPECT().IterateLastValidatorPowers(gomock.Any(), gomock.Any()).DoAndReturn( + func(ctx sdk.Context, cb func(sdk.ValAddress, int64) bool) { + for i, val := range vals { + if stop := cb(sdk.ValAddress(val.OperatorAddress), powers[i]); stop { + break + } + } + }) + maxValidatorsCall := mockStakingKeeper.EXPECT().MaxValidators(gomock.Any()).Return(maxValidators) + + if times == -1 { + iteratorCall.AnyTimes() + maxValidatorsCall.AnyTimes() + } else { + iteratorCall.Times(times) + maxValidatorsCall.Times(times) + } + + // set up mocks for GetValidator calls + for _, val := range vals { + getValCall := mockStakingKeeper.EXPECT().GetValidator(gomock.Any(), sdk.ValAddress(val.OperatorAddress)).Return(val, true) + if times == -1 { + getValCall.AnyTimes() + } else { + getValCall.Times(times) + } + } +} diff --git a/testutil/keeper/mocks.go b/testutil/keeper/mocks.go index 0ad09a0be6..783144eb49 100644 --- a/testutil/keeper/mocks.go +++ b/testutil/keeper/mocks.go @@ -105,34 +105,6 @@ func (mr *MockStakingKeeperMockRecorder) GetLastValidatorPower(ctx, operator int return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLastValidatorPower", reflect.TypeOf((*MockStakingKeeper)(nil).GetLastValidatorPower), ctx, operator) } -// GetLastValidators mocks base method. -func (m *MockStakingKeeper) GetLastValidators(ctx types0.Context) []types5.Validator { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetLastValidators", ctx) - ret0, _ := ret[0].([]types5.Validator) - return ret0 -} - -// GetLastValidators indicates an expected call of GetLastValidators. -func (mr *MockStakingKeeperMockRecorder) GetLastValidators(ctx interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLastValidators", reflect.TypeOf((*MockStakingKeeper)(nil).GetLastValidators), ctx) -} - -// GetParams mocks base method. -func (m *MockStakingKeeper) GetParams(ctx types0.Context) types5.Params { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetParams", ctx) - ret0, _ := ret[0].(types5.Params) - return ret0 -} - -// GetParams indicates an expected call of GetParams. -func (mr *MockStakingKeeperMockRecorder) GetParams(ctx interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetParams", reflect.TypeOf((*MockStakingKeeper)(nil).GetParams), ctx) -} - // GetRedelegationByUnbondingID mocks base method. func (m *MockStakingKeeper) GetRedelegationByUnbondingID(ctx types0.Context, id uint64) (types5.Redelegation, bool) { m.ctrl.T.Helper() @@ -371,20 +343,6 @@ func (mr *MockStakingKeeperMockRecorder) PutUnbondingOnHold(ctx, id interface{}) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PutUnbondingOnHold", reflect.TypeOf((*MockStakingKeeper)(nil).PutUnbondingOnHold), ctx, id) } -// SetParams mocks base method. -func (m *MockStakingKeeper) SetParams(ctx types0.Context, p types5.Params) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SetParams", ctx, p) - ret0, _ := ret[0].(error) - return ret0 -} - -// SetParams indicates an expected call of SetParams. -func (mr *MockStakingKeeperMockRecorder) SetParams(ctx, p interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetParams", reflect.TypeOf((*MockStakingKeeper)(nil).SetParams), ctx, p) -} - // Slash mocks base method. func (m *MockStakingKeeper) Slash(arg0 types0.Context, arg1 types0.ConsAddress, arg2, arg3 int64, arg4 types0.Dec) math.Int { m.ctrl.T.Helper() diff --git a/testutil/keeper/unit_test_helpers.go b/testutil/keeper/unit_test_helpers.go index ef208cb907..65cec0b784 100644 --- a/testutil/keeper/unit_test_helpers.go +++ b/testutil/keeper/unit_test_helpers.go @@ -215,7 +215,9 @@ func SetupForStoppingConsumerChain(t *testing.T, ctx sdk.Context, providerKeeper *providerkeeper.Keeper, mocks MockedKeepers, ) { t.Helper() - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) + + SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, []int64{}, 1) + expectations := GetMocksForCreateConsumerClient(ctx, &mocks, "chainID", clienttypes.NewHeight(4, 5)) expectations = append(expectations, GetMocksForSetConsumerChain(ctx, &mocks, "chainID")...) diff --git a/x/ccv/consumer/keeper/changeover_test.go b/x/ccv/consumer/keeper/changeover_test.go index c431f43477..ba70e56041 100644 --- a/x/ccv/consumer/keeper/changeover_test.go +++ b/x/ccv/consumer/keeper/changeover_test.go @@ -3,7 +3,6 @@ package keeper_test import ( "testing" - "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" sdkcryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" @@ -30,6 +29,8 @@ func TestChangeoverToConsumer(t *testing.T) { cIds[4].SDKStakingValidator(), } + powers := []int64{55, 87324, 2, 42389479, 9089080} + // Instantiate 5 ics val updates for use in test initialValUpdates := []abci.ValidatorUpdate{ {Power: 55, PubKey: cIds[5].TMProtoCryptoPublicKey()}, @@ -41,7 +42,7 @@ func TestChangeoverToConsumer(t *testing.T) { testCases := []struct { name string - // Last standalone validators that will be mock returned from stakingKeeper.GetLastValidators() + // Last standalone validators that will be mock returned from consumerKeeper.GetLastBondedValidators() lastSovVals []stakingtypes.Validator // Val updates corresponding to initial valset set for ccv set initGenesis initialValUpdates []abci.ValidatorUpdate @@ -100,10 +101,13 @@ func TestChangeoverToConsumer(t *testing.T) { // Set initial valset, as would be done in InitGenesis consumerKeeper.SetInitialValSet(ctx, tc.initialValUpdates) - // Setup mocked return value for stakingKeeper.GetLastValidators() - gomock.InOrder( - mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return(tc.lastSovVals), - ) + // Setup mocked return value for consumerkeeper.GetLastBondedValidators() + uthelpers.SetupMocksForLastBondedValidatorsExpectation( + mocks.MockStakingKeeper, + 180, // max validators + tc.lastSovVals, + powers, + -1) // any times // Add ref to standalone staking keeper consumerKeeper.SetStandaloneStakingKeeper(mocks.MockStakingKeeper) diff --git a/x/ccv/consumer/keeper/keeper.go b/x/ccv/consumer/keeper/keeper.go index b8751344b8..9dde578a4e 100644 --- a/x/ccv/consumer/keeper/keeper.go +++ b/x/ccv/consumer/keeper/keeper.go @@ -331,7 +331,7 @@ func (k Keeper) GetLastStandaloneValidators(ctx sdk.Context) []stakingtypes.Vali if !k.IsPreCCV(ctx) || k.standaloneStakingKeeper == nil { panic("cannot get last standalone validators if not in pre-ccv state, or if standalone staking keeper is nil") } - return k.standaloneStakingKeeper.GetLastValidators(ctx) + return k.GetLastBondedValidators(ctx) } // GetElapsedPacketMaturityTimes returns a slice of already elapsed PacketMaturityTimes, sorted by maturity times, @@ -690,3 +690,9 @@ func (k Keeper) IsPrevStandaloneChain(ctx sdk.Context) bool { store := ctx.KVStore(k.storeKey) return store.Has(types.PrevStandaloneChainKey()) } + +// GetLastBondedValidators iterates the last validator powers in the staking module +// and returns the first MaxValidators many validators with the largest powers. +func (k Keeper) GetLastBondedValidators(ctx sdk.Context) []stakingtypes.Validator { + return ccv.GetLastBondedValidatorsUtil(ctx, k.standaloneStakingKeeper, k.Logger(ctx)) +} diff --git a/x/ccv/consumer/keeper/keeper_test.go b/x/ccv/consumer/keeper/keeper_test.go index 19856211b5..4b9a59fbba 100644 --- a/x/ccv/consumer/keeper/keeper_test.go +++ b/x/ccv/consumer/keeper/keeper_test.go @@ -184,11 +184,14 @@ func TestGetLastSovereignValidators(t *testing.T) { cId1 := crypto.NewCryptoIdentityFromIntSeed(11) val := cId1.SDKStakingValidator() val.Description.Moniker = "sanity check this is the correctly serialized val" - gomock.InOrder( - mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{ - val, - }), + testkeeper.SetupMocksForLastBondedValidatorsExpectation( + mocks.MockStakingKeeper, + 180, + []stakingtypes.Validator{val}, + []int64{1000}, + 1, ) + lastSovVals := ck.GetLastStandaloneValidators(ctx) require.Equal(t, []stakingtypes.Validator{val}, lastSovVals) require.Equal(t, "sanity check this is the correctly serialized val", diff --git a/x/ccv/provider/keeper/grpc_query.go b/x/ccv/provider/keeper/grpc_query.go index ee00556f06..4b6fbd0b90 100644 --- a/x/ccv/provider/keeper/grpc_query.go +++ b/x/ccv/provider/keeper/grpc_query.go @@ -70,7 +70,7 @@ func (k Keeper) GetConsumerChain(ctx sdk.Context, chainID string) (types.Chain, // Get MinPowerInTop_N var minPowerInTopN int64 if found && topN > 0 { - res, err := k.ComputeMinPowerToOptIn(ctx, k.stakingKeeper.GetLastValidators(ctx), topN) + res, err := k.ComputeMinPowerToOptIn(ctx, k.GetLastBondedValidators(ctx), topN) if err != nil { return types.Chain{}, fmt.Errorf("failed to compute min power to opt in for chain (%s): %w", chainID, err) } @@ -381,7 +381,7 @@ func (k Keeper) hasToValidate( } // if the validator was not part of the last epoch, check if the validator is going to be part of te next epoch - bondedValidators := k.stakingKeeper.GetLastValidators(ctx) + bondedValidators := k.GetLastBondedValidators(ctx) if topN, found := k.GetTopN(ctx, chainID); found && topN > 0 { // in a Top-N chain, we automatically opt in all validators that belong to the top N minPower, err := k.ComputeMinPowerToOptIn(ctx, bondedValidators, topN) @@ -395,7 +395,7 @@ func (k Keeper) hasToValidate( // if the validator is opted in and belongs to the validators of the next epoch, then if nothing changes // the validator would have to validate in the next epoch if k.IsOptedIn(ctx, chainID, provAddr) { - nextValidators := k.ComputeNextValidators(ctx, chainID, k.stakingKeeper.GetLastValidators(ctx)) + nextValidators := k.ComputeNextValidators(ctx, chainID, bondedValidators) for _, v := range nextValidators { consAddr := sdk.ConsAddress(v.ProviderConsAddr) if provAddr.ToSdkConsAddr().Equals(consAddr) { diff --git a/x/ccv/provider/keeper/grpc_query_test.go b/x/ccv/provider/keeper/grpc_query_test.go index 4f3825238f..4d59a140b5 100644 --- a/x/ccv/provider/keeper/grpc_query_test.go +++ b/x/ccv/provider/keeper/grpc_query_test.go @@ -184,7 +184,7 @@ func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) { valConsAddr, _ := val.GetConsAddr() providerAddr := types.NewProviderConsAddress(valConsAddr) mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valConsAddr).Return(val, true).AnyTimes() - mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{val}).AnyTimes() + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{val}, []int64{1}, -1) // -1 to allow the calls "AnyTimes" req := types.QueryConsumerChainsValidatorHasToValidateRequest{ ProviderAddress: providerAddr.String(), @@ -203,14 +203,15 @@ func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) { ConsumerPublicKey: &crypto.PublicKey{ Sum: &crypto.PublicKey_Ed25519{ Ed25519: []byte{1}, - }}}) + }, + }, + }) // set `providerAddr` as an opted-in validator on "chain3" pk.SetOptedIn(ctx, "chain3", providerAddr) // `providerAddr` has to validate "chain1" because it is a consumer validator in this chain, as well as "chain3" - // because it opted in, in "chain3" and `providerAddr` belongs to the bonded validators (see the mocking of `GetLastValidators` - // above) + // because it opted in, in "chain3" and `providerAddr` belongs to the bonded validators expectedChains := []string{"chain1", "chain3"} res, err := pk.QueryConsumerChainsValidatorHasToValidate(ctx, &req) @@ -268,7 +269,8 @@ func TestGetConsumerChain(t *testing.T) { {OperatorAddress: "cosmosvaloper1tflk30mq5vgqjdly92kkhhq3raev2hnz6eete3"}, // 500 power } powers := []int64{50, 150, 300, 500} // sum = 1000 - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Return(vals).AnyTimes() + maxValidators := uint32(180) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, maxValidators, vals, powers, -1) // -1 to allow the calls "AnyTimes" for i, val := range vals { mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), val.GetOperator()).Return(powers[i]).AnyTimes() diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index 1810bcc0f8..a48e70e80d 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -63,8 +63,7 @@ func (k Keeper) HandleOptOut(ctx sdk.Context, chainID string, providerAddr types "validator with consensus address %s could not be found", providerAddr.ToSdkConsAddr()) } power := k.stakingKeeper.GetLastValidatorPower(ctx, validator.GetOperator()) - minPowerToOptIn, err := k.ComputeMinPowerToOptIn(ctx, k.stakingKeeper.GetLastValidators(ctx), topN) - + minPowerToOptIn, err := k.ComputeMinPowerToOptIn(ctx, k.GetLastBondedValidators(ctx), topN) if err != nil { k.Logger(ctx).Error("failed to compute min power to opt in for chain", "chain", chainID, "error", err) return errorsmod.Wrapf( diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index cb1fbab9d4..4209f7f6ba 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -129,7 +129,7 @@ func TestHandleOptOutFromTopNChain(t *testing.T) { valDConsAddr, _ := valD.GetConsAddr() mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valDConsAddr).Return(valD, true).AnyTimes() - mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{valA, valB, valC, valD}).AnyTimes() + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 4, []stakingtypes.Validator{valA, valB, valC, valD}, []int64{1, 2, 3, 4}, -1) // -1 to allow mocks AnyTimes // opt in all validators providerKeeper.SetOptedIn(ctx, chainID, types.NewProviderConsAddress(valAConsAddr)) diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 34bb5b6971..bbb1858fa4 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -293,7 +293,7 @@ func (k Keeper) MakeConsumerGenesis( } // get the bonded validators from the staking module - bondedValidators := k.stakingKeeper.GetLastValidators(ctx) + bondedValidators := k.GetLastBondedValidators(ctx) if topN, found := k.GetTopN(ctx, chainID); found && topN > 0 { // in a Top-N chain, we automatically opt in all validators that belong to the top N diff --git a/x/ccv/provider/keeper/proposal_test.go b/x/ccv/provider/keeper/proposal_test.go index c561960a49..a7e26534b5 100644 --- a/x/ccv/provider/keeper/proposal_test.go +++ b/x/ccv/provider/keeper/proposal_test.go @@ -3,11 +3,12 @@ package keeper_test import ( "bytes" "encoding/json" - stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" "sort" "testing" "time" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" + clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" ibctmtypes "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint" _go "github.com/cosmos/ics23/go" @@ -116,7 +117,7 @@ func TestHandleConsumerAdditionProposal(t *testing.T) { if tc.expAppendProp { // Mock calls are only asserted if we expect a client to be created. - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) // returns empty validator set gomock.InOrder( testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, tc.prop.ChainId, clienttypes.NewHeight(2, 3))..., ) @@ -160,7 +161,7 @@ func TestCreateConsumerClient(t *testing.T) { description: "No state mutation, new client should be created", setup: func(providerKeeper *providerkeeper.Keeper, ctx sdk.Context, mocks *testkeeper.MockedKeepers) { // Valid client creation is asserted with mock expectations here - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) // returns empty validator set gomock.InOrder( testkeeper.GetMocksForCreateConsumerClient(ctx, mocks, "chainID", clienttypes.NewHeight(4, 5))..., ) @@ -176,7 +177,7 @@ func TestCreateConsumerClient(t *testing.T) { mocks.MockStakingKeeper.EXPECT().UnbondingTime(gomock.Any()).Times(0) mocks.MockClientKeeper.EXPECT().CreateClient(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(gomock.Any(), gomock.Any()).Times(0) - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(0) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 0) // returns empty validator set }, expClientCreated: false, }, @@ -848,7 +849,7 @@ func TestMakeConsumerGenesis(t *testing.T) { // ctx = ctx.WithChainID("testchain1") // chainID is obtained from ctx ctx = ctx.WithBlockHeight(5) // RevisionHeight obtained from ctx - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) gomock.InOrder(testkeeper.GetMocksForMakeConsumerGenesis(ctx, &mocks, 1814400000000000)...) // matches params from jsonString @@ -1094,7 +1095,8 @@ func TestBeginBlockInit(t *testing.T) { // opt in a sample validator so the chain's proposal can successfully execute validator := cryptotestutil.NewCryptoIdentityFromIntSeed(0).SDKStakingValidator() consAddr, _ := validator.GetConsAddr() - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Return([]stakingtypes.Validator{validator}).AnyTimes() + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{validator}, []int64{0}, -1) // -1 to allow any number of calls + mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), validator.GetOperator()).Return(int64(1)).AnyTimes() providerKeeper.SetOptedIn(ctx, pendingProps[4].ChainId, providertypes.NewProviderConsAddress(consAddr)) @@ -1199,7 +1201,7 @@ func TestBeginBlockCCR(t *testing.T) { expectations := []*gomock.Call{} for _, prop := range pendingProps { // A consumer chain is setup corresponding to each prop, making these mocks necessary - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) expectations = append(expectations, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, prop.ChainId, clienttypes.NewHeight(2, 3))...) expectations = append(expectations, testkeeper.GetMocksForSetConsumerChain(ctx, &mocks, prop.ChainId)...) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 852b70928b..f9ec094a70 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -218,7 +218,7 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) { valUpdateID := k.GetValidatorSetUpdateId(ctx) // current valset update ID // get the bonded validators from the staking module - bondedValidators := k.stakingKeeper.GetLastValidators(ctx) + bondedValidators := k.GetLastBondedValidators(ctx) for _, chainID := range k.GetAllRegisteredConsumerChainIDs(ctx) { currentValidators := k.GetConsumerValSet(ctx, chainID) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 5f74d93424..a4fe6e60ff 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -70,7 +70,7 @@ func TestQueueVSCPackets(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mocks := testkeeper.NewMockedKeepers(ctrl) - mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Times(1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, []int64{}, 1) pk := testkeeper.NewInMemProviderKeeper(keeperParams, mocks) // no-op if tc.packets is empty @@ -760,14 +760,16 @@ func TestEndBlockVSU(t *testing.T) { // create 4 sample lastValidators var lastValidators []stakingtypes.Validator var valAddresses []sdk.ValAddress + var powers []int64 for i := 0; i < 4; i++ { validator := crypto.NewCryptoIdentityFromIntSeed(i).SDKStakingValidator() lastValidators = append(lastValidators, validator) valAddresses = append(valAddresses, validator.GetOperator()) mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), validator.GetOperator()).Return(int64(i + 1)).AnyTimes() + powers = append(powers, int64(i+1)) } - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Return(lastValidators).AnyTimes() + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 5, lastValidators, powers, -1) // set a sample client for a consumer chain so that `GetAllConsumerChains` in `QueueVSCPackets` iterates at least once providerKeeper.SetConsumerClientId(ctx, chainID, "clientID") @@ -822,7 +824,7 @@ func TestQueueVSCPacketsWithPowerCapping(t *testing.T) { valEPubKey, _ := valE.TmConsPublicKey() mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valEConsAddr).Return(valE, true).AnyTimes() - mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{valA, valB, valC, valD, valE}).AnyTimes() + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 5, []stakingtypes.Validator{valA, valB, valC, valD, valE}, []int64{1, 3, 4, 8, 16}, -1) // add a consumer chain providerKeeper.SetConsumerClientId(ctx, "chainID", "clientID") diff --git a/x/ccv/provider/keeper/validator_set_update.go b/x/ccv/provider/keeper/validator_set_update.go index 7fa175e1b9..9ba2c961ab 100644 --- a/x/ccv/provider/keeper/validator_set_update.go +++ b/x/ccv/provider/keeper/validator_set_update.go @@ -9,6 +9,7 @@ import ( abci "github.com/cometbft/cometbft/abci/types" "github.com/cosmos/interchain-security/v4/x/ccv/provider/types" + ccv "github.com/cosmos/interchain-security/v4/x/ccv/types" ) // SetConsumerValidator sets provided consumer `validator` on the consumer chain with `chainID` @@ -189,3 +190,9 @@ func (k Keeper) FilterValidators( return nextValidators } + +// GetLastBondedValidators iterates the last validator powers in the staking module +// and returns the first MaxValidators many validators with the largest powers. +func (k Keeper) GetLastBondedValidators(ctx sdk.Context) []stakingtypes.Validator { + return ccv.GetLastBondedValidatorsUtil(ctx, k.stakingKeeper, k.Logger(ctx)) +} diff --git a/x/ccv/provider/proposal_handler_test.go b/x/ccv/provider/proposal_handler_test.go index 8d5b696b40..cd4395e38d 100644 --- a/x/ccv/provider/proposal_handler_test.go +++ b/x/ccv/provider/proposal_handler_test.go @@ -4,13 +4,14 @@ import ( "testing" "time" + sdk "github.com/cosmos/cosmos-sdk/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" - sdk "github.com/cosmos/cosmos-sdk/types" distributiontypes "github.com/cosmos/cosmos-sdk/x/distribution/types" govv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" testkeeper "github.com/cosmos/interchain-security/v4/testutil/keeper" "github.com/cosmos/interchain-security/v4/x/ccv/provider" @@ -96,7 +97,7 @@ func TestProviderProposalHandler(t *testing.T) { // Mock expectations depending on expected outcome switch { case tc.expValidConsumerAddition: - mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1) + testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 1, []stakingtypes.Validator{}, []int64{}, 1) gomock.InOrder(testkeeper.GetMocksForCreateConsumerClient( ctx, &mocks, "chainID", clienttypes.NewHeight(2, 3), )...) diff --git a/x/ccv/types/expected_keepers.go b/x/ccv/types/expected_keepers.go index 77ac40eecf..f1c543405e 100644 --- a/x/ccv/types/expected_keepers.go +++ b/x/ccv/types/expected_keepers.go @@ -52,7 +52,6 @@ type StakingKeeper interface { Delegation(ctx sdk.Context, addr sdk.AccAddress, valAddr sdk.ValAddress) stakingtypes.DelegationI MaxValidators(ctx sdk.Context) uint32 GetLastTotalPower(ctx sdk.Context) math.Int - GetLastValidators(ctx sdk.Context) (validators []stakingtypes.Validator) BondDenom(ctx sdk.Context) (res string) GetUnbondingDelegationsFromValidator(ctx sdk.Context, valAddr sdk.ValAddress) (ubds []stakingtypes.UnbondingDelegation) GetRedelegationsFromSrcValidator(ctx sdk.Context, valAddr sdk.ValAddress) (reds []stakingtypes.Redelegation) diff --git a/x/ccv/types/utils.go b/x/ccv/types/utils.go index ae85240256..cb4e86d6b2 100644 --- a/x/ccv/types/utils.go +++ b/x/ccv/types/utils.go @@ -12,10 +12,12 @@ import ( host "github.com/cosmos/ibc-go/v7/modules/core/24-host" errorsmod "cosmossdk.io/errors" + "github.com/cometbft/cometbft/libs/log" cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/bech32" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" abci "github.com/cometbft/cometbft/abci/types" tmprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" @@ -123,3 +125,39 @@ func GetConsAddrFromBech32(bech32str string) (sdk.ConsAddress, error) { } return sdk.ConsAddress(addr), nil } + +func GetLastBondedValidatorsUtil(ctx sdk.Context, stakingKeeper StakingKeeper, logger log.Logger) []stakingtypes.Validator { + maxVals := stakingKeeper.MaxValidators(ctx) + + lastPowers := make([]stakingtypes.LastValidatorPower, maxVals) + + i := 0 + stakingKeeper.IterateLastValidatorPowers(ctx, func(addr sdk.ValAddress, power int64) (stop bool) { + lastPowers[i] = stakingtypes.LastValidatorPower{Address: addr.String(), Power: power} + i++ + return i >= int(maxVals) // stop iteration if true + }) + + // truncate the lastPowers + lastPowers = lastPowers[:i] + + bondedValidators := make([]stakingtypes.Validator, len(lastPowers)) + + for index, p := range lastPowers { + addr, err := sdk.ValAddressFromBech32(p.Address) + if err != nil { + logger.Error("Invalid validator address", "address", p.Address, "error", err) + continue + } + + val, found := stakingKeeper.GetValidator(ctx, addr) + if !found { + logger.Error("Validator not found", "address", addr.String()) + continue + } + + // gather all the bonded validators in order to construct the consumer validator set for consumer chain `chainID` + bondedValidators[index] = val + } + return bondedValidators +}