From c07c99e44d997dfbc25de7e6d174922d27f7d8e6 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 27 Aug 2024 09:30:55 +0200 Subject: [PATCH 1/5] add UT --- x/ccv/provider/keeper/grpc_query.go | 32 +++-- x/ccv/provider/keeper/partial_set_security.go | 42 ++++++- .../keeper/partial_set_security_test.go | 113 ++++++++++++++++-- x/ccv/provider/keeper/proposal.go | 12 +- x/ccv/provider/keeper/relay.go | 5 +- 5 files changed, 162 insertions(+), 42 deletions(-) diff --git a/x/ccv/provider/keeper/grpc_query.go b/x/ccv/provider/keeper/grpc_query.go index 5408cc606a..7bfdf00fd0 100644 --- a/x/ccv/provider/keeper/grpc_query.go +++ b/x/ccv/provider/keeper/grpc_query.go @@ -437,29 +437,27 @@ 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) + 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 // 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..b6f448027c 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 opt-in for a topN chain + if !optedIn && k.GetTopN(ctx, consumerId) > 0 { + optedIn = k.HasMinPower(ctx, providerAddr, uint64(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,34 @@ 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 uint64) bool { + val, err := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerAddr.Address) + if err != nil { + k.Logger(ctx).Error("cannot get last validator power %s: %s", providerAddr, err) + return false + } + + valAddr, err := sdk.ValAddressFromBech32(val.GetOperator()) + if err != nil { + k.Logger(ctx).Error("could not retrieve validator address %s: %s", + val.GetOperator(), 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: %s: %s", + val.GetOperator(), err) + return false + } + + return uint64(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..81f362ab6c 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 is LT min power + require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 2)) + // validator is GTE min power + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) + + // when validator is opted-in it can validate regardless of it the min power + providerKeeper.SetOptedIn(ctx, consumerID, types.NewProviderConsAddress(consAddr)) + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0)) + + // With OptIn chains, validator can validator only if it's opted-in + providerKeeper.SetConsumerPowerShapingParameters(ctx, consumerID, types.PowerShapingParameters{Top_N: 0}) + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0)) // 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, 0)) + providerKeeper.SetAllowlist(ctx, consumerID, types.NewProviderConsAddress(consAddr)) + require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0)) // 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, 0)) // 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, 0)) } 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 := uint64(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..1a08aa280c 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) } + minPowerToOptIn := 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,20 +244,21 @@ 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) + minPowerToOptIn, err = k.ComputeMinPowerInTopN(ctx, activeValidators, powerShapingParameters.Top_N) if err != nil { return gen, nil, err } // log the minimum power in top N k.Logger(ctx).Info("minimum power in top N at consumer genesis", "consumerId", consumerId, - "minPower", minPower, + "minPower", minPowerToOptIn, ) - k.OptInTopNValidators(ctx, consumerId, activeValidators, minPower) - k.SetMinimumPowerInTopN(ctx, consumerId, minPower) + k.OptInTopNValidators(ctx, consumerId, activeValidators, minPowerToOptIn) + k.SetMinimumPowerInTopN(ctx, consumerId, minPowerToOptIn) } + // 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, minPowerToOptIn) 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) From 8499f3cac3f9b0423529ce7369726986cbf38c9a Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 27 Aug 2024 09:52:48 +0200 Subject: [PATCH 2/5] nits --- x/ccv/provider/keeper/partial_set_security_test.go | 8 ++++---- x/ccv/provider/keeper/proposal.go | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index 81f362ab6c..947f4bdde7 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -397,16 +397,16 @@ func TestCanValidateChain(t *testing.T) { // 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 is LT min power + // validator's power is LT the min power require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 2)) - // validator is GTE min power + // 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 it the min power + // 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, 0)) - // With OptIn chains, validator can validator only if it's opted-in + // 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, 0)) diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 1a08aa280c..f170ed9bdd 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -232,7 +232,7 @@ func (k Keeper) MakeConsumerGenesis( return gen, nil, errorsmod.Wrapf(stakingtypes.ErrNoValidatorFound, "error getting last bonded validators: %s", err) } - minPowerToOptIn := int64(0) + 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 @@ -244,21 +244,21 @@ func (k Keeper) MakeConsumerGenesis( } // in a Top-N chain, we automatically opt in all validators that belong to the top N - minPowerToOptIn, err = k.ComputeMinPowerInTopN(ctx, activeValidators, powerShapingParameters.Top_N) + minPower, err = k.ComputeMinPowerInTopN(ctx, activeValidators, powerShapingParameters.Top_N) if err != nil { return gen, nil, err } // log the minimum power in top N k.Logger(ctx).Info("minimum power in top N at consumer genesis", "consumerId", consumerId, - "minPower", minPowerToOptIn, + "minPower", minPower, ) - k.OptInTopNValidators(ctx, consumerId, activeValidators, minPowerToOptIn) - k.SetMinimumPowerInTopN(ctx, consumerId, minPowerToOptIn) + 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, minPowerToOptIn) + nextValidators := k.ComputeNextValidators(ctx, consumerId, bondedValidators, minPower) k.SetConsumerValSet(ctx, consumerId, nextValidators) // get the initial updates with the latest set consumer public keys From 687a877136d9e37ddc3cbb6dccfe362e51995fd7 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 27 Aug 2024 10:59:26 +0200 Subject: [PATCH 3/5] address comments --- x/ccv/provider/keeper/grpc_query.go | 4 ++- x/ccv/provider/keeper/partial_set_security.go | 31 ++++++++++++++----- .../keeper/partial_set_security_test.go | 2 +- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/x/ccv/provider/keeper/grpc_query.go b/x/ccv/provider/keeper/grpc_query.go index 7bfdf00fd0..4a167599ec 100644 --- a/x/ccv/provider/keeper/grpc_query.go +++ b/x/ccv/provider/keeper/grpc_query.go @@ -441,13 +441,15 @@ func (k Keeper) hasToValidate( minPowerToOptIn := int64(0) // If the consumer is TopN compute the minimum power if topN := k.GetTopN(ctx, consumerId); topN > 0 { + // 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 lastVals, err := k.GetLastBondedValidators(ctx) if err != nil { diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index b6f448027c..dcfd7be3a6 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -327,7 +327,7 @@ func (k Keeper) CanValidateChain(ctx sdk.Context, consumerId string, providerAdd // check if the validator is automatically opt-in for a topN chain if !optedIn && k.GetTopN(ctx, consumerId) > 0 { - optedIn = k.HasMinPower(ctx, providerAddr, uint64(minPowerToOptIn)) + optedIn = k.HasMinPower(ctx, providerAddr, minPowerToOptIn) } // only consider opted-in validators @@ -386,26 +386,41 @@ func (k Keeper) ComputeNextValidators(ctx sdk.Context, consumerId string, bonded } // 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 uint64) bool { +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 %s: %s", providerAddr, err) + 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 %s: %s", - val.GetOperator(), err) + 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: %s: %s", - val.GetOperator(), err) + k.Logger(ctx).Error("could not retrieve last power of validator address", + "operator address", + val.GetOperator(), + "error", + err, + ) return false } - return uint64(power) >= minPower + 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 947f4bdde7..3809fc7b10 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -921,7 +921,7 @@ func TestHasMinPower(t *testing.T) { }, } - minPower := uint64(10) + minPower := int64(10) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { From 7e1a6479534b652523ac1e73942fe22125a72b28 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 27 Aug 2024 10:59:37 +0200 Subject: [PATCH 4/5] Update x/ccv/provider/keeper/partial_set_security.go Co-authored-by: insumity --- x/ccv/provider/keeper/partial_set_security.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/ccv/provider/keeper/partial_set_security.go b/x/ccv/provider/keeper/partial_set_security.go index dcfd7be3a6..6787429a6f 100644 --- a/x/ccv/provider/keeper/partial_set_security.go +++ b/x/ccv/provider/keeper/partial_set_security.go @@ -325,7 +325,7 @@ func (k Keeper) CanValidateChain(ctx sdk.Context, consumerId string, providerAdd // check if the validator is already opted-in optedIn := k.IsOptedIn(ctx, consumerId, providerAddr) - // check if the validator is automatically opt-in for a topN chain + // 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) } From 013de320b53784d84323944068b93c6089e4a766 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 27 Aug 2024 11:11:04 +0200 Subject: [PATCH 5/5] fix tests --- x/ccv/provider/keeper/partial_set_security_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/x/ccv/provider/keeper/partial_set_security_test.go b/x/ccv/provider/keeper/partial_set_security_test.go index 3809fc7b10..671150f865 100644 --- a/x/ccv/provider/keeper/partial_set_security_test.go +++ b/x/ccv/provider/keeper/partial_set_security_test.go @@ -404,26 +404,26 @@ func TestCanValidateChain(t *testing.T) { // 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, 0)) + 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, 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, 0)) + require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) providerKeeper.SetAllowlist(ctx, consumerID, types.NewProviderConsAddress(consAddr)) - require.True(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 0)) + 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, 0)) + 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, 0)) + require.False(t, providerKeeper.CanValidateChain(ctx, consumerID, providerAddr, 1)) } func TestCapValidatorSet(t *testing.T) {