Regen network/multistore upgrades #5500

anilcse · 2020-01-09T16:37:35Z

Closes: regen-network#7

Description

For contributor use:

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…ork/multistore-upgrades

lgtm-com · 2020-01-09T16:49:56Z

This pull request introduces 1 alert when merging 62ff38f into 869531c - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

…ork/multistore-upgrades

x/upgrade/internal/types/storeloader.go

x/upgrade/internal/types/storeloader_test.go

x/upgrade/abci_test.go

x/upgrade/internal/types/storeloader.go

x/upgrade/internal/keeper/keeper.go

lgtm-com · 2020-01-10T07:29:25Z

This pull request introduces 1 alert when merging 3a71d54 into 3df3887 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

…network/cosmos-sdk into sahith/multistore

Sahith/multistore

lgtm-com · 2020-01-13T19:39:15Z

This pull request introduces 1 alert when merging 28f7bca into d0a5b9b - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

ethanfrey

A number of minor cleanup comments. However, two major design questions:

I see the two loaders: StoreLoaderWithUpgrade and UpgradeableStoreLoader. And a bit confused about usage. UpgradeableStoreLoader requires loading the migration paths from disks, which I believe is now a deprecated approach. StoreLoaderWithUpgrade seems to do what the PR requests, but is marked as only for testing. The usage should be clarified.

The other issue I have is how the upgrade handler is supposed to set this. I think @aaronc needs to answer this (ideally showing a code sample that compiles). From the issue:

