Problem: iavl stores end up with inconsistent versions after upgrade

[yihuang]

Summary of Bug

We found a app hash mismatch in our testnet which upgrade to sdk 0.46 recently, we traced to a inconsistent iavl tree version number in a store that is added in the upgrade.

The hypothesis for how this happens is like this, the migration is slow, so some nodes manually killed the node during the commit event, which will trigger following bug:

• first startup
  • handle store upgrades, there's a newly added feeibc store, set the initialVersion to latestVersion+1.
• first commit
  • commit feeibc at initialVersion
  • kill node before flush metadata.
• second startup, replay the same block
  • the commit info for feeibc don't exists, so call LoadVersion with target version 0, which will load whatever latest version of the iavl tree, which is latestVersion+1.
• second commit
  • feeibc store is commited normally, with version increased to latestVersion + 2, while the other stores is at latestVersion + 1.

Then as long as feeibc store is empty, the issue get unnoticed, until some day it's been written to and trigger the app hash mismatch.

Version

Steps to Reproduce

[alexanderbez]

@yihuang are the repro steps just based on hypothesis, or are you certain that Commit is abrupted prior to metadata being flushed?

[yihuang]

@yihuang are the repro steps just based on hypothesis, or are you certain that Commit is abrupted prior to metadata being flushed?

The steps are hypothesis, will try to reproduce locally at Monday.
What's observed is the bad node always save feeibc store one version bigger than the other stores, the commit info is like this:

...
feegrant 5504246
feeibc 5504247
feemarket 5504246
...

And the log shows the node is killed several times during the upgrade migration.

[alexanderbez]

Hypothesis makes sense. Unfortunately, there's no direct way to use the same DB batch object when committing all the IAVL stores and the metadata. In other words, it's not atomic. Another reason why I really dislike this multi-logical-db store approach.

Note, the store refactor has an item for using a single logical store.

[yihuang]

@alexanderbez what do you think should be the best short term solution?

• when a new iavl store is found not empty, reject by return error
• when a new iavl store is found not empty, override the version
• make the commit atomic.

[alexanderbez]

when a new iavl store is found not empty, reject by return error

If we return an error, how will the module's store be used successfully? Will the chain operate correctly, specifically for the new module?

when a new iavl store is found not empty, override the version

Seems like the most plausible solution 👍

make the commit atomic

This is the ideal solution, but unfortunately, not possible in the current design. #12986 should solve this by using a single logical SC store, but that is a future upcoming refactor.

[yihuang]

when a new iavl store is found not empty, override the version

Seems like the most plausible solution 👍

I'm not sure how to do this though, do we ignore the loaded roots, and force it to be a empty iavl store, will it work when commit?

[alexanderbez]

No, we just fix the version/height of the faulty module to be correct, in this case +1. This would happen in the root-MS.

[yihuang]

fixed by #13530