x{stake,slash,gov,distrib} In-place Store Migrations #8504

amaury1093 · 2021-02-03T14:19:50Z

Description

Add in-place store key migrations in x{stake,slash,gov,distrib} module's legacy folder:

copy the old keys.go (from the 41), put them in legacy/041/keys.go
add tests that migration scripts from 041 to 042 work
update SPEC for these modules

ref: #8345
depends on: #8485

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

…345-migration

…k into am-8345-migration

Co-authored-by: Robert Zaremba <[email protected]>

…k into am-8345-migration

Co-authored-by: Robert Zaremba <[email protected]>

robert-zaremba

Few remarks:

Let's avoid extending Keeper responsibility for handling migrations. In a comment I proposed 2 options.
We put migrations in legacy package. This pattern forces to copy current code to into a legacy package, before it gets legacy
I would prefer to limit the amount of code we copy (there are functions we copied which are not used).

simapp/app_test.go

x/distribution/keeper/migrations.go

robert-zaremba · 2021-02-22T15:26:19Z

x/distribution/keeper/migrations.go

+// Migrate1 implements MigrationKeeper.Migrate1 method.
+func (keeper Keeper) Migrate1(ctx sdk.Context) error {
+	return v042.MigrateStore(ctx, keeper.storeKey)
+}


Instead of enlarging the Keeper interface and responsibilities, let's create a Migrator structure:

type Migrator struct { keeper Keeper } // in RegisterServies m := Migrator(keeper) cfg.RegisterMigration(types.ModuleName, 1, m.Migrate1)

Other idea is just to make this functions directly:

