feat: state migration from IAVL to SMT (ADR-040) #10962

gsk967 · 2022-01-18T07:19:22Z

State migration from iavl to smt

This migration works on upgrade handler

app.registerUpgradeHandlers()


func (app *SimApp) registerUpgradeHandlers() {
	// This is the upgrade plan name we used in the gov proposal.
	upgradeName := "iavl-to-smt-upgrade"
	app.UpgradeKeeper.SetUpgradeHandler(upgradeName, func(ctx sdk.Context, plan upgradetypes.Plan, _ module.VersionMap) (module.VersionMap, error) {
	
	        fromVM := map[string]uint64{
			...
		}
		
	        err := store2.MigrateV2(iavlStore, v2Store)
                if err != nil {
                    return fromVM, err 
                }
		
		return app.mm.RunMigrations(ctx, app.configurator, fromVM)
	})
}

Description

Closes: #XXXX

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

State migration from iavl to smt

i-norden · 2022-01-19T15:09:34Z

For #10803 and #10891

roysc

This should work, but we should introduce it in the context of the command where it will be used, and include a unit test. We should make the usage clear: the v2 store schema needs to contain all the v1 stores at initialization.

store/v2/multi/migration.go

i-norden

A few comments, thanks!

store/v2/multi/migration.go

…-sdk into sai/migration_for_adr40

gsk967 · 2022-01-27T05:29:02Z

@i-norden I address the pr comments, Can you please review it again.

store/v2/multi/migration.go

MigrationV2 will return RootStore as output

gsk967 · 2022-03-04T06:38:51Z

@robert-zaremba @roysc I address the pr comments, can you look into those changesd

roysc

LGTM

robert-zaremba

not everything was addressed.

store/rootmulti/store.go

robert-zaremba · 2022-03-11T11:23:44Z

store/rootmulti/store.go

