feat!: remove functionality that stops validators to opt-in on multiple chains with same chain ID (#2249)

* refactor: move opt-in methods to partial_set_security.go

* refactor: move methods to power-shaping.go

* refactor: move ComputeNextValidators to validator_set_update.go

* handle error in test

* remove ProviderConsAddrToOptedInConsumerIds index

* make ComputeNextValidators return error

* remove logger from GetLastBondedValidatorsUtil

Author: mpoke

tests/mbt/driver/setup.go

@@ -377,8 +377,9 @@ func (s *Driver) ConfigureNewPath(consumerChain, providerChain *ibctesting.TestC
 		stakingValidators = append(stakingValidators, v)
 	}
 
-	considerAll := func(providerAddr providertypes.ProviderConsAddress) bool { return true }
-	nextValidators := s.providerKeeper().FilterValidators(s.providerCtx(), string(consumerChainId), stakingValidators, considerAll)
+	considerAll := func(providerAddr providertypes.ProviderConsAddress) (bool, error) { return true, nil }
+	nextValidators, err := s.providerKeeper().FilterValidators(s.providerCtx(), string(consumerChainId), stakingValidators, considerAll)
+	require.NoError(s.t, err)
 	err = s.providerKeeper().SetConsumerValSet(s.providerCtx(), string(consumerChainId), nextValidators)
 	require.NoError(s.t, err)
 

testutil/ibc_testing/generic_setup.go

@@ -176,8 +176,6 @@ func AddConsumer[Tp testutil.ProviderApp, Tc testutil.ConsumerApp](
 	for _, v := range lastVals {
 		consAddr, _ := v.GetConsAddr()
 		providerKeeper.SetOptedIn(providerChain.GetContext(), consumerId, providertypes.NewProviderConsAddress(consAddr))
-		err = providerKeeper.AppendOptedInConsumerId(providerChain.GetContext(), providertypes.NewProviderConsAddress(consAddr), consumerId)
-		s.Require().NoError(err)
 	}
 
 	// commit the state on the provider chain

x/ccv/consumer/keeper/keeper.go

@@ -720,5 +720,5 @@ func (k Keeper) GetLastBondedValidators(ctx sdk.Context) ([]stakingtypes.Validat
 	if err != nil {
 		return nil, err
 	}
-	return ccv.GetLastBondedValidatorsUtil(ctx, k.standaloneStakingKeeper, k.Logger(ctx), maxVals)
+	return ccv.GetLastBondedValidatorsUtil(ctx, k.standaloneStakingKeeper, maxVals)
 }

x/ccv/provider/keeper/consumer_lifecycle.go

@@ -335,10 +335,13 @@ func (k Keeper) MakeConsumerGenesis(
 	}
 
 	// 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, powerShapingParameters, minPower)
+	nextValidators, err := k.ComputeNextValidators(ctx, consumerId, bondedValidators, powerShapingParameters, minPower)
+	if err != nil {
+		return gen, nil, fmt.Errorf("unable to compute the next validators in MakeConsumerGenesis, consumerId(%s): %w", consumerId, err)
+	}
 	err = k.SetConsumerValSet(ctx, consumerId, nextValidators)
 	if err != nil {
-		return gen, nil, fmt.Errorf("unable to set consumer validator set in MakeConsumerGenesis: %s", err)
+		return gen, nil, fmt.Errorf("unable to set consumer validator set in MakeConsumerGenesis, consumerId(%s): %w", consumerId, err)
 	}
 
 	// get the initial updates with the latest set consumer public keys

x/ccv/provider/keeper/distribution_test.go

@@ -330,3 +330,50 @@ func TestChangeRewardDenoms(t *testing.T) {
 	attributes = keeper.ChangeRewardDenoms(ctx, denomsToAdd, denomsToRemove)
 	require.Len(t, attributes, 0) // No attributes should be returned since the denom is not registered
 }
+
+func TestHandleSetConsumerCommissionRate(t *testing.T) {
+	providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
+	defer ctrl.Finish()
+
+	providerAddr := providertypes.NewProviderConsAddress([]byte("providerAddr"))
+
+	// trying to set a commission rate to a unknown consumer chain
+	require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, "unknownChainID", providerAddr, math.LegacyZeroDec()))
+
+	// setup a pending consumer chain
+	consumerId := "0"
+	providerKeeper.FetchAndIncrementConsumerId(ctx)
+	providerKeeper.SetConsumerPhase(ctx, consumerId, providertypes.CONSUMER_PHASE_INITIALIZED)
+
+	// check that there's no commission rate set for the validator yet
+	_, found := providerKeeper.GetConsumerCommissionRate(ctx, consumerId, providerAddr)
+	require.False(t, found)
+
+	mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(math.LegacyZeroDec(), nil).Times(1)
+	require.NoError(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, consumerId, providerAddr, math.LegacyOneDec()))
+
+	// check that the commission rate is now set
+	cr, found := providerKeeper.GetConsumerCommissionRate(ctx, consumerId, providerAddr)
+	require.Equal(t, math.LegacyOneDec(), cr)
+	require.True(t, found)
+
+	// set minimum rate of 1/2
+	commissionRate := math.LegacyNewDec(1).Quo(math.LegacyNewDec(2))
+	mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(commissionRate, nil).AnyTimes()
+
+	// try to set a rate slightly below the minimum
+	require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(
+		ctx,
+		consumerId,
+		providerAddr,
+		commissionRate.Sub(math.LegacyNewDec(1).Quo(math.LegacyNewDec(100)))), // 0.5 - 0.01
+		"commission rate should be rejected (below min), but is not",
+	)
+
+	// set a valid commission equal to the minimum
+	require.NoError(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, consumerId, providerAddr, commissionRate))
+	// check that the rate was set
+	cr, found = providerKeeper.GetConsumerCommissionRate(ctx, consumerId, providerAddr)
+	require.Equal(t, commissionRate, cr)
+	require.True(t, found)
+}

