Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add in-place store migrations #8485

Merged
merged 33 commits into from
Feb 10, 2021
Merged

Add in-place store migrations #8485

merged 33 commits into from
Feb 10, 2021

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Feb 1, 2021

Description

This PR adds infrastructure to perform module-specific in-place store migrations. It also adds migration logic for the following modules:

In-place migrations follow the design outlined in #8429, namely:

  • each module now has its own ConsensusVersion. All consensus-breaking changes in module should increment this field.
  • modules, when introducing breaking changes, must register their migration functions from version N to N+1 inside the Configurator.
  • ModuleManager has a RunMigrations method, which loops through all modules and calls their registered migrations incrementally. RunMigrations is meant to be called from within an UpgradeHandler.

ref: #8345


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

simapp/app.go Outdated Show resolved Hide resolved
types/module/module.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #8485 (5add13a) into master (816306b) will decrease coverage by 0.09%.
The diff coverage is 21.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8485      +/-   ##
==========================================
- Coverage   61.50%   61.40%   -0.10%     
==========================================
  Files         651      656       +5     
  Lines       37253    37336      +83     
==========================================
+ Hits        22911    22927      +16     
- Misses      11940    12007      +67     
  Partials     2402     2402              
Impacted Files Coverage Δ
testutil/context.go 0.00% <0.00%> (ø)
types/module/module.go 82.71% <0.00%> (-9.07%) ⬇️
x/auth/module.go 52.77% <0.00%> (-1.51%) ⬇️
x/auth/vesting/module.go 52.17% <0.00%> (-2.38%) ⬇️
x/authz/module.go 48.83% <0.00%> (-1.17%) ⬇️
x/bank/keeper/migrations.go 0.00% <0.00%> (ø)
x/bank/legacy/v040/keys.go 0.00% <0.00%> (ø)
x/bank/module.go 51.16% <0.00%> (-3.84%) ⬇️
x/crisis/module.go 32.43% <0.00%> (-0.91%) ⬇️
x/distribution/module.go 54.76% <0.00%> (-1.34%) ⬇️
... and 19 more

types/module/configurator.go Outdated Show resolved Hide resolved
types/module/configurator.go Outdated Show resolved Hide resolved
types/module/configurator.go Outdated Show resolved Hide resolved
@amaury1093
Copy link
Contributor Author

amaury1093 commented Feb 3, 2021

I'm marking this as R4R. Only x/bank is migrated here, serving as PoC to the ConsensusVersion/RunMigrations change. The other modules introduce a big line diff (+2k), so they are done in #8504.

@amaury1093 amaury1093 marked this pull request as ready for review February 3, 2021 14:26
@amaury1093
Copy link
Contributor Author

R4R again.

Let's create an ADR based on this work. I created a task for it: #8532

I'm happy to take care of that, but I'm expecting 2weeks+ before it gets merged. Given that we want to get 0.42 out of the door quite fast, are we okay to merge this PR first?

@aaronc
Copy link
Member

aaronc commented Feb 8, 2021

R4R again.

Let's create an ADR based on this work. I created a task for it: #8532

I'm happy to take care of that, but I'm expecting 2weeks+ before it gets merged. Given that we want to get 0.42 out of the door quite fast, are we okay to merge this PR first?

I am yes.

@robert-zaremba
Copy link
Collaborator

R4R again.

Let's create an ADR based on this work. I created a task for it: #8532

I'm happy to take care of that, but I'm expecting 2weeks+ before it gets merged. Given that we want to get 0.42 out of the door quite fast, are we okay to merge this PR first?

👍 sure, it's just for having it on track.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre approving: let's get the #8485 (comment) (migrations starting from 1) done.

@amaury1093
Copy link
Contributor Author

let's get the #8485 (comment) (migrations starting from 1) done.

It's actually done already!

@robert-zaremba
Copy link
Collaborator

@alessio , @marbar3778 - We were discussing internally (Regen) about this issue.
We decided to not touch ed25519 in the scope of this task, and propose to leave SDK ed25519 only for handling Tendermint validators. I will open a discussion to talk more about this issue.

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep this API clean. Let's discuss if any of this approach isn't clear.

@@ -328,6 +341,30 @@ func (m *Manager) ExportGenesis(ctx sdk.Context, cdc codec.JSONMarshaler) map[st
return genesisData
}

// MigrationHandler is the migration function that each module registers.
type MigrationHandler func(store sdk.Context, storeKey sdk.StoreKey, cdc codec.Marshaler) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type MigrationHandler func(store sdk.Context, storeKey sdk.StoreKey, cdc codec.Marshaler) error
type MigrationHandler func(store sdk.Context) error

All this stuff is not needed. Let's keep the AP simple

// RegisterMigration registers an in-place store migration for a module. The
// handler is a migration script to perform in-place migrations from version
// `forVersion` to version `forVersion+1`.
RegisterMigration(moduleName string, forVersion uint64, handler MigrationHandler) error
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
Marshaler() codec.Marshaler
}

