From d07b42454167f53f656f1897457dbf8c355662e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= Date: Tue, 5 Apr 2022 18:10:59 +0200 Subject: [PATCH 1/4] imp: ScheduleUpgradeNoHeightValidation for automated unplanned upgrades --- x/upgrade/keeper/keeper.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 52b47755b977..fe754b99c8a9 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -170,14 +170,19 @@ func (k Keeper) getModuleVersion(ctx sdk.Context, name string) (uint64, bool) { // ScheduleUpgrade will also write the upgraded client to the upgraded client path // if an upgraded client is specified in the plan func (k Keeper) ScheduleUpgrade(ctx sdk.Context, plan types.Plan) error { - if err := plan.ValidateBasic(); err != nil { - return err - } - if plan.Height <= ctx.BlockHeight() { return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "upgrade cannot be scheduled in the past") } + return k.ScheduleUpgradeNoHeightCheck(ctx, plan) +} + +// ScheduleUpgradeNoHeightCheck schedules the upgrade without a height check +func (k Keeper) ScheduleUpgradeNoHeightCheck(ctx sdk.Context, plan types.Plan) error { + if err := plan.ValidateBasic(); err != nil { + return sdkerrors.Wrapf(err, "invalid plan") + } + if k.GetDoneHeight(ctx, plan.Name) != 0 { return sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "upgrade with name %s has already been completed", plan.Name) } @@ -364,7 +369,7 @@ func (k Keeper) DumpUpgradeInfoToDisk(height int64, p types.Plan) error { return err } - return os.WriteFile(upgradeInfoFilePath, info, 0600) + return os.WriteFile(upgradeInfoFilePath, info, 0o600) } // GetUpgradeInfoPath returns the upgrade info file path From 26fadcb8b871fbc6272faeede17e7f15d959651d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= Date: Tue, 5 Apr 2022 18:16:11 +0200 Subject: [PATCH 2/4] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79b5c3169f71..e651d5d839a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features +* (x/upgrade) [\#11551](https://github.com/cosmos/cosmos-sdk/pull/11551) Implement `ScheduleUpgradeNoHeightValidation` for chains to perform an automated upgrade without having to go though governance. * (tx) [#\11533](https://github.com/cosmos/cosmos-sdk/pull/11533) Register [`EIP191`](https://eips.ethereum.org/EIPS/eip-191) as an available `SignMode` for chains to use. * (x/genutil) [\#11500](https://github.com/cosmos/cosmos-sdk/pull/11500) Fix GenTx validation and adjust error messages * [\#11430](https://github.com/cosmos/cosmos-sdk/pull/11430) Introduce a new `grpc-only` flag, such that when enabled, will start the node in a query-only mode. Note, gRPC MUST be enabled with this flag. From 9936c02b4b6a45ed94c5b08935992552b0d0ed43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= Date: Wed, 6 Apr 2022 09:55:28 +0200 Subject: [PATCH 3/4] address comments --- CHANGELOG.md | 2 +- x/upgrade/keeper/keeper.go | 19 ++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e651d5d839a6..9eae9779a45c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Features -* (x/upgrade) [\#11551](https://github.com/cosmos/cosmos-sdk/pull/11551) Implement `ScheduleUpgradeNoHeightValidation` for chains to perform an automated upgrade without having to go though governance. +* (x/upgrade) [\#11551](https://github.com/cosmos/cosmos-sdk/pull/11551) Update `ScheduleUpgrade` for chains to schedule an automated upgrade on `BeginBlock` without having to go though governance. * (tx) [#\11533](https://github.com/cosmos/cosmos-sdk/pull/11533) Register [`EIP191`](https://eips.ethereum.org/EIPS/eip-191) as an available `SignMode` for chains to use. * (x/genutil) [\#11500](https://github.com/cosmos/cosmos-sdk/pull/11500) Fix GenTx validation and adjust error messages * [\#11430](https://github.com/cosmos/cosmos-sdk/pull/11430) Introduce a new `grpc-only` flag, such that when enabled, will start the node in a query-only mode. Note, gRPC MUST be enabled with this flag. diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index fe754b99c8a9..a17530d091c7 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -167,20 +167,17 @@ func (k Keeper) getModuleVersion(ctx sdk.Context, name string) (uint64, bool) { // ScheduleUpgrade schedules an upgrade based on the specified plan. // If there is another Plan already scheduled, it will cancel and overwrite it. -// ScheduleUpgrade will also write the upgraded client to the upgraded client path -// if an upgraded client is specified in the plan +// ScheduleUpgrade will also write the upgraded IBC ClientState to the upgraded client +// path if it is specified in the plan. func (k Keeper) ScheduleUpgrade(ctx sdk.Context, plan types.Plan) error { - if plan.Height <= ctx.BlockHeight() { - return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "upgrade cannot be scheduled in the past") + if err := plan.ValidateBasic(); err != nil { + return err } - return k.ScheduleUpgradeNoHeightCheck(ctx, plan) -} - -// ScheduleUpgradeNoHeightCheck schedules the upgrade without a height check -func (k Keeper) ScheduleUpgradeNoHeightCheck(ctx sdk.Context, plan types.Plan) error { - if err := plan.ValidateBasic(); err != nil { - return sdkerrors.Wrapf(err, "invalid plan") + // NOTE: allow for the possibility of chains to schedule upgrades in begin block of the same block + // as a strategy for emergency hard fork recoveries + if plan.Height < ctx.BlockHeight() { + return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "upgrade cannot be scheduled in the past") } if k.GetDoneHeight(ctx, plan.Name) != 0 { From 91df92ca06e9bd3eae9d007bb9596abd2bce3601 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Federico=20Kunze=20K=C3=BCllmer?= Date: Thu, 7 Apr 2022 12:51:57 +0200 Subject: [PATCH 4/4] fix tests --- x/upgrade/abci_test.go | 43 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index c3f1ea6bd1a8..8f3b8f52dbfd 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -34,7 +34,6 @@ type TestSuite struct { var s TestSuite func setupTest(t *testing.T, height int64, skip map[int64]bool) TestSuite { - db := dbm.NewMemDB() app := simapp.NewSimappWithCustomOptions(t, false, simapp.SetupOptions{ Logger: log.NewNopLogger(), @@ -59,14 +58,14 @@ func TestRequireName(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{}}) - require.NotNil(t, err) + require.Error(t, err) require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err) } func TestRequireFutureBlock(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) - err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight()}}) - require.NotNil(t, err) + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight() - 1}}) + require.Error(t, err) require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err) } @@ -74,7 +73,7 @@ func TestDoHeightUpgrade(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) t.Log("Verify can schedule an upgrade") err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight() + 1}}) - require.Nil(t, err) + require.NoError(t, err) VerifyDoUpgrade(t) } @@ -83,9 +82,9 @@ func TestCanOverwriteScheduleUpgrade(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) t.Log("Can overwrite plan") err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "bad_test", Height: s.ctx.BlockHeight() + 10}}) - require.Nil(t, err) + require.NoError(t, err) err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight() + 1}}) - require.Nil(t, err) + require.NoError(t, err) VerifyDoUpgrade(t) } @@ -132,7 +131,7 @@ func TestHaltIfTooNew(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) t.Log("Verify that we don't panic with registered plan not in database at all") var called int - s.keeper.SetUpgradeHandler("future", func(ctx sdk.Context, plan types.Plan, vm module.VersionMap) (module.VersionMap, error) { + s.keeper.SetUpgradeHandler("future", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { called++ return vm, nil }) @@ -175,10 +174,10 @@ func TestCanClear(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) t.Log("Verify upgrade is scheduled") err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight() + 100}}) - require.Nil(t, err) + require.NoError(t, err) err = s.handler(s.ctx, &types.CancelSoftwareUpgradeProposal{Title: "cancel"}) - require.Nil(t, err) + require.NoError(t, err) VerifyCleared(t, s.ctx) } @@ -187,11 +186,11 @@ func TestCantApplySameUpgradeTwice(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) height := s.ctx.BlockHeader().Height + 1 err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: height}}) - require.Nil(t, err) + require.NoError(t, err) VerifyDoUpgrade(t) t.Log("Verify an executed upgrade \"test\" can't be rescheduled") err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: height}}) - require.NotNil(t, err) + require.Error(t, err) require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err) } @@ -237,9 +236,7 @@ func VerifySet(t *testing.T, skipUpgradeHeights map[int64]bool) { } func TestContains(t *testing.T) { - var ( - skipOne int64 = 11 - ) + var skipOne int64 = 11 s := setupTest(t, 10, map[int64]bool{skipOne: true}) VerifySet(t, map[int64]bool{skipOne: true}) @@ -298,7 +295,7 @@ func TestUpgradeSkippingOne(t *testing.T) { req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: skipOne}}) - require.Nil(t, err) + require.NoError(t, err) t.Log("Verify if skip upgrade flag clears upgrade plan in one case and does upgrade on another") VerifySet(t, map[int64]bool{skipOne: true}) @@ -311,7 +308,7 @@ func TestUpgradeSkippingOne(t *testing.T) { t.Log("Verify the second proposal is not skipped") err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop2", Plan: types.Plan{Name: "test2", Height: skipTwo}}) - require.Nil(t, err) + require.NoError(t, err) // Setting block height of proposal test2 newCtx = newCtx.WithBlockHeight(skipTwo) VerifyDoUpgradeWithCtx(t, newCtx, "test2") @@ -333,7 +330,7 @@ func TestUpgradeSkippingOnlyTwo(t *testing.T) { req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: skipOne}}) - require.Nil(t, err) + require.NoError(t, err) t.Log("Verify if skip upgrade flag clears upgrade plan in both cases and does third upgrade") VerifySet(t, map[int64]bool{skipOne: true, skipTwo: true}) @@ -346,7 +343,7 @@ func TestUpgradeSkippingOnlyTwo(t *testing.T) { // A new proposal with height in skipUpgradeHeights err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop2", Plan: types.Plan{Name: "test2", Height: skipTwo}}) - require.Nil(t, err) + require.NoError(t, err) // Setting block height of proposal test2 newCtx = newCtx.WithBlockHeight(skipTwo) require.NotPanics(t, func() { @@ -355,7 +352,7 @@ func TestUpgradeSkippingOnlyTwo(t *testing.T) { t.Log("Verify a new proposal is not skipped") err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop3", Plan: types.Plan{Name: "test3", Height: skipThree}}) - require.Nil(t, err) + require.NoError(t, err) newCtx = newCtx.WithBlockHeight(skipThree) VerifyDoUpgradeWithCtx(t, newCtx, "test3") @@ -370,7 +367,7 @@ func TestUpgradeWithoutSkip(t *testing.T) { newCtx := s.ctx.WithBlockHeight(s.ctx.BlockHeight() + 1).WithBlockTime(time.Now()) req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight() + 1}}) - require.Nil(t, err) + require.NoError(t, err) t.Log("Verify if upgrade happens without skip upgrade") require.Panics(t, func() { s.module.BeginBlock(newCtx, req) @@ -437,7 +434,7 @@ func TestBinaryVersion(t *testing.T) { }) err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}}) - require.Nil(t, err) + require.NoError(t, err) newCtx := s.ctx.WithBlockHeight(12) s.keeper.ApplyUpgrade(newCtx, types.Plan{ @@ -454,7 +451,7 @@ func TestBinaryVersion(t *testing.T) { "test panic: upgrade needed", func() (sdk.Context, abci.RequestBeginBlock) { err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: 13}}) - require.Nil(t, err) + require.NoError(t, err) newCtx := s.ctx.WithBlockHeight(13) req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}