zetacored migrations run twice

[gartnera]

The zetacored migrations seem to be running twice. To reproduce, upgrade from v14 to v15 to a third version:

9:01PM INF Running upgrade handler for v15
9:01PM INF migrating module observer from version 6 to version 7
9:01PM INF Migrating observer store from v6 to v7
9:01PM INF Block rewards 9620949074074074074.074070733466756687 are greater than emission pool balance 0.000000000000000000
9:01PM INF executed block height=90 module=consensus num_invalid_txs=0 num_valid_txs=0 server=node

9:06PM INF Running upgrade handler for v1713982165
9:06PM INF migrating module observer from version 6 to version 7
9:06PM INF Migrating observer store from v6 to v7
9:06PM INF Block rewards 9620949074074074074.074070733466756687 are greater than emission pool balance 0.000000000000000000
9:06PM INF executed block height=250 module=consensus num_invalid_txs=0 num_valid_txs=0 server=node

The way that we choose which migrations to run seems to differ quite a bit from how cosmos suggests. First, there is this logic which means that we will always run the last migration from the observer module:

node/app/setup_handlers.go

Lines 47 to 49 in 022ea57

	func (v VersionMigrator) TriggerMigration(moduleName string) module.VersionMap {
	v.v[moduleName] = v.v[moduleName] - 1
	return v.v

We also seem to be just ignoring the x/upgrade state version map and forcibly setting all modules to the latest version:

node/app/setup_handlers.go

Lines 19 to 21 in 022ea57

	for m, mb := range app.mm.Modules {
	vm[m] = mb.ConsensusVersion()
	}

This has effect of always skipping migrations of any module other than observer. It apparently will also result in InitGenesis not being called for any new modules. See RunMigrations for more info. If we just want to always skip InitGenesis for all modules, then the correct logic would look more like this:

for m, mb := range app.mm.Modules {
	if vm[m] == 0 {
		vm[m] = mb.ConsensusVersion()
	}
}

We also do not seem to be ever calling app.UpgradeKeeper.SetModuleVersionMap(ctx, app.mm.GetVersionMap()) in InitChainer so the upgrade state is never committed to the disk. Because of this, the upgradeHandler fromVM version map always returns 0.

[skosito]

re migration run twice: I think if you upgrade from v15->v16 you have 1 upgrade handler and one consensus version for modules being upgraded (in this case observer consensus version is bumped from 6 to 7, therefore migration is run)

if you want to upgrade v16->v17 and again upgrade observer module, you again bump consensus version and add migration script and so on, but also upgrade handler changes, where you would remove triggering observer migration if you don't need it, or add new migration from consensus version 7 to 8 etc - basically it is also new upgrade handler for every upgrade

i think we store only last upgrade handler in codebase, so we are modifying it at the moment with new upgrades, but for next upgrade upgrade handler will probably be different

in situation you mention, upgrade handler for v17 would not contain part that triggers observer migration

setting manually to latest ConsensusVersion is recommended in upgrade handler afaik, if you want to skip init genesis as you mentioned (or in other words, if module is not new), but i agree we don't store module version map, which is why we need to manually trigger migration by reducing consensus version -1

this causes extra effort and more manual work if we want to run cosmos modules upgrades when we upgrade cosmos-sdk version (eg in cosmos 0.47 upgrade PR i have to do the same for cosmos modules to trigger their migrations, otherwise it wont be detected automatically because we dont store the map and fromVM is always empty #2039 (comment))

so we should store this map, and remove consensus version - 1 part

[gartnera]

so we should store this map, and remove consensus version - 1 part

All I think we can do for now is add the app.UpgradeKeeper.SetModuleVersionMap(ctx, app.mm.GetVersionMap()) to v16 release, then untangle the rest in v17? We need one release where the version map gets committed before we can really change much it seems.

[kingpinXD]

The function is already called automatically I belive when the upgrade is applied
https://github.com/zeta-chain/cosmos-sdk/blob/b3f95061fadeb51ffc731b60673c422ed5096948/x/upgrade/keeper/keeper.go#L365-L365

We can see the versions on mainnet here
https://zetachain-mainnet-archive.allthatnode.com:1317/cosmos/upgrade/v1beta1/module_versions

However, I do agree manually decrementing the version is not the correct approach
We should directly be sending the VersionMap received , the SDK basically uses the version from the Map vs the Version from Module.Connsesus version , to trigger the upgrade .

[kingpinXD]

THe issue with initi genesis , should also be solved because new modules would not exist in the fromVM , which is much more cleaner that using using some some weird logic like this .

		for m, mb := range app.mm.Modules {
			if _, ok := vm[m]; ok {
				vm[m] = mb.ConsensusVersion()
			}
		}

Just posting this as an example to explain what I mean

[gartnera]

The function is already called automatically I belive when the upgrade is applied

Ah I understand now the point of calling app.UpgradeKeeper.SetModuleVersionMap(ctx, app.ModuleManager.GetVersionMap()) in InitChainer is to skip the migrations on brand new chain initialization.

So the consensus versions should already be committed to the disk on v14. I think we can remove the - 1 now then?

[skosito]

The function is already called automatically I belive when the upgrade is applied

Ah I understand now the point of calling app.UpgradeKeeper.SetModuleVersionMap(ctx, app.ModuleManager.GetVersionMap()) in InitChainer is to skip the migrations on brand new chain initialization.

So the consensus versions should already be committed to the disk on v14. I think we can remove the - 1 now then?

but maybe we still need to add this to init chainer to test upgrades locally? currently fromVm is always empty when testing upgrades locally because store is empty in that case, and we need -1 to trigger it?

[kingpinXD]

Yeah, exactly. We can remove the -1 for now. It should solve the following problems for us.

• Remove the need to manually trigger migrations, Incrementing the consensus version in the module.go would automatically trigger migrations
• Initi genesis would get called automatically if the module names are added to the Added section .

I can create a PR and run some tests.
Related Issue : #1824