From bd37d18a3bebb06ec9a6836ed0a0e0dbeeea9d2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 14:43:30 +0100 Subject: [PATCH 01/12] refactor: add tendermint manual pruning function --- modules/core/02-client/migrations/store.go | 71 +++++++++ .../core/02-client/migrations/store_test.go | 150 ++++++++++++++++++ modules/core/24-host/parse.go | 23 +++ modules/core/24-host/parse_test.go | 30 ++++ 4 files changed, 274 insertions(+) create mode 100644 modules/core/02-client/migrations/store.go create mode 100644 modules/core/02-client/migrations/store_test.go diff --git a/modules/core/02-client/migrations/store.go b/modules/core/02-client/migrations/store.go new file mode 100644 index 00000000000..f5a07def82d --- /dev/null +++ b/modules/core/02-client/migrations/store.go @@ -0,0 +1,71 @@ +package migrations + +import ( + "fmt" + "strings" + + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/store/prefix" + storetypes "github.com/cosmos/cosmos-sdk/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + + clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" + host "github.com/cosmos/ibc-go/v6/modules/core/24-host" + "github.com/cosmos/ibc-go/v6/modules/core/exported" + ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" +) + +// PruneTendermintConsensusStates prunes all expired tendermint consensus states. This function +// may optionally be called during in-place store migrations. The 02-client store key must be provided. +func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, storeKey storetypes.StoreKey) error { + store := ctx.KVStore(storeKey) + + // iterate over 02-client store with prefix: clients/07-tendermint, + // example iterator.Key = -100/clientState (second half of the clientID + clientStore specific keys) + tendermintClientPrefix := []byte(fmt.Sprintf("%s/%s", host.KeyClientStorePrefix, exported.Tendermint)) + iterator := sdk.KVStorePrefixIterator(store, tendermintClientPrefix) + + var clients []string + + // collect all clients to avoid performing store state changes during iteration + defer iterator.Close() + for ; iterator.Valid(); iterator.Next() { + path := string(iterator.Key()) + if !strings.Contains(path, host.KeyClientState) { + // skip non client state keys + continue + } + + clientID, err := host.ParseClientStatePath(path) + if err != nil { + return err + } + + clients = append(clients, clientID) + } + + for _, clientID := range clients { + clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID)) + clientStore := prefix.NewStore(ctx.KVStore(storeKey), clientPrefix) + + bz := clientStore.Get(host.ClientStateKey()) + if bz == nil { + return clienttypes.ErrClientNotFound + } + + var clientState exported.ClientState + if err := cdc.UnmarshalInterface(bz, &clientState); err != nil { + return sdkerrors.Wrap(err, "failed to unmarshal client state bytes into tendermint client state") + } + + tmClientState, ok := clientState.(*ibctm.ClientState) + if !ok { + return sdkerrors.Wrap(clienttypes.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint") + } + + ibctm.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) + } + + return nil +} diff --git a/modules/core/02-client/migrations/store_test.go b/modules/core/02-client/migrations/store_test.go new file mode 100644 index 00000000000..bd01fb5a663 --- /dev/null +++ b/modules/core/02-client/migrations/store_test.go @@ -0,0 +1,150 @@ +package migrations_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/suite" + + "github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations" + "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" + host "github.com/cosmos/ibc-go/v6/modules/core/24-host" + "github.com/cosmos/ibc-go/v6/modules/core/exported" + ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" + ibctesting "github.com/cosmos/ibc-go/v6/testing" +) + +type MigrationsTestSuite struct { + suite.Suite + + coordinator *ibctesting.Coordinator + + chainA *ibctesting.TestChain + chainB *ibctesting.TestChain +} + +// TestMigrationsTestSuite runs all the tests within this package. +func TestMigrationsTestSuite(t *testing.T) { + suite.Run(t, new(MigrationsTestSuite)) +} + +// SetupTest creates a coordinator with 2 test chains. +func (suite *MigrationsTestSuite) SetupTest() { + suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) + suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) + suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2)) +} + +// test pruning of multiple expired tendermint consensus states +func (suite *MigrationsTestSuite) TestMigrateStoreTendermint() { + // create path and setup clients + path1 := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupClients(path1) + + path2 := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupClients(path2) + + pruneHeightMap := make(map[*ibctesting.Path][]exported.Height) + unexpiredHeightMap := make(map[*ibctesting.Path][]exported.Height) + + for _, path := range []*ibctesting.Path{path1, path2} { + // collect all heights expected to be pruned + var pruneHeights []exported.Height + pruneHeights = append(pruneHeights, path.EndpointA.GetClientState().GetLatestHeight()) + + // these heights will be expired and also pruned + for i := 0; i < 3; i++ { + err := path.EndpointA.UpdateClient() + suite.Require().NoError(err) + + pruneHeights = append(pruneHeights, path.EndpointA.GetClientState().GetLatestHeight()) + } + + // double chedck all information is currently stored + for _, pruneHeight := range pruneHeights { + consState, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, pruneHeight) + suite.Require().True(ok) + suite.Require().NotNil(consState) + + ctx := suite.chainA.GetContext() + clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, path.EndpointA.ClientID) + + processedTime, ok := ibctm.GetProcessedTime(clientStore, pruneHeight) + suite.Require().True(ok) + suite.Require().NotNil(processedTime) + + processedHeight, ok := ibctm.GetProcessedHeight(clientStore, pruneHeight) + suite.Require().True(ok) + suite.Require().NotNil(processedHeight) + + expectedConsKey := ibctm.GetIterationKey(clientStore, pruneHeight) + suite.Require().NotNil(expectedConsKey) + } + pruneHeightMap[path] = pruneHeights + } + + // Increment the time by a week + suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) + + for _, path := range []*ibctesting.Path{path1, path2} { + // create the consensus state that can be used as trusted height for next update + var unexpiredHeights []exported.Height + err := path.EndpointA.UpdateClient() + suite.Require().NoError(err) + unexpiredHeights = append(unexpiredHeights, path.EndpointA.GetClientState().GetLatestHeight()) + + err = path.EndpointA.UpdateClient() + suite.Require().NoError(err) + unexpiredHeights = append(unexpiredHeights, path.EndpointA.GetClientState().GetLatestHeight()) + + unexpiredHeightMap[path] = unexpiredHeights + } + + // Increment the time by another week, then update the client. + // This will cause the consensus states created before the first time increment + // to be expired + suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) + err := migrations.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().GetKey(host.StoreKey)) + suite.Require().NoError(err) + + for _, path := range []*ibctesting.Path{path1, path2} { + ctx := suite.chainA.GetContext() + clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, path.EndpointA.ClientID) + + // ensure everything has been pruned + for i, pruneHeight := range pruneHeightMap[path] { + consState, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, pruneHeight) + suite.Require().False(ok, i) + suite.Require().Nil(consState, i) + + processedTime, ok := ibctm.GetProcessedTime(clientStore, pruneHeight) + suite.Require().False(ok, i) + suite.Require().Equal(uint64(0), processedTime, i) + + processedHeight, ok := ibctm.GetProcessedHeight(clientStore, pruneHeight) + suite.Require().False(ok, i) + suite.Require().Nil(processedHeight, i) + + expectedConsKey := ibctm.GetIterationKey(clientStore, pruneHeight) + suite.Require().Nil(expectedConsKey, i) + } + + // ensure metadata is set for unexpired consensus state + for _, height := range unexpiredHeightMap[path] { + consState, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, height) + suite.Require().True(ok) + suite.Require().NotNil(consState) + + processedTime, ok := ibctm.GetProcessedTime(clientStore, height) + suite.Require().True(ok) + suite.Require().NotEqual(uint64(0), processedTime) + + processedHeight, ok := ibctm.GetProcessedHeight(clientStore, height) + suite.Require().True(ok) + suite.Require().NotEqual(types.ZeroHeight(), processedHeight) + + consKey := ibctm.GetIterationKey(clientStore, height) + suite.Require().Equal(host.ConsensusStateKey(height), consKey) + } + } +} diff --git a/modules/core/24-host/parse.go b/modules/core/24-host/parse.go index 8c3459500d9..37ae8e1460d 100644 --- a/modules/core/24-host/parse.go +++ b/modules/core/24-host/parse.go @@ -32,6 +32,29 @@ func ParseIdentifier(identifier, prefix string) (uint64, error) { return sequence, nil } +// ParseClientStatePath returns the client ID from a client state path. It returns +// an error if the provided path is invalid. +func ParseClientStatePath(path string) (string, error) { + split := strings.Split(path, "/") + if len(split) != 3 { + return "", sdkerrors.Wrapf(ErrInvalidPath, "cannot parse client state path %s", path) + } + + if split[0] != string(KeyClientStorePrefix) { + return "", sdkerrors.Wrapf(ErrInvalidPath, "path does not begin with client store prefix: expected %s, got %s", KeyClientStorePrefix, split[0]) + } + + if split[2] != KeyClientState { + return "", sdkerrors.Wrapf(ErrInvalidPath, "path does not end with client state key: expected %s, got %s", KeyClientState, split[2]) + } + + if strings.TrimSpace(split[1]) == "" { + return "", sdkerrors.Wrap(ErrInvalidPath, "clientID is empty") + } + + return split[1], nil +} + // ParseConnectionPath returns the connection ID from a full path. It returns // an error if the provided path is invalid. func ParseConnectionPath(path string) (string, error) { diff --git a/modules/core/24-host/parse_test.go b/modules/core/24-host/parse_test.go index ea30f671cde..7e39cfdfc47 100644 --- a/modules/core/24-host/parse_test.go +++ b/modules/core/24-host/parse_test.go @@ -1,6 +1,7 @@ package host_test import ( + "fmt" "math" "testing" @@ -8,6 +9,7 @@ import ( connectiontypes "github.com/cosmos/ibc-go/v6/modules/core/03-connection/types" host "github.com/cosmos/ibc-go/v6/modules/core/24-host" + ibctesting "github.com/cosmos/ibc-go/v6/testing" ) func TestParseIdentifier(t *testing.T) { @@ -46,3 +48,31 @@ func TestParseIdentifier(t *testing.T) { } } } + +func TestParseClientStatePath(t *testing.T) { + testCases := []struct { + name string + path string + expPass bool + }{ + {"valid", host.FullClientStatePath(ibctesting.FirstClientID), true}, + {"path too large", fmt.Sprintf("clients/clients/%s/clientState", ibctesting.FirstClientID), false}, + {"path too small", fmt.Sprintf("clients/%s", ibctesting.FirstClientID), false}, + {"path does not begin with client store", fmt.Sprintf("cli/%s/%s", ibctesting.FirstClientID, host.KeyClientState), false}, + {"path does not end with client state key", fmt.Sprintf("%s/%s/consensus", string(host.KeyClientStorePrefix), ibctesting.FirstClientID), false}, + {"client ID is empty", host.FullClientStatePath(""), false}, + {"client ID is only spaces", host.FullClientStatePath(" "), false}, + } + + for _, tc := range testCases { + clientID, err := host.ParseClientStatePath(tc.path) + + if tc.expPass { + require.NoError(t, err, tc.name) + require.Equal(t, ibctesting.FirstClientID, clientID) + } else { + require.Error(t, err, tc.name) + require.Equal(t, "", clientID) + } + } +} From 7ad76de262f73dab34273df80d133783e2120633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 14:57:48 +0100 Subject: [PATCH 02/12] chore: add extra checks into pruning test ensure the pruning function does not modify non tendermint client types --- .../core/02-client/migrations/store_test.go | 48 +++++++++++++++---- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/modules/core/02-client/migrations/store_test.go b/modules/core/02-client/migrations/store_test.go index bd01fb5a663..f2bda003d2d 100644 --- a/modules/core/02-client/migrations/store_test.go +++ b/modules/core/02-client/migrations/store_test.go @@ -36,18 +36,38 @@ func (suite *MigrationsTestSuite) SetupTest() { } // test pruning of multiple expired tendermint consensus states -func (suite *MigrationsTestSuite) TestMigrateStoreTendermint() { - // create path and setup clients - path1 := ibctesting.NewPath(suite.chainA, suite.chainB) - suite.coordinator.SetupClients(path1) +func (suite *MigrationsTestSuite) TestPruneTendermintConsensusStates() { + // create multiple tendermint clients and a solo machine client + // the solo machine is used to verify this pruning function only modifies + // the tendermint store. - path2 := ibctesting.NewPath(suite.chainA, suite.chainB) - suite.coordinator.SetupClients(path2) + numTMClients := 3 + paths := make([]*ibctesting.Path, numTMClients) + + for i := 0; i < numTMClients; i++ { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.SetupClients(path) + + paths[i] = path + } + + solomachine := ibctesting.NewSolomachine(suite.T(), suite.chainA.Codec, "06-solomachine-0", "testing", 1) + smClientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), solomachine.ClientID) + + // set client state + bz, err := suite.chainA.App.AppCodec().MarshalInterface(solomachine.ClientState()) + suite.Require().NoError(err) + smClientStore.Set(host.ClientStateKey(), bz) + + bz, err = suite.chainA.App.AppCodec().MarshalInterface(solomachine.ConsensusState()) + suite.Require().NoError(err) + smHeight := types.NewHeight(0, 1) + smClientStore.Set(host.ConsensusStateKey(smHeight), bz) pruneHeightMap := make(map[*ibctesting.Path][]exported.Height) unexpiredHeightMap := make(map[*ibctesting.Path][]exported.Height) - for _, path := range []*ibctesting.Path{path1, path2} { + for _, path := range paths { // collect all heights expected to be pruned var pruneHeights []exported.Height pruneHeights = append(pruneHeights, path.EndpointA.GetClientState().GetLatestHeight()) @@ -86,7 +106,7 @@ func (suite *MigrationsTestSuite) TestMigrateStoreTendermint() { // Increment the time by a week suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) - for _, path := range []*ibctesting.Path{path1, path2} { + for _, path := range paths { // create the consensus state that can be used as trusted height for next update var unexpiredHeights []exported.Height err := path.EndpointA.UpdateClient() @@ -104,10 +124,10 @@ func (suite *MigrationsTestSuite) TestMigrateStoreTendermint() { // This will cause the consensus states created before the first time increment // to be expired suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) - err := migrations.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().GetKey(host.StoreKey)) + err = migrations.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().GetKey(host.StoreKey)) suite.Require().NoError(err) - for _, path := range []*ibctesting.Path{path1, path2} { + for _, path := range paths { ctx := suite.chainA.GetContext() clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, path.EndpointA.ClientID) @@ -147,4 +167,12 @@ func (suite *MigrationsTestSuite) TestMigrateStoreTendermint() { suite.Require().Equal(host.ConsensusStateKey(height), consKey) } } + + // verify that solomachine client and consensus state were not removed + smClientStore = suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), solomachine.ClientID) + bz = smClientStore.Get(host.ClientStateKey()) + suite.Require().NotEmpty(bz) + + bz = smClientStore.Get(host.ConsensusStateKey(smHeight)) + suite.Require().NotEmpty(bz) } From 5e9f208fac8755c323c2b102a0c3480a766dd419 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 15:17:47 +0100 Subject: [PATCH 03/12] chore: add docs and logging for tendermint pruning --- docs/migrations/v6-to-v7.md | 61 ++++++++------------ modules/core/02-client/migrations/store.go | 14 +++-- modules/light-clients/07-tendermint/store.go | 6 +- 3 files changed, 39 insertions(+), 42 deletions(-) diff --git a/docs/migrations/v6-to-v7.md b/docs/migrations/v6-to-v7.md index 70e9519f4e3..65e3c35b7c2 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -13,7 +13,31 @@ There are four sections based on the four potential user groups of this document ## Chains -- No relevant changes were made in this release. +Chains will perform automatic migrations to remove a localhost client, if it exists and to migrate the solomachine to the v3 of the protobuf definition. + +An optional upgrade handler has been added to prune expired tendermint consensus states. +Add the following to the function call to the upgrade handler in `app/app.go`, to perform the optional state pruning. + +```go +import ( + // ... + ibcclientmigrations "github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations" +) + +// ... + +app.UpgradeKeeper.SetUpgradeHandler( + upgradeName, + func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) { + // prune expired tendermint consensus states + ibcclientmigrations.PruneTendermintExpiredConsensusStates(ctx, app.Codec, appCodec, keys[ibchost.StoreKey]) + + return app.mm.RunMigrations(ctx, app.configurator, fromVM) + }, +) +``` + +Checkout the logs to see how many consensus states are pruned. ## IBC Apps @@ -55,41 +79,6 @@ The function `GetTimestampAtHeight` has been added to the `ClientState` interfac The `GetRoot` function has been removed from consensus state interface since it was not used by core IBC. -### Light client implementations - -The `09-localhost` light client implementation has been removed because it is currently non-functional. - -An upgrade handler has been added to supply chain developers with the logic needed to prune the ibc client store and successfully complete the removal of `09-localhost`. -Add the following to the application upgrade handler in `app/app.go`, calling `MigrateToV6` to perform store migration logic. - -```go -import ( - // ... - ibcv6 "github.com/cosmos/ibc-go/v6/modules/core/migrations/v6" -) - -// ... - -app.UpgradeKeeper.SetUpgradeHandler( - upgradeName, - func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) { - // prune the 09-localhost client from the ibc client store - ibcv6.MigrateToV6(ctx, app.IBCKeeper.ClientKeeper) - - return app.mm.RunMigrations(ctx, app.configurator, fromVM) - }, -) -``` - -Please note the above upgrade handler is optional and should only be run if chains have an existing `09-localhost` client stored in state. -A simple query can be performed to check for a `09-localhost` client on chain. - -For example: - -``` -simd query ibc client states | grep 09-localhost -``` - ### Client Keeper Keeper function `CheckMisbehaviourAndUpdateState` has been removed since function `UpdateClient` can now handle updating `ClientState` on `ClientMessage` type which can be any `Misbehaviour` implementations. diff --git a/modules/core/02-client/migrations/store.go b/modules/core/02-client/migrations/store.go index f5a07def82d..7f847ca0923 100644 --- a/modules/core/02-client/migrations/store.go +++ b/modules/core/02-client/migrations/store.go @@ -10,7 +10,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" + "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v6/modules/core/24-host" "github.com/cosmos/ibc-go/v6/modules/core/exported" ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" @@ -45,13 +45,15 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor clients = append(clients, clientID) } + var totalPruned int + for _, clientID := range clients { clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID)) clientStore := prefix.NewStore(ctx.KVStore(storeKey), clientPrefix) bz := clientStore.Get(host.ClientStateKey()) if bz == nil { - return clienttypes.ErrClientNotFound + return types.ErrClientNotFound } var clientState exported.ClientState @@ -61,11 +63,15 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor tmClientState, ok := clientState.(*ibctm.ClientState) if !ok { - return sdkerrors.Wrap(clienttypes.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint") + return sdkerrors.Wrap(types.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint") } - ibctm.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) + amtPruned := ibctm.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) + totalPruned = totalPruned + amtPruned } + clientLogger := ctx.Logger().With("module", "x/"+host.ModuleName+"/"+types.SubModuleName) + clientLogger.Info("pruned expired tendermint consensus states", "total", totalPruned) + return nil } diff --git a/modules/light-clients/07-tendermint/store.go b/modules/light-clients/07-tendermint/store.go index d46d9b01796..ce513c2cd68 100644 --- a/modules/light-clients/07-tendermint/store.go +++ b/modules/light-clients/07-tendermint/store.go @@ -273,11 +273,11 @@ func GetPreviousConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, h // PruneAllExpiredConsensusStates iterates over all consensus states for a given // client store. If a consensus state is expired, it is deleted and its metadata -// is deleted. +// is deleted. The number of consensus states pruned is returned. func PruneAllExpiredConsensusStates( ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec, clientState *ClientState, -) { +) int { var heights []exported.Height pruneCb := func(height exported.Height) bool { @@ -299,6 +299,8 @@ func PruneAllExpiredConsensusStates( deleteConsensusState(clientStore, height) deleteConsensusMetadata(clientStore, height) } + + return len(heights) } // Helper function for GetNextConsensusState and GetPreviousConsensusState From 5cb1262f11fdf9b9c2e1333c608eb80dedbb4946 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 15:24:55 +0100 Subject: [PATCH 04/12] Apply suggestions from code review --- modules/core/02-client/migrations/store.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/core/02-client/migrations/store.go b/modules/core/02-client/migrations/store.go index 7f847ca0923..b7c4eb8646b 100644 --- a/modules/core/02-client/migrations/store.go +++ b/modules/core/02-client/migrations/store.go @@ -17,12 +17,11 @@ import ( ) // PruneTendermintConsensusStates prunes all expired tendermint consensus states. This function -// may optionally be called during in-place store migrations. The 02-client store key must be provided. +// may optionally be called during in-place store migrations. The ibc store key must be provided. func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, storeKey storetypes.StoreKey) error { store := ctx.KVStore(storeKey) - // iterate over 02-client store with prefix: clients/07-tendermint, - // example iterator.Key = -100/clientState (second half of the clientID + clientStore specific keys) + // iterate over ibc store with prefix: host.ClientStoreKeyPrefix/07-tendermint, tendermintClientPrefix := []byte(fmt.Sprintf("%s/%s", host.KeyClientStorePrefix, exported.Tendermint)) iterator := sdk.KVStorePrefixIterator(store, tendermintClientPrefix) From 33c64d64726cd6b51e8cf77ad89320daa4882934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 15:25:27 +0100 Subject: [PATCH 05/12] Apply suggestions from code review --- docs/migrations/v6-to-v7.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/migrations/v6-to-v7.md b/docs/migrations/v6-to-v7.md index 65e3c35b7c2..dba1e46bbc1 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -13,9 +13,9 @@ There are four sections based on the four potential user groups of this document ## Chains -Chains will perform automatic migrations to remove a localhost client, if it exists and to migrate the solomachine to the v3 of the protobuf definition. +Chains will perform automatic migrations to remove existing localhost clients and to migrate the solomachine to the v3 of the protobuf definition. -An optional upgrade handler has been added to prune expired tendermint consensus states. +An optional upgrade handler has been added to prune expired tendermint consensus states. It may be used during any upgrade (not just to v7) Add the following to the function call to the upgrade handler in `app/app.go`, to perform the optional state pruning. ```go From f6bbe6fcfea49cba2973bfa207ffd9d1f2dc3c45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 18:16:03 +0100 Subject: [PATCH 06/12] Apply suggestions from code review Co-authored-by: Damian Nolan --- docs/migrations/v6-to-v7.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/migrations/v6-to-v7.md b/docs/migrations/v6-to-v7.md index dba1e46bbc1..6bcc87cecd8 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -13,9 +13,9 @@ There are four sections based on the four potential user groups of this document ## Chains -Chains will perform automatic migrations to remove existing localhost clients and to migrate the solomachine to the v3 of the protobuf definition. +Chains will perform automatic migrations to remove existing localhost clients and to migrate the solomachine to v3 of the protobuf definition. -An optional upgrade handler has been added to prune expired tendermint consensus states. It may be used during any upgrade (not just to v7) +An optional upgrade handler has been added to prune expired tendermint consensus states. It may be used during any upgrade (from v7 onwards). Add the following to the function call to the upgrade handler in `app/app.go`, to perform the optional state pruning. ```go From f7d8871dfb6d528a2376b87a205d1dfa7d2d0f81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 18:16:23 +0100 Subject: [PATCH 07/12] Update modules/core/02-client/migrations/store.go --- modules/core/02-client/migrations/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/02-client/migrations/store.go b/modules/core/02-client/migrations/store.go index b7c4eb8646b..cec42d05672 100644 --- a/modules/core/02-client/migrations/store.go +++ b/modules/core/02-client/migrations/store.go @@ -21,7 +21,7 @@ import ( func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, storeKey storetypes.StoreKey) error { store := ctx.KVStore(storeKey) - // iterate over ibc store with prefix: host.ClientStoreKeyPrefix/07-tendermint, + // iterate over ibc store with prefix: clients/07-tendermint, tendermintClientPrefix := []byte(fmt.Sprintf("%s/%s", host.KeyClientStorePrefix, exported.Tendermint)) iterator := sdk.KVStorePrefixIterator(store, tendermintClientPrefix) From 348b21e3f07fe33e3e2a5ee2eed27862821ba082 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 18:25:09 +0100 Subject: [PATCH 08/12] chore: move tendermint migrations to 07-tendermint directory --- docs/migrations/v6-to-v7.md | 6 ++-- .../07-tendermint/migrations.go} | 10 +++---- .../07-tendermint/migrations_test.go} | 28 ++----------------- 3 files changed, 10 insertions(+), 34 deletions(-) rename modules/{core/02-client/migrations/store.go => light-clients/07-tendermint/migrations.go} (89%) rename modules/{core/02-client/migrations/store_test.go => light-clients/07-tendermint/migrations_test.go} (88%) diff --git a/docs/migrations/v6-to-v7.md b/docs/migrations/v6-to-v7.md index 65e3c35b7c2..e2fbbf39d13 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -21,7 +21,7 @@ Add the following to the function call to the upgrade handler in `app/app.go`, t ```go import ( // ... - ibcclientmigrations "github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations" + ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" ) // ... @@ -29,8 +29,8 @@ import ( app.UpgradeKeeper.SetUpgradeHandler( upgradeName, func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) { - // prune expired tendermint consensus states - ibcclientmigrations.PruneTendermintExpiredConsensusStates(ctx, app.Codec, appCodec, keys[ibchost.StoreKey]) + // prune expired tendermint consensus states to save storage space + ibctm.PruneTendermintConsensusStates(ctx, app.Codec, appCodec, keys[ibchost.StoreKey]) return app.mm.RunMigrations(ctx, app.configurator, fromVM) }, diff --git a/modules/core/02-client/migrations/store.go b/modules/light-clients/07-tendermint/migrations.go similarity index 89% rename from modules/core/02-client/migrations/store.go rename to modules/light-clients/07-tendermint/migrations.go index 7f847ca0923..0b94793a0f2 100644 --- a/modules/core/02-client/migrations/store.go +++ b/modules/light-clients/07-tendermint/migrations.go @@ -1,4 +1,4 @@ -package migrations +package tendermint import ( "fmt" @@ -13,7 +13,6 @@ import ( "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v6/modules/core/24-host" "github.com/cosmos/ibc-go/v6/modules/core/exported" - ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" ) // PruneTendermintConsensusStates prunes all expired tendermint consensus states. This function @@ -45,6 +44,8 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor clients = append(clients, clientID) } + // keep track of the total consensus states pruned so chains can + // understand how much space is saved when the migration is run var totalPruned int for _, clientID := range clients { @@ -61,13 +62,12 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor return sdkerrors.Wrap(err, "failed to unmarshal client state bytes into tendermint client state") } - tmClientState, ok := clientState.(*ibctm.ClientState) + tmClientState, ok := clientState.(*ClientState) if !ok { return sdkerrors.Wrap(types.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint") } - amtPruned := ibctm.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) - totalPruned = totalPruned + amtPruned + totalPruned += PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) } clientLogger := ctx.Logger().With("module", "x/"+host.ModuleName+"/"+types.SubModuleName) diff --git a/modules/core/02-client/migrations/store_test.go b/modules/light-clients/07-tendermint/migrations_test.go similarity index 88% rename from modules/core/02-client/migrations/store_test.go rename to modules/light-clients/07-tendermint/migrations_test.go index f2bda003d2d..f9b6f2edc53 100644 --- a/modules/core/02-client/migrations/store_test.go +++ b/modules/light-clients/07-tendermint/migrations_test.go @@ -1,11 +1,8 @@ -package migrations_test +package tendermint_test import ( - "testing" "time" - "github.com/stretchr/testify/suite" - "github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations" "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v6/modules/core/24-host" @@ -14,29 +11,8 @@ import ( ibctesting "github.com/cosmos/ibc-go/v6/testing" ) -type MigrationsTestSuite struct { - suite.Suite - - coordinator *ibctesting.Coordinator - - chainA *ibctesting.TestChain - chainB *ibctesting.TestChain -} - -// TestMigrationsTestSuite runs all the tests within this package. -func TestMigrationsTestSuite(t *testing.T) { - suite.Run(t, new(MigrationsTestSuite)) -} - -// SetupTest creates a coordinator with 2 test chains. -func (suite *MigrationsTestSuite) SetupTest() { - suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) - suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) - suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2)) -} - // test pruning of multiple expired tendermint consensus states -func (suite *MigrationsTestSuite) TestPruneTendermintConsensusStates() { +func (suite *TendermintTestSuite) TestPruneTendermintConsensusStates() { // create multiple tendermint clients and a solo machine client // the solo machine is used to verify this pruning function only modifies // the tendermint store. From 4399a427dc04704b179b88843130f27891ceae1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 18:27:00 +0100 Subject: [PATCH 09/12] chore: add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f70c712e94f..ace7aea41b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,6 +111,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features +* (light-clients/07-tendermint) [\#2800](https://github.com/cosmos/ibc-go/pull/2800) Add optional in-place store migration function to prune all expired tendermint consensus states. * (apps/27-interchain-accounts) [\#2147](https://github.com/cosmos/ibc-go/pull/2147) Adding a `SubmitTx` gRPC endpoint for the ICS27 Controller module which allows owners of interchain accounts to submit transactions. This replaces the previously existing need for authentication modules to implement this standard functionality. * (testing/simapp) [\#2190](https://github.com/cosmos/ibc-go/pull/2190) Adding the new `x/group` cosmos-sdk module to simapp. From 78fd0a31e5f28c326a833a0d003953eb508f5d87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 21 Nov 2022 18:29:27 +0100 Subject: [PATCH 10/12] chore: update imports --- modules/light-clients/07-tendermint/migrations.go | 8 ++++---- modules/light-clients/07-tendermint/migrations_test.go | 9 ++++----- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/modules/light-clients/07-tendermint/migrations.go b/modules/light-clients/07-tendermint/migrations.go index a62b24eb81a..ba1ef6e2661 100644 --- a/modules/light-clients/07-tendermint/migrations.go +++ b/modules/light-clients/07-tendermint/migrations.go @@ -10,7 +10,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" + clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v6/modules/core/24-host" "github.com/cosmos/ibc-go/v6/modules/core/exported" ) @@ -53,7 +53,7 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor bz := clientStore.Get(host.ClientStateKey()) if bz == nil { - return types.ErrClientNotFound + return clienttypes.ErrClientNotFound } var clientState exported.ClientState @@ -63,13 +63,13 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor tmClientState, ok := clientState.(*ClientState) if !ok { - return sdkerrors.Wrap(types.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint") + return sdkerrors.Wrap(clienttypes.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint") } totalPruned += PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) } - clientLogger := ctx.Logger().With("module", "x/"+host.ModuleName+"/"+types.SubModuleName) + clientLogger := ctx.Logger().With("module", "x/"+host.ModuleName+"/"+clienttypes.SubModuleName) clientLogger.Info("pruned expired tendermint consensus states", "total", totalPruned) return nil diff --git a/modules/light-clients/07-tendermint/migrations_test.go b/modules/light-clients/07-tendermint/migrations_test.go index f9b6f2edc53..c7fe56af5db 100644 --- a/modules/light-clients/07-tendermint/migrations_test.go +++ b/modules/light-clients/07-tendermint/migrations_test.go @@ -3,8 +3,7 @@ package tendermint_test import ( "time" - "github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations" - "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" + clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v6/modules/core/24-host" "github.com/cosmos/ibc-go/v6/modules/core/exported" ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" @@ -37,7 +36,7 @@ func (suite *TendermintTestSuite) TestPruneTendermintConsensusStates() { bz, err = suite.chainA.App.AppCodec().MarshalInterface(solomachine.ConsensusState()) suite.Require().NoError(err) - smHeight := types.NewHeight(0, 1) + smHeight := clienttypes.NewHeight(0, 1) smClientStore.Set(host.ConsensusStateKey(smHeight), bz) pruneHeightMap := make(map[*ibctesting.Path][]exported.Height) @@ -100,7 +99,7 @@ func (suite *TendermintTestSuite) TestPruneTendermintConsensusStates() { // This will cause the consensus states created before the first time increment // to be expired suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) - err = migrations.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().GetKey(host.StoreKey)) + err = ibctm.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().GetKey(host.StoreKey)) suite.Require().NoError(err) for _, path := range paths { @@ -137,7 +136,7 @@ func (suite *TendermintTestSuite) TestPruneTendermintConsensusStates() { processedHeight, ok := ibctm.GetProcessedHeight(clientStore, height) suite.Require().True(ok) - suite.Require().NotEqual(types.ZeroHeight(), processedHeight) + suite.Require().NotEqual(clienttypes.ZeroHeight(), processedHeight) consKey := ibctm.GetIterationKey(clientStore, height) suite.Require().Equal(host.ConsensusStateKey(height), consKey) From 20e4d8d5813799d1d454d63c6ded98739677dd38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 23 Nov 2022 16:43:17 +0100 Subject: [PATCH 11/12] chore: update migration doc header --- docs/migrations/v6-to-v7.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/migrations/v6-to-v7.md b/docs/migrations/v6-to-v7.md index 88c6e3d3dcc..e5b2ed04759 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -1,4 +1,4 @@ -# Migrating from ibc-go v5 to v6 +# Migrating from ibc-go v6 to v7 This document is intended to highlight significant changes which may require more information than presented in the CHANGELOG. Any changes that must be done by a user of ibc-go should be documented here. From 69961f07f5474f8f0c117d7431412305e451626e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 29 Nov 2022 16:07:11 +0100 Subject: [PATCH 12/12] chore: rename clients to clientIDs --- modules/light-clients/07-tendermint/migrations.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/light-clients/07-tendermint/migrations.go b/modules/light-clients/07-tendermint/migrations.go index 4ae9d8edeb7..c369e6b6836 100644 --- a/modules/light-clients/07-tendermint/migrations.go +++ b/modules/light-clients/07-tendermint/migrations.go @@ -24,7 +24,7 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor tendermintClientPrefix := []byte(fmt.Sprintf("%s/%s", host.KeyClientStorePrefix, exported.Tendermint)) iterator := sdk.KVStorePrefixIterator(store, tendermintClientPrefix) - var clients []string + var clientIDs []string // collect all clients to avoid performing store state changes during iteration defer iterator.Close() @@ -36,14 +36,14 @@ func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, stor } clientID := host.MustParseClientStatePath(path) - clients = append(clients, clientID) + clientIDs = append(clientIDs, clientID) } // keep track of the total consensus states pruned so chains can // understand how much space is saved when the migration is run var totalPruned int - for _, clientID := range clients { + for _, clientID := range clientIDs { clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID)) clientStore := prefix.NewStore(ctx.KVStore(storeKey), clientPrefix)