Bank Metadata store key format changed between v0.45 and v0.46

[okwme]

Summary of Bug

We ran into a bug in gaia's upgrade handler related to the bank module and coin metadata. Basically we were getting validate genesis errors because the coin metadata was missing Name and Symbol from the Metadata. We decided to add them to the bank keeper like this:

func CreateUpgradeHandler(
	mm *module.Manager,
	configurator module.Configurator,
	keepers *keepers.AppKeepers,
) upgradetypes.UpgradeHandler {
  return func(ctx sdk.Context, plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {

    // Add atom name and symbol into the bank keeper
    atomMetaData, found := keepers.BankKeeper.GetDenomMetaData(ctx, "uatom")
    if !found {
      return nil, errors.New("atom denom not found")
    }

    atomMetaData.Name = "Cosmos Hub Atom"
    atomMetaData.Symbol = "ATOM"
    keepers.BankKeeper.SetDenomMetaData(ctx, atomMetaData)
...
  }
}

however we kept getting the error atom denom not found.

The GetDenomMetaData should take the Base denom string (uatom) in this case and retrieve it using the bank keeper as follows:

// Cosmos SDK v0.46.x

// GetDenomMetaData retrieves the denomination metadata. returns the metadata and true if the denom exists,
// false otherwise.
func (k BaseKeeper) GetDenomMetaData(ctx sdk.Context, denom string) (types.Metadata, bool) {
	store := ctx.KVStore(k.storeKey)
	store = prefix.NewStore(store, types.DenomMetadataPrefix)

	bz := store.Get(conv.UnsafeStrToBytes(denom))
	if bz == nil {
		return types.Metadata{}, false
	}

	var metadata types.Metadata
	k.cdc.MustUnmarshal(bz, &metadata)

	return metadata, true
}

As you can see here the store is created using types.DenomMetadataPrefix and then denom is used as the key to retrieve the metadata. HOWEVER, this is not how the bank keeper originally stored this metadata in v0.45 of the Cosmos SDK. If we jump into SetDenomMetadata of the bank keeper of Cosmos SDK v0.45 we will see a different schema for storing the Metadata:

// Cosmos SDK v0.45.x

// SetDenomMetaData sets the denominations metadata
func (k BaseKeeper) SetDenomMetaData(ctx sdk.Context, denomMetaData types.Metadata) {
	store := ctx.KVStore(k.storeKey)
	denomMetaDataStore := prefix.NewStore(store, types.DenomMetadataKey(denomMetaData.Base))

	m := k.cdc.MustMarshal(&denomMetaData)
	denomMetaDataStore.Set([]byte(denomMetaData.Base), m)
}

Here we can see that the store is created using a different version of the types.DenomMetadataKey, one that takes Base as a parameter and subsequently appends with the Base string:

// Cosmos SDK v0.45.x

// KVStore keys
var (
	BalancesPrefix      = []byte("balances")
	SupplyKey           = []byte{0x00}
	DenomMetadataPrefix = []byte{0x1}
)

// DenomMetadataKey returns the denomination metadata key.
func DenomMetadataKey(denom string) []byte {
	d := []byte(denom)
	return append(DenomMetadataPrefix, d...)
}

As opposed to Cosmos SDK v0.46.x where there is no use of DenomMetadataKey.

// Cosmos SDK v0.46.x

// SetDenomMetaData sets the denominations metadata
func (k BaseKeeper) SetDenomMetaData(ctx sdk.Context, denomMetaData types.Metadata) {
	store := ctx.KVStore(k.storeKey)
	denomMetaDataStore := prefix.NewStore(store, types.DenomMetadataPrefix)

	m := k.cdc.MustMarshal(&denomMetaData)
	denomMetaDataStore.Set([]byte(denomMetaData.Base), m)
}

---

TLDR:

Bank module changed the format for the key when storing coin metadata, so without an explicit migration between versions the application was unable to properly retrieve metadata with the old key after an upgrade from v0.45.x to v0.46.x.

Open Question:

We can modify and replace the store used in Gaia v8 with Cosmos SDK v0.46.x so that this is not an issue. But is this the correct way to address the problem? Do modules have their own KV migrations for situations like this or is it the responsibility of the app developer to notice changes like this and ensure upgrade logic is written in app.go to ensure continuity? Are there other apps this could impact or other ways to ensure that other modules haven't befallen a similar fate?

There was another recent issue in gaia that was moved to Cosmos SDK and then moved back to gaia because it was pointed out that it is the responsibility of the application to ensure data integrity. Is this an other instance of such an event and if so how do we make sure that application developers are aware of this kind of breaking change?

I see that the item was included under State Machine Breaking as follows:

• (x/bank) fix!: remove denom from DenomMetadata key #9890 Remove duplicate denom from denom metadata key.
  The other state breaking changes seem to be built with backwards compatibility in mind. Would this have been addressed differently if this outcome had been considered? Should we just try to ensure a message that has an extra emphasis on ensuring your KV store is migrated to the new format during an upgrade? I'm not familiar enough with module development cycles to know where this could have been improved but would like to use it as an example so we can make an improvement, either on the side of app developer due diligence and change log review, or on the sdk side if there was some way to implement this change that wouldn't have resulted in a broken module during an upgrade.

Version

Steps to Reproduce

[okwme]

just discovered this PR #12028 that has migration logic for staking module. Would protocol normally be to add something like this for bank module when KV store was changed?

[amaury1093]

The migration for key format change is included in the SDK, here:
https://github.com/cosmos/cosmos-sdk/blob/v0.46.0/x/bank/migrations/v046/store.go#L18

I think you need to call your keepers.BankKeeper.GetDenomMetaData function after the bank migration above has run. My hypothesis is that you're calling keepers.BankKeeper.GetDenomMetaData before the store migration, so it uses new code (new key format) to query old db (old key format). I would suggest to run all store migrations first, then update the metadata.

@okwme if you have a gaia branch with this error, I can try to help there

[okwme]

Thank you @AmauryM ! you're right that there is in fact a migration for bank that I missed somehow. You are also correct that we were attempting to call GetDenomMetaData before the migration had taken place (although just the wrong order within the handler).

However after digging a bit deeper I believe that the bug DOES in fact exist despite the fact that it shouldn't have been visible from the current configuration (WHAT ARE THE CHANCES ACTUALLY??). After re-ordering the upgrade handler so that the migration occurred before we attempted to retrieve from the newly migrated store, it still fails with the same error! When iterating over the keys and value that are in the store, instead of uatomuatom as before it just shows uatomu which demonstrates it is the wrong length. The addition of one is from the incorrect expectation that the store prefix (0x1) should be accounted for.

I opened a PR with a fix:
#13813

It's very unlikely that there would be 2 bugs that resulted in the same error happening simultaneously. But does that seem like the correct assessment to you?

I'm working off of this branch https://github.com/cosmos/gaia/tree/okwme/test-upgrade-debug. To see the bug in action do the following steps:

• build gaia v7 and move the built binary into ./build/gaiad7
• build gaia v8 with make build so it is built insde build as ./build/gaiad
• inside of one terminal run ./run-gaiad-v7.sh
• as soon as it produces the first block, in another terminal run ./run-upgrade-commands.sh 30 200
• this will make a gov proposal and vote on it to perform an upgrade at height 30
• once the first terminal reaches 30 and stops, kill all actions in both terminals
• then run ./run-gaiad-v8.sh to start the new binary and run the migration / upgrade and see the error from the sdk's bank migration