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

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

Merged
merged 42 commits into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
e900fb1
-added consensus version tracking to x/upgrade
technicallyty Mar 2, 2021
036db4f
-added interface to module manager -added e2e test for migrations usi…
technicallyty Mar 3, 2021
b640766
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 3, 2021
8c7b105
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 3, 2021
5c1ae59
Changed MigrationMap identifier to VersionMap
technicallyty Mar 4, 2021
4c48f35
updated docs
technicallyty Mar 4, 2021
6db21e3
forgot this
technicallyty Mar 4, 2021
c9b435a
added line to changelog for this PR
technicallyty Mar 4, 2021
1c5b132
Change set consensus version function to match adr 041 spec
technicallyty Mar 8, 2021
304abfa
add documentation
technicallyty Mar 8, 2021
d06bbbd
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 8, 2021
5c75ead
Merge branch 'ty-8514-module_tracking' of https://github.com/technica…
technicallyty Mar 8, 2021
e53423e
remove newline from changelog
technicallyty Mar 8, 2021
c36086f
updated example in simapp for RunMigrations, SetCurrentConsensusVersi…
technicallyty Mar 8, 2021
758c67d
switch TestMigrations to use Require instead of t.Fatal
technicallyty Mar 8, 2021
0dac9c2
Update CHANGELOG.md
technicallyty Mar 10, 2021
8f526ef
docs for SetVersionManager
technicallyty Mar 10, 2021
027740d
-init genesis method added -removed panics/fails from setting consens…
technicallyty Mar 11, 2021
18eaed2
merge master
technicallyty Mar 11, 2021
fc38e5a
update identifiers to be more go-like
technicallyty Mar 12, 2021
28d5c04
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 12, 2021
1fca959
update docs and UpgradeHandler fnc sig
technicallyty Mar 15, 2021
d912f4f
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 15, 2021
a99c538
Upgrade Keeper now takes a VersionMap instead of a VersionManager int…
technicallyty Mar 15, 2021
e6424ce
upgrade keeper transition to Version Map
technicallyty Mar 16, 2021
9b40fe7
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 16, 2021
799b646
cleanup, added versionmap return to RunMigrations
technicallyty Mar 16, 2021
0f9447c
quick fix
technicallyty Mar 16, 2021
a157c48
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 17, 2021
b839a59
Update docs/architecture/adr-041-in-place-store-migrations.md
technicallyty Mar 17, 2021
75f8ae5
remove support for versionmap field on upgrade keeper
technicallyty Mar 17, 2021
22ab55e
cleanup
technicallyty Mar 17, 2021
47ce533
merge adr change
technicallyty Mar 17, 2021
7f5b6aa
rename get/set version map keeper functions
technicallyty Mar 18, 2021
8547426
update adr doc to match name changes
technicallyty Mar 18, 2021
1504a2d
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 18, 2021
c3072ab
remove redudant line
technicallyty Mar 19, 2021
1f82fff
Merge branch 'ty-8514-module_tracking' of https://github.com/technica…
technicallyty Mar 19, 2021
cf1343f
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 19, 2021
3f6bea8
Merge branch 'master' into ty-8514-module_tracking
aaronc Mar 19, 2021
d987555
Merge branch 'master' into ty-8514-module_tracking
technicallyty Mar 19, 2021
5d6e4ab
Merge branch 'master' into ty-8514-module_tracking
mergify[bot] Mar 19, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#8629](https://github.com/cosmos/cosmos-sdk/pull/8629) Deprecated `SetFullFundraiserPath` from `Config` in favor of `SetPurpose` and `SetCoinType`.
* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove IBC logic from x/upgrade. Deprecates IBC fields in an Upgrade Plan. IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added.
* (x/bank) [\#8517](https://github.com/cosmos/cosmos-sdk/pull/8517) `SupplyI` interface and `Supply` are removed and uses `sdk.Coins` for supply tracking
* (x/upgrade) [\#8743](https://github.com/cosmos/cosmos-sdk/pull/8743) `UpgradeHandler` requires new argument `VersionManager` which retreives module consensus versions from upgrade's new store.
technicallyty marked this conversation as resolved.
Show resolved Hide resolved

### State Machine Breaking

Expand Down
19 changes: 10 additions & 9 deletions simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,8 @@ func NewSimApp(
params.NewAppModule(app.ParamsKeeper),
authz.NewAppModule(appCodec, app.AuthzKeeper, app.AccountKeeper, app.BankKeeper, app.interfaceRegistry),
)
// give upgrade keeper the module manager
app.UpgradeKeeper.SetVersionManager(app.mm)

// During begin block slashing happens after distr.BeginBlocker so that
// there is nothing left over in the validator fee pool, so as to keep the
Expand Down Expand Up @@ -551,19 +553,18 @@ func (app *SimApp) RegisterTendermintService(clientCtx client.Context) {
//
// Example:
// cfg := module.NewConfigurator(...)
// app.UpgradeKeeper.SetUpgradeHandler("store-migration", func(ctx sdk.Context, plan upgradetypes.Plan) {
// err := app.RunMigrations(ctx, module.MigrationMap{
// "bank": 1, // Migrate x/bank from v1 to current x/bank's ConsensusVersion
// "staking": 8, // Migrate x/staking from v8 to current x/staking's ConsensusVersion
// })
// if err != nil {
// panic(err)
// }
// app.UpgradeKeeper.SetUpgradeHandler("store-migration", func(ctx sdk.Context, plan upgradetypes.Plan, versionMap module.VersionMap) {
// return app.RunMigrations(ctx, versionMap)
// })
func (app *SimApp) RunMigrations(ctx sdk.Context, migrateFromVersions module.MigrationMap) error {
func (app *SimApp) RunMigrations(ctx sdk.Context, migrateFromVersions module.VersionMap) error {
return app.mm.RunMigrations(ctx, app.configurator, migrateFromVersions)
}

// Returns a map (VersionMap) of key module name and value module consensus version
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
func (app *SimApp) GetConsensusVersions() module.VersionMap {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
return app.mm.GetConsensusVersions()
}

// RegisterSwaggerAPI registers swagger route with API Server
func RegisterSwaggerAPI(ctx client.Context, rtr *mux.Router) {
statikFS, err := fs.New()
Expand Down
2 changes: 1 addition & 1 deletion simapp/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func TestRunMigrations(t *testing.T) {
// their latest ConsensusVersion.
err = app.RunMigrations(
app.NewContext(true, tmproto.Header{Height: app.LastBlockHeight()}),
module.MigrationMap{
module.VersionMap{
"bank": 1,
"auth": auth.AppModule{}.ConsensusVersion(),
"authz": authz.AppModule{}.ConsensusVersion(),
Expand Down
22 changes: 19 additions & 3 deletions types/module/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,12 +333,16 @@ func (m *Manager) ExportGenesis(ctx sdk.Context, cdc codec.JSONMarshaler) map[st
// MigrationHandler is the migration function that each module registers.
type MigrationHandler func(sdk.Context) error

// MigrationMap is a map of moduleName -> version, where version denotes the
// VersionMap is a map of moduleName -> version, where version denotes the
// version from which we should perform the migration for each module.
type MigrationMap map[string]uint64
type VersionMap map[string]uint64

type VersionManager interface {
GetConsensusVersions() VersionMap
}

// RunMigrations performs in-place store migrations for all modules.
func (m Manager) RunMigrations(ctx sdk.Context, cfg Configurator, migrateFromVersions MigrationMap) error {
func (m Manager) RunMigrations(ctx sdk.Context, cfg Configurator, migrateFromVersions VersionMap) error {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
c, ok := cfg.(configurator)
if !ok {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", configurator{}, cfg)
Expand Down Expand Up @@ -395,3 +399,15 @@ func (m *Manager) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
Events: ctx.EventManager().ABCIEvents(),
}
}

// GetConsensusVersions gets consensus version from all modules
func (m *Manager) GetConsensusVersions() VersionMap {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
vermap := make(VersionMap)
for _, v := range m.Modules {
version := v.ConsensusVersion()
name := v.Name()
vermap[name] = version
}

return vermap
}
6 changes: 3 additions & 3 deletions x/upgrade/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func VerifyDoUpgrade(t *testing.T) {
})

t.Log("Verify that the upgrade can be successfully applied with a handler")
s.keeper.SetUpgradeHandler("test", func(ctx sdk.Context, plan types.Plan) {})
s.keeper.SetUpgradeHandler("test", func(ctx sdk.Context, plan types.Plan, versionMap module.VersionMap) error { return nil })
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
require.NotPanics(t, func() {
s.module.BeginBlock(newCtx, req)
})
Expand All @@ -144,7 +144,7 @@ func VerifyDoUpgradeWithCtx(t *testing.T, newCtx sdk.Context, proposalName strin
})

t.Log("Verify that the upgrade can be successfully applied with a handler")
s.keeper.SetUpgradeHandler(proposalName, func(ctx sdk.Context, plan types.Plan) {})
s.keeper.SetUpgradeHandler(proposalName, func(ctx sdk.Context, plan types.Plan, versionMap module.VersionMap) error { return nil })
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
require.NotPanics(t, func() {
s.module.BeginBlock(newCtx, req)
})
Expand All @@ -156,7 +156,7 @@ func TestHaltIfTooNew(t *testing.T) {
s := setupTest(10, map[int64]bool{})
t.Log("Verify that we don't panic with registered plan not in database at all")
var called int
s.keeper.SetUpgradeHandler("future", func(ctx sdk.Context, plan types.Plan) { called++ })
s.keeper.SetUpgradeHandler("future", func(ctx sdk.Context, plan types.Plan, versionMap module.VersionMap) error { called++; return nil })

newCtx := s.ctx.WithBlockHeight(s.ctx.BlockHeight() + 1).WithBlockTime(time.Now())
req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
Expand Down
3 changes: 2 additions & 1 deletion x/upgrade/keeper/grpc_query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/simapp"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/upgrade/types"
)

Expand Down Expand Up @@ -110,7 +111,7 @@ func (suite *UpgradeTestSuite) TestAppliedCurrentPlan() {
suite.app.UpgradeKeeper.ScheduleUpgrade(suite.ctx, plan)

suite.ctx = suite.ctx.WithBlockHeight(expHeight)
suite.app.UpgradeKeeper.SetUpgradeHandler(planName, func(ctx sdk.Context, plan types.Plan) {})
suite.app.UpgradeKeeper.SetUpgradeHandler(planName, func(_ sdk.Context, _ types.Plan, _ module.VersionMap) error { return nil })
suite.app.UpgradeKeeper.ApplyUpgrade(suite.ctx, plan)

req = &types.QueryAppliedPlanRequest{Name: planName}
Expand Down
51 changes: 50 additions & 1 deletion x/upgrade/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
store "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/upgrade/types"
)

Expand All @@ -28,6 +29,7 @@ type Keeper struct {
storeKey sdk.StoreKey
cdc codec.BinaryMarshaler
upgradeHandlers map[string]types.UpgradeHandler
versionManager module.VersionManager
}

// NewKeeper constructs an upgrade Keeper
Expand All @@ -48,6 +50,45 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl
k.upgradeHandlers[name] = upgradeHandler
}

func (k *Keeper) SetVersionManager(vm module.VersionManager) {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
k.versionManager = vm
}

// SetCurrentConsensusVersions saves the consensus versions retrieved from module.Manager
func (k Keeper) SetCurrentConsensusVersions(ctx sdk.Context) error {
if k.versionManager == nil {
return sdkerrors.Wrap(sdkerrors.ErrLogic, "upgrade keeper VersionManager was nil, please call SetVersionManager on the keeper first")
}
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
modules := k.versionManager.GetConsensusVersions()
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
store := ctx.KVStore(k.storeKey)
versionStore := prefix.NewStore(store, []byte{types.VersionMapByte})
for modName, ver := range modules {
nameBytes := []byte(modName)
verBytes := make([]byte, 8)
binary.LittleEndian.PutUint64(verBytes, ver)
versionStore.Set(nameBytes, verBytes)
}
return nil
}

// GetConsensusVersions gets a VersionMap from state
func (k Keeper) GetConsensusVersions(ctx sdk.Context) module.VersionMap {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
store := ctx.KVStore(k.storeKey)
it := sdk.KVStorePrefixIterator(store, []byte{types.VersionMapByte})

vermap := make(module.VersionMap)
defer it.Close()
for ; it.Valid(); it.Next() {
moduleBytes := it.Key()
// first byte is prefix key, so we remove it here
name := string(moduleBytes[1:])
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
moduleVersion := binary.LittleEndian.Uint64(it.Value())
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
vermap[name] = moduleVersion
}

return vermap
}

// ScheduleUpgrade schedules an upgrade based on the specified plan.
// If there is another Plan already scheduled, it will overwrite it
// (implicitly cancelling the current plan)
Expand Down Expand Up @@ -191,7 +232,15 @@ func (k Keeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) {
panic("ApplyUpgrade should never be called without first checking HasHandler")
}

handler(ctx, plan)
err := handler(ctx, plan, k.GetConsensusVersions(ctx))
if err != nil {
panic(err)
}

err = k.SetCurrentConsensusVersions(ctx)
if err != nil {
panic(err)
}
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved

// Must clear IBC state after upgrade is applied as it is stored separately from the upgrade plan.
// This will prevent resubmission of upgrade msg after upgrade is already completed.
Expand Down
35 changes: 33 additions & 2 deletions x/upgrade/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"github.com/cosmos/cosmos-sdk/simapp"
store "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/x/bank"
"github.com/cosmos/cosmos-sdk/x/upgrade/keeper"
"github.com/cosmos/cosmos-sdk/x/upgrade/types"
)
Expand Down Expand Up @@ -135,7 +137,7 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() {
Height: 123450000,
},
setup: func() {
s.app.UpgradeKeeper.SetUpgradeHandler("all-good", func(_ sdk.Context, _ types.Plan) {})
s.app.UpgradeKeeper.SetUpgradeHandler("all-good", func(_ sdk.Context, _ types.Plan, _ module.VersionMap) error { return nil })
s.app.UpgradeKeeper.ApplyUpgrade(s.ctx, types.Plan{
Name: "all-good",
Info: "some text here",
Expand All @@ -152,7 +154,7 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() {
s.Run(tc.name, func() {
// reset suite
s.SetupTest()

s.app.UpgradeKeeper.SetVersionManager(s.app)
// setup test case
tc.setup()

Expand Down Expand Up @@ -211,6 +213,35 @@ func (s *KeeperTestSuite) TestSetUpgradedClient() {

}

// Mock version manager for TestMigrations
type mockVersionManager struct{}

func (m mockVersionManager) GetConsensusVersions() module.VersionMap {
vermap := make(module.VersionMap)
vermap["bank"] = 1
return vermap
}

// Tests that the underlying state of x/upgrade is set correctly after
// an upgrade.
func (s *KeeperTestSuite) TestMigrations() {
mockVM := mockVersionManager{}
s.app.UpgradeKeeper.SetVersionManager(mockVM)
err := s.app.UpgradeKeeper.SetCurrentConsensusVersions(s.ctx)
s.Require().Nil(err)
s.app.UpgradeKeeper.SetUpgradeHandler("dummy", func(_ sdk.Context, _ types.Plan, _ module.VersionMap) error { return nil })
dummyPlan := types.Plan{
Name: "dummy",
Info: "some text here",
Time: s.ctx.BlockTime().Add(time.Hour),
}

s.app.UpgradeKeeper.SetVersionManager(s.app)
s.app.UpgradeKeeper.ApplyUpgrade(s.ctx, dummyPlan)
vermap := s.app.UpgradeKeeper.GetConsensusVersions(s.ctx)
s.Require().Equal(bank.AppModule{}.ConsensusVersion(), vermap["bank"])
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
}

func TestKeeperTestSuite(t *testing.T) {
suite.Run(t, new(KeeperTestSuite))
}
2 changes: 1 addition & 1 deletion x/upgrade/spec/01_concepts.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ and not defined on a per-module basis. Registering a `Handler` is done via
`Keeper#SetUpgradeHandler` in the application.

```go
type UpgradeHandler func(Context, Plan)
type UpgradeHandler func(Context, Plan, VersionMap) error
```

During each `EndBlock` execution, the `x/upgrade` module checks if there exists a
Expand Down
14 changes: 11 additions & 3 deletions x/upgrade/spec/02_state.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@ order: 2
# State

The internal state of the `x/upgrade` module is relatively minimal and simple. The
state only contains the currently active upgrade `Plan` (if one exists) by key
`0x0` and if a `Plan` is marked as "done" by key `0x1`.
state contains the currently active upgrade `Plan` (if one exists) by key
`0x0` and if a `Plan` is marked as "done" by key `0x1`. Additionally, the state
contains the consensus versions of all app modules in the application. The versions
are stored as little endian `uint64`, and can be accessed with prefix `0x2` appended
by the corresponding module name of type `string`.

The `x/upgrade` module contains no genesis state.
- Plan: `0x0 -> Plan`
- Done: `0x1 | byte(plan name) -> BigEndian(Block Height)`
- ConsensusVersion: `0x2 | byte(module name) -> LittleEndian(Module Consensus Version)`
technicallyty marked this conversation as resolved.
Show resolved Hide resolved


The `x/upgrade` module contains no genesis state.
3 changes: 2 additions & 1 deletion x/upgrade/types/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ package types

import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
)

// UpgradeHandler specifies the type of function that is called when an upgrade is applied
type UpgradeHandler func(ctx sdk.Context, plan Plan)
type UpgradeHandler func(ctx sdk.Context, plan Plan, versionMap module.VersionMap) error
Copy link
Member

@aaronc aaronc Mar 9, 2021

Choose a reason for hiding this comment

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

I thought the initial plan was to make this:

type UpgradeHandler func(ctx sdk.Context, plan Plan, versionMap module.VersionMap) (module.VersionMap, error)

Is the reason for not doing this because we want to support setting versions in InitChain?

Copy link
Contributor

@amaury1093 amaury1093 Mar 10, 2021

Choose a reason for hiding this comment

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

I'm failing to see how the return module.VersionMap is used. Right now, in ApplyUpgrade we do:

err := handler(ctx, plan, versionMapFromState) // Run the handler with module versions from state
k.SetCurrentConsensusVersion(ctx); // Once the handler is run, update state with new module versions

so I don't see the utility of returning the (I suppose new) versionMap from 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.

I guess my original idea was that we don't pass a reference to ModuleManager into x/upgrade but instead communicate the new versions with the return type... However, to avoid bike-shedding let's just go with this approach for now as I think they're basically equivalent.

Copy link
Member

Choose a reason for hiding this comment

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

Ah... actually I think this is an issue. So the code in this PR fails if the module manager is not passed in. We should either:

  • fail gracefully if the module manager is not set, or
  • take the alternate approach I'm suggesting

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the Aarons approach. Limiting dependencies (especially for something as big as ModuleManager) sounds as a good approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK sgtm, we can do like this 👍

Copy link
Contributor Author

@technicallyty technicallyty Mar 15, 2021

Choose a reason for hiding this comment

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

What if we just avoid the extra field altogether, export the setConsensusVersions function and update it to take a VersionMap? The versionMap field on the keeper only ever gets used once anyways, which is from the InitGenesis call where we set the initial consensus versions to state. If we go with my proposed change, we don't need the extra field on upgrade's keeper, nor the InitGenesis code. Are there any advantages to committing the versions to state through InitGenesis rather than outright at the end of simapp?

We add this line to the bottom of simapp

ctx := app.BaseApp.NewUncachedContext(true, tmproto.Header{})
app.CapabilityKeeper.InitializeAndSeal(ctx)
+ app.UpgradeKeeper.SetConsensusVersions(ctx, app.mm.GetVersionMap())

This is in conjunction with the changes proposed above btw!

thoughts @AmauryM, @robert-zaremba, @aaronc ?

Copy link
Contributor

@amaury1093 amaury1093 Mar 16, 2021

Choose a reason for hiding this comment

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

Are there any advantages to committing the versions to state through InitGenesis rather than outright at the end of simapp?

InitGenesis is run once per chain, while "end of simapp" is run once per binary startup.

End of simapp would not work in the following scenario:

  • old binary runs, saves old VersionMap in x/upgrade store
  • swap binary
  • new binary runs. It doesn't run InitGenesis (it's not a new chain), but it runs "end of simapp", so saves app.mm.VersionMap to x/upgrade store
  • after that, it runs the UpgradeHandler. But it detects that the Versionmap inside x/upgrade store is exactly the same as the app.mm.VersionMap one, so it doesn't run any migration.

Does that make sense?

We need to call UpgradeKeeper.SetConsensusVersions twice:

  • once in InitGenesis
  • a second time inside the ApplyUpgrade method, right after the upgrade handler runs

Copy link
Member

Choose a reason for hiding this comment

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

Definitely true what you're saying about simapp @AmauryM.

Here's what I suggest:

  • have a UpgradeKeeper.SetConsensusVersions(VersionMap) method - this sets current versions which get written during InitChain. So in app.go, users will write:
upgradeKeeper.SetConsensusVersions(mm.GetConsensusVersions())
  • add a VersionMap return parameter to RunMigrations and a VersionMap return parameter to Upgrade Handler, so that most upgrade handlers will simply be implemented as:
func(ctx sdk.Context, plan Plan, versionMap module.VersionMap) (VersionMap, error) {
  return mm.RunMigrations(ctx, versionMap)
}

Unless there is a strong objection, I suggest we go with this. And if we need to go into any more details, I suggest we get on a short call to reach consensus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, changes implemented

3 changes: 3 additions & 0 deletions x/upgrade/types/keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ const (
// DoneByte is a prefix for to look up completed upgrade plan by name
DoneByte = 0x1

// VersionMapByte is a prefix to look up module names (key) and versions (value)
VersionMapByte = 0x2

// KeyUpgradedIBCState is the key under which upgraded ibc state is stored in the upgrade store
KeyUpgradedIBCState = "upgradedIBCState"

Expand Down