x/bank/module.go Outdated
@@ -100,6 +101,7 @@ type AppModule struct {
func (am AppModule) RegisterServices(cfg module.Configurator) {
types.RegisterMsgServer(cfg.MsgServer(), keeper.NewMsgServerImpl(am.keeper))
types.RegisterQueryServer(cfg.QueryServer(), am.keeper)
cfg.RegisterMigration(types.ModuleName, 0, v042.MigrateStore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store key and codec are in the keeper already so they can be passed from the keeper to the migration handler closure not from the module manager into the handler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even if the keeper didn't have this stuff already we can add it to Configurator so that all services have it rather than separately to each migration handler. Does that make sense? I think it will be much cleaner this way. Otherwise every time we need some new bit of configuration stuff then we keep adding more params to migration handler. Instead we can keep the migration handler params minimal and pass around other static configuration higher up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store key and codec are in the keeper already so they can be passed from the keeper to the migration handler closure not from the module manager into the handler.

Re store key: it is in the keeper yes, but not accessible by the AppModule at this line (cannot do am.keeper.storeKey).

Happy to discuss it today (or feel free to push to this PR if it's faster).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so I think the right way is to make the migration a keeper method.

Basically:

cfg.RegisterMigration(types.ModuleName, 0, func(ctx sdk.Context) error {
  return am.keeper.Migrate0(ctx)
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, yeah that works, but it also pollutes the keeper. In bank, the Keeper is an interface, so we'd need to put it as a public method there, and it'd also be exposed to other modules.

I also like the fact that migration scripts are in each module's legacy folder.

I prefer the current approach, but if you think the keeper's way is cleaner, I'm okay to change it. (cc @robert-zaremba, maybe you have an opinion, since you reviewed this PR?)

Copy link
Member

@aaronc aaronc Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ADR 033, we include both StoreKey and Marshaler on Configurator and we could explore that step. An alternative to polluting the keeper is just to put the storeKey and codec in AppModule when it gets created. Or we can just make a MigrationKeeper interface and cast Keeper to that and we still keep the scripts in legacy but import them from the keeper... But I really am pretty strongly opposed to making these parameters on the migration handler, it's just not the right place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can just make a MigrationKeeper interface and cast Keeper to that and we still keep the scripts in legacy but import them from the keeper

I went with this in 41e0339. I feel strongly about having those scripts in legacy, because I am copy/pasting legacy code into those folders.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree they should be in legacy.

What do you think about having StoreKey and Marshaler on Configurator?

One big thing I'm trying to pay attention to with APIs is not trying to pass too many things with function params. What ends up happening is that people say, oh we need just one more parameter and then the function signature changes. It's much less breaking to add fields to structs and methods to interfaces.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is worth keeping in mind: https://blog.golang.org/module-compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about having StoreKey and Marshaler on Configurator?

Having Marshaler on Configurator is useless imo (for migrations), because most (all?) AppModuleBasics already have a cdc.

Would StoreKey on Configurator look like func StoreKey(moduleName string) sdk.StoreKey? How do you avoid moduleA doing in its AppModule: b:=configurator.StoreKey("moduleB")?

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. Left a bunch of comments mostly about improving docs.

simapp/app.go Outdated Show resolved Hide resolved
types/module/module.go Outdated Show resolved Hide resolved
types/module/module.go Outdated Show resolved Hide resolved
types/module/configurator.go Outdated Show resolved Hide resolved
types/module/configurator.go Show resolved Hide resolved
x/auth/module.go Outdated Show resolved Hide resolved
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 🎉

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 10, 2021
@mergify mergify bot merged commit dfc8dd8 into master Feb 10, 2021
@mergify mergify bot deleted the am-8345-migration branch February 10, 2021 17:49
// store. The key must not contain the perfix BalancesPrefix as the prefix store
// iterator discards the actual prefix.
func AddressFromBalancesStore(key []byte) sdk.AccAddress {
addr := key[:v040auth.AddrLen]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 checks are unnecessary and the first one is bound to panic if I pass in a key less than v040auth.AddrLen. Please remove this one, or move if after ensure that there are at least v040auth.AddrLen values.

Copy link
Contributor Author

@amaury1093 amaury1093 Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this code (in the legacy/ folder) is copy-pasted from a previous commit of the SDK. To avoid breaking the migration scripts inadvertently (they are hard to debug), I prefer to keep them as is.

Accessing key slices before checking key length also makes me feel uncomfortable, I created #8451 for the current code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @amaurymartiny!

@ethanfrey ethanfrey mentioned this pull request Mar 16, 2021
4 tasks
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* 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 <robert@zaremba.ch>

* Update simapp/app_test.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* Adderss reviews

* Fix tests

* Update testutil/context.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* 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

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Aaron Craelius <aaron@regen.network>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Store C:x/bank
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants