Regen network/in place migrations

[ethanfrey]

I moved this code to regen-network/old-in-place-migrations and pushing new code based on #2 to this branch (to keep the name).

Idea is to create a unit test scenario for testing inplace migrations

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

• Wrote tests

• Updated relevant documentation (docs/)

• Added entries in PENDING.md with issue #

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[ethanfrey]

To see the loading panic (with unknown store keys):

git checkout 4d1315184e3160252da7c470d088e4a0a5ea63ea
go test ./simapp/ -run TestInplace -v

Output...

=== RUN   TestInplaceMigrationFrom034
cInfo.Version: 525653
  acc: (525653) 66DD52E5B9FFB8222D00523E63C790EC3681E94E5284DB9BCB9C0434B66B6EE4
  fee: (525653) 3F16AB65DC47F2A686D615491152A51C3513D09A8E3D4A445C069D67B7DC7DF7
  geo: (525653) 
  upgrade: (525653) 
  gov: (525653) 47D9B06442E048391331C8DDEFC8D655E37B2B57B2F2E4E0034E735D3A1FEFE9
  mint: (525653) FBED198296762CBB295BE0856EC42CF6CEA71BCF5BF18119E19A0D92537DE94D
  slashing: (525653) 8E0956850D907CC773D8AE9DCA0705F88ABB381DE3CFF97FF9168AD9A8833E77
  main: (525653) 2E57EF93607105F42E543C7609C7D321A303A03E98E88D26F84DD529D391D160
  distr: (525653) B6893F64AC2553C56567967FAF97DADC96A6D5ECEC16F08EAAB5117297E91ED1
  staking: (525653) FE3FD34BD458E81E57623ECC1C26B8857CC7E24BD7E7363844C61DC09C42C28C
  params: (525653) 539933C1E216269117E887F15CF7C913DC671158B9CD6EC5E57E5866775748C9
--- FAIL: TestInplaceMigrationFrom034 (0.13s)
panic: Unknown name fee [recovered]
        panic: Unknown name fee

goroutine 42 [running]:
testing.tRunner.func1(0xc00015d800)
        /usr/lib/go-1.12/src/testing/testing.go:830 +0x388
panic(0xf4bc00, 0xc0000fddc0)
        /usr/lib/go-1.12/src/runtime/panic.go:522 +0x1b5
github.com/cosmos/cosmos-sdk/store/rootmulti.(*Store).nameToKey(0xc0001c0620, 0xc000479973, 0x3, 0xc000a71330, 0xc0000fd8a0)
        /home/ethan/go/src/github.com/cosmos/cosmos-sdk/store/rootmulti/store.go:422 +0x177
github.com/cosmos/cosmos-sdk/store/rootmulti.(*Store).LoadVersion(0xc0001c0620, 0x80555, 0x80555, 0xc0004bd0e0)
        /home/ethan/go/src/github.com/cosmos/cosmos-sdk/store/rootmulti/store.go:135 +0x751
github.com/cosmos/cosmos-sdk/store/rootmulti.(*Store).LoadLatestVersion(0xc0001c0620, 0xfdbba0, 0xc00097cc01)
        /home/ethan/go/src/github.com/cosmos/cosmos-sdk/store/rootmulti/store.go:100 +0x4f
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).LoadLatestVersion(0xc0000d8900, 0xc0000fd890, 0x12d7678, 0xc0001c0310)
        /home/ethan/go/src/github.com/cosmos/cosmos-sdk/baseapp/baseapp.go:178 +0x34
github.com/cosmos/cosmos-sdk/simapp.NewSimApp(0x147b0c0, 0xc0000fcc90, 0x148cf80, 0xc00047c1c0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, ...)
        /home/ethan/go/src/github.com/cosmos/cosmos-sdk/simapp/app.go:224 +0x4ab5
github.com/cosmos/cosmos-sdk/simapp.TestInplaceMigrationFrom034(0xc00015d800)
        /home/ethan/go/src/github.com/cosmos/cosmos-sdk/simapp/inplace_test.go:24 +0x1c6
