From f6388022252fce3131ac7cd2b0a33a5797f3b2ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 30 Nov 2022 15:40:30 +0100 Subject: [PATCH 1/7] refactor: initial changes to use IterateClientStates --- .../migrations/v7/expected_keepers.go | 14 ++++ modules/core/02-client/migrations/v7/store.go | 68 ++++++------------- .../07-tendermint/expected_keepers.go | 14 ++++ .../light-clients/07-tendermint/migrations.go | 37 ++-------- 4 files changed, 56 insertions(+), 77 deletions(-) create mode 100644 modules/core/02-client/migrations/v7/expected_keepers.go create mode 100644 modules/light-clients/07-tendermint/expected_keepers.go diff --git a/modules/core/02-client/migrations/v7/expected_keepers.go b/modules/core/02-client/migrations/v7/expected_keepers.go new file mode 100644 index 00000000000..7c6d2c36475 --- /dev/null +++ b/modules/core/02-client/migrations/v7/expected_keepers.go @@ -0,0 +1,14 @@ +package v7 + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v6/modules/core/exported" +) + +// ClientKeeper expected account IBC client keeper +type ClientKeeper interface { + GetClientState(ctx sdk.Context, clientID string) (exported.ClientState, bool) + IterateClientStates(ctx sdk.Context, prefix []byte, cb func(string, exported.ClientState) bool) + ClientStore(ctx sdk.Context, clientID string) sdk.KVStore +} diff --git a/modules/core/02-client/migrations/v7/store.go b/modules/core/02-client/migrations/v7/store.go index 2be30467000..8823ca6268c 100644 --- a/modules/core/02-client/migrations/v7/store.go +++ b/modules/core/02-client/migrations/v7/store.go @@ -29,18 +29,16 @@ const Localhost string = "09-localhost" // - Pruning all solo machine consensus states // - Removing the localhost client // - Asserting existing tendermint clients are properly registered on the chain codec -func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec) error { - store := ctx.KVStore(storeKey) - - if err := handleSolomachineMigration(ctx, store, cdc); err != nil { +func MigrateStore(ctx sdk.Context, keeper ClientKeeper, cdc codec.BinaryCodec) error { + if err := handleSolomachineMigration(ctx, keeper, cdc); err != nil { return err } - if err := handleTendermintMigration(ctx, store, cdc); err != nil { + if err := handleTendermintMigration(ctx, keeper); err != nil { return err } - if err := handleLocalhostMigration(ctx, store, cdc); err != nil { + if err := handleLocalhostMigration(ctx, keeper, cdc); err != nil { return err } @@ -49,15 +47,14 @@ func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.Binar // handleSolomachineMigration iterates over the solo machine clients and migrates client state from // protobuf definition v2 to v3. All consensus states stored outside of the client state are pruned. -func handleSolomachineMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec) error { - clients, err := collectClients(ctx, store, exported.Solomachine) +func handleSolomachineMigration(ctx sdk.Context, keeper ClientKeeper, cdc codec.BinaryCodec) error { + clients, err := collectClients(ctx, keeper, exported.Solomachine) if err != nil { return err } for _, clientID := range clients { - clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID)) - clientStore := prefix.NewStore(store, clientPrefix) + clientStore := keeper.ClientStore(ctx, clientID) bz := clientStore.Get(host.ClientStateKey()) if bz == nil { @@ -92,8 +89,8 @@ func handleSolomachineMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.Bi // handlerTendermintMigration asserts that the tendermint client in state can be decoded properly. // This ensures the upgrading chain properly registered the tendermint client types on the chain codec. -func handleTendermintMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec) error { - clients, err := collectClients(ctx, store, exported.Tendermint) +func handleTendermintMigration(ctx sdk.Context, keeper ClientKeeper) error { + clients, err := collectClients(ctx, keeper, exported.Tendermint) if err != nil { return err } @@ -103,21 +100,12 @@ func handleTendermintMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.Bin } clientID := clients[0] - - clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID)) - clientStore := prefix.NewStore(store, 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") + clientState, ok := keeper.GetClientState(ctx, clientID) + if !ok { + return sdkerrors.Wrapf(clienttypes.ErrClientNotFound, "clientID %s", clientID) } - _, ok := clientState.(*ibctm.ClientState) + _, ok = clientState.(*ibctm.ClientState) if !ok { return sdkerrors.Wrap(clienttypes.ErrInvalidClient, "client state is not tendermint even though client id contains 07-tendermint") } @@ -150,28 +138,14 @@ func handleLocalhostMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.Bina // avoid state corruption as modifying state during iteration is unsafe. A special case // for tendermint clients is included as only one tendermint clientID is required for // v7 migrations. -func collectClients(ctx sdk.Context, store sdk.KVStore, clientType string) (clients []string, err error) { - clientPrefix := []byte(fmt.Sprintf("%s/%s", host.KeyClientStorePrefix, clientType)) - iterator := sdk.KVStorePrefixIterator(store, clientPrefix) - - 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 := host.MustParseClientStatePath(path) - clients = append(clients, clientID) - - // optimization: exit after a single tendermint client iteration - if strings.Contains(clientID, exported.Tendermint) { - return clients, nil - } - } - - return clients, nil +func collectClients(ctx sdk.Context, keeper ClientKeeper, clientType string) (clients []string, err error) { + var clientIDs []string + keeper.IterateClientStates(ctx, nil, func(clientID string, _ exported.ClientState) bool { + clientIDs = append(clientIDs, clientID) + return false + }) + + return clientIDs, nil } // removeAllClientConsensusStates removes all client consensus states from the associated diff --git a/modules/light-clients/07-tendermint/expected_keepers.go b/modules/light-clients/07-tendermint/expected_keepers.go new file mode 100644 index 00000000000..f98115a6981 --- /dev/null +++ b/modules/light-clients/07-tendermint/expected_keepers.go @@ -0,0 +1,14 @@ +package tendermint + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v6/modules/core/exported" +) + +// ClientKeeper expected account IBC client keeper +type ClientKeeper interface { + GetClientState(ctx sdk.Context, clientID string) (exported.ClientState, bool) + IterateClientStates(ctx sdk.Context, prefix []byte, cb func(string, exported.ClientState) bool) + ClientStore(ctx sdk.Context, clientID string) sdk.KVStore +} diff --git a/modules/light-clients/07-tendermint/migrations.go b/modules/light-clients/07-tendermint/migrations.go index c369e6b6836..1f304fa3973 100644 --- a/modules/light-clients/07-tendermint/migrations.go +++ b/modules/light-clients/07-tendermint/migrations.go @@ -17,44 +17,21 @@ import ( // PruneTendermintConsensusStates prunes all expired tendermint consensus states. This function // 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 ibc store with prefix: clients/07-tendermint, - tendermintClientPrefix := []byte(fmt.Sprintf("%s/%s", host.KeyClientStorePrefix, exported.Tendermint)) - iterator := sdk.KVStorePrefixIterator(store, tendermintClientPrefix) - +func PruneTendermintConsensusStates(ctx sdk.Context, clientKeeper ClientKeeper) error { var clientIDs []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 := host.MustParseClientStatePath(path) + k.IterateClientStates(ctx, nil, func(clientID string, _ exported.ClientState) bool { clientIDs = append(clientIDs, clientID) - } + return false + }) // 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 clientIDs { - 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") + clientState, ok := clientKeeper.GetClientState(ctx, clientID) + if !ok { + return sdkerrors.Wrapf(clienttypes.ErrClientNotFound, "clientID %s", clientID) } tmClientState, ok := clientState.(*ClientState) From 682457c7d30db5f6d32502cb0b48cc6a240d1980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 30 Nov 2022 15:48:29 +0100 Subject: [PATCH 2/7] chore: fix tests and build --- modules/core/02-client/keeper/migrations.go | 2 +- modules/core/02-client/migrations/v7/store.go | 12 ++++-------- modules/core/02-client/migrations/v7/store_test.go | 6 +++--- modules/light-clients/07-tendermint/migrations.go | 9 +++------ .../light-clients/07-tendermint/migrations_test.go | 2 +- 5 files changed, 12 insertions(+), 19 deletions(-) diff --git a/modules/core/02-client/keeper/migrations.go b/modules/core/02-client/keeper/migrations.go index c63a6762f5b..5dd73cb4689 100644 --- a/modules/core/02-client/keeper/migrations.go +++ b/modules/core/02-client/keeper/migrations.go @@ -34,5 +34,5 @@ func (m Migrator) Migrate1to2(ctx sdk.Context) error { // - removes the localhost client // - asserts that existing tendermint clients are properly registered on the chain codec func (m Migrator) Migrate2to3(ctx sdk.Context) error { - return v7.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc) + return v7.MigrateStore(ctx, m.keeper, m.keeper.cdc) } diff --git a/modules/core/02-client/migrations/v7/store.go b/modules/core/02-client/migrations/v7/store.go index 8823ca6268c..0960ccc26e6 100644 --- a/modules/core/02-client/migrations/v7/store.go +++ b/modules/core/02-client/migrations/v7/store.go @@ -1,13 +1,10 @@ package v7 import ( - "fmt" "strings" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" - "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" @@ -38,7 +35,7 @@ func MigrateStore(ctx sdk.Context, keeper ClientKeeper, cdc codec.BinaryCodec) e return err } - if err := handleLocalhostMigration(ctx, keeper, cdc); err != nil { + if err := handleLocalhostMigration(ctx, keeper); err != nil { return err } @@ -114,15 +111,14 @@ func handleTendermintMigration(ctx sdk.Context, keeper ClientKeeper) error { } // handleLocalhostMigration removes all client and consensus states associated with the localhost client type. -func handleLocalhostMigration(ctx sdk.Context, store sdk.KVStore, cdc codec.BinaryCodec) error { - clients, err := collectClients(ctx, store, Localhost) +func handleLocalhostMigration(ctx sdk.Context, keeper ClientKeeper) error { + clients, err := collectClients(ctx, keeper, Localhost) if err != nil { return err } for _, clientID := range clients { - clientPrefix := []byte(fmt.Sprintf("%s/%s/", host.KeyClientStorePrefix, clientID)) - clientStore := prefix.NewStore(store, clientPrefix) + clientStore := keeper.ClientStore(ctx, clientID) // delete the client state clientStore.Delete(host.ClientStateKey()) diff --git a/modules/core/02-client/migrations/v7/store_test.go b/modules/core/02-client/migrations/v7/store_test.go index c67425ea8bd..8353cb59959 100644 --- a/modules/core/02-client/migrations/v7/store_test.go +++ b/modules/core/02-client/migrations/v7/store_test.go @@ -49,11 +49,11 @@ func (suite *MigrationsV7TestSuite) TestMigrateStoreTendermint() { cdc := codec.NewProtoCodec(registry) solomachine.RegisterInterfaces(registry) - err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().GetKey(host.StoreKey), cdc) + err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, cdc) suite.Require().Error(err) ibctm.RegisterInterfaces(registry) - err = v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().GetKey(host.StoreKey), cdc) + err = v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, cdc) suite.Require().NoError(err) } @@ -79,7 +79,7 @@ func (suite *MigrationsV7TestSuite) TestMigrateStore() { suite.createSolomachineClients(solomachines) suite.createLocalhostClients() - err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().GetKey(host.StoreKey), suite.chainA.App.AppCodec()) + err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, suite.chainA.App.AppCodec()) suite.Require().NoError(err) suite.assertSolomachineClients(solomachines) diff --git a/modules/light-clients/07-tendermint/migrations.go b/modules/light-clients/07-tendermint/migrations.go index 1f304fa3973..550087552d5 100644 --- a/modules/light-clients/07-tendermint/migrations.go +++ b/modules/light-clients/07-tendermint/migrations.go @@ -1,12 +1,7 @@ package tendermint 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" @@ -17,7 +12,7 @@ import ( // PruneTendermintConsensusStates prunes all expired tendermint consensus states. This function // may optionally be called during in-place store migrations. The ibc store key must be provided. -func PruneTendermintConsensusStates(ctx sdk.Context, clientKeeper ClientKeeper) error { +func PruneTendermintConsensusStates(ctx sdk.Context, clientKeeper ClientKeeper, cdc codec.BinaryCodec) error { var clientIDs []string k.IterateClientStates(ctx, nil, func(clientID string, _ exported.ClientState) bool { clientIDs = append(clientIDs, clientID) @@ -29,6 +24,8 @@ func PruneTendermintConsensusStates(ctx sdk.Context, clientKeeper ClientKeeper) var totalPruned int for _, clientID := range clientIDs { + clientStore := clientKeeper.ClientStore(ctx, clientID) + clientState, ok := clientKeeper.GetClientState(ctx, clientID) if !ok { return sdkerrors.Wrapf(clienttypes.ErrClientNotFound, "clientID %s", clientID) diff --git a/modules/light-clients/07-tendermint/migrations_test.go b/modules/light-clients/07-tendermint/migrations_test.go index c7fe56af5db..ed2a60ad1fa 100644 --- a/modules/light-clients/07-tendermint/migrations_test.go +++ b/modules/light-clients/07-tendermint/migrations_test.go @@ -99,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 = ibctm.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().GetKey(host.StoreKey)) + err = ibctm.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, suite.chainA.App.AppCodec()) suite.Require().NoError(err) for _, path := range paths { From cce66361bb8a90be481974e422f674eaa97d4c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 30 Nov 2022 15:49:35 +0100 Subject: [PATCH 3/7] chore: update migration docs --- 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 a92331fd239..839d12708b1 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -30,7 +30,7 @@ app.UpgradeKeeper.SetUpgradeHandler( upgradeName, func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) { // prune expired tendermint consensus states to save storage space - ibctm.PruneTendermintConsensusStates(ctx, app.Codec, appCodec, keys[ibchost.StoreKey]) + ibctm.PruneTendermintConsensusStates(ctx, app.IBCKeeper.ClientKeeper, app.Codec) return app.mm.RunMigrations(ctx, app.configurator, fromVM) }, From 227759baec247528f831910ccafa70af7b4b4215 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 30 Nov 2022 15:53:23 +0100 Subject: [PATCH 4/7] Update modules/core/02-client/migrations/v7/expected_keepers.go --- modules/core/02-client/migrations/v7/expected_keepers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/02-client/migrations/v7/expected_keepers.go b/modules/core/02-client/migrations/v7/expected_keepers.go index 7c6d2c36475..3f1593df84f 100644 --- a/modules/core/02-client/migrations/v7/expected_keepers.go +++ b/modules/core/02-client/migrations/v7/expected_keepers.go @@ -6,7 +6,7 @@ import ( "github.com/cosmos/ibc-go/v6/modules/core/exported" ) -// ClientKeeper expected account IBC client keeper +// ClientKeeper expected IBC client keeper type ClientKeeper interface { GetClientState(ctx sdk.Context, clientID string) (exported.ClientState, bool) IterateClientStates(ctx sdk.Context, prefix []byte, cb func(string, exported.ClientState) bool) From 2b26ecfc53ac7f8c3b7df72ac435e967082e51c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 30 Nov 2022 16:00:36 +0100 Subject: [PATCH 5/7] chore: further code cleanup --- modules/core/02-client/migrations/v7/expected_keepers.go | 1 + modules/core/02-client/migrations/v7/store.go | 3 +-- modules/light-clients/07-tendermint/expected_keepers.go | 2 ++ modules/light-clients/07-tendermint/migrations.go | 3 +-- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/modules/core/02-client/migrations/v7/expected_keepers.go b/modules/core/02-client/migrations/v7/expected_keepers.go index 7c6d2c36475..b0c10f6bcb0 100644 --- a/modules/core/02-client/migrations/v7/expected_keepers.go +++ b/modules/core/02-client/migrations/v7/expected_keepers.go @@ -9,6 +9,7 @@ import ( // ClientKeeper expected account IBC client keeper type ClientKeeper interface { GetClientState(ctx sdk.Context, clientID string) (exported.ClientState, bool) + SetClientState(ctx sdk.Context, clientID string, clientState exported.ClientState) IterateClientStates(ctx sdk.Context, prefix []byte, cb func(string, exported.ClientState) bool) ClientStore(ctx sdk.Context, clientID string) sdk.KVStore } diff --git a/modules/core/02-client/migrations/v7/store.go b/modules/core/02-client/migrations/v7/store.go index 0960ccc26e6..7ff7a5797f0 100644 --- a/modules/core/02-client/migrations/v7/store.go +++ b/modules/core/02-client/migrations/v7/store.go @@ -75,8 +75,7 @@ func handleSolomachineMigration(ctx sdk.Context, keeper ClientKeeper, cdc codec. return sdkerrors.Wrap(err, "failed to unmarshal client state bytes into solo machine client state") } - // update solomachine in store - clientStore.Set(host.ClientStateKey(), bz) + keeper.SetClientState(ctx, clientID, &updatedClientState) removeAllClientConsensusStates(clientStore) } diff --git a/modules/light-clients/07-tendermint/expected_keepers.go b/modules/light-clients/07-tendermint/expected_keepers.go index f98115a6981..d69af64b4ea 100644 --- a/modules/light-clients/07-tendermint/expected_keepers.go +++ b/modules/light-clients/07-tendermint/expected_keepers.go @@ -2,6 +2,7 @@ package tendermint import ( sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/tendermint/tendermint/libs/log" "github.com/cosmos/ibc-go/v6/modules/core/exported" ) @@ -11,4 +12,5 @@ type ClientKeeper interface { GetClientState(ctx sdk.Context, clientID string) (exported.ClientState, bool) IterateClientStates(ctx sdk.Context, prefix []byte, cb func(string, exported.ClientState) bool) ClientStore(ctx sdk.Context, clientID string) sdk.KVStore + Logger(ctx sdk.Context) log.Logger } diff --git a/modules/light-clients/07-tendermint/migrations.go b/modules/light-clients/07-tendermint/migrations.go index 550087552d5..20c7031bacb 100644 --- a/modules/light-clients/07-tendermint/migrations.go +++ b/modules/light-clients/07-tendermint/migrations.go @@ -6,7 +6,6 @@ import ( 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" ) @@ -39,7 +38,7 @@ func PruneTendermintConsensusStates(ctx sdk.Context, clientKeeper ClientKeeper, totalPruned += PruneAllExpiredConsensusStates(ctx, clientStore, cdc, tmClientState) } - clientLogger := ctx.Logger().With("module", "x/"+host.ModuleName+"/"+clienttypes.SubModuleName) + clientLogger := clientKeeper.Logger(ctx) clientLogger.Info("pruned expired tendermint consensus states", "total", totalPruned) return nil From e27a3a0af4b96650a43f50a31d0d3b161a91c520 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 30 Nov 2022 16:06:00 +0100 Subject: [PATCH 6/7] fix: add back optimization --- modules/core/02-client/migrations/v7/store.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/modules/core/02-client/migrations/v7/store.go b/modules/core/02-client/migrations/v7/store.go index 7ff7a5797f0..176cc608e26 100644 --- a/modules/core/02-client/migrations/v7/store.go +++ b/modules/core/02-client/migrations/v7/store.go @@ -137,6 +137,12 @@ func collectClients(ctx sdk.Context, keeper ClientKeeper, clientType string) (cl var clientIDs []string keeper.IterateClientStates(ctx, nil, func(clientID string, _ exported.ClientState) bool { clientIDs = append(clientIDs, clientID) + + // optimization: exit after a single tendermint client iteration + if strings.Contains(clientID, exported.Tendermint) { + return true + } + return false }) From 68963677fcad5da43ae820ba8e3f6958881b2665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Thu, 1 Dec 2022 12:52:34 +0100 Subject: [PATCH 7/7] refactor: pause work to split up into multiple prs --- docs/migrations/v6-to-v7.md | 2 +- modules/core/02-client/keeper/migrations.go | 2 +- modules/core/02-client/migrations/v7/store.go | 30 +++++++++++++------ .../02-client/migrations/v7/store_test.go | 9 ++++-- .../light-clients/07-tendermint/migrations.go | 4 +-- .../07-tendermint/migrations_test.go | 2 +- testing/simapp/app.go | 2 +- testing/simapp/upgrades/v7/upgrades.go | 7 +++-- 8 files changed, 37 insertions(+), 21 deletions(-) diff --git a/docs/migrations/v6-to-v7.md b/docs/migrations/v6-to-v7.md index 839d12708b1..00c648f1573 100644 --- a/docs/migrations/v6-to-v7.md +++ b/docs/migrations/v6-to-v7.md @@ -30,7 +30,7 @@ app.UpgradeKeeper.SetUpgradeHandler( upgradeName, func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) { // prune expired tendermint consensus states to save storage space - ibctm.PruneTendermintConsensusStates(ctx, app.IBCKeeper.ClientKeeper, app.Codec) + ibctm.PruneTendermintConsensusStates(ctx, app.Codec, app.IBCKeeper.ClientKeeper) return app.mm.RunMigrations(ctx, app.configurator, fromVM) }, diff --git a/modules/core/02-client/keeper/migrations.go b/modules/core/02-client/keeper/migrations.go index 5dd73cb4689..3df061adc43 100644 --- a/modules/core/02-client/keeper/migrations.go +++ b/modules/core/02-client/keeper/migrations.go @@ -34,5 +34,5 @@ func (m Migrator) Migrate1to2(ctx sdk.Context) error { // - removes the localhost client // - asserts that existing tendermint clients are properly registered on the chain codec func (m Migrator) Migrate2to3(ctx sdk.Context) error { - return v7.MigrateStore(ctx, m.keeper, m.keeper.cdc) + return v7.MigrateStore(ctx, m.keeper.cdc, m.keeper) } diff --git a/modules/core/02-client/migrations/v7/store.go b/modules/core/02-client/migrations/v7/store.go index 176cc608e26..00805d03600 100644 --- a/modules/core/02-client/migrations/v7/store.go +++ b/modules/core/02-client/migrations/v7/store.go @@ -1,6 +1,7 @@ package v7 import ( + "fmt" "strings" "github.com/cosmos/cosmos-sdk/codec" @@ -26,8 +27,8 @@ const Localhost string = "09-localhost" // - Pruning all solo machine consensus states // - Removing the localhost client // - Asserting existing tendermint clients are properly registered on the chain codec -func MigrateStore(ctx sdk.Context, keeper ClientKeeper, cdc codec.BinaryCodec) error { - if err := handleSolomachineMigration(ctx, keeper, cdc); err != nil { +func MigrateStore(ctx sdk.Context, cdc codec.BinaryCodec, keeper ClientKeeper) error { + if err := handleSolomachineMigration(ctx, cdc, keeper); err != nil { return err } @@ -44,7 +45,7 @@ func MigrateStore(ctx sdk.Context, keeper ClientKeeper, cdc codec.BinaryCodec) e // handleSolomachineMigration iterates over the solo machine clients and migrates client state from // protobuf definition v2 to v3. All consensus states stored outside of the client state are pruned. -func handleSolomachineMigration(ctx sdk.Context, keeper ClientKeeper, cdc codec.BinaryCodec) error { +func handleSolomachineMigration(ctx sdk.Context, cdc codec.BinaryCodec, keeper ClientKeeper) error { clients, err := collectClients(ctx, keeper, exported.Solomachine) if err != nil { return err @@ -90,6 +91,7 @@ func handleTendermintMigration(ctx sdk.Context, keeper ClientKeeper) error { if err != nil { return err } + fmt.Println(clients) if len(clients) > 1 { return sdkerrors.Wrap(sdkerrors.ErrLogic, "more than one Tendermint client collected") @@ -133,18 +135,28 @@ func handleLocalhostMigration(ctx sdk.Context, keeper ClientKeeper) error { // avoid state corruption as modifying state during iteration is unsafe. A special case // for tendermint clients is included as only one tendermint clientID is required for // v7 migrations. -func collectClients(ctx sdk.Context, keeper ClientKeeper, clientType string) (clients []string, err error) { +func collectClients(ctx sdk.Context, keeper ClientKeeper, clientType string) ([]string, error) { + store := ctx.KVStore(k.storeKey) + iterator := sdk.KVStorePrefixIterator(store, host.PrefixedClientStoreKey(prefix)) + var clientIDs []string - keeper.IterateClientStates(ctx, nil, func(clientID string, _ exported.ClientState) bool { + + 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 := host.MustParseClientStatePath(path) clientIDs = append(clientIDs, clientID) // optimization: exit after a single tendermint client iteration if strings.Contains(clientID, exported.Tendermint) { - return true + return clientIDs, nil } - - return false - }) + } return clientIDs, nil } diff --git a/modules/core/02-client/migrations/v7/store_test.go b/modules/core/02-client/migrations/v7/store_test.go index 8353cb59959..82587090ddf 100644 --- a/modules/core/02-client/migrations/v7/store_test.go +++ b/modules/core/02-client/migrations/v7/store_test.go @@ -1,6 +1,7 @@ package v7_test import ( + "fmt" "strconv" "testing" @@ -44,16 +45,17 @@ func TestIBCTestSuite(t *testing.T) { func (suite *MigrationsV7TestSuite) TestMigrateStoreTendermint() { path := ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.SetupClients(path) + fmt.Println("here") registry := codectypes.NewInterfaceRegistry() cdc := codec.NewProtoCodec(registry) solomachine.RegisterInterfaces(registry) - err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, cdc) + err := v7.MigrateStore(suite.chainA.GetContext(), cdc, suite.chainA.GetSimApp().IBCKeeper.ClientKeeper) suite.Require().Error(err) ibctm.RegisterInterfaces(registry) - err = v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, cdc) + err = v7.MigrateStore(suite.chainA.GetContext(), cdc, suite.chainA.GetSimApp().IBCKeeper.ClientKeeper) suite.Require().NoError(err) } @@ -61,6 +63,7 @@ func (suite *MigrationsV7TestSuite) TestMigrateStoreTendermint() { // ensure that solo machine clients are migrated and their consensus states are removed // ensure the localhost is deleted entirely. func (suite *MigrationsV7TestSuite) TestMigrateStore() { + fmt.Println("there") paths := []*ibctesting.Path{ ibctesting.NewPath(suite.chainA, suite.chainB), ibctesting.NewPath(suite.chainA, suite.chainB), @@ -79,7 +82,7 @@ func (suite *MigrationsV7TestSuite) TestMigrateStore() { suite.createSolomachineClients(solomachines) suite.createLocalhostClients() - err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, suite.chainA.App.AppCodec()) + err := v7.MigrateStore(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper) suite.Require().NoError(err) suite.assertSolomachineClients(solomachines) diff --git a/modules/light-clients/07-tendermint/migrations.go b/modules/light-clients/07-tendermint/migrations.go index 20c7031bacb..5b8878c7754 100644 --- a/modules/light-clients/07-tendermint/migrations.go +++ b/modules/light-clients/07-tendermint/migrations.go @@ -11,9 +11,9 @@ import ( // PruneTendermintConsensusStates prunes all expired tendermint consensus states. This function // may optionally be called during in-place store migrations. The ibc store key must be provided. -func PruneTendermintConsensusStates(ctx sdk.Context, clientKeeper ClientKeeper, cdc codec.BinaryCodec) error { +func PruneTendermintConsensusStates(ctx sdk.Context, cdc codec.BinaryCodec, clientKeeper ClientKeeper) error { var clientIDs []string - k.IterateClientStates(ctx, nil, func(clientID string, _ exported.ClientState) bool { + clientKeeper.IterateClientStates(ctx, []byte(exported.Tendermint), func(clientID string, _ exported.ClientState) bool { clientIDs = append(clientIDs, clientID) return false }) diff --git a/modules/light-clients/07-tendermint/migrations_test.go b/modules/light-clients/07-tendermint/migrations_test.go index ed2a60ad1fa..36ae9143ee2 100644 --- a/modules/light-clients/07-tendermint/migrations_test.go +++ b/modules/light-clients/07-tendermint/migrations_test.go @@ -99,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 = ibctm.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper, suite.chainA.App.AppCodec()) + err = ibctm.PruneTendermintConsensusStates(suite.chainA.GetContext(), suite.chainA.App.AppCodec(), suite.chainA.GetSimApp().IBCKeeper.ClientKeeper) suite.Require().NoError(err) for _, path := range paths { diff --git a/testing/simapp/app.go b/testing/simapp/app.go index 633199155e7..b1f5379a02f 100644 --- a/testing/simapp/app.go +++ b/testing/simapp/app.go @@ -899,7 +899,7 @@ func (app *SimApp) setupUpgradeHandlers() { app.mm, app.configurator, app.appCodec, - app.keys[ibchost.StoreKey], + app.IBCKeeper.ClientKeeper, ), ) } diff --git a/testing/simapp/upgrades/v7/upgrades.go b/testing/simapp/upgrades/v7/upgrades.go index a9072ed63a2..154b3f81e17 100644 --- a/testing/simapp/upgrades/v7/upgrades.go +++ b/testing/simapp/upgrades/v7/upgrades.go @@ -2,11 +2,11 @@ package v7 import ( "github.com/cosmos/cosmos-sdk/codec" - storetypes "github.com/cosmos/cosmos-sdk/store/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/module" upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" + clientkeeper "github.com/cosmos/ibc-go/v6/modules/core/02-client/keeper" ibctm "github.com/cosmos/ibc-go/v6/modules/light-clients/07-tendermint" ) @@ -20,13 +20,14 @@ func CreateUpgradeHandler( mm *module.Manager, configurator module.Configurator, cdc codec.BinaryCodec, - hostStoreKey *storetypes.KVStoreKey, + clientKeeper clientkeeper.Keeper, ) upgradetypes.UpgradeHandler { return func(ctx sdk.Context, _ upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) { // OPTIONAL: prune expired tendermint consensus states to save storage space - if err := ibctm.PruneTendermintConsensusStates(ctx, cdc, hostStoreKey); err != nil { + if err := ibctm.PruneTendermintConsensusStates(ctx, cdc, clientKeeper); err != nil { return nil, err } + return mm.RunMigrations(ctx, configurator, vm) } }