fix!: validate initial height and set default values for init consume…

…r params (#2357) * add initial height validation * set default consumer init params on creation * add changelog entries * add test for evmos like chain ID * fix UTs * set the default RevisionHeight to 1 * fix e2e tests * use temporary patched Hermes version in compability tests --------- Co-authored-by: Simon Noetzlin <[email protected]>
cosmos · Oct 18, 2024 · c2a1c76 · c2a1c76
1 parent 21dc2de
commit c2a1c76
Show file tree

Hide file tree

Showing 16 changed files with 166 additions and 33 deletions.
diff --git a/.changelog/unreleased/improvements/2357-init-params.md b/.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))
diff --git a/.changelog/unreleased/state-breaking/2357-init-params.md b/.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))
diff --git a/Dockerfile b/Dockerfile
@@ -29,8 +29,8 @@ RUN go mod tidy
 RUN make install
 
 # Get Hermes build
-## TODO: import Hermes release from ghcr.io/informalsystems repository when 
-## a Hermes release contains the patch in 
+# TODO: import Hermes release from ghcr.io/informalsystems repository when 
+# a Hermes release contains the patch in 
 # https://github.com/informalsystems/hermes/pull/4182
 FROM --platform=linux/amd64 otacrew/hermes-ics:latest AS hermes-builder
 

diff --git a/Dockerfile.combined b/Dockerfile.combined
@@ -24,7 +24,10 @@ FROM --platform=linux/amd64 ${PROVIDER_IMAGE} AS provider
 FROM --platform=linux/amd64 ${CONSUMER_IMAGE} AS consumer
 
 # Get Hermes build
-FROM --platform=linux/amd64 ghcr.io/informalsystems/hermes:1.10.2 AS hermes-builder
+# TODO: import Hermes release from ghcr.io/informalsystems repository when 
+# a Hermes release contains the patch in 
+# https://github.com/informalsystems/hermes/pull/4182
+FROM --platform=linux/amd64 otacrew/hermes-ics:latest AS hermes-builder
 
 
 # Get GoRelayer

diff --git a/tests/e2e/actions.go b/tests/e2e/actions.go
@@ -637,7 +637,7 @@ func (tr Chain) submitConsumerAdditionProposal(
 		DistributionTransmissionChannel:   action.DistributionChannel,
 	}
 
-	consumerId := tr.CreateConsumer(providerChainCfg.ChainId, consumerChainCfg.ChainId, action.From, Metadata, nil, nil)
+	consumerId := tr.CreateConsumer(providerChainCfg.ChainId, consumerChainCfg.ChainId, action.From, Metadata, &initializationParameters, nil)
 	authority := "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn"
 	// Set the new created consumer-id on the chain's config
 	consumerChainCfg.ConsumerId = consumerId

diff --git a/testutil/keeper/unit_test_helpers.go b/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(),

diff --git a/x/ccv/provider/keeper/consumer_lifecycle_test.go b/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])

diff --git a/x/ccv/provider/keeper/grpc_query_test.go b/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)

diff --git a/x/ccv/provider/keeper/msg_server.go b/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
 	}

diff --git a/x/ccv/provider/keeper/msg_server_test.go b/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"

diff --git a/x/ccv/provider/keeper/permissionless.go b/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
 }

diff --git a/x/ccv/provider/keeper/permissionless_test.go b/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(),

diff --git a/x/ccv/provider/types/msg.go b/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
+}
diff --git a/x/ccv/provider/types/msg_test.go b/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)
+		}
+	}
+}