improve UX around running InitGenesis() in RunMigrations()

[p0mvn]

Summary

Currently, the default behaviour for any new module is to run InitGenesis() in RunMigrations(). This is called from the upgrade handler:

cosmos-sdk/types/module/module.go

Lines 620 to 629 in 9569b34

	} else {
	ctx.Logger().Info(fmt.Sprintf("adding a new module: %s", moduleName))
	if module, ok := m.Modules[moduleName].(HasGenesis); ok {
	moduleValUpdates := module.InitGenesis(ctx, c.cdc, module.DefaultGenesis(c.cdc))
	// The module manager assumes only one module will update the
	// validator set, and it can't be a new module.
	if len(moduleValUpdates) > 0 {
	return nil, errorsmod.Wrapf(sdkerrors.ErrLogic, "validator InitGenesis update is already set by another module")
	}
	}

However, this is not always desired as the chain might be migrating state from old module to a new module. The call to InitGenesis() erases any migrated parameters and state, leading to problems that are hard to identify and debug.

Problem Definition

In Osmosis, we've run into this issue on 2 different occasions. In both cases, this problem ended up being non-trivial to debug as we simply observed missing parameters after e2e testing our upgrades and no additional context. We have come up with a workaround:
https://github.com/osmosis-labs/osmosis/blob/b49e35e180fdffa8dfe2730f2105803b9045bf89/app/upgrades/v15/upgrades.go#L41-L44

It should also be possible to call RunMigrations() prior to performing any inter-module migrations in the upgrade handler to avoid having RunMigrations() overwrite the inter-module state changes.

However, both approaches not solve the major problem around UX and the difficulty with identifying and debugging it. There should be a guard against missing this low-level but important detail.

Proposal

We should make it so that chain developers must specifically identify for which new modules to run InitGenesis() during the upgrade. If a new module is added without an explicit setting to disable or enable running InitGenesis() during the next upgrade, the binary should not start and spit out a descriptive error message about where to configure this.

[tac0turtle]

@julienrbrt brought this up today, he was going to start looking into it.

[julienrbrt]

Yes, somewhat related too: #5041
I'll start on it after my logger PR.

[ValarDragon]

Is this slated for SDK v0.48, or a future release?

[julienrbrt]

Is this slated for SDK v0.48, or a future release?

Thanks for the reminder, given v0.48 is not feature frozen yet, I will open the PR before :)

[ValarDragon]

TYSM!!

[julienrbrt]

We haven't really changed how migrations were done for v2 and kept it the same.
While the UX has footguns, it is explained in the RunMigrations() what to do to avoid them:

// As an app developer, if you wish to skip running InitGenesis for your new
// module "foo", you need to manually pass a `fromVM` argument to this function
// foo's module version set to its latest ConsensusVersion. That way, the diff
// between the function's `fromVM` and `udpatedVM` will be empty, hence not
// running anything for foo.

Given that for most users that won't be the case, and the extra configurability of RunMigrations() was making it less usable, I am going to close this as won't do.