-func (rs *Store) GetStores() map[types.StoreKey]types.CommitKVStore {
-	return rs.stores
+// GetStoreNames returns the storeNames and StoreKeys
+func (rs *Store) GetStoreNames() map[string]types.StoreKey {


Why renaming? If we want to rename then let's be more precise. I suggest: StoreKeysByName (no need for Get prefix)

robert-zaremba · 2022-03-11T11:24:03Z

store/v2/multi/migration.go

+)
+
+// MigrateV2 will migrate the state from iavl to smt
+func MigrateV2(rootMultiStore *v1Store.Store, store2db dbm.DBConnection, storeConfig StoreConfig) (*Store, error) {


Suggested change

func MigrateV2(rootMultiStore *v1Store.Store, store2db dbm.DBConnection, storeConfig StoreConfig) (*Store, error) {

func MigrateFromV1(rootMultiStore *v1Store.Store, store2db dbm.DBConnection, storeConfig StoreConfig) (*Store, error) {

robert-zaremba · 2022-03-11T11:25:46Z

store/v2/multi/migration.go

+
+	// iterate through the rootmulti stores and save the key/values into smt tree
+	for _, store := range stores {
+		subStore, err := rootStore.getSubstore(store.name)


shouldn't we make sure that we use the same store key when doing migration?

we are registering the storeKeys into v2Store from v1Store
https://github.com/cosmos/cosmos-sdk/pull/10962/files#diff-ad3a972d3e8f738c0ee269b134134e2beee4e5f0ee9849b0c71eb7cc9d669ce1R24

store/v2/multi/migration_test.go

roysc

Lgtm except for the remaining renames

roysc · 2022-03-15T11:44:13Z

store/rootmulti/snapshot_test.go

@@ -210,7 +210,8 @@ func TestMultistoreSnapshotRestore(t *testing.T) {
 	require.Equal(t, *dummyExtensionItem.GetExtension(), *nextItem.GetExtension())

 	assert.Equal(t, source.LastCommitID(), target.LastCommitID())
-	for key, sourceStore := range source.GetStores() {
+	for _, key := range source.GetStoreNames() {


Need to finish the renames in this file

robert-zaremba · 2022-03-17T08:56:34Z

@gsk967 you should "resolve" addressed suggestions, it helps with review.

robert-zaremba

Approving. Please add Changelog updates.

gsk967 · 2022-03-17T10:14:38Z

@robert-zaremba Update the changelog and move the migration into alpha1

robert-zaremba · 2022-03-18T23:38:13Z

@gsk967 a test is not passing.

feat: add migration for adr40

4cc992f

State migration from iavl to smt

github-actions bot added the C:Store label Jan 18, 2022

i-norden requested review from i-norden, roysc and robert-zaremba January 18, 2022 15:04

i-norden changed the title ~~feat: add migration for adr40~~ feat: ADR-040: add migration for adr40 Jan 19, 2022

i-norden requested a review from aaronc January 19, 2022 15:10

roysc reviewed Jan 19, 2022

View reviewed changes

store/v2/multi/migration.go Outdated Show resolved Hide resolved

chore: removed sort from migration func

df0dc4f

gsk967 requested a review from roysc January 20, 2022 08:05

Merge branch 'master' into sai/migration_for_adr40

fca170d

i-norden reviewed Jan 25, 2022

View reviewed changes

Sai Kumar and others added 3 commits January 27, 2022 10:57

chore: address the pr comments

79583ec

Merge branch 'sai/migration_for_adr40' of github.com:vulcanize/cosmos…

bcfeff1

…-sdk into sai/migration_for_adr40

Merge branch 'master' into sai/migration_for_adr40

d511fa3

gsk967 requested a review from i-norden January 27, 2022 05:29

gsk967 changed the title ~~feat: ADR-040: add migration for adr40~~ feat (ADR-040) : state migration from IAVL to SMT Jan 27, 2022

gsk967 marked this pull request as ready for review January 27, 2022 05:31

gsk967 requested a review from alexanderbez as a code owner January 27, 2022 05:31

gsk967 added 2 commits January 27, 2022 21:42

Merge branch 'master' into sai/migration_for_adr40

00b99a7

Merge branch 'master' into sai/migration_for_adr40

0eaec31

roysc suggested changes Jan 28, 2022

View reviewed changes

store/v2/multi/migration.go Outdated Show resolved Hide resolved

store/v2/multi/migration.go Outdated Show resolved Hide resolved

store/v2/multi/migration.go Outdated Show resolved Hide resolved

Sai Kumar and others added 2 commits January 29, 2022 14:09

test: add test for migrationv2

9f496e9

Merge branch 'master' into sai/migration_for_adr40

39b303b

gsk967 changed the title ~~feat (ADR-040) : state migration from IAVL to SMT~~ feat: state migration from IAVL to SMT (ADR-040) Jan 29, 2022

gsk967 requested a review from roysc January 29, 2022 13:51

gsk967 and others added 2 commits January 31, 2022 18:24

Merge branch 'master' into sai/migration_for_adr40

8b16dad

chore: change the MigrationV2

b305280

MigrationV2 will return RootStore as output

gsk967 and others added 3 commits March 4, 2022 11:09

Merge branch 'master' into sai/migration_for_adr40

4a9feb5

chore: address the pr comments

677639e

Merge branch 'master' into sai/migration_for_adr40

074da95

gsk967 requested review from robert-zaremba and roysc March 4, 2022 06:35

roysc approved these changes Mar 4, 2022

View reviewed changes

gsk967 added 3 commits March 6, 2022 19:30

Merge branch 'master' into sai/migration_for_adr40

a05f72e

Merge branch 'master' into sai/migration_for_adr40

6eb51d1

Merge branch 'master' into sai/migration_for_adr40

c16dc89

robert-zaremba reviewed Mar 11, 2022

View reviewed changes

gsk967 and others added 2 commits March 11, 2022 18:52

Merge branch 'master' into sai/migration_for_adr40

119b77a

address the pr comments

a97ed5a

gsk967 requested a review from robert-zaremba March 11, 2022 14:03

Merge branch 'master' into sai/migration_for_adr40

179743c

roysc reviewed Mar 15, 2022

View reviewed changes

address the pr comments

bb61b5d

gsk967 requested a review from roysc March 15, 2022 12:18

roysc approved these changes Mar 15, 2022

View reviewed changes

gsk967 added 2 commits March 16, 2022 10:30

Merge branch 'master' into sai/migration_for_adr40

f381e09

Merge branch 'master' into sai/migration_for_adr40

7eec931

robert-zaremba approved these changes Mar 17, 2022

View reviewed changes

chore: update changelog

1af9b3b

Merge branch 'master' into sai/migration_for_adr40

ec331a0

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 18, 2022

Merge branch 'master' into sai/migration_for_adr40

e21a6f8

mergify bot merged commit ea672d4 into cosmos:master Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: state migration from IAVL to SMT (ADR-040) #10962

feat: state migration from IAVL to SMT (ADR-040) #10962

gsk967 commented Jan 18, 2022 •

edited

Loading

i-norden commented Jan 19, 2022

roysc left a comment

i-norden left a comment

gsk967 commented Jan 27, 2022

gsk967 commented Mar 4, 2022

roysc left a comment

robert-zaremba left a comment

robert-zaremba Mar 11, 2022

robert-zaremba Mar 11, 2022

robert-zaremba Mar 11, 2022 •

edited

Loading

gsk967 Mar 11, 2022

roysc left a comment

roysc Mar 15, 2022

gsk967 Mar 15, 2022

robert-zaremba commented Mar 17, 2022

robert-zaremba left a comment •

edited

Loading

gsk967 commented Mar 17, 2022

robert-zaremba commented Mar 18, 2022

	func MigrateV2(rootMultiStore v1Store.Store, store2db dbm.DBConnection, storeConfig StoreConfig) (Store, error) {
	func MigrateFromV1(rootMultiStore v1Store.Store, store2db dbm.DBConnection, storeConfig StoreConfig) (Store, error) {

feat: state migration from IAVL to SMT (ADR-040) #10962

feat: state migration from IAVL to SMT (ADR-040) #10962

Conversation

gsk967 commented Jan 18, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

i-norden commented Jan 19, 2022

roysc left a comment

Choose a reason for hiding this comment

i-norden left a comment

Choose a reason for hiding this comment

gsk967 commented Jan 27, 2022

gsk967 commented Mar 4, 2022

roysc left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Mar 11, 2022

Choose a reason for hiding this comment

robert-zaremba Mar 11, 2022

Choose a reason for hiding this comment

robert-zaremba Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

gsk967 Mar 11, 2022

Choose a reason for hiding this comment

roysc left a comment

Choose a reason for hiding this comment

roysc Mar 15, 2022

Choose a reason for hiding this comment

gsk967 Mar 15, 2022

Choose a reason for hiding this comment

robert-zaremba commented Mar 17, 2022

robert-zaremba left a comment • edited Loading

Choose a reason for hiding this comment

gsk967 commented Mar 17, 2022

robert-zaremba commented Mar 18, 2022

gsk967 commented Jan 18, 2022 •

edited

Loading

robert-zaremba Mar 11, 2022 •

edited

Loading

robert-zaremba left a comment •

edited

Loading