x/upgrade should track module versions

[amaury1093]

Summary

x/upgrade should track ConsensusVersions of different modules.

Problem Definition

In-place store migrations (#8485) introduce a ConsensusVersion for each module. When performing upgrades, the app developer needs to tack & hardcode a migrations map into the UpgradeHandler (see example here)

It would be good to store this migrations map in x/upgrade itself.

Proposal

• Idea: add a migrationsMap argument to the UpgradeHandler function signature.

cc @aaronc @robert-zaremba

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[aaronc]

Tagging this as 0.42 nice-to-have.

I think the way this would concretely work is:

• add ModuleVersionMap (map[string]uint64) param and return value to UpgradeHandler. (We should also add an error return value, no?) When there is no current version map in state, this value will be empty and upgrade handlers are expected to know the current versions
• add a new ModuleManager param to the upgrade NewKeeper constructor where ModuleManager is:

type ModuleManager interface {
  GetConsensusVersions() ModuleVersionMap
}

when InitChain is run the initial ModuleManager.GetConsensusVersions() value will be recorded to state so that future upgrades can be more automatic.

• add a ModuleVersionMap return param to module.Manager.RunMigrations

[robert-zaremba]

I would like to bring back the idea of on-the fly upgrades using a block number:

1. we release a new version, it has a hardcoded switch for new functionality based on the block number
2. the switch can have also a condition currentBlock == updateBlock && store[moduleX]["lastmigration"] != updateBlock - when true it will run migrations.

Initially I though it will kick the consensus stability, but after reflection it won't be worse the current approach - because now it's even more tricky: all nodes at the same time need to do a restart, run migrations and resume. The clear benefit are:

• we eliminate the risk of daemon restart
• updates and migrations is depending on the block number - so more predictable, than the binary / restart condition.

When doing a new release we will remove the legacy branches (switch) (so to keep the code clean) and add new ones if needed.

[amaury1093]

I don't think I understand.

updates and migrations is depending on the block number

This is already possible now.

we eliminate the risk of daemon restart

if an upgrade happens at block N+1, binary1 is running sdk-v041 until block N and binary2 will be running sdk-v042 from N+1, you need to do a binary switch at N. There's no (easy) way a binary can run two versions of the sdk at the same time.

[robert-zaremba]

@AmauryM - what I'm proposing is different. In my proposal there is no binary swap.

More specificall: binary V+1 (so for next version), will have logic for both V and V+1 and migrations required for going from V to V+1.
The migration happens exactly at height(V+1).
So a validator can update (start running a new binary) any time before height(V+1).

Then, when doing V+2 we remove old migrations and old logic (from version V). We could keep it if needed though.

This could be tricky to implement if we have a significant app structure change (like ADR-33).

[aaronc]

On the fly upgrades in the same binary will be confounded by subtle client compatible, but consensus breaking changes to protobuf schemas and generated code. It will be very hard if not impossible to get this right! Switching binaries at the same height is resistant to subtle consensus breaking changes, is tested and has automated tooling in the form of cosmovisor. I have tried to work out how to do what you're saying @robert-zaremba and it's not a bad idea in theory but will not work with protobuf. Also I'm not sure what is tricky about the current approach. I'm pretty sure the concerns you're raising have been addressed by the current upgrade module.