x/ccv/provider/keeper/grpc_query.go

@@ -348,7 +348,10 @@ func (k Keeper) QueryConsumerValidators(goCtx context.Context, req *types.QueryC
 			}
 		}
 
-		consumerValSet = k.ComputeNextValidators(ctx, consumerId, bondedValidators, powerShapingParameters, minPower)
+		consumerValSet, err = k.ComputeNextValidators(ctx, consumerId, bondedValidators, powerShapingParameters, minPower)
+		if err != nil {
+			return nil, status.Error(codes.Internal, fmt.Sprintf("failed to compute the next validators for chain %s: %s", consumerId, err))
+		}
 
 		// sort the address of the validators by ascending lexical order as they were persisted to the store
 		sort.Slice(consumerValSet, func(i, j int) bool {
@@ -476,7 +479,10 @@ func (k Keeper) hasToValidate(
 	if err != nil {
 		return false, err
 	}
-	nextValidators := k.ComputeNextValidators(ctx, consumerId, lastVals, powerShapingParameters, minPowerToOptIn)
+	nextValidators, err := k.ComputeNextValidators(ctx, consumerId, lastVals, powerShapingParameters, minPowerToOptIn)
+	if err != nil {
+		return false, err
+	}
 	for _, v := range nextValidators {
 		consAddr := sdk.ConsAddress(v.ProviderConsAddr)
 		if provAddr.ToSdkConsAddr().Equals(consAddr) {

x/ccv/provider/keeper/keeper.go

@@ -735,69 +735,6 @@ func (k Keeper) GetAllActiveConsumerIds(ctx sdk.Context) []string {
 	return consumerIds
 }
 
-func (k Keeper) SetOptedIn(
-	ctx sdk.Context,
-	consumerId string,
-	providerConsAddress types.ProviderConsAddress,
-) {
-	store := ctx.KVStore(k.storeKey)
-	store.Set(types.OptedInKey(consumerId, providerConsAddress), []byte{})
-}
-
-func (k Keeper) DeleteOptedIn(
-	ctx sdk.Context,
-	consumerId string,
-	providerAddr types.ProviderConsAddress,
-) {
-	store := ctx.KVStore(k.storeKey)
-	store.Delete(types.OptedInKey(consumerId, providerAddr))
-}
-
-func (k Keeper) IsOptedIn(
-	ctx sdk.Context,
-	consumerId string,
-	providerAddr types.ProviderConsAddress,
-) bool {
-	store := ctx.KVStore(k.storeKey)
-	return store.Get(types.OptedInKey(consumerId, providerAddr)) != nil
-}
-
-// GetAllOptedIn returns all the opted-in validators on chain `consumerId`
-func (k Keeper) GetAllOptedIn(
-	ctx sdk.Context,
-	consumerId string,
-) (providerConsAddresses []types.ProviderConsAddress) {
-	store := ctx.KVStore(k.storeKey)
-	key := types.StringIdWithLenKey(types.OptedInKeyPrefix(), consumerId)
-	iterator := storetypes.KVStorePrefixIterator(store, key)
-	defer iterator.Close()
-
-	for ; iterator.Valid(); iterator.Next() {
-		providerConsAddresses = append(providerConsAddresses, types.NewProviderConsAddress(iterator.Key()[len(key):]))
-	}
-
-	return providerConsAddresses
-}
-
-// DeleteAllOptedIn deletes all the opted-in validators for chain with `consumerId`
-func (k Keeper) DeleteAllOptedIn(
-	ctx sdk.Context,
-	consumerId string,
-) {
-	store := ctx.KVStore(k.storeKey)
-	key := types.StringIdWithLenKey(types.OptedInKeyPrefix(), consumerId)
-	iterator := storetypes.KVStorePrefixIterator(store, key)
-
-	var keysToDel [][]byte
-	defer iterator.Close()
-	for ; iterator.Valid(); iterator.Next() {
-		keysToDel = append(keysToDel, iterator.Key())
-	}
-	for _, delKey := range keysToDel {
-		store.Delete(delKey)
-	}
-}
-
 // SetConsumerCommissionRate sets a per-consumer chain commission rate
 // for the given validator address
 func (k Keeper) SetConsumerCommissionRate(

x/ccv/provider/keeper/keeper_test.go

@@ -1,7 +1,6 @@
 package keeper_test
 
 import (
-	"bytes"
 	"fmt"
 	"sort"
 	"testing"
@@ -293,56 +292,6 @@ func TestSetSlashLog(t *testing.T) {
 	require.False(t, providerKeeper.GetSlashLog(ctx, addrWithoutDoubleSigns))
 }
 
-func TestGetAllOptedIn(t *testing.T) {
-	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
-	defer ctrl.Finish()
-
-	expectedOptedInValidators := []providertypes.ProviderConsAddress{
-		providertypes.NewProviderConsAddress([]byte("providerAddr1")),
-		providertypes.NewProviderConsAddress([]byte("providerAddr2")),
-		providertypes.NewProviderConsAddress([]byte("providerAddr3")),
-	}
-
-	for _, expectedOptedInValidator := range expectedOptedInValidators {
-		providerKeeper.SetOptedIn(ctx, CONSUMER_ID, expectedOptedInValidator)
-	}
-
-	actualOptedInValidators := providerKeeper.GetAllOptedIn(ctx, CONSUMER_ID)
-
-	// sort validators first to be able to compare
-	sortOptedInValidators := func(addresses []providertypes.ProviderConsAddress) {
-		sort.Slice(addresses, func(i, j int) bool {
-			return bytes.Compare(addresses[i].ToSdkConsAddr(), addresses[j].ToSdkConsAddr()) < 0
-		})
-	}
-	sortOptedInValidators(expectedOptedInValidators)
-	sortOptedInValidators(actualOptedInValidators)
-	require.Equal(t, expectedOptedInValidators, actualOptedInValidators)
-}
-
-// TestOptedIn tests the `SetOptedIn`, `IsOptedIn`, `DeleteOptedIn` and `DeleteAllOptedIn` methods
-func TestOptedIn(t *testing.T) {
-	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
-	defer ctrl.Finish()
-
-	optedInValidator1 := providertypes.NewProviderConsAddress([]byte("providerAddr1"))
-	optedInValidator2 := providertypes.NewProviderConsAddress([]byte("providerAddr2"))
-
-	require.False(t, providerKeeper.IsOptedIn(ctx, CONSUMER_ID, optedInValidator1))
-	providerKeeper.SetOptedIn(ctx, CONSUMER_ID, optedInValidator1)
-	require.True(t, providerKeeper.IsOptedIn(ctx, CONSUMER_ID, optedInValidator1))
-	providerKeeper.DeleteOptedIn(ctx, CONSUMER_ID, optedInValidator1)
-	require.False(t, providerKeeper.IsOptedIn(ctx, CONSUMER_ID, optedInValidator1))
-
-	providerKeeper.SetOptedIn(ctx, CONSUMER_ID, optedInValidator1)
-	providerKeeper.SetOptedIn(ctx, CONSUMER_ID, optedInValidator2)
-	require.True(t, providerKeeper.IsOptedIn(ctx, CONSUMER_ID, optedInValidator1))
-	require.True(t, providerKeeper.IsOptedIn(ctx, CONSUMER_ID, optedInValidator2))
-	providerKeeper.DeleteAllOptedIn(ctx, CONSUMER_ID)
-	require.False(t, providerKeeper.IsOptedIn(ctx, CONSUMER_ID, optedInValidator1))
-	require.False(t, providerKeeper.IsOptedIn(ctx, CONSUMER_ID, optedInValidator2))
-}
-
 // TestConsumerCommissionRate tests the `SetConsumerCommissionRate`, `GetConsumerCommissionRate`, and `DeleteConsumerCommissionRate` methods
 func TestConsumerCommissionRate(t *testing.T) {
 	providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))

x/ccv/provider/keeper/key_assignment_test.go

@@ -788,14 +788,15 @@ func TestSimulatedAssignmentsAndUpdateApplication(t *testing.T) {
 				})
 			}
 
-			nextValidators := k.FilterValidators(ctx, CONSUMERID, bondedValidators,
-				func(providerAddr types.ProviderConsAddress) bool {
-					return true
+			nextValidators, err := k.FilterValidators(ctx, CONSUMERID, bondedValidators,
+				func(providerAddr types.ProviderConsAddress) (bool, error) {
+					return true, nil
 				})
-			valSet, error := k.GetConsumerValSet(ctx, CONSUMERID)
-			require.NoError(t, error)
+			require.NoError(t, err)
+			valSet, err := k.GetConsumerValSet(ctx, CONSUMERID)
+			require.NoError(t, err)
 			updates = providerkeeper.DiffValidators(valSet, nextValidators)
-			err := k.SetConsumerValSet(ctx, CONSUMERID, nextValidators)
+			err = k.SetConsumerValSet(ctx, CONSUMERID, nextValidators)
 			require.NoError(t, err)
 
 			consumerValset.apply(updates)

x/ccv/provider/keeper/msg_server_test.go

@@ -1,10 +1,11 @@
 package keeper_test
 
 import (
-	"github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	"testing"
 	"time"
 
+	"github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
+
 	"github.com/stretchr/testify/require"
 
 	"github.com/cosmos/cosmos-sdk/codec/address"
@@ -242,6 +243,7 @@ func TestUpdateConsumer(t *testing.T) {
 			InitializationParameters: &expectedInitializationParameters,
 			PowerShapingParameters:   nil,
 		})
+	require.NoError(t, err)
 	// assert the chain is not scheduled to launch
 	consumerIds, err = providerKeeper.GetConsumersToBeLaunched(ctx, previousSpawnTime)
 	require.NoError(t, err)