Change the upgrade.Keeper SetUpgradeHandlermethod to:SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandler, storeUpgrades []storytypes.StoreUpgrades)`
so that whenever store key renames/deletions are needed they can be registered with the upgrade handler

Add a hook to the upgrade module that configures BaseApp and the multi-store before starting the ABCI app. The multi-store upgrades should only be performed when there is an upgrade-info.json present and the version of the store matches the upgrade height in the json file. This will prevent store upgrades from happening too early.

Clearly the argument is still not added to the SetHandler method. However, I see no clean way to add such a callback. I doubt BaseApp is ever supposed to be passed into a module, and not sure about import cycles and such. Also, the issue is that this info must be available without any calls to the KVStore. (Thus the original design to load from json file). If there is zero or one SetHandler registered, then we can use the data (or nil) as the pending store upgrades. And expose some call GetPendingStoreUpgrades that just inspects the map set up by SetHandler. BUT, if there are multiple SetHandler calls in the app, we cannot check the db to see which is pending to properly implement this.

I would recommend NOT doing this in SetHandler, and just adding this directly in app.go. In fact app.go must be updated for each upgrade, and is in the new binary, so it can store this info.

So, when preparing for an upgrade, we would have two lines:

# prepare the next update upgrade handler
upgradeKeeper.SetHandler("next-update", migrationFunc)
# only if we want to do upgrades in next update 
storeLoader := upgrade.StoreLoaderWithUpgrade(storeUpgradesForNextUpdate)
app.SetStoreLoader(storeLoader)

ethanfrey · 2020-01-29T10:53:07Z

simapp/app.go

@@ -196,7 +196,7 @@ func NewSimApp(
 	app.CrisisKeeper = crisis.NewKeeper(
 		app.subspaces[crisis.ModuleName], invCheckPeriod, app.SupplyKeeper, auth.FeeCollectorName,
 	)
-	app.UpgradeKeeper = upgrade.NewKeeper(skipUpgradeHeights, keys[upgrade.StoreKey], app.cdc)
+	app.UpgradeKeeper = upgrade.NewKeeper(skipUpgradeHeights, keys[upgrade.StoreKey], app.cdc, DefaultNodeHome)


This is just the default home. What happens when running with a different home flag (--home=/foo/bar)? I think you need to read the home path with viper, like I did in wasmd: https://github.com/cosmwasm/wasmd/blob/master/app/app.go#L213-L215

If you have a better way to get this info, let me know and we can fix both binaries. But this was the best way I could think of to get the home dir dynamically

@ethanfrey that is up to the specific application. You can see how that is handled in gaia

Actually the whole home processing stuff is burried in the client libs in cosmos-sdk not in the app. The default is set by app, but how we override with --home or --home-client is standardized

ethanfrey · 2020-01-29T10:54:58Z

x/upgrade/internal/keeper/keeper.go

+		return err
+	}
+
+	var upgradeInfo store.UpgradeInfo


So UpgradeInfo includes StoreUpgrades. Where do we set those? I never see those set by searching the diff (but maybe I missed it)

The idea I had was for just Plan to get serialized to json. I think that's likely how it should work. The store upgrades wouldn't get serialized to disk here.

x/upgrade/internal/keeper/keeper.go

ethanfrey · 2020-01-29T10:58:44Z

x/upgrade/internal/types/storeloader.go

+
+// StoreLoaderWithUpgrade is used to prepare baseapp with a fixed StoreLoader
+// pattern. This is useful in test cases, or with custom upgrade loading logic.
+func StoreLoaderWithUpgrade(storeUpgrades *storetypes.StoreUpgrades) baseapp.StoreLoader {


If this is called in app.go (which I think it is), then we should do the viper stuff there, and just pass this as a second argument. You need to update app.go to use viper.GetString(flags.FlagHome) instead of DefaultNodeHome when creating the upgrade keeper as well, so let's just use that value and pass it in here as well.

keeping viper calls out of the modules and pushing them up into the cobra cli code is preferred.

ethanfrey · 2020-01-29T11:03:52Z

x/upgrade/internal/types/storeloader.go

+		}
+
+		// Check if the current commit version and upgrade height matches
+		if (len(upgrades.StoreUpgrades.Renamed) > 0 || len(upgrades.StoreUpgrades.Deleted) > 0) &&


This looks like similar logic (but cleaned up) from the original upgrade handler.

Big issue here, is that we now never write the store upgrades from the pre-upgrade binary (design was that they would be in the proposal), but use them from the post-upgrade binary (written in code). I understand the reason behind this change, however, here we check value from file.

I would actually remove the StoreUpgrades field form UpgradeInfo as it is confusing and not used elsewhere in the code (I think). And you should be checking eg. len(storeUpgrades.Renamed) here. I see you do use the proper storeUpgrades as an argument to ms.LoadLatestVersionAndUpgrade, so it seems that you just missed to upgrade the if statement.

Yeah,sorry missed this. Changed in next commit

ethanfrey · 2020-01-29T11:05:10Z

x/upgrade/internal/types/storeloader.go

+//
+// This is useful for in place migrations when a store key is renamed between
+// two versions of the software.
+func UpgradeableStoreLoader(upgradeInfoPath string) baseapp.StoreLoader {


This is an implementation of the older technique, right?
The code looks correct to me, but I am a bit confused when and where we would use one or the other.

This was to address store changes saved to disk, but as that is deprecated I'll remove the one which takes path

ethanfrey · 2020-01-29T11:11:00Z

x/upgrade/internal/types/storeloader_test.go

+
+	upgradeInfoFilePath := filepath.Join(homeDir, "upgrade-info.json")
+
+	data := []byte(`{"height": 0, "store_upgrades": {"renamed":[{"old_key": "bnk", "new_key": "bank"}]}}`)


I assume to use a non-zero height? We want to use the same height on BeginBlock, right? They are supposed to check equality.

If this is eg. height=2, then we mount as below and show that no migrations happen (even with the migration handler installed). Then commit eg. 2 blocks, so the iavl tree is now stored at height 2. Then mount the store again (with committed height =2) and validate the stores were renamed properly.

A bit more complicated, but important to ensure the checks for "only at height" work out properly

Yes, will clean this up

aaronc · 2020-01-29T19:28:15Z

x/upgrade/internal/keeper/keeper.go

+		return err
+	}
+
+	var upgradeInfo store.UpgradeInfo


The idea I had was for just Plan to get serialized to json. I think that's likely how it should work. The store upgrades wouldn't get serialized to disk here.

aaronc · 2020-01-30T00:53:26Z

So, when preparing for an upgrade, we would have two lines:

# prepare the next update upgrade handler
upgradeKeeper.SetHandler("next-update", migrationFunc)
# only if we want to do upgrades in next update 
storeLoader := upgrade.StoreLoaderWithUpgrade(storeUpgradesForNextUpdate)
app.SetStoreLoader(storeLoader)

Since this is a pretty infrequent operation, I think something that puts the burden in app.go is okay. Basically somewhere, maybe even just in app.go there needs to be some logic that:

reads upgrade height and upgrade name from upgrade-info.json
checks if there are are store upgrades for this upgrade name
checks if the current store version is equal to the upgrade height in upgrade-info.json
if so, apply the store upgrades in initialization

So this could be as simple as something like:

upgradeName, upgradeHeight := upgradeKeeper.ReadUpgradeInfoFromDisk()
if upgradeName == "myUpgrade" {
  // configure store loader that checks if version == upgradeHeight and applies store upgrades
  app.SetStoreLoader(UpgradeStoreLoader(upgradeHeight, storeUpgrades)
}

This could also be more self-contained if we added a store upgrades parameter to SetHandler:

upgradeKeeper.SetHandler("next-update", migrationFunc, storeUpgrades)
app.SetStoreLoader(upgradeKeeper.UpgradeStoreLoader())

Happy to get on a quick call to talk this through if that would help.

…ork/multistore-upgrades

lgtm-com · 2020-01-31T11:27:25Z

This pull request introduces 1 alert when merging 3cc4be0 into 6890feb - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

aaronc

utACK

anilcse · 2020-02-21T03:52:39Z

@alexanderbez can you check this PR

alexanderbez

Looks good @anilcse! I left some minor feedback on the PR. Take a look :)

alexanderbez · 2020-02-21T10:10:10Z

simapp/app.go

@@ -189,7 +192,7 @@ func NewSimApp(
 	app.CrisisKeeper = crisis.NewKeeper(
 		app.subspaces[crisis.ModuleName], invCheckPeriod, app.SupplyKeeper, auth.FeeCollectorName,
 	)
-	app.UpgradeKeeper = upgrade.NewKeeper(skipUpgradeHeights, keys[upgrade.StoreKey], app.cdc)
+	app.UpgradeKeeper = upgrade.NewKeeper(skipUpgradeHeights, keys[upgrade.StoreKey], app.cdc, viper.GetString(flags.FlagHome))


The viper dep should not be used here. We need to figure out a way to provide the home value app constructor instead.

Something we need to address in another PR is really cleaning up the app constructor. We should use variadic option functions maybe.

This might help: #5548

Yes, we should address that issue. But I would suggest doing it separately to keep this PR as lean as possible. For the time being, simply have the SimApp constructor take a new parameter or even introduce variadic options.

alexanderbez · 2020-02-21T10:10:53Z

store/types/store.go

@@ -44,6 +44,13 @@ type StoreUpgrades struct {
 	Deleted []string      `json:"deleted"`
 }

+// UpgradeInfo defines height and name of the upgrade
+// to ensure multistore upgrades happens only at matching height


Suggested change

// to ensure multistore upgrades happens only at matching height

// to ensure multistore upgrades happen only at matching height.

alexanderbez · 2020-02-21T10:27:59Z

x/upgrade/abci.go

+			// Write upgrade info to disk
+			// UpgradeStoreLoader uses this info to perform or skip store migrations


// Write the upgrade info to disk. The UpgradeStoreLoader uses this info to perform or skip // store migrations.

x/upgrade/abci_test.go

alexanderbez · 2020-02-21T10:30:23Z

x/upgrade/internal/keeper/keeper.go

@@ -130,3 +140,66 @@ func (k Keeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) {
 func (k Keeper) IsSkipHeight(height int64) bool {
 	return k.skipUpgradeHeights[height]
 }
+
+// DumpUpgradeInfoToDisk adds plan height to upgrade-info.json


Suggested change

// DumpUpgradeInfoToDisk adds plan height to upgrade-info.json

// DumpUpgradeInfoToDisk writes upgrade information to UpgradeInfoFileName.

alexanderbez · 2020-02-21T10:32:02Z

x/upgrade/internal/keeper/keeper.go

+func (k Keeper) GetUpgradeInfoPath() (string, error) {
+	upgradeInfoFileDir := path.Join(k.GetHomeDir(), "data")
+
+	if _, err := os.Stat(upgradeInfoFileDir); os.IsNotExist(err) {


import tmos "github.com/tendermint/tendermint/libs/os"

Use tmos.EnsureDir here instead of duplicating code 👍

alexanderbez · 2020-02-21T10:32:27Z

x/upgrade/internal/keeper/keeper.go

+}
+
+// GetHomeDir returns the height at which the given upgrade was executed
+func (k Keeper) GetHomeDir() string {


Does this need to be exposed/used anywhere outside the keeper?

Not really. I will change that

alexanderbez · 2020-02-21T10:34:35Z

x/upgrade/internal/keeper/keeper.go

+
+	var upgradeInfo store.UpgradeInfo
+	json.Unmarshal(data, &upgradeInfo)
+	return upgradeInfo.Name, upgradeInfo.Height


You're returning all the values from the UpgradeInfo type. You might as well just return the entire UpgradeInfo struct which will cleanup the method signature 👍

Updated the required changes

x/upgrade/spec/01_concepts.md

alexanderbez · 2020-02-21T10:47:25Z

x/upgrade/spec/01_concepts.md

@@ -56,6 +56,17 @@ During each `EndBlock` execution, the `x/upgrade` module checks if there exists
 `Handler` is executed. If the `Plan` is expected to execute but no `Handler` is registered
 or if the binary was upgraded too early, the node will gracefully panic and exit.

+## StoreLoader
+The `x/upgrade` module also facilitates store migrations as part of the upgrade. The


I think we certainly need way more detail and explanation here. This is a core component of the upgrade procedure. I'd elaborate and explain concepts and procedures such as when and how a file is written to disk. What the file contains and why. How the file is used to migrate state....etc.

Fix review updates

…network/cosmos-sdk into regen-network/multistore-upgrades

…ork/multistore-upgrades # Conflicts: # simapp/app.go # x/upgrade/keeper/keeper.go

alexanderbez

ACK -- just need to resolve the merge conflict ;)

…ork/multistore-upgrades # Conflicts: # x/upgrade/keeper/keeper.go

…network/cosmos-sdk into regen-network/multistore-upgrades

anilcse · 2020-02-27T17:33:20Z

ACK -- just need to resolve the merge conflict ;)

Updated

anilcse and others added 11 commits December 28, 2019 00:26

Remove multistore upgrades from baseapp

3815c2c

Add upgrade storeloaders in x/upgrade

2b09687

Write upgrade height before the old binary panics

78819e2

Add multistore upgrade alias

d7bd14e

Fix gofmt

0ce25e5

Modified storeLoader to check height

237454c

Rebase with upstream/master

ccefbd6

Add tests for dumping upgrade-info to filesystem

c9a01e0

Fix storeloader tests

7f0dd8b

Fix imports

faf25f1

Merge branch 'master' of github.com:cosmos/cosmos-sdk into regen-netw…

62ff38f

…ork/multistore-upgrades

Merge branch 'master' of github.com:cosmos/cosmos-sdk into regen-netw…

6b6456b

…ork/multistore-upgrades

golangcibot reviewed Jan 10, 2020

View reviewed changes

Fix imports

3a71d54

Sahith Reddy Narahari and others added 5 commits January 10, 2020 19:32

Added home path to keeper

1971c7e

Performed code cleanup

82daafb

Resolved conflicts

a82fcb5

Merge branch 'regen-network/multistore-upgrades' of github.com:regen-…

5625f23

…network/cosmos-sdk into sahith/multistore

Added home path to keeper

28f7bca

Sahith/multistore

ethanfrey reviewed Jan 29, 2020

View reviewed changes

aaronc requested changes Jan 30, 2020

View reviewed changes

Sahith Reddy Narahari and others added 3 commits January 30, 2020 19:53

Removed store loader from disk

f8841b5

Merge branch 'master' of github.com:cosmos/cosmos-sdk into regen-netw…

3cc4be0

…ork/multistore-upgrades

revert unexpected change

bcdea87

Refactor upgrade storeloader declarations

ccdc853

anilcse added 5 commits February 5, 2020 18:09

Refactor and update store migrations usage doc

1e048d2

Update godoc

5e3d8e8

Fix godoc

a3b2c3e

Update upgrade spec with StoreLoader

19d1268

Update upgrade spec doc

c0a5bf0

aaronc approved these changes Feb 5, 2020

View reviewed changes

Merge branch 'master' into regen-network/multistore-upgrades

a20eb9a

alexanderbez added R4R C:x/upgrade labels Feb 21, 2020

alexanderbez suggested changes Feb 21, 2020

View reviewed changes

anilcse added 8 commits February 22, 2020 02:55

Fix review updates

daa8f4a

Fix fmt

05d35d8

Merge pull request #28 from regen-network/anil/multistore-upgrades

5500774

Fix review updates

Update upgrade concepts doc

9917cfd

Merge branch 'regen-network/multistore-upgrades' of github.com:regen-…

c268fec

…network/cosmos-sdk into regen-network/multistore-upgrades

Update simapp constructor to accept homePath

2d0bb5a

Merge branch 'master' of github.com:cosmos/cosmos-sdk into regen-netw…

c42582a

…ork/multistore-upgrades # Conflicts: # simapp/app.go # x/upgrade/keeper/keeper.go

Merge branch 'master' into regen-network/multistore-upgrades

175190c

anilcse requested a review from alexanderbez February 27, 2020 08:10

Update x/upgrade/spec/01_concepts.md

0959685

alexanderbez approved these changes Feb 27, 2020

View reviewed changes

anilcse added 2 commits February 27, 2020 23:01

Merge branch 'master' of github.com:cosmos/cosmos-sdk into regen-netw…

6120306

…ork/multistore-upgrades # Conflicts: # x/upgrade/keeper/keeper.go

Merge branch 'regen-network/multistore-upgrades' of github.com:regen-…

a8a4dbf

…network/cosmos-sdk into regen-network/multistore-upgrades

anilcse requested a review from alexanderbez February 27, 2020 19:04

Merge branch 'master' into regen-network/multistore-upgrades

1e22e61

alexanderbez merged commit 0b74491 into cosmos:master Feb 28, 2020

ryanchristo deleted the regen-network/multistore-upgrades branch December 12, 2022 18:16

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#5500: Regen network/multistore upgrades

bc5b8f9


		upgradeInfoFilePath := filepath.Join(homeDir, "upgrade-info.json")

		data := []byte(`{"height": 0, "store_upgrades": {"renamed":[{"old_key": "bnk", "new_key": "bank"}]}}`)

	// to ensure multistore upgrades happens only at matching height
	// to ensure multistore upgrades happen only at matching height.

		// Write upgrade info to disk
		// UpgradeStoreLoader uses this info to perform or skip store migrations

	// DumpUpgradeInfoToDisk adds plan height to upgrade-info.json
	// DumpUpgradeInfoToDisk writes upgrade information to UpgradeInfoFileName.

Regen network/multistore upgrades #5500

Regen network/multistore upgrades #5500

Conversation

anilcse commented Jan 9, 2020 • edited by alexanderbez Loading

Description

lgtm-com bot commented Jan 9, 2020

lgtm-com bot commented Jan 10, 2020

lgtm-com bot commented Jan 13, 2020

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Jan 30, 2020

lgtm-com bot commented Jan 31, 2020

aaronc left a comment

Choose a reason for hiding this comment

anilcse commented Feb 21, 2020

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

anilcse commented Feb 27, 2020

anilcse commented Jan 9, 2020 •

edited by alexanderbez

Loading