diff --git a/x/ccv/provider/keeper/grpc_query.go b/x/ccv/provider/keeper/grpc_query.go index 5408cc606a..4a167599ec 100644 --- a/x/ccv/provider/keeper/grpc_query.go +++ b/x/ccv/provider/keeper/grpc_query.go @@ -437,29 +437,29 @@ func (k Keeper) hasToValidate( if err != nil { return false, nil } + + minPowerToOptIn := int64(0) + // If the consumer is TopN compute the minimum power if topN := k.GetTopN(ctx, consumerId); topN > 0 { - // in a Top-N chain, we automatically opt in all validators that belong to the top N - minPower, found := k.GetMinimumPowerInTopN(ctx, consumerId) - if found { - k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower) - } else { - k.Logger(ctx).Error("did not find min power in top N for chain", "chain", consumerId) + // compute the minimum power to opt-in since the one in the state is stale + // Note that the effective min power will be computed at the end of the epoch + minPowerToOptIn, err = k.ComputeMinPowerInTopN(ctx, activeValidators, topN) + if err != nil { + return false, err } } - // if the validator is opted in and belongs to the validators of the next epoch, then if nothing changes + // if the validator 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, consumerId, provAddr) { - lastVals, err := k.GetLastBondedValidators(ctx) - if err != nil { - return false, err - } - nextValidators := k.ComputeNextValidators(ctx, consumerId, lastVals) - for _, v := range nextValidators { - consAddr := sdk.ConsAddress(v.ProviderConsAddr) - if provAddr.ToSdkConsAddr().Equals(consAddr) { - return true, nil - } + lastVals, err := k.GetLastBondedValidators(ctx) + if err != nil { + return false, err + } + nextValidators := k.ComputeNextValidators(ctx, consumerId, lastVals, minPowerToOptIn) + for _, v := range nextValidators { + consAddr := sdk.ConsAddress(v.ProviderConsAddr) + if provAddr.ToSdkConsAddr().Equals(consAddr) { + return true, nil } } diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index 305f0f358b..6787429a6f 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -137,7 +137,6 @@ func (k Keeper) OptInTopNValidators(ctx sdk.Context, consumerId string, bondedVa // if validator already exists it gets overwritten k.SetOptedIn(ctx, consumerId, types.NewProviderConsAddress(consAddr)) - k.SetOptedIn(ctx, consumerId, types.NewProviderConsAddress(consAddr)) } // else validators that do not belong to the top N validators but were opted in, remain opted in } } @@ -322,9 +321,17 @@ func NoMoreThanPercentOfTheSum(validators []types.ConsensusValidator, percent ui // CanValidateChain returns true if the validator `providerAddr` is opted-in to chain with `consumerId` and the allowlist // and denylist do not prevent the validator from validating the chain. -func (k Keeper) CanValidateChain(ctx sdk.Context, consumerId string, providerAddr types.ProviderConsAddress) bool { +func (k Keeper) CanValidateChain(ctx sdk.Context, consumerId string, providerAddr types.ProviderConsAddress, minPowerToOptIn int64) bool { + // check if the validator is already opted-in + optedIn := k.IsOptedIn(ctx, consumerId, providerAddr) + + // check if the validator is automatically opted-in for a topN chain + if !optedIn && k.GetTopN(ctx, consumerId) > 0 { + optedIn = k.HasMinPower(ctx, providerAddr, minPowerToOptIn) + } + // only consider opted-in validators - return k.IsOptedIn(ctx, consumerId, providerAddr) && + return optedIn && // if an allowlist is declared, only consider allowlisted validators (k.IsAllowlistEmpty(ctx, consumerId) || k.IsAllowlisted(ctx, consumerId, providerAddr)) && @@ -352,7 +359,7 @@ func (k Keeper) FulfillsMinStake(ctx sdk.Context, consumerId string, providerAdd } // ComputeNextValidators computes the validators for the upcoming epoch based on the currently `bondedValidators`. -func (k Keeper) ComputeNextValidators(ctx sdk.Context, consumerId string, bondedValidators []stakingtypes.Validator) []types.ConsensusValidator { +func (k Keeper) ComputeNextValidators(ctx sdk.Context, consumerId string, bondedValidators []stakingtypes.Validator, minPowerToOptIn int64) []types.ConsensusValidator { // sort the bonded validators by number of staked tokens in descending order sort.Slice(bondedValidators, func(i, j int) bool { return bondedValidators[i].GetBondedTokens().GT(bondedValidators[j].GetBondedTokens()) @@ -371,9 +378,49 @@ func (k Keeper) ComputeNextValidators(ctx sdk.Context, consumerId string, bonded nextValidators := k.FilterValidators(ctx, consumerId, bondedValidators, func(providerAddr types.ProviderConsAddress) bool { - return k.CanValidateChain(ctx, consumerId, providerAddr) && k.FulfillsMinStake(ctx, consumerId, providerAddr) + return k.CanValidateChain(ctx, consumerId, providerAddr, minPowerToOptIn) && k.FulfillsMinStake(ctx, consumerId, providerAddr) }) nextValidators = k.CapValidatorSet(ctx, consumerId, nextValidators) return k.CapValidatorsPower(ctx, consumerId, nextValidators) } + +// HasMinPower returns true if the `providerAddr` voting power is GTE than the given minimum power +func (k Keeper) HasMinPower(ctx sdk.Context, providerAddr types.ProviderConsAddress, minPower int64) bool { + val, err := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerAddr.Address) + if err != nil { + k.Logger(ctx).Error( + "cannot get last validator power", + "provider address", + providerAddr, + "error", + err, + ) + return false + } + + valAddr, err := sdk.ValAddressFromBech32(val.GetOperator()) + if err != nil { + k.Logger(ctx).Error( + "could not retrieve validator address", + "operator address", + val.GetOperator(), + "error", + err, + ) + return false + } + + power, err := k.stakingKeeper.GetLastValidatorPower(ctx, valAddr) + if err != nil { + k.Logger(ctx).Error("could not retrieve last power of validator address", + "operator address", + val.GetOperator(), + "error", + err, + ) + return false + } + + return power >= minPower +} diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index c710e7f299..671150f865 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -2,6 +2,7 @@ package keeper_test import ( "bytes" + "fmt" "sort" "testing" @@ -384,29 +385,45 @@ func TestCanValidateChain(t *testing.T) { providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) defer ctrl.Finish() - validator := createStakingValidator(ctx, mocks, 0, 1, 1) + consumerID := "0" + + validator := createStakingValidator(ctx, mocks, 0, 1, 1) // adds GetLastValidatorPower expectation to mocks consAddr, _ := validator.GetConsAddr() providerAddr := types.NewProviderConsAddress(consAddr) // with no allowlist or denylist, the validator has to be opted in, in order to consider it - require.False(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr)) - providerKeeper.SetOptedIn(ctx, "consumerId", types.NewProviderConsAddress(consAddr)) - require.True(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr)) + require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0)) + + // with TopN chains, the validator can be considered, + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), providerAddr.Address).Return(validator, nil).Times(2) + providerKeeper.SetConsumerPowerShapingParameters(ctx, consumerID, types.PowerShapingParameters{Top_N: 50}) + // validator's power is LT the min power + require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 2)) + // validator's power is GTE the min power + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) + + // when validator is opted-in it can validate regardless of its min power + providerKeeper.SetOptedIn(ctx, consumerID, types.NewProviderConsAddress(consAddr)) + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 2)) + + // With OptIn chains, validator can validate only if it has already opted-in + providerKeeper.SetConsumerPowerShapingParameters(ctx, consumerID, types.PowerShapingParameters{Top_N: 0}) + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 2)) // create an allow list but do not add the validator `providerAddr` to it validatorA := createStakingValidator(ctx, mocks, 1, 1, 2) consAddrA, _ := validatorA.GetConsAddr() - providerKeeper.SetAllowlist(ctx, "consumerId", types.NewProviderConsAddress(consAddrA)) - require.False(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr)) - providerKeeper.SetAllowlist(ctx, "consumerId", types.NewProviderConsAddress(consAddr)) - require.True(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr)) + providerKeeper.SetAllowlist(ctx, consumerID, types.NewProviderConsAddress(consAddrA)) + require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) + providerKeeper.SetAllowlist(ctx, consumerID, types.NewProviderConsAddress(consAddr)) + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) // create a denylist but do not add validator `providerAddr` to it - providerKeeper.SetDenylist(ctx, "consumerId", types.NewProviderConsAddress(consAddrA)) - require.True(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr)) + providerKeeper.SetDenylist(ctx, consumerID, types.NewProviderConsAddress(consAddrA)) + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) // add validator `providerAddr` to the denylist - providerKeeper.SetDenylist(ctx, "consumerId", types.NewProviderConsAddress(consAddr)) - require.False(t, providerKeeper.CanValidateChain(ctx, "consumerId", providerAddr)) + providerKeeper.SetDenylist(ctx, consumerID, types.NewProviderConsAddress(consAddr)) + require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) } func TestCapValidatorSet(t *testing.T) { @@ -820,7 +837,7 @@ func TestIfInactiveValsDisallowedProperty(t *testing.T) { providerKeeper.SetParams(ctx, params) // Compute the next validators - nextVals := providerKeeper.ComputeNextValidators(ctx, "consumerId", vals) + nextVals := providerKeeper.ComputeNextValidators(ctx, "consumerId", vals, 0) // Check that the length of nextVals is at most maxProviderConsensusVals require.LessOrEqual(r, len(nextVals), int(maxProviderConsensusVals), "The length of nextVals should be at most maxProviderConsensusVals") @@ -843,3 +860,73 @@ func TestIfInactiveValsDisallowedProperty(t *testing.T) { } }) } + +func TestHasMinPower(t *testing.T) { + pk, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + + providerConsPubKey := ed25519.GenPrivKeyFromSecret([]byte{1}).PubKey() + consAddr := sdk.ConsAddress(providerConsPubKey.Address()) + providerAddr := types.NewProviderConsAddress(consAddr) + + testCases := []struct { + name string + minPower uint64 + expectation func(sdk.ConsAddress, testkeeper.MockedKeepers) + hasMinPower bool + }{ + { + name: "cannot find validator by cons address", + expectation: func(sdk.ConsAddress, testkeeper.MockedKeepers) { + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr). + Return(stakingtypes.Validator{}, fmt.Errorf("validator not found")).Times(1) + }, + hasMinPower: false, + }, { + name: "cannot convert validator operator address", + expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) { + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr).Return(stakingtypes.Validator{OperatorAddress: "xxxx"}, nil).Times(1) + }, + hasMinPower: false, + }, { + name: "cannot find last validator power", + expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) { + valAddr := sdk.ValAddress(providerAddr.Address.Bytes()) + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr). + Return(stakingtypes.Validator{OperatorAddress: valAddr.String()}, nil) + mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr). + Return(int64(0), fmt.Errorf("last power not found")).Times(1) + }, + hasMinPower: false, + }, { + name: "validator power is LT min power", + expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) { + valAddr := sdk.ValAddress(providerAddr.Address.Bytes()) + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr). + Return(stakingtypes.Validator{OperatorAddress: valAddr.String()}, nil) + mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr). + Return(int64(5), nil).Times(1) + }, + hasMinPower: false, + }, { + name: "validator power is GTE min power", + expectation: func(consAddr sdk.ConsAddress, mocks testkeeper.MockedKeepers) { + valAddr := sdk.ValAddress(providerAddr.Address.Bytes()) + mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(gomock.Any(), consAddr). + Return(stakingtypes.Validator{OperatorAddress: valAddr.String()}, nil) + mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr). + Return(int64(10), nil).Times(1) + }, + hasMinPower: true, + }, + } + + minPower := int64(10) + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.expectation(consAddr, mocks) + require.Equal(t, tc.hasMinPower, pk.HasMinPower(ctx, providerAddr, minPower)) + }) + } +} diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 8361eb0dbb..f170ed9bdd 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -232,6 +232,7 @@ func (k Keeper) MakeConsumerGenesis( return gen, nil, errorsmod.Wrapf(stakingtypes.ErrNoValidatorFound, "error getting last bonded validators: %s", err) } + minPower := int64(0) if powerShapingParameters.Top_N > 0 { // get the consensus active validators // we do not want to base the power calculation for the top N @@ -243,7 +244,7 @@ func (k Keeper) MakeConsumerGenesis( } // in a Top-N chain, we automatically opt in all validators that belong to the top N - minPower, err := k.ComputeMinPowerInTopN(ctx, activeValidators, powerShapingParameters.Top_N) + minPower, err = k.ComputeMinPowerInTopN(ctx, activeValidators, powerShapingParameters.Top_N) if err != nil { return gen, nil, err } @@ -255,8 +256,9 @@ func (k Keeper) MakeConsumerGenesis( k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower) k.SetMinimumPowerInTopN(ctx, consumerId, minPower) } + // need to use the bondedValidators, not activeValidators, here since the chain might be opt-in and allow inactive vals - nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators) + nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators, minPower) k.SetConsumerValSet(ctx, consumerId, nextValidators) // get the initial updates with the latest set consumer public keys diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index b0e8bb4e83..cfa8ec5a9f 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -199,6 +199,7 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) { } topN := k.GetTopN(ctx, consumerId) + minPower := int64(0) if topN > 0 { // in a Top-N chain, we automatically opt in all validators that belong to the top N // of the active validators @@ -208,7 +209,7 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) { panic(fmt.Errorf("failed to get active validators: %w", err)) } - minPower, err := k.ComputeMinPowerInTopN(ctx, activeValidators, topN) + minPower, err = k.ComputeMinPowerInTopN(ctx, activeValidators, topN) if err != nil { // we panic, since the only way to proceed would be to opt in all validators, which is not the intended behavior panic(fmt.Errorf("failed to compute min power to opt in for chain %v: %w", consumerId, err)) @@ -220,7 +221,7 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) { k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower) } - nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators) + nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators, minPower) valUpdates := DiffValidators(currentValidators, nextValidators) k.SetConsumerValSet(ctx, consumerId, nextValidators)