Add in-place and genesis migrations #205

colin-axner · 2021-05-31T12:06:18Z

Description

Adds an in-place migration for SDK v0.40 -> ibc-go v1.0.0 and genesis migrations

Solo machine proto definition changed (FrozenSequence because a boolean). This may lead to indeterminate decoding errors. During in-place and genesis migrations, we will decode the solomachine into the v1 definition, then we will migrate it to v2 and set this in the store or genesis. All consensus states for solo machines are removed. The legacy solo machine is generated into legacy/v100

Also all expired tendermint consensus states are pruned

ref: #11

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

Prunes solomachines and expired tendermint consensus states via an x/upgrade

modules/core/02-client/legacy/v100/store.go

AdityaSripal

Looks great! Just have a question on solo machine cons states

modules/core/02-client/legacy/v100/store.go

fix iteration bug remove solo machine connections remove solo machine channels

modules/core/04-channel/legacy/v100/store.go

codecov-commenter · 2021-06-02T11:36:03Z

Codecov Report

Merging #205 (fed9443) into main (2548ab5) will decrease coverage by 0.87%.
The diff coverage is 51.64%.

@@            Coverage Diff             @@
##             main     #205      +/-   ##
==========================================
- Coverage   80.11%   79.23%   -0.88%     
==========================================
  Files         109      115       +6     
  Lines        6507     6719     +212     
==========================================
+ Hits         5213     5324     +111     
- Misses        932     1017      +85     
- Partials      362      378      +16

Impacted Files	Coverage Δ
modules/core/03-connection/types/params.go	`76.92% <ø> (ø)`
modules/core/keeper/migrations.go	`0.00% <0.00%> (ø)`
modules/light-clients/07-tendermint/types/store.go	`78.67% <0.00%> (-8.11%)`	⬇️
...odules/light-clients/07-tendermint/types/update.go	`81.08% <ø> (ø)`
modules/core/02-client/legacy/v100/solomachine.go	`4.34% <4.34%> (ø)`
modules/core/02-client/keeper/migrations.go	`50.00% <50.00%> (ø)`
modules/core/02-client/legacy/v100/store.go	`64.86% <64.86%> (ø)`
modules/core/02-client/legacy/v100/genesis.go	`80.76% <80.76%> (ø)`
modules/core/legacy/v100/genesis.go	`87.50% <87.50%> (ø)`
modules/core/module.go	`56.09% <100.00%> (+2.25%)`	⬆️
... and 7 more

colin-axner · 2021-06-03T08:45:52Z

I thought about this some more. Instead of deleting the solo machines I'm going to migrate the proto definitions. I will generate the v1 solo machine proto files in the legacy dir, iterate over the client store and migrate the old proto definition to the new one (changing frozen sequence to be a boolean)

The existing solo machines are still considered active so they don't need to be deleted. This avoids risk of deleting the wrong clients/connections/channels and it'll make genesis migration a lot easier. I'm not sure how I would be capable of efficiently iterating all channels to delete those associated with solo machines

Genesis migration will do the same, iterate over the clients, unmarshal if solo machine, migrate to new definition, marshal and set in genesis

Regenerate v1 solo machine definition in 02-client legacy Migrate from v1 to v2 solo machine client state Prune v1 solo machine consensus states

colin-axner · 2021-06-08T11:32:48Z

Our genesis migrations will not fit into the interface defined by the SDK since it needs access to the genesis block time (this block time must also be updated before the migration is ran). Even if we didn't need the block time, the interface for adding genesis migrations outside of the SDK is clumsy. We would need to add another version migration. Normally the SDK might go from v0.40 -> v0.43. We would need to add a migration which goes from v0.43 -> v0.43.0-ibc or something like that. Basically doing the migrations for v0.43.0 for IBC

The better way to do this is to just explicitly call the IBC migrations without abiding to the migration interface.

It'll look something like

newIBCGenState, err := v100.Migrate(IBCGenState, genesisTime)
if err != nil {
    return err
}
genesis.AppState[ibchost.ModuleName] = newIBCGenState

I'll add the code explicitly into our migration doc and open a pr on gaia

cc @shahankhatch

colin-axner · 2021-06-08T11:42:40Z

Actually, gaia won't need this as it should use the in-place migrations (not a genesis restart)

… into colin/11-migration-scripts

docs/migrations/ibc-migration-043.md

modules/core/02-client/keeper/migrations.go

modules/core/02-client/legacy/v100/genesis_test.go

modules/core/keeper/migrations.go

modules/core/module.go

modules/core/02-client/legacy/v100/store.go

modules/core/legacy/v100/genesis.go