cfg.RegisterMigration(types.ModuleName, 1, func (ctx) {return v42.MigrateStore(ctx, keeper.storeKey)

func (ctx) {return v42.MigrateStore(ctx, keeper.storeKey)

This would be ideal, but we can't access private keeper.storeKey there.

OK for the Migrator way.

robert-zaremba · 2021-02-22T15:56:50Z

x/distribution/legacy/v040/keys.go

+	binary.BigEndian.PutUint64(periodBz, period)
+	prefix := GetValidatorSlashEventKeyPrefix(v, height)
+	return append(prefix, periodBz...)
+}


Do we need to copy all this functions? For example, it seams that the function above is not used in migrations.

Yeah, for legacy stuff, I'm not trying to optimize. Just copy-pasting the whole file seems easier than going through in details what's needed and what's not.

lmk what you think.

robert-zaremba · 2021-02-22T15:58:45Z

x/distribution/legacy/v042/helpers.go

@@ -0,0 +1,70 @@
+package v042


why do we create legacy/v042? This version is not even released .

Yeah okay, we can create that later, I'll use types for now.

I removed the legacy/*/keys.go in f3ca3e5 . However I kept the v042 package, because the migration script from 0.41->0.42 lives in legacy/*/v042.

So that was my point - why putting the migration code in legacy/*/v042? v0.42 is not legacy. It's "current"

Ah I see. Well imo it shoud live in a folder called x/module/{something}/v042. We can rename something from "legacy" to "migrations" in a future PR if you want.

yeah, migrations sounds better

x/distribution/spec/02_state.md

…into am-8345-modules

robert-zaremba

Good. Thanks for updating the package structure.

aaronc

Can you explain your logic for starting ConsensusVersion at 1 @AmauryM? Maybe you addressed this in #8485 already but it seems like it would be simplest if we just used the default value of 0. What scenarios are you trying to deal with using 1? Are you thinking that 0 would represent the absence of a module?

amaury1093 · 2021-02-23T17:22:58Z

@aaronc Main reason is to explicit set values for all modules in the MigrationMap to avoid potential mistakes: #8485 (comment)

Edit: "Are you thinking that 0 would represent the absence of a module?" Thinking about it, that could be useful too? To differentiate new modules from existing ConsensusVersion=0 modules during an upgrade.

aaronc · 2021-02-23T19:26:32Z

@aaronc Main reason is to explicit set values for all modules in the MigrationMap to avoid potential mistakes: #8485 (comment)

Edit: "Are you thinking that 0 would represent the absence of a module?" Thinking about it, that could be useful too? To differentiate new modules from existing ConsensusVersion=0 modules during an upgrade.

Okay, so basically if the existing version is 0, that actually means the module was just added in this upgrade and therefore shouldn't have any old state migrated. Is that the idea?

amaury1093 · 2021-02-23T19:33:34Z

Okay, so basically if the existing version is 0, that actually means the module was just added in this upgrade and therefore shouldn't have any old state migrated. Is that the idea?

Yes.

anilcse · 2021-02-25T07:50:01Z

x/bank/keeper/migrations.go

-func (keeper BaseKeeper) Migrate1(ctx sdk.Context) error {
-	return v042.MigrateStore(ctx, keeper.storeKey)
+// Migrate1 migrates from version 1 to 2.
+func (m Migrator) Migrate1(ctx sdk.Context) error {


How about naming this as Migrate1to2?

anilcse · 2021-02-25T07:52:21Z

x/distribution/legacy/v042/helpers.go

@@ -0,0 +1,70 @@
+package v042


yeah, migrations sounds better

* Add 1st version of migrate * Put migration logic into Configurator * add test to bank store migration * add test for configurator * Error if no migration found * Remove RunMigrations from Configurator interface * Update spec * Rename folders * copy-paste from keys.go * Fix nil map * rename function * Update simapp/app.go Co-authored-by: Robert Zaremba <[email protected]> * Update simapp/app_test.go Co-authored-by: Robert Zaremba <[email protected]> * Adderss reviews * Fix tests * Update testutil/context.go Co-authored-by: Robert Zaremba <[email protected]> * Update docs for ConsensusVersion * Rename to forVersion * Fix tests * Check error early * Return 1 for intiial version * Use MigrationKeeper * Fix test * Revert adding marshaler to Configurator * Godoc updates * Update docs * Add distrib legacy folder * Add tests for distrib migration * Add gov migrations * Copy paste whole keys file * Add gov migrations * Add staking * Fix staking tests * Update spec and module.go * Update to latest changes * Update migration scripts * capability to 1 * Fix tests * Add package godoc * Remove whitespace * Remove global * Use Migrator * Remove 042 keys files * Fix build * Unlambda * Rename to Migrate1to2 Co-authored-by: Robert Zaremba <[email protected]>

amaury1093 added 10 commits February 1, 2021 16:18

Add 1st version of migrate

10d72d7

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-8…

8036fca

…345-migration

Put migration logic into Configurator

a9a406e

add test to bank store migration

9ed3987

add test for configurator

a095d3a

Merge branch 'master' into am-8345-migration

d550bff

Error if no migration found

b7141f9

Remove RunMigrations from Configurator interface

2d554ce

Update spec

3df3f44

Rename folders

ba6bd44

amaury1093 added C:x/staking C:x/gov C:x/slashing C:x/distribution distribution module related C:Store labels Feb 3, 2021

This was referenced Feb 3, 2021

Add in-place store migrations #8485

Merged

Refactor store keys to allow variable-length addresses #8345

Closed

amaury1093 added 3 commits February 3, 2021 15:26

Merge branch 'master' into am-8345-migration

197b7ce

copy-paste from keys.go

424932c

Merge branch 'am-8345-migration' of ssh://github.com/cosmos/cosmos-sd…

254a71f

…k into am-8345-migration

amaury1093 force-pushed the am-8345-modules branch from 7c76245 to f9decc0 Compare February 3, 2021 14:47

amaury1093 added 2 commits February 3, 2021 16:00

Fix nil map

b6c0714

rename function

9ab7f13

clevinson assigned robert-zaremba Feb 4, 2021

amaury1093 and others added 6 commits February 5, 2021 16:11

Update simapp/app.go

fc076f5

Co-authored-by: Robert Zaremba <[email protected]>

Update simapp/app_test.go

ae7b7dc

Co-authored-by: Robert Zaremba <[email protected]>

Adderss reviews

2c26734

Merge branch 'am-8345-migration' of ssh://github.com/cosmos/cosmos-sd…

eb15047

…k into am-8345-migration

Fix tests

291a9e6

Update testutil/context.go

41d29ed

Co-authored-by: Robert Zaremba <[email protected]>

clevinson requested a review from odeke-em February 19, 2021 15:09

Merge branch 'master' into am-8345-modules

0e24bad

robert-zaremba reviewed Feb 22, 2021

View reviewed changes

amaury1093 added 7 commits February 23, 2021 11:17

Remove whitespace

4942857

Remove global

30d9045

Use Migrator

828a41b

Remove 042 keys files

f3ca3e5

Fix build

0e17848

Merge branch 'am-8345-modules' of ssh://github.com/cosmos/cosmos-sdk …

95f524f

…into am-8345-modules

Unlambda

5456591

amaury1093 requested review from robert-zaremba and anilcse February 23, 2021 10:41

Merge branch 'master' into am-8345-modules

b5ff927

robert-zaremba approved these changes Feb 23, 2021

View reviewed changes

aaronc reviewed Feb 23, 2021

View reviewed changes

anilcse approved these changes Feb 25, 2021

View reviewed changes

Rename to Migrate1to2

036bd30

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 25, 2021

Merge branch 'master' into am-8345-modules

21e8a15

mergify bot merged commit ba74a7c into master Feb 25, 2021

mergify bot deleted the am-8345-modules branch February 25, 2021 10:43

amaury1093 mentioned this pull request Feb 25, 2021

Rename x/{mod}/legacy to x/{mod}/migrations #8700

Closed

4 tasks

This was referenced Mar 2, 2021

x/upgrade: added consensus version tracking (part of ADR-041) #8743

Merged

ADR 041: In-Place Store Migrations #8646

Merged

ethanfrey mentioned this pull request Mar 16, 2021

Can't perform software upgrade #8885

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x{stake,slash,gov,distrib} In-place Store Migrations #8504

x{stake,slash,gov,distrib} In-place Store Migrations #8504

amaury1093 commented Feb 3, 2021 •

edited

Loading

robert-zaremba left a comment

robert-zaremba Feb 22, 2021

robert-zaremba Feb 22, 2021

amaury1093 Feb 23, 2021

amaury1093 Feb 23, 2021

robert-zaremba Feb 22, 2021

amaury1093 Feb 23, 2021

robert-zaremba Feb 22, 2021

amaury1093 Feb 23, 2021

amaury1093 Feb 23, 2021 •

edited

Loading

robert-zaremba Feb 23, 2021

amaury1093 Feb 23, 2021

anilcse Feb 25, 2021

amaury1093 Feb 25, 2021

robert-zaremba left a comment

aaronc left a comment

amaury1093 commented Feb 23, 2021 •

edited

Loading

aaronc commented Feb 23, 2021

amaury1093 commented Feb 23, 2021

anilcse Feb 25, 2021

anilcse Feb 25, 2021

x{stake,slash,gov,distrib} In-place Store Migrations #8504

x{stake,slash,gov,distrib} In-place Store Migrations #8504

Conversation

amaury1093 commented Feb 3, 2021 • edited Loading

Description

robert-zaremba 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

Choose a reason for hiding this comment

amaury1093 Feb 23, 2021 • edited Loading

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

robert-zaremba left a comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

amaury1093 commented Feb 23, 2021 • edited Loading

aaronc commented Feb 23, 2021

amaury1093 commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Feb 3, 2021 •

edited

Loading

amaury1093 Feb 23, 2021 •

edited

Loading

amaury1093 commented Feb 23, 2021 •

edited

Loading