fix!: validate initial height and set default values for init consumer params

.changelog/unreleased/improvements/2357-init-params.md

@@ -0,0 +1,3 @@
+- `[x/provider]` Add validation for initial height and set
+  default values for consumer initialization params.
+  ([\#2357](https://github.com/cosmos/interchain-security/pull/2357))

.changelog/unreleased/state-breaking/2357-init-params.md

@@ -0,0 +1,3 @@
+- `[x/provider]` Add validation for initial height and set
+  default values for consumer initialization params.
+  ([\#2357](https://github.com/cosmos/interchain-security/pull/2357))

testutil/keeper/unit_test_helpers.go

@@ -223,7 +223,7 @@ func SetupForDeleteConsumerChain(t *testing.T, ctx sdk.Context,
 	t.Helper()
 
 	expectations := GetMocksForCreateConsumerClient(ctx, &mocks,
-		"chainID", clienttypes.NewHeight(4, 5))
+		"chainID", clienttypes.NewHeight(0, 5))
 	expectations = append(expectations, GetMocksForSetConsumerChain(ctx, &mocks, "chainID")...)
 
 	gomock.InOrder(expectations...)
@@ -286,7 +286,7 @@ func GetTestConsumerMetadata() providertypes.ConsumerMetadata {
 
 func GetTestInitializationParameters() providertypes.ConsumerInitializationParameters {
 	return providertypes.ConsumerInitializationParameters{
-		InitialHeight:                     clienttypes.NewHeight(4, 5),
+		InitialHeight:                     clienttypes.NewHeight(0, 5),
 		GenesisHash:                       []byte("gen_hash"),
 		BinaryHash:                        []byte("bin_hash"),
 		SpawnTime:                         time.Now().UTC(),
@@ -323,7 +323,7 @@ func GetTestMsgUpdateConsumer() providertypes.MsgUpdateConsumer {
 func GetTestMsgConsumerAddition() providertypes.MsgConsumerAddition {
 	return providertypes.MsgConsumerAddition{
 		ChainId:                           "a ChainId",
-		InitialHeight:                     clienttypes.NewHeight(4, 5),
+		InitialHeight:                     clienttypes.NewHeight(0, 5),
 		GenesisHash:                       []byte(base64.StdEncoding.EncodeToString([]byte("gen_hash"))),
 		BinaryHash:                        []byte(base64.StdEncoding.EncodeToString([]byte("bin_hash"))),
 		SpawnTime:                         time.Now(),

x/ccv/provider/keeper/consumer_lifecycle_test.go

@@ -56,7 +56,8 @@ func TestPrepareConsumerForLaunch(t *testing.T) {
 
 func TestInitializeConsumer(t *testing.T) {
 	now := time.Now().UTC()
-	consumerId := "13"
+	consumerId := CONSUMER_ID
+	chainId := CONSUMER_CHAIN_ID
 
 	testCases := []struct {
 		name           string
@@ -69,6 +70,7 @@ func TestInitializeConsumer(t *testing.T) {
 			spawnTime: now,
 			setup: func(pk *providerkeeper.Keeper, ctx sdk.Context, spawnTime time.Time) {
 				pk.SetConsumerPhase(ctx, consumerId, providertypes.CONSUMER_PHASE_REGISTERED)
+				pk.SetConsumerChainId(ctx, consumerId, chainId)
 				err := pk.SetConsumerInitializationParameters(ctx, consumerId,
 					providertypes.ConsumerInitializationParameters{
 						SpawnTime: spawnTime,
@@ -89,6 +91,7 @@ func TestInitializeConsumer(t *testing.T) {
 			spawnTime: now,
 			setup: func(pk *providerkeeper.Keeper, ctx sdk.Context, spawnTime time.Time) {
 				pk.SetConsumerPhase(ctx, consumerId, providertypes.CONSUMER_PHASE_LAUNCHED)
+				pk.SetConsumerChainId(ctx, consumerId, chainId)
 				err := pk.SetConsumerInitializationParameters(ctx, consumerId,
 					providertypes.ConsumerInitializationParameters{
 						SpawnTime: spawnTime,
@@ -110,6 +113,7 @@ func TestInitializeConsumer(t *testing.T) {
 			spawnTime: now,
 			setup: func(pk *providerkeeper.Keeper, ctx sdk.Context, spawnTime time.Time) {
 				pk.SetConsumerPhase(ctx, consumerId, providertypes.CONSUMER_PHASE_REGISTERED)
+				pk.SetConsumerChainId(ctx, consumerId, chainId)
 				err := pk.SetConsumerInitializationParameters(ctx, consumerId,
 					providertypes.ConsumerInitializationParameters{
 						SpawnTime: time.Time{},
@@ -150,7 +154,7 @@ func TestBeginBlockLaunchConsumers(t *testing.T) {
 
 	initializationParameters := []providertypes.ConsumerInitializationParameters{
 		{
-			InitialHeight:                     clienttypes.NewHeight(3, 4),
+			InitialHeight:                     clienttypes.NewHeight(0, 4),
 			GenesisHash:                       []byte{},
 			BinaryHash:                        []byte{},
 			SpawnTime:                         now.Add(-time.Hour * 2).UTC(),
@@ -163,7 +167,7 @@ func TestBeginBlockLaunchConsumers(t *testing.T) {
 			DistributionTransmissionChannel:   "",
 		},
 		{
-			InitialHeight:                     clienttypes.NewHeight(3, 4),
+			InitialHeight:                     clienttypes.NewHeight(0, 4),
 			GenesisHash:                       []byte{},
 			BinaryHash:                        []byte{},
 			SpawnTime:                         now.Add(-time.Hour).UTC(),
@@ -176,7 +180,7 @@ func TestBeginBlockLaunchConsumers(t *testing.T) {
 			DistributionTransmissionChannel:   "",
 		},
 		{
-			InitialHeight:                     clienttypes.NewHeight(3, 4),
+			InitialHeight:                     clienttypes.NewHeight(0, 4),
 			GenesisHash:                       []byte{},
 			BinaryHash:                        []byte{},
 			SpawnTime:                         now.Add(time.Hour).UTC(),
@@ -189,7 +193,7 @@ func TestBeginBlockLaunchConsumers(t *testing.T) {
 			DistributionTransmissionChannel:   "",
 		},
 		{
-			InitialHeight:                     clienttypes.NewHeight(3, 4),
+			InitialHeight:                     clienttypes.NewHeight(0, 4),
 			GenesisHash:                       []byte{},
 			BinaryHash:                        []byte{},
 			SpawnTime:                         now.Add(-time.Hour).UTC(),
@@ -202,7 +206,7 @@ func TestBeginBlockLaunchConsumers(t *testing.T) {
 			DistributionTransmissionChannel:   "",
 		},
 		{
-			InitialHeight:                     clienttypes.NewHeight(3, 4),
+			InitialHeight:                     clienttypes.NewHeight(0, 4),
 			GenesisHash:                       []byte{},
 			BinaryHash:                        []byte{},
 			SpawnTime:                         now.Add(-time.Minute).UTC(),
@@ -283,11 +287,11 @@ func TestBeginBlockLaunchConsumers(t *testing.T) {
 
 	// Expect genesis and client creation for only the first, second, and fifth chains (spawn time already passed and valid)
 	expectedCalls := testkeeper.GetMocksForMakeConsumerGenesis(ctx, &mocks, time.Hour)
-	expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain0", clienttypes.NewHeight(3, 4))...)
+	expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain0", clienttypes.NewHeight(0, 4))...)
 	expectedCalls = append(expectedCalls, testkeeper.GetMocksForMakeConsumerGenesis(ctx, &mocks, time.Hour)...)
-	expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain1", clienttypes.NewHeight(3, 4))...)
+	expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain1", clienttypes.NewHeight(0, 4))...)
 	expectedCalls = append(expectedCalls, testkeeper.GetMocksForMakeConsumerGenesis(ctx, &mocks, time.Hour)...)
-	expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain3", clienttypes.NewHeight(3, 4))...)
+	expectedCalls = append(expectedCalls, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain3", clienttypes.NewHeight(0, 4))...)
 
 	gomock.InOrder(expectedCalls...)
 
@@ -479,7 +483,7 @@ func TestCreateConsumerClient(t *testing.T) {
 
 				// Valid client creation is asserted with mock expectations here
 				gomock.InOrder(
-					testkeeper.GetMocksForCreateConsumerClient(ctx, mocks, CONSUMER_CHAIN_ID, clienttypes.NewHeight(4, 5))...,
+					testkeeper.GetMocksForCreateConsumerClient(ctx, mocks, CONSUMER_CHAIN_ID, clienttypes.NewHeight(0, 5))...,
 				)
 			},
 			expClientCreated: true,
@@ -774,7 +778,7 @@ func TestBeginBlockStopConsumers(t *testing.T) {
 		chainId := chainIds[i]
 		// A consumer chain is setup corresponding to each consumerId, making these mocks necessary
 		expectations = append(expectations, testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks,
-			chainId, clienttypes.NewHeight(2, 3))...)
+			chainId, clienttypes.NewHeight(0, 3))...)
 		expectations = append(expectations, testkeeper.GetMocksForSetConsumerChain(ctx, &mocks, chainId)...)
 	}
 	// Only first two consumer chains should be stopped
@@ -789,7 +793,7 @@ func TestBeginBlockStopConsumers(t *testing.T) {
 	for i, consumerId := range consumerIds {
 		// Setup a valid consumer chain for each consumerId
 		initializationRecord := testkeeper.GetTestInitializationParameters()
-		initializationRecord.InitialHeight = clienttypes.NewHeight(2, 3)
+		initializationRecord.InitialHeight = clienttypes.NewHeight(0, 3)
 		registrationRecord := testkeeper.GetTestConsumerMetadata()
 
 		providerKeeper.SetConsumerChainId(ctx, consumerId, chainIds[i])

x/ccv/provider/keeper/grpc_query_test.go

@@ -312,11 +312,15 @@ func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) {
 
 	// set up some consumer chains
 	for i := 0; i < consumerNum; i++ {
-		chainID := "consumer-" + strconv.Itoa(i)
+		revisionNumber := i + 1
+		chainID := "consumer-" + strconv.Itoa(revisionNumber)
 		metadata := types.ConsumerMetadata{Name: chainID}
+		initializationParameters := types.DefaultConsumerInitializationParameters()
+		initializationParameters.InitialHeight.RevisionNumber = uint64(revisionNumber)
 		msg := types.MsgCreateConsumer{
-			ChainId:  chainID,
-			Metadata: metadata,
+			ChainId:                  chainID,
+			Metadata:                 metadata,
+			InitializationParameters: &initializationParameters,
 		}
 		resp, err := msgServer.CreateConsumer(ctx, &msg)
 		require.NoError(t, err)
@@ -535,7 +539,7 @@ func TestQueryConsumerChain(t *testing.T) {
 	defer ctrl.Finish()
 
 	consumerId := "0"
-	chainId := "consumer-1"
+	chainId := "consumer"
 
 	req := types.QueryConsumerChainRequest{
 		ConsumerId: consumerId,
@@ -644,11 +648,15 @@ func TestQueryConsumerChains(t *testing.T) {
 
 	// create four consumer chains in different phase
 	for i := 0; i < consumerNum; i++ {
-		chainID := "consumer-" + strconv.Itoa(i)
+		revisionNumber := i + 1
+		chainID := "consumer-" + strconv.Itoa(revisionNumber)
 		metadata := types.ConsumerMetadata{Name: chainID}
+		initializationParameters := types.DefaultConsumerInitializationParameters()
+		initializationParameters.InitialHeight.RevisionNumber = uint64(revisionNumber)
 		msg := types.MsgCreateConsumer{
-			ChainId:  chainID,
-			Metadata: metadata,
+			ChainId:                  chainID,
+			Metadata:                 metadata,
+			InitializationParameters: &initializationParameters,
 		}
 		resp, err := msgServer.CreateConsumer(ctx, &msg)
 		require.NoError(t, err)

x/ccv/provider/keeper/msg_server.go

@@ -378,7 +378,7 @@ func (k msgServer) CreateConsumer(goCtx context.Context, msg *types.MsgCreateCon
 
 	// initialization parameters are optional and hence could be nil;
 	// in that case, set the default
-	initializationParameters := types.ConsumerInitializationParameters{} // default params
+	initializationParameters := types.DefaultConsumerInitializationParameters() // default params
 	if msg.InitializationParameters != nil {
 		initializationParameters = *msg.InitializationParameters
 	}

x/ccv/provider/keeper/msg_server_test.go

@@ -84,7 +84,7 @@ func TestUpdateConsumer(t *testing.T) {
 	// create a chain before updating it
 	createConsumerResponse, err := msgServer.CreateConsumer(ctx,
 		&providertypes.MsgCreateConsumer{
-			Submitter: "submitter", ChainId: "chainId",
+			Submitter: "submitter", ChainId: "chainId-1",
 			Metadata: providertypes.ConsumerMetadata{
 				Name:        "name",
 				Description: "description",
@@ -113,6 +113,7 @@ func TestUpdateConsumer(t *testing.T) {
 	}
 
 	expectedInitializationParameters := testkeeper.GetTestInitializationParameters()
+	expectedInitializationParameters.InitialHeight.RevisionNumber = 1
 	expectedPowerShapingParameters := testkeeper.GetTestPowerShapingParameters()
 
 	expectedOwnerAddress := "cosmos1dkas8mu4kyhl5jrh4nzvm65qz588hy9qcz08la"

x/ccv/provider/keeper/permissionless.go

@@ -134,6 +134,14 @@ func (k Keeper) SetConsumerInitializationParameters(ctx sdk.Context, consumerId
 	if err != nil {
 		return fmt.Errorf("failed to marshal initialization parameters (%+v) for consumer id (%s): %w", parameters, consumerId, err)
 	}
+	chainId, err := k.GetConsumerChainId(ctx, consumerId)
+	if err != nil {
+		return fmt.Errorf("failed to get consumer chain ID for consumer id (%s): %w", consumerId, err)
+	}
+	// validate that the initial height matches the chain ID
+	if err := types.ValidateInitialHeight(parameters.InitialHeight, chainId); err != nil {
+		return fmt.Errorf("invalid initial height for consumer id (%s): %w", consumerId, err)
+	}
 	store.Set(types.ConsumerIdToInitializationParametersKey(consumerId), bz)
 	return nil
 }

x/ccv/provider/keeper/permissionless_test.go

@@ -150,14 +150,16 @@ func TestConsumerInitializationParameters(t *testing.T) {
 		HistoricalEntries:                 456,
 		DistributionTransmissionChannel:   "distribution_transmission_channel",
 	}
-	providerKeeper.SetConsumerInitializationParameters(ctx, CONSUMER_ID, expectedInitializationParameters)
+	providerKeeper.SetConsumerChainId(ctx, CONSUMER_ID, "chain-1")
+	err = providerKeeper.SetConsumerInitializationParameters(ctx, CONSUMER_ID, expectedInitializationParameters)
+	require.NoError(t, err)
 	actualInitializationParameters, err := providerKeeper.GetConsumerInitializationParameters(ctx, CONSUMER_ID)
 	require.NoError(t, err)
 	require.Equal(t, expectedInitializationParameters, actualInitializationParameters)
 
 	// assert that overwriting the current initialization record works
 	expectedInitializationParameters = providertypes.ConsumerInitializationParameters{
-		InitialHeight:                     types.Height{RevisionNumber: 2, RevisionHeight: 3},
+		InitialHeight:                     types.Height{RevisionNumber: 1, RevisionHeight: 3},
 		GenesisHash:                       []byte{2, 3},
 		BinaryHash:                        []byte{4, 5},
 		SpawnTime:                         time.Unix(2, 3).UTC(),

x/ccv/provider/types/msg.go

@@ -3,9 +3,11 @@ package types
 import (
 	"encoding/json"
 	"fmt"
-	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
 	"strings"
 
+	"github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
+
+	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
 	ibctmtypes "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint"
 
 	errorsmod "cosmossdk.io/errors"
@@ -620,3 +622,11 @@ func validateProviderAddress(addr, signer string) error {
 
 	return nil
 }
+
+func ValidateInitialHeight(initialHeight clienttypes.Height, chainID string) error {
+	revision := clienttypes.ParseChainID(chainID)
+	if initialHeight.RevisionNumber != revision {
+		return fmt.Errorf("chain ID (%s) doesn't match revision number (%d)", chainID, initialHeight.RevisionNumber)
+	}
+	return nil
+}

x/ccv/provider/types/msg_test.go

@@ -647,3 +647,66 @@ func TestMsgAssignConsumerKeyValidateBasic(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateInitialHeight(t *testing.T) {
+	testCases := []struct {
+		name          string
+		chainId       string
+		initialHeight clienttypes.Height
+		expPass       bool
+	}{
+		{
+			name:    "valid with revision number",
+			chainId: "chain-1",
+			initialHeight: clienttypes.Height{
+				RevisionNumber: 1,
+				RevisionHeight: 0,
+			},
+			expPass: true,
+		},
+		{
+			name:    "valid without revision number",
+			chainId: "chain",
+			initialHeight: clienttypes.Height{
+				RevisionNumber: 0,
+				RevisionHeight: 0,
+			},
+			expPass: true,
+		},
+		{
+			name:    "invalid without revision number",
+			chainId: "chain",
+			initialHeight: clienttypes.Height{
+				RevisionNumber: 1,
+				RevisionHeight: 0,
+			},
+			expPass: false,
+		},
+		{
+			name:    "invalid without revision number",
+			chainId: "chain-1",
+			initialHeight: clienttypes.Height{
+				RevisionNumber: 0,
+				RevisionHeight: 0,
+			},
+			expPass: false,
+		},
+		{
+			name:    "valid: evmos-like chain IDs",
+			chainId: "evmos_9001-2",
+			initialHeight: clienttypes.Height{
+				RevisionNumber: 2,
+				RevisionHeight: 0,
+			},
+			expPass: true,
+		},
+	}
+	for _, tc := range testCases {
+		err := types.ValidateInitialHeight(tc.initialHeight, tc.chainId)
+		if tc.expPass {
+			require.NoError(t, err, "valid case: '%s' should not return error. got %w", tc.name, err)
+		} else {
+			require.Error(t, err, "invalid case: '%s' must return error but got none", tc.name)
+		}
+	}
+}

x/ccv/provider/types/provider.go

@@ -0,0 +1,28 @@
+package types
+
+import (
+	"time"
+
+	clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
+
+	ccv "github.com/cosmos/interchain-security/v6/x/ccv/types"
+)
+
+func DefaultConsumerInitializationParameters() ConsumerInitializationParameters {
+	return ConsumerInitializationParameters{
+		InitialHeight: clienttypes.Height{
+			RevisionNumber: 1,
+			RevisionHeight: 0,
+		},
+		GenesisHash:                       []byte{},
+		BinaryHash:                        []byte{},
+		SpawnTime:                         time.Time{},
+		UnbondingPeriod:                   ccv.DefaultConsumerUnbondingPeriod,
+		CcvTimeoutPeriod:                  ccv.DefaultCCVTimeoutPeriod,
+		TransferTimeoutPeriod:             ccv.DefaultTransferTimeoutPeriod,
+		ConsumerRedistributionFraction:    ccv.DefaultConsumerRedistributeFrac,
+		BlocksPerDistributionTransmission: ccv.DefaultBlocksPerDistributionTransmission,
+		HistoricalEntries:                 ccv.DefaultHistoricalEntries,
+		DistributionTransmissionChannel:   "",
+	}
+}

x/ccv/types/params.go

@@ -34,10 +34,10 @@ const (
 	// (and for consistency with other protobuf schemas defined for ccv).
 	DefaultHistoricalEntries = int64(stakingtypes.DefaultHistoricalEntries)
 
-	// In general, the default unbonding period on the consumer is one week less
+	// In general, the default unbonding period on the consumer is one day less
 	// than the default unbonding period on the provider, where the provider uses
 	// the staking module default.
-	DefaultConsumerUnbondingPeriod = stakingtypes.DefaultUnbondingTime - 7*24*time.Hour
+	DefaultConsumerUnbondingPeriod = stakingtypes.DefaultUnbondingTime - 24*time.Hour
 
 	// Default retry delay period is 1 hour.
 	DefaultRetryDelayPeriod = time.Hour