testing.tRunner(0xc00015d800, 0x12d7590)
        /usr/lib/go-1.12/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
        /usr/lib/go-1.12/src/testing/testing.go:916 +0x357
FAIL    github.com/cosmos/cosmos-sdk/simapp     0.165s

[aaronc]

So here are my thoughts:

There should be a method on rootmulti.Store called LoadLatestVersionAndUpgrade which includes a parameter StoreUpgrades which is something like:

type StoreUpgrades struct {
  New []StoreKey
  Renamed []StoreRename
  Deleted []StoreKey
}

type StoreRename struct {
  OldKey StoreKey
  NewKey StoreKey
}

The upgrade keeper gets these changes:

• BaseApp gets a reference to upgrade.Keeper and uses it in LoadLatestVersion
• when an upgrade is needed and before crashing, the upgrade keeper writes a file $APP_HOME/data/upgrade-info.json which includes the upgrade info (including name and block height)
• SetUpgradeHandler takes another param StoreUpgrades
• when the new binary starts and LoadLatestVersion is called, the upgrade keeper gets called to check for data/upgrade-info.json, if it exists and a handler for this upgrade exists, delete the file, and call LoadLatestVersionAndUpgrade and the StoreUpgrades setup in the upgrade handler

The reason for doing the upgrades in the new binary I can think of are:

• If store upgrades are done in the previous version or by some special CLI command this either creates a new version which messes up block height, or messes up the last version of the db which is from the previous block. Thus, it makes sense that store upgrades are part of the new version that gets committed in the first block of the new binary
• Minimizing operator overhead. The new binary should have full knowledge of what it needs to do to upgrade and should be able to detect whether and which upgrade is needed using only on disk state (either the db or this special $APP_HOME/data/upgrade-info.json file), without the operator needing to do to many special procedures that differ from upgrade to upgrade

How does that sound?

[ethanfrey]

Sounds good. I wonder if the cosmos team will approve this change to BaseApp, but since the framework isn't configurable enough to allow these changes without modifying the code, and this feature would be an essential piece of future apps, I think this should be fine.

I would accept a nil upgrade.Keeper in the constructor (actually just refer to an interface with that one method), and do the normal load if not present, so apps not using the upgrade module can work as normal. We just add an optional hook for this, and don't directly import the module in BaseApp either. This would be minimally invasive to others building on the sdk and should be acceptable.

[aaronc]

Exactly. It doesn't even need to be a constructor param, it can be SetUpgradeKeeper.

…

On Mon, Jul 15, 2019, 04:56 Ethan Frey ***@***.***> wrote: Sounds good. I wonder if the cosmos team will approve this change to BaseApp, but since the framework isn't configurable enough to allow these changes without modifying the code, and this feature would be an essential piece of future apps, I think this should be fine. I would accept a nil upgrade.Keeper in the constructor (actually just refer to an interface with that one method), and do the normal load if not present, so apps not using the upgrade module can work as normal. We just add an optional hook for this, and don't directly import the module in BaseApp either. This would be minimally invasive to others building on the sdk and should be acceptable. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1?email_source=notifications&email_token=AAAL6FVXSRQZ42BI5TYF55TP7Q3TPA5CNFSM4ICOTOIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ5CYNY#issuecomment-511323191>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAL6FXXCIQZ2PLCALBYLBTP7Q3TPANCNFSM4ICOTOIA> .

[ethanfrey]

I will make those changes in a different PR, rebased on the upgrade handler one

[ethanfrey]

This PR seems to contain quite important migrations (for supply):
cosmos#4686

[ethanfrey]

Implementation of LoadLatestVersionAndUpgrade along with the BaseApp changes are not ready for review in cosmos#4724

[ethanfrey]

I would consider making a new "in-place migrations" branch based on top of cosmos#4724 to replace this one. Hopefully, that can pass basic tests.

[ethanfrey]

I just added a db dump in last commit (which we don't want to merge... too big).

Basic tests pass now, and nice example to set up using upgrades in the SimApp