AdityaSripal

I believe you need to set processedHeight and Iteration keys for the unexpired tendermint consensus states that you migrate over.

docs/migrations/ibc-migration-043.md

modules/core/02-client/legacy/v100/genesis.go

AdityaSripal · 2021-06-09T18:01:13Z

modules/core/02-client/legacy/v100/store.go

+			if err = ibctmtypes.PruneAllExpiredConsensusStates(ctx, clientStore, cdc, clientState.(*ibctmtypes.ClientState)); err != nil {
+				return err
+			}


We also need to set iteration and height keys for unexpired consensus states

… into colin/11-migration-scripts

Test that the legacy solo machines can be successfully unmarshalled. This requires registering an implementation for the legacy solo machine. An implemenation which panics has been added. This implementation should only be registered against a clientCtx during a migrate cli cmd. The implementation is only briefly used in order to decode the previous solo machine set in genesis.

colin-axner · 2021-06-10T12:13:14Z

modules/core/02-client/legacy/v100/store.go

+
+// addConsensusMetadata adds the iteration key and processed height for all unexpired tendermint consensus states
+// These keys were not included in the previous release of the IBC module.
+func addConsensusMetadata(ctx sdk.Context, clientStore sdk.KVStore, cdc codec.BinaryCodec, clientState *ibctmtypes.ClientState) error {


I decided not to modify PruneAllExpiredConsensusStates as that seems like a useful ibctmtypes function by itself. Since the insertion of the iteration key and processed height key is logic specific to this migration, I decided to iterate the consensus state again and do it in a separate iteration. It is less efficient, but I think better code wise. Open to changing if efficiency is a big concern

colin-axner · 2021-06-10T12:13:57Z

modules/core/02-client/legacy/v100/solomachine.go

+// NOTE: this is a mock implmentation for exported.ClientState. This implementation
+// should only be registered on the InterfaceRegistry during cli command genesis migration.
+// This implementation is only used to successfully unmarshal the previous solo machine
+// client state and consensus state and migrate them to the new implementations. When the proto
+// codec unmarshals, it calls UnpackInterfaces() to create a cached value of the any. The
+// UnpackInterfaces function for IdenitifiedClientState will attempt to unpack the any to
+// exported.ClientState. If the solomachine v1 type is not registered against the exported.ClientState
+// the unmarshal will fail. This implementation will panic on every interface function.
+// The same is done for the ConsensusState.


there's really no way around this. go semantic versioning will help a lot in the future

colin-axner · 2021-06-10T12:14:49Z

modules/core/legacy/v100/genesis_test.go

+	// migrate store get expected genesis
+	// store migration and genesis migration should produce identical results
+	err := clientv100.MigrateStore(path.EndpointA.Chain.GetContext(), path.EndpointA.Chain.GetSimApp().GetKey(host.StoreKey), path.EndpointA.Chain.App.AppCodec())
+	suite.Require().NoError(err)
+	expectedClientGenState := ibcclient.ExportGenesis(path.EndpointA.Chain.GetContext(), path.EndpointA.Chain.App.GetIBCKeeper().ClientKeeper)
+


this is the main test code added (this was copied from previous tests)

… into colin/11-migration-scripts

colin-axner · 2021-06-15T09:32:44Z

docs/migrations/ibc-migration-043.md

+```go
+app.UpgradeKeeper.SetUpgradeHandler("my-upgrade-proposal",
+        func(ctx sdk.Context, _ upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) {
+            // set max expected block time parameter. Replace the default with your expected value
+            // https://github.com/cosmos/ibc-go/blob/release/v1.0.x/docs/ibc/proto-docs.md#params-2
+            app.IBCKeeper.ConnectionKeeper.SetParams(ctx, ibcconnectiontypes.DefaultParams())
+
+            fromVM := map[string]uint64{
+                ... // other modules
+                "ibc":          1,
+                ... 
+            }   
+            return app.mm.RunMigrations(ctx, app.configurator, fromVM)
+        })      


colin-axner · 2021-06-15T14:25:18Z

docs/migrations/ibc-migration-043.md

+newGenState, err = ibcv100.MigrateGenesis(newGenState, clientCtx, *genDoc, expectedTimePerBlock)
+if err != nil {
+    return err 
+}


tested, got as far as I could. Final confirmation of IBC breaking upgrades is blocked on upstream changes (SDK is moving protobuf definition for upgrade/gov from v1beta1 to v1 and the current pr on the SDK has minor bugs)

I confirmed the migration updated the genesis, as far as I can tell, everything works on the IBC end, although I didn't get a chance to test that IBC clients were successfully upgraded. At the very least the chain started fine

The genesis client metadata was being set independently for each unexpired height. It needed to be moved outside the unexpired for loop

AdityaSripal

Looks good, just a couple nits

AdityaSripal · 2021-06-15T10:23:09Z

modules/core/02-client/legacy/v100/genesis.go

+									// iteration key and processed height keys.
+									if bytes.Equal(metadata.Key, ibctmtypes.ProcessedTimeKey(height)) {
+										clientMetadata = append(clientMetadata, types.GenesisMetadata{
+											Key:   ibctmtypes.ProcessedHeightKey(height),


Add godoc saying we're setting processed height to self height, which is still safe

AdityaSripal · 2021-06-16T12:46:29Z

modules/core/02-client/legacy/v100/genesis.go

+									// if we find the processed time metadata for an unexpired height, add the
+									// iteration key and processed height keys.
+									if bytes.Equal(metadata.Key, ibctmtypes.ProcessedTimeKey(height)) {
+										clientMetadata = append(clientMetadata, types.GenesisMetadata{


Couldn't you do this with just one append call??

wow, I've always worked under the impression append() took only 2 arguments

modules/core/02-client/legacy/v100/store.go

AdityaSripal · 2021-06-16T13:00:16Z

modules/core/02-client/legacy/v100/store_test.go

+	processedHeight, ok := ibctmtypes.GetProcessedHeight(clientStore, height)
+	suite.Require().True(ok)
+	suite.Require().Equal(types.GetSelfHeight(path.EndpointA.Chain.GetContext()), processedHeight)
+
+	consKey := ibctmtypes.GetIterationKey(clientStore, height)


Didn't this metadata already get created in original client update?

I guess you proved metadata did something by checking value of processed height

Yea, the purpose of this to ensure the unexpired consensus state metadata did not get pruned. I wrote this before adding the addConsensusMetadata function

found a bug, since I was using the iteration keys for iterating, it wasn't updating the metadata since the iteration keys hadn't been set

changed the structure, set new metadata for all consensus states (using traditional iteration), then prune expired consensus states

modules/core/02-client/legacy/v100/genesis_test.go

There was a bug in adding consensus metadata since it relied on the iteration key not yet set. This is fixed by using traditional iteration using the consensus state key, setting metadata for all consensus states, and then pruning expired consensus states. The store test has been updated to set create two tendermint clients

colin-axner · 2021-06-17T11:38:29Z

modules/core/02-client/legacy/v100/store_test.go

+
+		// remove processed height and iteration keys since these were missing from previous version of ibc module
+		clientStore := path.EndpointA.Chain.App.GetIBCKeeper().ClientKeeper.ClientStore(path.EndpointA.Chain.GetContext(), path.EndpointA.ClientID)
+		for _, height := range unexpiredHeights {
+			clientStore.Delete(ibctmtypes.ProcessedHeightKey(height))
+			clientStore.Delete(ibctmtypes.IterationKey(height))
+		}


this now enforces a check on the metadata being added

colin-axner · 2021-06-17T11:39:51Z

modules/core/02-client/legacy/v100/store_test.go

-	// these heights will be expired and also pruned
-	for i := 0; i < 3; i++ {
-		path.EndpointA.UpdateClient()
+	for _, path := range []*ibctesting.Path{path1, path2} {


I just added an extra for loop to test multiple paths at the same time

Polish for go vet

add in-place migrations

4f874d9

Prunes solomachines and expired tendermint consensus states via an x/upgrade

colin-axner added this to the 1.0.0 milestone May 31, 2021

colin-axner requested review from AdityaSripal, cwgoes and fedekunze May 31, 2021 12:06

colin-axner commented May 31, 2021

View reviewed changes

modules/core/02-client/legacy/v100/store.go Outdated Show resolved Hide resolved

AdityaSripal requested changes Jun 1, 2021

View reviewed changes

modules/core/02-client/legacy/v100/store.go Outdated Show resolved Hide resolved

update migrations

f8ca013

fix iteration bug remove solo machine connections remove solo machine channels

colin-axner commented Jun 2, 2021

View reviewed changes

modules/core/04-channel/legacy/v100/store.go Outdated Show resolved Hide resolved

colin-axner added 2 commits June 7, 2021 13:17

migrate solomachine from v1 to v2 during in place migration

29c1c48

Regenerate v1 solo machine definition in 02-client legacy Migrate from v1 to v2 solo machine client state Prune v1 solo machine consensus states

fix build

db0db21

colin-axner changed the title ~~Add in-place migrations~~ Add in-place and genesis migrations Jun 7, 2021

colin-axner added 2 commits June 7, 2021 18:11

add genesis migration

972ca3c

code cleanup

314aaba

Merge branch 'main' into colin/11-migration-scripts

40a30fa

colin-axner added 6 commits June 8, 2021 16:34

add store migration test for expired tendermint consensus states

863fdbd

Merge branch 'colin/11-migration-scripts' of github.com:cosmos/ibc-go…

a36801f

… into colin/11-migration-scripts

finish adding in place migration store tests

863b50a

add genesis test for solo machines

545056e

fix genesis migration bug, add tendermint tests

40484bb

test fix, changelog, migration docs

a24618d

colin-axner commented Jun 9, 2021

View reviewed changes

colin-axner added 2 commits June 9, 2021 18:23

Apply suggestions from code review

6d9bcdd

Merge branch 'main' into colin/11-migration-scripts

6c6fc52

colin-axner marked this pull request as ready for review June 9, 2021 16:23

colin-axner requested a review from AdityaSripal June 9, 2021 16:23

AdityaSripal requested changes Jun 9, 2021

View reviewed changes

AdityaSripal and others added 5 commits June 9, 2021 14:10

Update docs/migrations/ibc-migration-043.md

a67102f

apply Aditya's review suggestions

7e1f40f

Merge branch 'colin/11-migration-scripts' of github.com:cosmos/ibc-go…

3896a2a

… into colin/11-migration-scripts

fix tests

7edac1e

colin-axner commented Jun 10, 2021

View reviewed changes

Merge branch 'main' into colin/11-migration-scripts

a970700

colin-axner mentioned this pull request Jun 10, 2021

Technical Debt Tracker #145

Closed

6 tasks

colin-axner added 2 commits June 15, 2021 11:29

add migration support for max expected time per block

7ceca61

Merge branch 'colin/11-migration-scripts' of github.com:cosmos/ibc-go…

ca070bd

… into colin/11-migration-scripts

colin-axner requested a review from AdityaSripal June 15, 2021 09:31

colin-axner commented Jun 15, 2021

View reviewed changes

fix docs

0fcd0c4

colin-axner commented Jun 15, 2021

View reviewed changes

colin-axner added 2 commits June 16, 2021 17:31

fix bug found by Aditya

900b907

The genesis client metadata was being set independently for each unexpired height. It needed to be moved outside the unexpired for loop

remove unnecessary code

fed9443

AdityaSripal approved these changes Jun 17, 2021

View reviewed changes

colin-axner commented Jun 17, 2021

View reviewed changes

colin-axner enabled auto-merge (squash) June 17, 2021 11:40

colin-axner merged commit 2e95805 into main Jun 17, 2021

colin-axner deleted the colin/11-migration-scripts branch June 17, 2021 11:43

colin-axner mentioned this pull request Jul 7, 2021

add export genesis tests #243

Open

3 tasks

faddat referenced this pull request in notional-labs/ibc-go Feb 23, 2022

Merge pull request #205 from CosmWasm/go_vet

46e0189

Polish for go vet

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in-place and genesis migrations #205

Add in-place and genesis migrations #205

colin-axner commented May 31, 2021 •

edited

Loading

AdityaSripal left a comment

codecov-commenter commented Jun 2, 2021 •

edited

Loading

colin-axner commented Jun 3, 2021

colin-axner commented Jun 8, 2021

colin-axner commented Jun 8, 2021

AdityaSripal left a comment

AdityaSripal Jun 9, 2021

colin-axner Jun 10, 2021

colin-axner Jun 10, 2021

colin-axner Jun 10, 2021

colin-axner Jun 15, 2021

colin-axner Jun 15, 2021 •

edited

Loading

AdityaSripal left a comment

AdityaSripal Jun 15, 2021

AdityaSripal Jun 16, 2021

colin-axner Jun 17, 2021

AdityaSripal Jun 16, 2021

colin-axner Jun 17, 2021

colin-axner Jun 17, 2021

colin-axner Jun 17, 2021

colin-axner Jun 17, 2021 •

edited

Loading

Add in-place and genesis migrations #205

Add in-place and genesis migrations #205

Conversation

colin-axner commented May 31, 2021 • edited Loading

Description

AdityaSripal left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 2, 2021 • edited Loading

Codecov Report

colin-axner commented Jun 3, 2021

colin-axner commented Jun 8, 2021

colin-axner commented Jun 8, 2021

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

colin-axner commented May 31, 2021 •

edited

Loading

codecov-commenter commented Jun 2, 2021 •

edited

Loading

colin-axner Jun 15, 2021 •

edited

Loading

colin-axner Jun 17, 2021 •

edited

Loading