From 7ab1def17b059f9a4f92ffcd0b9905453f7d60a5 Mon Sep 17 00:00:00 2001 From: j75689 Date: Tue, 21 Mar 2023 09:19:58 +0800 Subject: [PATCH] chore: refine upgrade module for code quality --- x/upgrade/doc.go | 146 ------------------------------ x/upgrade/keeper/keeper.go | 21 +---- x/upgrade/keeper/keeper_option.go | 1 + x/upgrade/keeper/keeper_test.go | 43 --------- x/upgrade/types/keys.go | 10 -- x/upgrade/types/upgrade_config.go | 6 ++ 6 files changed, 8 insertions(+), 219 deletions(-) delete mode 100644 x/upgrade/doc.go diff --git a/x/upgrade/doc.go b/x/upgrade/doc.go deleted file mode 100644 index 5589ac6eda..0000000000 --- a/x/upgrade/doc.go +++ /dev/null @@ -1,146 +0,0 @@ -/* -Package upgrade provides a Cosmos SDK module that can be used for smoothly upgrading a live Cosmos chain to a -new software version. It accomplishes this by providing a BeginBlocker hook that prevents the blockchain state -machine from proceeding once a pre-defined upgrade block height has been reached. The module does not prescribe -anything regarding how governance decides to do an upgrade, but just the mechanism for coordinating the upgrade safely. -Without software support for upgrades, upgrading a live chain is risky because all of the validators need to pause -their state machines at exactly the same point in the process. If this is not done correctly, there can be state -inconsistencies which are hard to recover from. - -# General Workflow - -Let's assume we are running v0.38.0 of our software in our testnet and want to upgrade to v0.40.0. -How would this look in practice? First of all, we want to finalize the v0.40.0 release candidate -and there install a specially named upgrade handler (eg. "testnet-v2" or even "v0.40.0"). An upgrade -handler should be defined in a new version of the software to define what migrations -to run to migrate from the older version of the software. Naturally, this is app-specific rather -than module specific, and must be defined in `app.go`, even if it imports logic from various -modules to perform the actions. You can register them with `upgradeKeeper.SetUpgradeHandler` -during the app initialization (before starting the abci server), and they serve not only to -perform a migration, but also to identify if this is the old or new version (eg. presence of -a handler registered for the named upgrade). - -Once the release candidate along with an appropriate upgrade handler is frozen, -we can have a governance vote to approve this upgrade at some future block height (e.g. 200000). -This is known as an upgrade.Plan. The v0.38.0 code will not know of this handler, but will -continue to run until block 200000, when the plan kicks in at BeginBlock. It will check -for existence of the handler, and finding it missing, know that it is running the obsolete software, -and gracefully exit. - -Generally the application binary will restart on exit, but then will execute this BeginBlocker -again and exit, causing a restart loop. Either the operator can manually install the new software, -or you can make use of an external watcher daemon to possibly download and then switch binaries, -also potentially doing a backup. An example of such a daemon is https://github.com/cosmos/cosmos-sdk/tree/main/cosmovisor -described below under "Automation". - -When the binary restarts with the upgraded version (here v0.40.0), it will detect we have registered the -"testnet-v2" upgrade handler in the code, and realize it is the new version. It then will run the upgrade handler -and *migrate the database in-place*. Once finished, it marks the upgrade as done, and continues processing -the rest of the block as normal. Once 2/3 of the voting power has upgraded, the blockchain will immediately -resume the consensus mechanism. If the majority of operators add a custom `do-upgrade` script, this should -be a matter of minutes and not even require them to be awake at that time. - -# Integrating With An App - -Setup an upgrade Keeper for the app and then define a BeginBlocker that calls the upgrade -keeper's BeginBlocker method: - - func (app *myApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock { - app.upgradeKeeper.BeginBlocker(ctx, req) - return abci.ResponseBeginBlock{} - } - -The app must then integrate the upgrade keeper with its governance module as appropriate. The governance module -should call ScheduleUpgrade to schedule an upgrade and ClearUpgradePlan to cancel a pending upgrade. - -# Performing Upgrades - -Upgrades can be scheduled at a predefined block height. Once this block height is reached, the -existing software will cease to process ABCI messages and a new version with code that handles the upgrade must be deployed. -All upgrades are coordinated by a unique upgrade name that cannot be reused on the same blockchain. In order for the upgrade -module to know that the upgrade has been safely applied, a handler with the name of the upgrade must be installed. -Here is an example handler for an upgrade named "my-fancy-upgrade": - - app.upgradeKeeper.SetUpgradeHandler("my-fancy-upgrade", func(ctx sdk.Context, plan upgrade.Plan) { - // Perform any migrations of the state store needed for this upgrade - }) - -This upgrade handler performs the dual function of alerting the upgrade module that the named upgrade has been applied, -as well as providing the opportunity for the upgraded software to perform any necessary state migrations. Both the halt -(with the old binary) and applying the migration (with the new binary) are enforced in the state machine. Actually -switching the binaries is an ops task and not handled inside the sdk / abci app. - -Here is a sample code to set store migrations with an upgrade: - - // this configures a no-op upgrade handler for the "my-fancy-upgrade" upgrade - app.UpgradeKeeper.SetUpgradeHandler("my-fancy-upgrade", func(ctx sdk.Context, plan upgrade.Plan) { - // upgrade changes here - }) - - upgradeInfo, err := app.UpgradeKeeper.ReadUpgradeInfoFromDisk() - if err != nil { - // handle error - } - - if upgradeInfo.Name == "my-fancy-upgrade" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { - storeUpgrades := store.StoreUpgrades{ - Renamed: []store.StoreRename{{ - OldKey: "foo", - NewKey: "bar", - }}, - Deleted: []string{}, - } - - // configure store loader that checks if version == upgradeHeight and applies store upgrades - app.SetStoreLoader(upgrade.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades)) - } - -# Halt Behavior - -Before halting the ABCI state machine in the BeginBlocker method, the upgrade module will log an error -that looks like: - - UPGRADE "" NEEDED at height : - -where Name are Info are the values of the respective fields on the upgrade Plan. - -To perform the actual halt of the blockchain, the upgrade keeper simply panics which prevents the ABCI state machine -from proceeding but doesn't actually exit the process. Exiting the process can cause issues for other nodes that start -to lose connectivity with the exiting nodes, thus this module prefers to just halt but not exit. - -# Automation and Plan.Info - -We have deprecated calling out to scripts, instead with propose https://github.com/cosmos/cosmos-sdk/tree/main/cosmovisor -as a model for a watcher daemon that can launch simd as a subprocess and then read the upgrade log message -to swap binaries as needed. You can pass in information into Plan.Info according to the format -specified here https://github.com/cosmos/cosmos-sdk/tree/main/cosmovisor/README.md#auto-download . -This will allow a properly configured cosmsod daemon to auto-download new binaries and auto-upgrade. -As noted there, this is intended more for full nodes than validators. - -# Cancelling Upgrades - -There are two ways to cancel a planned upgrade - with on-chain governance or off-chain social consensus. -For the first one, there is a CancelSoftwareUpgrade proposal type, which can be voted on and will -remove the scheduled upgrade plan. Of course this requires that the upgrade was known to be a bad idea -well before the upgrade itself, to allow time for a vote. If you want to allow such a possibility, you -should set the upgrade height to be 2 * (votingperiod + depositperiod) + (safety delta) from the beginning of -the first upgrade proposal. Safety delta is the time available from the success of an upgrade proposal -and the realization it was a bad idea (due to external testing). You can also start a CancelSoftwareUpgrade -proposal while the original SoftwareUpgrade proposal is still being voted upon, as long as the voting -period ends after the SoftwareUpgrade proposal. - -However, let's assume that we don't realize the upgrade has a bug until shortly before it will occur -(or while we try it out - hitting some panic in the migration). It would seem the blockchain is stuck, -but we need to allow an escape for social consensus to overrule the planned upgrade. To do so, there's -a --unsafe-skip-upgrades flag to the start command, which will cause the node to mark the upgrade -as done upon hitting the planned upgrade height(s), without halting and without actually performing a migration. -If over two-thirds run their nodes with this flag on the old binary, it will allow the chain to continue through -the upgrade with a manual override. (This must be well-documented for anyone syncing from genesis later on). - -Example: - - simd start --unsafe-skip-upgrades ... - -NOTE: Here simd is used as an example binary, replace it with original binary -*/ -package upgrade diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index b7d6c74457..8d8d945852 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -27,7 +27,7 @@ type Keeper struct { cdc codec.BinaryCodec // App-wide binary codec upgradeHandlers map[string]types.UpgradeHandler // map of plan name to upgrade handler upgradeInitializer map[string]types.UpgradeInitializer // map of plan name to upgrade initializer - upgradeConfig *types.UpgradeConfig + upgradeConfig *types.UpgradeConfig // upgrade config for upcoming upgrade plan or upgraded plan } // NewKeeper constructs an upgrade Keeper which requires the following arguments: @@ -178,24 +178,6 @@ func (k Keeper) ScheduleUpgrade(ctx sdk.Context, plan types.Plan) error { return nil } -// SetUpgradedClient sets the expected upgraded client for the next version of this chain at the last height the current chain will commit. -func (k Keeper) SetUpgradedClient(ctx sdk.Context, planHeight int64, bz []byte) error { - store := ctx.KVStore(k.storeKey) - store.Set(types.UpgradedClientKey(planHeight), bz) - return nil -} - -// GetUpgradedClient gets the expected upgraded client for the next version of this chain -func (k Keeper) GetUpgradedClient(ctx sdk.Context, height int64) ([]byte, bool) { - store := ctx.KVStore(k.storeKey) - bz := store.Get(types.UpgradedClientKey(height)) - if len(bz) == 0 { - return nil, false - } - - return bz, true -} - // SetUpgradedConsensusState set the expected upgraded consensus state for the next version of this chain // using the last height committed on this chain. func (k Keeper) SetUpgradedConsensusState(ctx sdk.Context, planHeight int64, bz []byte) error { @@ -262,7 +244,6 @@ func (k Keeper) GetDoneHeight(ctx sdk.Context, name string) int64 { func (k Keeper) ClearIBCState(ctx sdk.Context, lastHeight int64) { // delete IBC client and consensus state from store if this is IBC plan store := ctx.KVStore(k.storeKey) - store.Delete(types.UpgradedClientKey(lastHeight)) store.Delete(types.UpgradedConsStateKey(lastHeight)) } diff --git a/x/upgrade/keeper/keeper_option.go b/x/upgrade/keeper/keeper_option.go index 0ea1088e0a..81b876423a 100644 --- a/x/upgrade/keeper/keeper_option.go +++ b/x/upgrade/keeper/keeper_option.go @@ -5,6 +5,7 @@ import ( "github.com/cosmos/cosmos-sdk/x/upgrade/types" ) +// convertUpgradeConfig converts serverconfig.UpgradeConfig to types.UpgradeConfig func convertUpgradeConfig(chainID string, plans []serverconfig.UpgradeConfig) (*types.UpgradeConfig, error) { upgradeConfig := types.NewUpgradeConfig() if chainID == types.MainnetChainID { diff --git a/x/upgrade/keeper/keeper_test.go b/x/upgrade/keeper/keeper_test.go index f4a1c3dbfa..69771f5202 100644 --- a/x/upgrade/keeper/keeper_test.go +++ b/x/upgrade/keeper/keeper_test.go @@ -157,49 +157,6 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() { } } -func (s *KeeperTestSuite) TestSetUpgradedClient() { - cs := []byte("IBC client state") - - cases := []struct { - name string - height int64 - setup func() - exists bool - }{ - { - name: "no upgraded client exists", - height: 10, - setup: func() {}, - exists: false, - }, - { - name: "success", - height: 10, - setup: func() { - s.app.UpgradeKeeper.SetUpgradedClient(s.ctx, 10, cs) - }, - exists: true, - }, - } - - for _, tc := range cases { - // reset suite - s.SetupTest() - - // setup test case - tc.setup() - - gotCs, exists := s.app.UpgradeKeeper.GetUpgradedClient(s.ctx, tc.height) - if tc.exists { - s.Require().Equal(cs, gotCs, "valid case: %s did not retrieve correct client state", tc.name) - s.Require().True(exists, "valid case: %s did not retrieve client state", tc.name) - } else { - s.Require().Nil(gotCs, "invalid case: %s retrieved valid client state", tc.name) - s.Require().False(exists, "invalid case: %s retrieved valid client state", tc.name) - } - } -} - // Tests that the underlying state of x/upgrade is set correctly after // an upgrade. func (s *KeeperTestSuite) TestMigrations() { diff --git a/x/upgrade/types/keys.go b/x/upgrade/types/keys.go index e1a30c77ae..fce5858938 100644 --- a/x/upgrade/types/keys.go +++ b/x/upgrade/types/keys.go @@ -26,20 +26,10 @@ const ( // KeyUpgradedIBCState is the key under which upgraded ibc state is stored in the upgrade store KeyUpgradedIBCState = "upgradedIBCState" - // KeyUpgradedClient is the sub-key under which upgraded client state will be stored - KeyUpgradedClient = "upgradedClient" - // KeyUpgradedConsState is the sub-key under which upgraded consensus state will be stored KeyUpgradedConsState = "upgradedConsState" ) -// UpgradedClientKey is the key under which the upgraded client state is saved -// Connecting IBC chains can verify against the upgraded client in this path before -// upgrading their clients -func UpgradedClientKey(height int64) []byte { - return []byte(fmt.Sprintf("%s/%d/%s", KeyUpgradedIBCState, height, KeyUpgradedClient)) -} - // UpgradedConsStateKey is the key under which the upgraded consensus state is saved // Connecting IBC chains can verify against the upgraded consensus state in this path before // upgrading their clients. diff --git a/x/upgrade/types/upgrade_config.go b/x/upgrade/types/upgrade_config.go index c316df92cc..cedb4dec1f 100644 --- a/x/upgrade/types/upgrade_config.go +++ b/x/upgrade/types/upgrade_config.go @@ -10,9 +10,11 @@ import ( // ) const ( + // EnablePublicDelegationUpgrade is the upgrade name for enabling public delegation EnablePublicDelegationUpgrade = "EnablePublicDelegationUpgrade" ) +// The default upgrade config for networks var ( MainnetChainID = "greenfield_9000-1" MainnetConfig = NewUpgradeConfig() @@ -35,11 +37,13 @@ type key struct { height int64 } +// UpgradeConfig is a list of upgrade plans type UpgradeConfig struct { keys map[string]*key elements map[int64][]*Plan } +// SetPlan sets a new upgrade plan func (c *UpgradeConfig) SetPlan(plan *Plan) *UpgradeConfig { if key, ok := c.keys[plan.Name]; ok { if c.elements[key.height][key.index].Height == plan.Height { @@ -56,6 +60,7 @@ func (c *UpgradeConfig) SetPlan(plan *Plan) *UpgradeConfig { return c } +// Clear removes all upgrade plans at a given height func (c *UpgradeConfig) Clear(height int64) { for _, plan := range c.elements[height] { delete(c.keys, plan.Name) @@ -63,6 +68,7 @@ func (c *UpgradeConfig) Clear(height int64) { c.elements[height] = nil } +// GetPlan returns the upgrade plan at a given height func (c *UpgradeConfig) GetPlan(height int64) []*Plan { plans, exist := c.elements[height] if exist && len(plans) != 0 {