From a23b5fa798719299626578fcf92807ff5d5d8b5e Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 17 Feb 2021 13:42:01 +0100 Subject: [PATCH 01/22] Initial draft --- .../adr-041-in-place-store-migrations.md | 57 +++++++++++++++++++ .../architecture/readme.md~origin_master-docs | 5 -- 2 files changed, 57 insertions(+), 5 deletions(-) create mode 100644 docs/architecture/adr-041-in-place-store-migrations.md delete mode 100644 docs/architecture/readme.md~origin_master-docs diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md new file mode 100644 index 000000000000..c4333884ae76 --- /dev/null +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -0,0 +1,57 @@ +# ADR 041: In-Place Store Migrations + +## Changelog + +- 17.02.2021: Initial Draft + +## Status + +Proposed + +## Abstract + +> "If you can't explain it simply, you don't understand it well enough." Provide a simplified and layman-accessible explanation of the ADR. +> A short (~200 word) description of the issue being addressed. + +## Context + +> This section describes the forces at play, including technological, political, social, and project local. These forces are probably in tension, and should be called out as such. The language in this section is value-neutral. It is simply describing facts. It should clearly explain the problem and motivation that the proposal aims to resolve. +> {context body} + +## Decision + +> This section describes our response to these forces. It is stated in full sentences, with active voice. "We will ..." +> {decision body} + +## Consequences + +> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. + +### Backwards Compatibility + +> All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. + +### Positive + +{positive consequences} + +### Negative + +{negative consequences} + +### Neutral + +{neutral consequences} + +## Further Discussions + +While an ADR is in the DRAFT or PROPOSED stage, this section should contain a summary of issues to be solved in future iterations (usually referencing comments from a pull-request discussion). +Later, this section can optionally list ideas or improvements the author or reviewers found during the analysis of this ADR. + +## Test Cases [optional] + +Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable. + +## References + +- {reference link} diff --git a/docs/architecture/readme.md~origin_master-docs b/docs/architecture/readme.md~origin_master-docs deleted file mode 100644 index f0c07cb68565..000000000000 --- a/docs/architecture/readme.md~origin_master-docs +++ /dev/null @@ -1,5 +0,0 @@ ---- -order: false -parent: - order: false ---- From 6f69bc589d9108e6a7d0bbe988f791369895ac79 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Fri, 19 Feb 2021 15:52:14 +0100 Subject: [PATCH 02/22] Draft --- .../adr-041-in-place-store-migrations.md | 70 ++++++++++++++++--- 1 file changed, 62 insertions(+), 8 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index c4333884ae76..136fea0705f8 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -10,18 +10,74 @@ Proposed ## Abstract -> "If you can't explain it simply, you don't understand it well enough." Provide a simplified and layman-accessible explanation of the ADR. -> A short (~200 word) description of the issue being addressed. +This ADR introduces a mechanism to perform automatic (no manual process involved) in-place store migrations during chain upgrades. ## Context -> This section describes the forces at play, including technological, political, social, and project local. These forces are probably in tension, and should be called out as such. The language in this section is value-neutral. It is simply describing facts. It should clearly explain the problem and motivation that the proposal aims to resolve. -> {context body} +When a chain upgrade introduces state-breaking changes inside modules, the current procedure consists of exporting the whole state into a JSON file (via the `simd export` command), running migration scripts on the JSON file (`simd migrate` command), clearing the stores (`simd unsafe-reset-all` command), and starting a new chain with the migrated JSON file as new genesis (optionally with a custom initial block height). An example of such a procedure can be seen [in the Cosmos Hub 3->4 migration guide](https://github.com/cosmos/gaia/blob/v4.0.3/docs/migration/cosmoshub-3.md#upgrade-procedure). + +While [cosmovisor](https://github.com/cosmos/cosmos-sdk/tree/v0.41.1/cosmovisor) aims to alleviate the difficulty of handling upgrades, this procedure is still cumbersome for multiple reasons: + +- The procedure takes time. It can take hours to run the `export` command, plus some additional hours to run `InitChain` on the fresh chain using the migrated JSON. +- The exported JSON file can be heavy (~100MB-1GB), making it difficult to view, edit and transfer. ## Decision -> This section describes our response to these forces. It is stated in full sentences, with active voice. "We will ..." -> {decision body} +We propose a migration procedure based on modifying the KV store in-place. This procedure does not manipulate intermediary JSON files. + +### Module `ConsensusVersion` + +We introduce a new method on the `AppModule` interface: + +```go +type AppModule interface { + // --snip-- + ConsensusVersion() uint64 +} +``` + +This methods returns an `uint64` which serves as state-breaking versioning of the module. It should be incremented on each consensus-breaking change introduced by the module. To avoid potential errors with default values, the initial version of a module MUST be set to 1. In the SDK, version 1 corresponds to the modules in the v0.41 series. + +### Module-Specific Migration Scripts + +For each consensus-breaking change introduced by the module, a migration script from ConsensusVersion `k` to version `k+1` sMUST be registered in the `Configurator` using its `RegisterMigration` method. + +```go +configurator.RegisterMigration("bank", 1, func(ctx sdk.Context) error { + // Perform x/banks's in-place store migrations from ConsensusVersion 1 to 2. +}) +``` + +For example, if the current ConsensusVersion of a module is `N` , then `N-1` migration scripts MUST be registered in the configurator. + +In the SDK, the migration scripts are handled by each module's keeper, because the keeper holds the `sdk.StoreKey` used to perform in-place store migrations. A `MigrationKeeper` interface is implemented by each keeper: + +```go +// MigrationKeeper is an interface that the keeper implements for handling +// in-place store migrations. +type MigrationKeeper interface { + // Migrate1 migrates the store from version 1 to 2. + Migrate1(ctx sdk.Context) error +} +``` + +Since migration scripts manipulate legacy code, they SHOULD live inside the `legacy/` folder of each module, and be called by the keeper's implementation of `MigrationKeeper`. + +```go +func (keeper BankKeeper) Migrate1(ctx sdk.Context) error { + return v042bank.MigrateStore(ctx, keeper.storeKey) // v042bank is package `x/bank/legacy/v042`. +} +``` + +Each module's migration scripts are specific to the module's store evolutions, an example of x/bank store key migration due to the introduction of ADR-028 length-prefixed addresses can be seen [here](https://github.com/cosmos/cosmos-sdk/blob/ef8dabcf0f2ecaf26db1c6c6d5922e9399458bb3/x/bank/legacy/v042/store.go#L15). + +### Tracking Module Versions in `x/upgrade` + +We introduce a new prefix store in `x/upgrade`'s store. This store will track each module's current version. + +### Running Migrations + +Once all the migration handlers are registered inside the configurator (this happens at startup), running migrations can happen any time by calling the `RunMigrations` method on `module.Manager`. ## Consequences @@ -29,8 +85,6 @@ Proposed ### Backwards Compatibility -> All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. - ### Positive {positive consequences} From 9f2444578445945a755bfb89a62c8d3ad31e1c3a Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Fri, 19 Feb 2021 17:56:13 +0100 Subject: [PATCH 03/22] Add x/upgrade stuff --- .../adr-041-in-place-store-migrations.md | 108 ++++++++++++++---- 1 file changed, 84 insertions(+), 24 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 136fea0705f8..46959d695e71 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -10,13 +10,13 @@ Proposed ## Abstract -This ADR introduces a mechanism to perform automatic (no manual process involved) in-place store migrations during chain upgrades. +This ADR introduces a mechanism to perform in-place store migrations during chain upgrades. ## Context When a chain upgrade introduces state-breaking changes inside modules, the current procedure consists of exporting the whole state into a JSON file (via the `simd export` command), running migration scripts on the JSON file (`simd migrate` command), clearing the stores (`simd unsafe-reset-all` command), and starting a new chain with the migrated JSON file as new genesis (optionally with a custom initial block height). An example of such a procedure can be seen [in the Cosmos Hub 3->4 migration guide](https://github.com/cosmos/gaia/blob/v4.0.3/docs/migration/cosmoshub-3.md#upgrade-procedure). -While [cosmovisor](https://github.com/cosmos/cosmos-sdk/tree/v0.41.1/cosmovisor) aims to alleviate the difficulty of handling upgrades, this procedure is still cumbersome for multiple reasons: +While [Cosmovisor](https://github.com/cosmos/cosmos-sdk/tree/v0.41.1/cosmovisor) aims to alleviate the difficulty of handling upgrades, this procedure is still cumbersome for multiple reasons: - The procedure takes time. It can take hours to run the `export` command, plus some additional hours to run `InitChain` on the fresh chain using the migrated JSON. - The exported JSON file can be heavy (~100MB-1GB), making it difficult to view, edit and transfer. @@ -40,12 +40,15 @@ This methods returns an `uint64` which serves as state-breaking versioning of th ### Module-Specific Migration Scripts -For each consensus-breaking change introduced by the module, a migration script from ConsensusVersion `k` to version `k+1` sMUST be registered in the `Configurator` using its `RegisterMigration` method. +For each consensus-breaking change introduced by the module, a migration script from ConsensusVersion `k` to version `k+1` MUST be registered in the `Configurator` using its `RegisterMigration` method. All modules receive a reference to the configurator in the `RegisterServices` method on `AppModule`, and this is where the migration scripts should be registered. ```go -configurator.RegisterMigration("bank", 1, func(ctx sdk.Context) error { +func (am AppModule) RegisterServices(cfg module.Configurator) { + // --snip-- + cfg.RegisterMigration(types.ModuleName, 1, func(ctx sdk.Context) error { // Perform x/banks's in-place store migrations from ConsensusVersion 1 to 2. -}) + }) +} ``` For example, if the current ConsensusVersion of a module is `N` , then `N-1` migration scripts MUST be registered in the configurator. @@ -56,16 +59,18 @@ In the SDK, the migration scripts are handled by each module's keeper, because t // MigrationKeeper is an interface that the keeper implements for handling // in-place store migrations. type MigrationKeeper interface { - // Migrate1 migrates the store from version 1 to 2. - Migrate1(ctx sdk.Context) error + // Migrate1 migrates the store from version 1 to 2. + Migrate1(ctx sdk.Context) error + + // ...Add more MigrateN scripts here. } ``` -Since migration scripts manipulate legacy code, they SHOULD live inside the `legacy/` folder of each module, and be called by the keeper's implementation of `MigrationKeeper`. +Since migration scripts manipulate legacy code, they should live inside the `legacy/` folder of each module, and be called by the keeper's implementation of `MigrationKeeper`. ```go func (keeper BankKeeper) Migrate1(ctx sdk.Context) error { - return v042bank.MigrateStore(ctx, keeper.storeKey) // v042bank is package `x/bank/legacy/v042`. + return v042bank.MigrateStore(ctx, keeper.storeKey) // v042bank is package `x/bank/legacy/v042`. } ``` @@ -73,39 +78,94 @@ Each module's migration scripts are specific to the module's store evolutions, a ### Tracking Module Versions in `x/upgrade` -We introduce a new prefix store in `x/upgrade`'s store. This store will track each module's current version. +We introduce a new prefix store in `x/upgrade`'s store. This store will track each module's current version, it can be thought as a map of module name to module ConsensusVersion. When performing a chain upgrade from an old binary to a new binary, the new binary will read the old ConsensusVersions of the store, compare them to the new ConsensusVersions hardcoded in the modules' `ConsensusVersion()`, and perform registered in-place store migrations for all modules that have a ConsensusVersion mismatch. + +The key prefix for all modules' current ConsensusVersion is `0x1`. The key/value format is: + +``` +0x1 ++ {module_name_bytes} => LittleEndian(module_consensus_version) +``` + +We add a new parameter `ModuleManager` `x/upgrade`'s `NewKeeper` constructor, where ModuleManager is: + +```go +// Map of module name => ConsensusVersion. +type MigrationMap map[string]uint64 + +type ModuleManager interface { + GetConsensusVersions() MigrationMap +} +``` + +When `InitChain` is run, the initial `ModuleManager.GetConsensusVersions()` value will be recorded to state. The UpgradeHandler signature needs to be updated to take a `MigrationMap`, as well as return an error: + +```diff +- type UpgradeHandler func(ctx sdk.Context, plan Plan) ++ type UpgradeHandler func(ctx sdk.Context, plan Plan, migrationMap MigrationMap) error +``` + +And applying an upgrade will fetch the `MigrationMap` from the `x/upgrade` store and pass it into the handler. + +```go +func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { + // --snip-- + handler(ctx, plan, k.GetConsensusVersions()) // k.GetConsensusVersions() fetches the MigrationMap stored in state. +} +``` ### Running Migrations -Once all the migration handlers are registered inside the configurator (this happens at startup), running migrations can happen any time by calling the `RunMigrations` method on `module.Manager`. +Once all the migration handlers are registered inside the configurator (which happens at startup), running migrations can happen by calling the `RunMigrations` method on `module.Manager`. This function will loop through all modules, and for each module: -## Consequences +- Fetch the old ConsensusVersion of the module from its `migrationMap` argument (let's call it `N`). +- Fetch the new ConsensusVersion of the module from the `ConsensusVersion()` method on `AppModule` (call it `M`). +- If `M>N`, run all registered migrations for the module sequentially from versions `N` to `N+1`, `N+1` to `N+2`, etc. until `M`. -> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. +If a required migration is missing (most likely because it has not been registered in the `Configurator`), then the `RunMigration` function will error. + +In practice, the `RunMigrations` method should be called from inside an `UpgradeHandler`. + +```go +app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, migrationMap MigrationMap) { + err := app.mm.RunMigrations(ctx, migrationMap) + if err != nil { + panic(err) + } +}) +``` + +Assuming a chain upgrades at block `N`, the procedure should run as follows: + +- the old binary will halt in `BeginBlock` at block `N`. In the store, the ConsensusVersions of the old binary's modules are stored. +- the new binary will start at block `N`. The UpgradeHandler is set in the new binary, so will run at `BeginBlock` of the new binary. As per the UpgradeHandler, it will run the `RunMigrations` above, migrating all module stores in-place, before the modules' own `BeginBlock`s. + +## Consequences ### Backwards Compatibility +This ADR introduces a new method `ConsensusVersion()` on `AppModule`, which all modules need to implement. It also alters the UpgradeHandler function signature. As such, it is not backwards-compatible. + +While modules MUST register their migration scripts when bumping ConsensusVersions, running those scripts using an upgrade handler is optional. An application may perfectly well decide to not call the `RunMigrations` inside its upgrade handler, and continue using the legacy JSON migration path. + ### Positive -{positive consequences} +- Perform chain upgrades without manipulating JSON files. +- While no benchmark has been made yet, it is probable that in-place store migrations will take less time than JSON migrations. The main reason supporting this claim is that both the `simd export` command on the old binary and the `InitChain` function on the new binary will be skipped. ### Negative -{negative consequences} +- Module developers MUST correctly track consensus-breaking changes in their modules. If a consensus-breaking change is introduced in a module without its corresponding `ConsensusVersion()` bump, then the `RunMigrations` function won't detect the migration, and the chain upgrade might be unsuccessful. Documentation should clearly reflect this. ### Neutral -{neutral consequences} +- Currently, Cosmovisor only handles JSON migrations. Its code should be updated to support in-place store migrations too. +- The SDK will continue to support JSON migrations via the existing `simd export` and `simd migrate` commands. +- The current ADR does not allow creating, renaming or deleting stores, only modifying existing store keys and values. The SDK already has the `StoreLoader` for those operations. ## Further Discussions -While an ADR is in the DRAFT or PROPOSED stage, this section should contain a summary of issues to be solved in future iterations (usually referencing comments from a pull-request discussion). -Later, this section can optionally list ideas or improvements the author or reviewers found during the analysis of this ADR. - -## Test Cases [optional] - -Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable. - ## References -- {reference link} +- Initial discussion: https://github.com/cosmos/cosmos-sdk/discussions/8429 +- Implementation of `ConsensusVersion` and `RunMigrations`: https://github.com/cosmos/cosmos-sdk/pull/8485 +- Issue discussing `x/upgrade` design: https://github.com/cosmos/cosmos-sdk/issues/8514 From f153843bc140984c73ae543bc3f39db624103037 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Fri, 19 Feb 2021 18:12:45 +0100 Subject: [PATCH 04/22] Tweaks --- .../adr-041-in-place-store-migrations.md | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 46959d695e71..f1ad95149a15 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -19,7 +19,7 @@ When a chain upgrade introduces state-breaking changes inside modules, the curre While [Cosmovisor](https://github.com/cosmos/cosmos-sdk/tree/v0.41.1/cosmovisor) aims to alleviate the difficulty of handling upgrades, this procedure is still cumbersome for multiple reasons: - The procedure takes time. It can take hours to run the `export` command, plus some additional hours to run `InitChain` on the fresh chain using the migrated JSON. -- The exported JSON file can be heavy (~100MB-1GB), making it difficult to view, edit and transfer. +- The exported JSON file can be heavy (~100MB-1GB), making it difficult to view, edit and transfer, which in turn introduces additional work to solve these problems (such as [streaming genesis](https://github.com/cosmos/cosmos-sdk/issues/6936)). ## Decision @@ -36,18 +36,22 @@ type AppModule interface { } ``` -This methods returns an `uint64` which serves as state-breaking versioning of the module. It should be incremented on each consensus-breaking change introduced by the module. To avoid potential errors with default values, the initial version of a module MUST be set to 1. In the SDK, version 1 corresponds to the modules in the v0.41 series. +This methods returns an `uint64` which serves as state-breaking versioning of the module. It MUST be incremented on each consensus-breaking change introduced by the module. To avoid potential errors with default values, the initial version of a module MUST be set to 1. In the SDK, version 1 corresponds to the modules in the v0.41 series. ### Module-Specific Migration Scripts -For each consensus-breaking change introduced by the module, a migration script from ConsensusVersion `k` to version `k+1` MUST be registered in the `Configurator` using its `RegisterMigration` method. All modules receive a reference to the configurator in the `RegisterServices` method on `AppModule`, and this is where the migration scripts should be registered. +For each consensus-breaking change introduced by the module, a migration script from ConsensusVersion `N` to version `N+1` MUST be registered in the `Configurator` using its newly-added `RegisterMigration` method. All modules receive a reference to the configurator in their `RegisterServices` method on `AppModule`, and this is where the migration scripts should be registered. ```go func (am AppModule) RegisterServices(cfg module.Configurator) { // --snip-- cfg.RegisterMigration(types.ModuleName, 1, func(ctx sdk.Context) error { - // Perform x/banks's in-place store migrations from ConsensusVersion 1 to 2. + // Perform in-place store migrations from ConsensusVersion 1 to 2. }) + cfg.RegisterMigration(types.ModuleName, 2, func(ctx sdk.Context) error { + // Perform in-place store migrations from ConsensusVersion 2 to 3. + }) + // etc. } ``` @@ -74,27 +78,25 @@ func (keeper BankKeeper) Migrate1(ctx sdk.Context) error { } ``` -Each module's migration scripts are specific to the module's store evolutions, an example of x/bank store key migration due to the introduction of ADR-028 length-prefixed addresses can be seen [here](https://github.com/cosmos/cosmos-sdk/blob/ef8dabcf0f2ecaf26db1c6c6d5922e9399458bb3/x/bank/legacy/v042/store.go#L15). +Each module's migration scripts are specific to the module's store evolutions, and are not described in this ADR. An example of x/bank store key migrations following the introduction of ADR-028 length-prefixed addresses can be seen [here](https://github.com/cosmos/cosmos-sdk/blob/ef8dabcf0f2ecaf26db1c6c6d5922e9399458bb3/x/bank/legacy/v042/store.go#L15). ### Tracking Module Versions in `x/upgrade` -We introduce a new prefix store in `x/upgrade`'s store. This store will track each module's current version, it can be thought as a map of module name to module ConsensusVersion. When performing a chain upgrade from an old binary to a new binary, the new binary will read the old ConsensusVersions of the store, compare them to the new ConsensusVersions hardcoded in the modules' `ConsensusVersion()`, and perform registered in-place store migrations for all modules that have a ConsensusVersion mismatch. - -The key prefix for all modules' current ConsensusVersion is `0x1`. The key/value format is: +We introduce a new prefix store in `x/upgrade`'s store. This store will track each module's current version, it can be modelized as a `map[string]uint64` of module name to module ConsensusVersion, and will be used when running the migrations (see next section for details). The key prefix used is `0x1`, and the key/value format is: ``` -0x1 ++ {module_name_bytes} => LittleEndian(module_consensus_version) +0x1 | {module_name_bytes} => LittleEndian(module_consensus_version) ``` We add a new parameter `ModuleManager` `x/upgrade`'s `NewKeeper` constructor, where ModuleManager is: ```go -// Map of module name => ConsensusVersion. -type MigrationMap map[string]uint64 - type ModuleManager interface { GetConsensusVersions() MigrationMap } + +// Map of module name => ConsensusVersion. +type MigrationMap map[string]uint64 ``` When `InitChain` is run, the initial `ModuleManager.GetConsensusVersions()` value will be recorded to state. The UpgradeHandler signature needs to be updated to take a `MigrationMap`, as well as return an error: @@ -104,12 +106,13 @@ When `InitChain` is run, the initial `ModuleManager.GetConsensusVersions()` valu + type UpgradeHandler func(ctx sdk.Context, plan Plan, migrationMap MigrationMap) error ``` -And applying an upgrade will fetch the `MigrationMap` from the `x/upgrade` store and pass it into the handler. +Applying an upgrade will fetch the `MigrationMap` from the `x/upgrade` store and pass it into the handler. -```go +```diff func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { // --snip-- - handler(ctx, plan, k.GetConsensusVersions()) // k.GetConsensusVersions() fetches the MigrationMap stored in state. +- handler(ctx, plan) ++ handler(ctx, plan, k.GetConsensusVersions()) // k.GetConsensusVersions() fetches the MigrationMap stored in state. } ``` @@ -117,11 +120,11 @@ func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { Once all the migration handlers are registered inside the configurator (which happens at startup), running migrations can happen by calling the `RunMigrations` method on `module.Manager`. This function will loop through all modules, and for each module: -- Fetch the old ConsensusVersion of the module from its `migrationMap` argument (let's call it `N`). -- Fetch the new ConsensusVersion of the module from the `ConsensusVersion()` method on `AppModule` (call it `M`). -- If `M>N`, run all registered migrations for the module sequentially from versions `N` to `N+1`, `N+1` to `N+2`, etc. until `M`. +- Fetch the old ConsensusVersion of the module from its `migrationMap` argument (let's call it `M`). +- Fetch the new ConsensusVersion of the module from the `ConsensusVersion()` method on `AppModule` (call it `N`). +- If `N>M`, run all registered migrations for the module sequentially `M -> M+1 -> M+2...` until `N`. -If a required migration is missing (most likely because it has not been registered in the `Configurator`), then the `RunMigration` function will error. +If a required migration is missing (e.g. if it has not been registered in the `Configurator`), then the `RunMigrations` function will error. In practice, the `RunMigrations` method should be called from inside an `UpgradeHandler`. @@ -136,8 +139,8 @@ app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgrad Assuming a chain upgrades at block `N`, the procedure should run as follows: -- the old binary will halt in `BeginBlock` at block `N`. In the store, the ConsensusVersions of the old binary's modules are stored. -- the new binary will start at block `N`. The UpgradeHandler is set in the new binary, so will run at `BeginBlock` of the new binary. As per the UpgradeHandler, it will run the `RunMigrations` above, migrating all module stores in-place, before the modules' own `BeginBlock`s. +- the old binary will halt in `BeginBlock` at block `N`. In its store, the ConsensusVersions of the old binary's modules are stored. +- the new binary will start at block `N`. The UpgradeHandler is set in the new binary, so will run at `BeginBlock` of the new binary. Inside `x/upgrade`'s `ApplyUpgrade`, the `MigrationMap` will be retrieved from the (old binary's) store, and passed into the `RunMigrations` functon, migrating all module stores in-place before the modules' own `BeginBlock`s. ## Consequences From 0eea75b69d7d2df1b01849a7371cc38bf0b67ffb Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:43:14 +0100 Subject: [PATCH 05/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Anil Kumar Kammari --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index f1ad95149a15..28def1382a9b 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -137,7 +137,7 @@ app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgrad }) ``` -Assuming a chain upgrades at block `N`, the procedure should run as follows: +Assuming a chain upgrades at block `n`, the procedure should run as follows: - the old binary will halt in `BeginBlock` at block `N`. In its store, the ConsensusVersions of the old binary's modules are stored. - the new binary will start at block `N`. The UpgradeHandler is set in the new binary, so will run at `BeginBlock` of the new binary. Inside `x/upgrade`'s `ApplyUpgrade`, the `MigrationMap` will be retrieved from the (old binary's) store, and passed into the `RunMigrations` functon, migrating all module stores in-place before the modules' own `BeginBlock`s. From 151149b5d209193ce6963490d82e730dc8a9c1e8 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:43:39 +0100 Subject: [PATCH 06/22] Reviews --- docs/architecture/adr-041-in-place-store-migrations.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index f1ad95149a15..1fab965666c5 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -16,7 +16,7 @@ This ADR introduces a mechanism to perform in-place store migrations during chai When a chain upgrade introduces state-breaking changes inside modules, the current procedure consists of exporting the whole state into a JSON file (via the `simd export` command), running migration scripts on the JSON file (`simd migrate` command), clearing the stores (`simd unsafe-reset-all` command), and starting a new chain with the migrated JSON file as new genesis (optionally with a custom initial block height). An example of such a procedure can be seen [in the Cosmos Hub 3->4 migration guide](https://github.com/cosmos/gaia/blob/v4.0.3/docs/migration/cosmoshub-3.md#upgrade-procedure). -While [Cosmovisor](https://github.com/cosmos/cosmos-sdk/tree/v0.41.1/cosmovisor) aims to alleviate the difficulty of handling upgrades, this procedure is still cumbersome for multiple reasons: +This procedure is cumbersome for multiple reasons: - The procedure takes time. It can take hours to run the `export` command, plus some additional hours to run `InitChain` on the fresh chain using the migrated JSON. - The exported JSON file can be heavy (~100MB-1GB), making it difficult to view, edit and transfer, which in turn introduces additional work to solve these problems (such as [streaming genesis](https://github.com/cosmos/cosmos-sdk/issues/6936)). @@ -85,9 +85,10 @@ Each module's migration scripts are specific to the module's store evolutions, a We introduce a new prefix store in `x/upgrade`'s store. This store will track each module's current version, it can be modelized as a `map[string]uint64` of module name to module ConsensusVersion, and will be used when running the migrations (see next section for details). The key prefix used is `0x1`, and the key/value format is: ``` -0x1 | {module_name_bytes} => LittleEndian(module_consensus_version) +0x2 | {bytes(module_name)} => LittleEndian(module_consensus_version) ``` +s We add a new parameter `ModuleManager` `x/upgrade`'s `NewKeeper` constructor, where ModuleManager is: ```go From 6dc29f96ea69e72d7d26f0240a3b3e43b3b0b8ca Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:49:21 +0100 Subject: [PATCH 07/22] Use migrator --- .../adr-041-in-place-store-migrations.md | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index e414f0c1fcea..2653a599de73 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -57,24 +57,21 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { For example, if the current ConsensusVersion of a module is `N` , then `N-1` migration scripts MUST be registered in the configurator. -In the SDK, the migration scripts are handled by each module's keeper, because the keeper holds the `sdk.StoreKey` used to perform in-place store migrations. A `MigrationKeeper` interface is implemented by each keeper: +In the SDK, the migration scripts are handled by each module's keeper, because the keeper holds the `sdk.StoreKey` used to perform in-place store migrations. To not overload the keeper, a `Migrator` wrapper is used by each module to handle the migration scripts: ```go -// MigrationKeeper is an interface that the keeper implements for handling -// in-place store migrations. -type MigrationKeeper interface { - // Migrate1 migrates the store from version 1 to 2. - Migrate1(ctx sdk.Context) error - - // ...Add more MigrateN scripts here. +// Migrator is a struct for handling in-place store migrations. +type Migrator struct { + keeper BaseKeeper } ``` -Since migration scripts manipulate legacy code, they should live inside the `legacy/` folder of each module, and be called by the keeper's implementation of `MigrationKeeper`. +Since migration scripts manipulate legacy code, they should live inside the `legacy/` folder of each module, and be called by the Migrator's methods. We propose the format `Migrate{M}to{N}` for method names. ```go -func (keeper BankKeeper) Migrate1(ctx sdk.Context) error { - return v042bank.MigrateStore(ctx, keeper.storeKey) // v042bank is package `x/bank/legacy/v042`. +// Migrate1to2 migrates from version 1 to 2. +func (m Migrator) Migrate1to2(ctx sdk.Context) error { + return v042bank.MigrateStore(ctx, m.keeper.storeKey) // v042bank is package `x/bank/legacy/v042`. } ``` @@ -89,10 +86,10 @@ We introduce a new prefix store in `x/upgrade`'s store. This store will track ea ``` s -We add a new parameter `ModuleManager` `x/upgrade`'s `NewKeeper` constructor, where ModuleManager is: +We add a new field `VersionManager` `x/upgrade`'s keeper, where `VersionManager` is: ```go -type ModuleManager interface { +type VersionManager interface { GetConsensusVersions() MigrationMap } From 94c0475179c6925b1fa34edc0d6cccd0ce850a5a Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:49:31 +0100 Subject: [PATCH 08/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 28def1382a9b..6719d1a64ee7 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -139,7 +139,7 @@ app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgrad Assuming a chain upgrades at block `n`, the procedure should run as follows: -- the old binary will halt in `BeginBlock` at block `N`. In its store, the ConsensusVersions of the old binary's modules are stored. +- the old binary will halt in `BeginBlock` when starting block `N`. In its store, the ConsensusVersions of the old binary's modules are stored. - the new binary will start at block `N`. The UpgradeHandler is set in the new binary, so will run at `BeginBlock` of the new binary. Inside `x/upgrade`'s `ApplyUpgrade`, the `MigrationMap` will be retrieved from the (old binary's) store, and passed into the `RunMigrations` functon, migrating all module stores in-place before the modules' own `BeginBlock`s. ## Consequences From 2f7fede2b203d6e6543eb46458b8bd19b2977c2c Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:49:41 +0100 Subject: [PATCH 09/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Aaron Craelius --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 6719d1a64ee7..92397fcec895 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -36,7 +36,7 @@ type AppModule interface { } ``` -This methods returns an `uint64` which serves as state-breaking versioning of the module. It MUST be incremented on each consensus-breaking change introduced by the module. To avoid potential errors with default values, the initial version of a module MUST be set to 1. In the SDK, version 1 corresponds to the modules in the v0.41 series. +This methods returns an `uint64` which serves as state-breaking version of the module. It MUST be incremented on each consensus-breaking change introduced by the module. To avoid potential errors with default values, the initial version of a module MUST be set to 1. In the SDK, version 1 corresponds to the modules in the v0.41 series. ### Module-Specific Migration Scripts From 8494883aa9fba0b76f3af747c7a0aa7fba2d9aeb Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:50:01 +0100 Subject: [PATCH 10/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 92397fcec895..ff8b83b34a00 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -10,7 +10,7 @@ Proposed ## Abstract -This ADR introduces a mechanism to perform in-place store migrations during chain upgrades. +This ADR introduces a mechanism to perform in-place state store migrations during chain software upgrades. ## Context From aaa991b5b6fbcad61c3ae8973a2a5c106ec877e6 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:50:10 +0100 Subject: [PATCH 11/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index ff8b83b34a00..aa876c80d28f 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -23,7 +23,7 @@ While [Cosmovisor](https://github.com/cosmos/cosmos-sdk/tree/v0.41.1/cosmovisor) ## Decision -We propose a migration procedure based on modifying the KV store in-place. This procedure does not manipulate intermediary JSON files. +We propose a migration procedure based on modifying the KV store in-place without involving the JSON export-process-import flow described above. ### Module `ConsensusVersion` From 3579a1025b60a5ed7575f19910a1d698fa323bd6 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:50:19 +0100 Subject: [PATCH 12/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index aa876c80d28f..5cb164c31f46 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -55,7 +55,7 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { } ``` -For example, if the current ConsensusVersion of a module is `N` , then `N-1` migration scripts MUST be registered in the configurator. +For example, if the new ConsensusVersion of a module is `N` , then `N-1` migration scripts MUST be registered in the configurator. In the SDK, the migration scripts are handled by each module's keeper, because the keeper holds the `sdk.StoreKey` used to perform in-place store migrations. A `MigrationKeeper` interface is implemented by each keeper: From 865713e72dac8806bf3af85a79b8c3405b34f047 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:50:41 +0100 Subject: [PATCH 13/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-041-in-place-store-migrations.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 5cb164c31f46..eb0685ac5959 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -95,8 +95,8 @@ type ModuleManager interface { GetConsensusVersions() MigrationMap } -// Map of module name => ConsensusVersion. -type MigrationMap map[string]uint64 +// Map of module name => new module Consensus Version. +type VersionMap map[string]uint64 ``` When `InitChain` is run, the initial `ModuleManager.GetConsensusVersions()` value will be recorded to state. The UpgradeHandler signature needs to be updated to take a `MigrationMap`, as well as return an error: From cb361b723a1e20adc8a5f423e4eedb8ede836150 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:50:51 +0100 Subject: [PATCH 14/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index eb0685ac5959..d2d062d9b74a 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -106,7 +106,7 @@ When `InitChain` is run, the initial `ModuleManager.GetConsensusVersions()` valu + type UpgradeHandler func(ctx sdk.Context, plan Plan, migrationMap MigrationMap) error ``` -Applying an upgrade will fetch the `MigrationMap` from the `x/upgrade` store and pass it into the handler. +To apply an upgrade, we query the `MigrationMap` from the `x/upgrade` store and pass it into the handler: ```diff func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { From 1f1d4ceec221c700b6fdd87b433b61ca71b94928 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 12:51:00 +0100 Subject: [PATCH 15/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index d2d062d9b74a..3abea10c77e8 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -120,7 +120,7 @@ func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { Once all the migration handlers are registered inside the configurator (which happens at startup), running migrations can happen by calling the `RunMigrations` method on `module.Manager`. This function will loop through all modules, and for each module: -- Fetch the old ConsensusVersion of the module from its `migrationMap` argument (let's call it `M`). +- Get the old ConsensusVersion of the module from its `migrationMap` argument (let's call it `M`). - Fetch the new ConsensusVersion of the module from the `ConsensusVersion()` method on `AppModule` (call it `N`). - If `N>M`, run all registered migrations for the module sequentially `M -> M+1 -> M+2...` until `N`. From 31357e6923fe60057904d80af668cef11b440552 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 13:01:55 +0100 Subject: [PATCH 16/22] More fixes --- .../adr-041-in-place-store-migrations.md | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 9377e25d3bdc..78d1a84ffeee 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -86,31 +86,31 @@ We introduce a new prefix store in `x/upgrade`'s store. This store will track ea ``` s -We add a new field `VersionManager` `x/upgrade`'s keeper, where `VersionManager` is: +We add a new private field `versionManager` of type `VersionManager` to `x/upgrade`'s keeper, where `VersionManager` is: ```go type VersionManager interface { - GetConsensusVersions() MigrationMap + GetConsensusVersions() VersionMap } // Map of module name => new module Consensus Version. type VersionMap map[string]uint64 ``` -When `InitChain` is run, the initial `ModuleManager.GetConsensusVersions()` value will be recorded to state. The UpgradeHandler signature needs to be updated to take a `MigrationMap`, as well as return an error: +The UpgradeHandler signature needs to be updated to take a `VersionMap`, as well as return an error: ```diff - type UpgradeHandler func(ctx sdk.Context, plan Plan) -+ type UpgradeHandler func(ctx sdk.Context, plan Plan, migrationMap MigrationMap) error ++ type UpgradeHandler func(ctx sdk.Context, plan Plan, versionMap VersionMap) error ``` -To apply an upgrade, we query the `MigrationMap` from the `x/upgrade` store and pass it into the handler: +To apply an upgrade, we query the `VersionMap` from the `x/upgrade` store and pass it into the handler. Right after the ```diff func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { // --snip-- - handler(ctx, plan) -+ handler(ctx, plan, k.GetConsensusVersions()) // k.GetConsensusVersions() fetches the MigrationMap stored in state. ++ err := handler(ctx, plan, k.GetConsensusVersions()) // k.GetConsensusVersions() fetches the VersionMap stored in state. } ``` @@ -118,7 +118,7 @@ func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { Once all the migration handlers are registered inside the configurator (which happens at startup), running migrations can happen by calling the `RunMigrations` method on `module.Manager`. This function will loop through all modules, and for each module: -- Get the old ConsensusVersion of the module from its `migrationMap` argument (let's call it `M`). +- Get the old ConsensusVersion of the module from its `VersionMap` argument (let's call it `M`). - Fetch the new ConsensusVersion of the module from the `ConsensusVersion()` method on `AppModule` (call it `N`). - If `N>M`, run all registered migrations for the module sequentially `M -> M+1 -> M+2...` until `N`. @@ -127,10 +127,10 @@ If a required migration is missing (e.g. if it has not been registered in the `C In practice, the `RunMigrations` method should be called from inside an `UpgradeHandler`. ```go -app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, migrationMap MigrationMap) { - err := app.mm.RunMigrations(ctx, migrationMap) +app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, versionMap VersionMap) error { + err := app.mm.RunMigrations(ctx, versionMap) if err != nil { - panic(err) + return err } }) ``` @@ -138,7 +138,7 @@ app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgrad Assuming a chain upgrades at block `n`, the procedure should run as follows: - the old binary will halt in `BeginBlock` when starting block `N`. In its store, the ConsensusVersions of the old binary's modules are stored. -- the new binary will start at block `N`. The UpgradeHandler is set in the new binary, so will run at `BeginBlock` of the new binary. Inside `x/upgrade`'s `ApplyUpgrade`, the `MigrationMap` will be retrieved from the (old binary's) store, and passed into the `RunMigrations` functon, migrating all module stores in-place before the modules' own `BeginBlock`s. +- the new binary will start at block `N`. The UpgradeHandler is set in the new binary, so will run at `BeginBlock` of the new binary. Inside `x/upgrade`'s `ApplyUpgrade`, the `VersionMap` will be retrieved from the (old binary's) store, and passed into the `RunMigrations` functon, migrating all module stores in-place before the modules' own `BeginBlock`s. ## Consequences @@ -159,7 +159,6 @@ While modules MUST register their migration scripts when bumping ConsensusVersio ### Neutral -- Currently, Cosmovisor only handles JSON migrations. Its code should be updated to support in-place store migrations too. - The SDK will continue to support JSON migrations via the existing `simd export` and `simd migrate` commands. - The current ADR does not allow creating, renaming or deleting stores, only modifying existing store keys and values. The SDK already has the `StoreLoader` for those operations. From 0fc97c5c2a244b5d5d66063907fe0ecac76adb2c Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 14:42:34 +0100 Subject: [PATCH 17/22] Add grpc, use functions --- .../adr-041-in-place-store-migrations.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 78d1a84ffeee..d0d92a617fdc 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -38,9 +38,9 @@ type AppModule interface { This methods returns an `uint64` which serves as state-breaking version of the module. It MUST be incremented on each consensus-breaking change introduced by the module. To avoid potential errors with default values, the initial version of a module MUST be set to 1. In the SDK, version 1 corresponds to the modules in the v0.41 series. -### Module-Specific Migration Scripts +### Module-Specific Migration Functions -For each consensus-breaking change introduced by the module, a migration script from ConsensusVersion `N` to version `N+1` MUST be registered in the `Configurator` using its newly-added `RegisterMigration` method. All modules receive a reference to the configurator in their `RegisterServices` method on `AppModule`, and this is where the migration scripts should be registered. +For each consensus-breaking change introduced by the module, a migration script from ConsensusVersion `N` to version `N+1` MUST be registered in the `Configurator` using its newly-added `RegisterMigration` method. All modules receive a reference to the configurator in their `RegisterServices` method on `AppModule`, and this is where the migration functions should be registered. The migration functions should be registered in increasing order. ```go func (am AppModule) RegisterServices(cfg module.Configurator) { @@ -55,9 +55,9 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { } ``` -For example, if the new ConsensusVersion of a module is `N` , then `N-1` migration scripts MUST be registered in the configurator. +For example, if the new ConsensusVersion of a module is `N` , then `N-1` migration functions MUST be registered in the configurator. -In the SDK, the migration scripts are handled by each module's keeper, because the keeper holds the `sdk.StoreKey` used to perform in-place store migrations. To not overload the keeper, a `Migrator` wrapper is used by each module to handle the migration scripts: +In the SDK, the migration functions are handled by each module's keeper, because the keeper holds the `sdk.StoreKey` used to perform in-place store migrations. To not overload the keeper, a `Migrator` wrapper is used by each module to handle the migration functions: ```go // Migrator is a struct for handling in-place store migrations. @@ -66,7 +66,7 @@ type Migrator struct { } ``` -Since migration scripts manipulate legacy code, they should live inside the `legacy/` folder of each module, and be called by the Migrator's methods. We propose the format `Migrate{M}to{N}` for method names. +Since migration functions manipulate legacy code, they should live inside the `legacy/` folder of each module, and be called by the Migrator's methods. We propose the format `Migrate{M}to{N}` for method names. ```go // Migrate1to2 migrates from version 1 to 2. @@ -75,7 +75,7 @@ func (m Migrator) Migrate1to2(ctx sdk.Context) error { } ``` -Each module's migration scripts are specific to the module's store evolutions, and are not described in this ADR. An example of x/bank store key migrations following the introduction of ADR-028 length-prefixed addresses can be seen [here](https://github.com/cosmos/cosmos-sdk/blob/ef8dabcf0f2ecaf26db1c6c6d5922e9399458bb3/x/bank/legacy/v042/store.go#L15). +Each module's migration functions are specific to the module's store evolutions, and are not described in this ADR. An example of x/bank store key migrations following the introduction of ADR-028 length-prefixed addresses can be seen [here](https://github.com/cosmos/cosmos-sdk/blob/ef8dabcf0f2ecaf26db1c6c6d5922e9399458bb3/x/bank/legacy/v042/store.go#L15). ### Tracking Module Versions in `x/upgrade` @@ -114,6 +114,8 @@ func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { } ``` +An gRPC query endpoint to query the `VersionMap` stored in `x/upgrade`'s state will also be added, so that app developers can query get double-check the `VersionMap` before the upgrade handler runs. + ### Running Migrations Once all the migration handlers are registered inside the configurator (which happens at startup), running migrations can happen by calling the `RunMigrations` method on `module.Manager`. This function will loop through all modules, and for each module: @@ -146,7 +148,7 @@ Assuming a chain upgrades at block `n`, the procedure should run as follows: This ADR introduces a new method `ConsensusVersion()` on `AppModule`, which all modules need to implement. It also alters the UpgradeHandler function signature. As such, it is not backwards-compatible. -While modules MUST register their migration scripts when bumping ConsensusVersions, running those scripts using an upgrade handler is optional. An application may perfectly well decide to not call the `RunMigrations` inside its upgrade handler, and continue using the legacy JSON migration path. +While modules MUST register their migration functions when bumping ConsensusVersions, running those scripts using an upgrade handler is optional. An application may perfectly well decide to not call the `RunMigrations` inside its upgrade handler, and continue using the legacy JSON migration path. ### Positive From 53a13abcceed77041c08884fd8089281a6ec1682 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 14:56:18 +0100 Subject: [PATCH 18/22] Add special case with 0 version --- .../adr-041-in-place-store-migrations.md | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index d0d92a617fdc..137e7143d584 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -6,7 +6,7 @@ ## Status -Proposed +Accepted ## Abstract @@ -97,6 +97,8 @@ type VersionManager interface { type VersionMap map[string]uint64 ``` +This `versionManager` field can be modified via the `SetVersionManager` field, and will allow the upgrade keeper to know the current versions of loaded modules. `SetVersionManager` MUST be called as early as possible in the app initialization; in the SDK's `simapp`, it is called in the `NewSimApp` constructor function. + The UpgradeHandler signature needs to be updated to take a `VersionMap`, as well as return an error: ```diff @@ -104,17 +106,24 @@ The UpgradeHandler signature needs to be updated to take a `VersionMap`, as well + type UpgradeHandler func(ctx sdk.Context, plan Plan, versionMap VersionMap) error ``` -To apply an upgrade, we query the `VersionMap` from the `x/upgrade` store and pass it into the handler. Right after the +To apply an upgrade, we query the `VersionMap` from the `x/upgrade` store and pass it into the handler. The handler runs the actual migration functions (see next section), and if successful, the current ConsensusVersions of all loaded modules will be stored into state. ```diff func (k UpgradeKeeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) { // --snip-- - handler(ctx, plan) + err := handler(ctx, plan, k.GetConsensusVersions()) // k.GetConsensusVersions() fetches the VersionMap stored in state. ++ if err != nil { ++ return err ++ } ++ ++ // Get the current ConsensusVersions of the loaded modules (retrieved from ++ // `k.versionManager`), and save them to state. ++ k.SetCurrentConsensusVersions() } ``` -An gRPC query endpoint to query the `VersionMap` stored in `x/upgrade`'s state will also be added, so that app developers can query get double-check the `VersionMap` before the upgrade handler runs. +An gRPC query endpoint to query the `VersionMap` stored in `x/upgrade`'s state will also be added, so that app developers can double-check the `VersionMap` before the upgrade handler runs. ### Running Migrations @@ -123,6 +132,7 @@ Once all the migration handlers are registered inside the configurator (which ha - Get the old ConsensusVersion of the module from its `VersionMap` argument (let's call it `M`). - Fetch the new ConsensusVersion of the module from the `ConsensusVersion()` method on `AppModule` (call it `N`). - If `N>M`, run all registered migrations for the module sequentially `M -> M+1 -> M+2...` until `N`. + - There is a special case where there is no ConsensusVersion for the module, as this means that the module has been newly added during the upgrade. In this case, no migration function is run, and the module's current ConsensusVersion is saved to `x/upgrade`'s store. If a required migration is missing (e.g. if it has not been registered in the `Configurator`), then the `RunMigrations` function will error. From 9e07b4d070df69f3dd3f55cdad35403ef5d8e70c Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 18:21:16 +0100 Subject: [PATCH 19/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Aaron Craelius --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 137e7143d584..fb6331c58afe 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -62,7 +62,7 @@ In the SDK, the migration functions are handled by each module's keeper, because ```go // Migrator is a struct for handling in-place store migrations. type Migrator struct { - keeper BaseKeeper + BaseKeeper } ``` From 41386caecbe61cc9372ba683aecff6eaee0c3d89 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 18:21:31 +0100 Subject: [PATCH 20/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Aaron Craelius --- docs/architecture/adr-041-in-place-store-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index fb6331c58afe..3f73c3fbdbbb 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -140,7 +140,7 @@ In practice, the `RunMigrations` method should be called from inside an `Upgrade ```go app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, versionMap VersionMap) error { - err := app.mm.RunMigrations(ctx, versionMap) + return app.mm.RunMigrations(ctx, versionMap) if err != nil { return err } From 606aec168e0e06c0648b224d564721c1625db6fb Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 18:21:50 +0100 Subject: [PATCH 21/22] Update docs/architecture/adr-041-in-place-store-migrations.md Co-authored-by: Aaron Craelius --- docs/architecture/adr-041-in-place-store-migrations.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index 3f73c3fbdbbb..ccb6b261ef4f 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -141,7 +141,6 @@ In practice, the `RunMigrations` method should be called from inside an `Upgrade ```go app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, versionMap VersionMap) error { return app.mm.RunMigrations(ctx, versionMap) - if err != nil { return err } }) From 633d07bdc6f684e3c3cf9897d26f14640c92d4c5 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 4 Mar 2021 18:24:19 +0100 Subject: [PATCH 22/22] Remove useless err return --- docs/architecture/adr-041-in-place-store-migrations.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/architecture/adr-041-in-place-store-migrations.md b/docs/architecture/adr-041-in-place-store-migrations.md index ccb6b261ef4f..2286c6d2d9bf 100644 --- a/docs/architecture/adr-041-in-place-store-migrations.md +++ b/docs/architecture/adr-041-in-place-store-migrations.md @@ -141,8 +141,6 @@ In practice, the `RunMigrations` method should be called from inside an `Upgrade ```go app.UpgradeKeeper.SetUpgradeHandler("my-plan", func(ctx sdk.Context, plan upgradetypes.Plan, versionMap VersionMap) error { return app.mm.RunMigrations(ctx, versionMap) - return err - } }) ```