From 0f65ae53fe0ec69e74d0bf36974a4635997e03c2 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Tue, 19 Oct 2021 18:00:19 +0530 Subject: [PATCH 01/17] add handler check in BeginBlock --- x/upgrade/abci.go | 9 +++++++++ x/upgrade/keeper/keeper.go | 13 +++++++++++++ 2 files changed, 22 insertions(+) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index e543bcd41c66..33f7312acdb4 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -22,6 +22,15 @@ import ( // skipUpgradeHeightArray is a set of block heights for which the upgrade must be skipped func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) + + k.IterateDoneUpgrades(ctx, func(name string, height int64) bool { + if !k.HasHandler(name) { + panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", name)) + } + + return false + }) + plan, found := k.GetUpgradePlan(ctx) if !found { return diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 8b667ad3173e..38251ca9bb4c 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -227,6 +227,19 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([] return bz, true } +// IterateDoneUpgrades iterate over all the applied upgrades. +// Callback to get all data, returns true to stop, false to keep reading +func (k Keeper) IterateDoneUpgrades(ctx sdk.Context, cb func(name string, height int64) bool) { + iter := sdk.KVStorePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) + defer iter.Close() + + stop := false + for ; iter.Valid() && !stop; iter.Next() { + bz := iter.Value() + stop = cb(string(iter.Key()), int64(binary.BigEndian.Uint64(bz))) + } +} + // GetDoneHeight returns the height at which the given upgrade was executed func (k Keeper) GetDoneHeight(ctx sdk.Context, name string) int64 { store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) From 70119764eab8c6e2c255b45484dc7c4bd994c477 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 3 Nov 2021 15:25:26 +0530 Subject: [PATCH 02/17] review changes --- x/upgrade/abci.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 33f7312acdb4..9ed3c17609cc 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -23,18 +23,25 @@ import ( func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) - k.IterateDoneUpgrades(ctx, func(name string, height int64) bool { - if !k.HasHandler(name) { - panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", name)) - } - - return false + var recentPlan string + k.IterateDoneUpgrades(ctx, func(name string, _ int64) bool { + recentPlan = name + return true }) plan, found := k.GetUpgradePlan(ctx) if !found { + if recentPlan != "" && !k.HasHandler(recentPlan) { + panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", recentPlan)) + } + return + } else { + if recentPlan != "" && !k.HasHandler(plan.Name) { + panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", recentPlan)) + } } + logger := ctx.Logger() // To make sure clear upgrade is executed at the same block From 9d8e964b2029e75c24d0989875b38c0d71d00158 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 3 Nov 2021 16:16:22 +0530 Subject: [PATCH 03/17] add tests --- x/upgrade/abci_test.go | 48 ++++++++++++++++++++++++++++++++++++++ x/upgrade/keeper/keeper.go | 9 ++++++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 16206447ac53..8e0550c1d90d 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -410,3 +410,51 @@ func TestDumpUpgradeInfoToFile(t *testing.T) { err = os.Remove(upgradeInfoFilePath) require.Nil(err) } + +func TestBinaryVersion(t *testing.T) { + s := setupTest(t, 10, map[int64]bool{}) + + t.Log("verify not panic: upgrade handler is present for applied upgrade") + s.keeper.SetUpgradeHandler("test0", func(ctx sdk.Context, plan types.Plan, fromVM module.VersionMap) (module.VersionMap, error) { + return nil, nil + }) + + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}}) + require.Nil(t, err) + + newCtx := s.ctx.WithBlockHeight(12) + s.keeper.ApplyUpgrade(newCtx, types.Plan{ + Name: "test0", + Height: 12, + }) + + newCtx = s.ctx.WithBlockHeight(13) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + require.NotPanics(t, func() { + s.module.BeginBlock(newCtx, req) + }) + + t.Log("verify not panic: upgrade handler is present for scheduled upgrade") + s.keeper.SetUpgradeHandler("test1", func(ctx sdk.Context, plan types.Plan, fromVM module.VersionMap) (module.VersionMap, error) { + return nil, nil + }) + + err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test1", Height: s.ctx.BlockHeight() + 2}}) + require.Nil(t, err) + + newCtx = s.ctx.WithBlockHeight(13) + req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + require.NotPanics(t, func() { + s.module.BeginBlock(newCtx, req) + }) + + t.Log("verify test panic: upgrade handler is not present for scheduled upgrade") + err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: s.ctx.BlockHeight() + 2}}) + require.Nil(t, err) + + newCtx = s.ctx.WithBlockHeight(13) + req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + require.Panics(t, func() { + s.module.BeginBlock(newCtx, req) + }) +} diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 38251ca9bb4c..d4d5ffb57f98 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -16,6 +16,7 @@ import ( "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/kv" "github.com/cosmos/cosmos-sdk/types/module" xp "github.com/cosmos/cosmos-sdk/x/upgrade/exported" "github.com/cosmos/cosmos-sdk/x/upgrade/types" @@ -236,10 +237,16 @@ func (k Keeper) IterateDoneUpgrades(ctx sdk.Context, cb func(name string, height stop := false for ; iter.Valid() && !stop; iter.Next() { bz := iter.Value() - stop = cb(string(iter.Key()), int64(binary.BigEndian.Uint64(bz))) + stop = cb(parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(bz))) } } +// parseDoneKey - split upgrade name from the done key +func parseDoneKey(key []byte) string { + kv.AssertKeyAtLeastLength(key, 2) + return string(key[1:]) +} + // GetDoneHeight returns the height at which the given upgrade was executed func (k Keeper) GetDoneHeight(ctx sdk.Context, name string) int64 { store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) From 00aa578ae38da1c5e9e3a13dd31adfcf61601d1b Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 3 Nov 2021 19:47:45 +0530 Subject: [PATCH 04/17] review changes --- x/upgrade/abci.go | 8 ++----- x/upgrade/abci_test.go | 8 +++---- x/upgrade/keeper/keeper.go | 16 ++++++------- x/upgrade/keeper/keeper_test.go | 41 +++++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 19 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 9ed3c17609cc..ece905e6b724 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -23,11 +23,7 @@ import ( func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) - var recentPlan string - k.IterateDoneUpgrades(ctx, func(name string, _ int64) bool { - recentPlan = name - return true - }) + recentPlan, _ := k.GetLastCompletedUpgrade(ctx) plan, found := k.GetUpgradePlan(ctx) if !found { @@ -37,7 +33,7 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { return } else { - if recentPlan != "" && !k.HasHandler(plan.Name) { + if !k.HasHandler(plan.Name) { panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", recentPlan)) } } diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 8e0550c1d90d..efab3a868142 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -415,8 +415,8 @@ func TestBinaryVersion(t *testing.T) { s := setupTest(t, 10, map[int64]bool{}) t.Log("verify not panic: upgrade handler is present for applied upgrade") - s.keeper.SetUpgradeHandler("test0", func(ctx sdk.Context, plan types.Plan, fromVM module.VersionMap) (module.VersionMap, error) { - return nil, nil + s.keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil }) err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}}) @@ -435,8 +435,8 @@ func TestBinaryVersion(t *testing.T) { }) t.Log("verify not panic: upgrade handler is present for scheduled upgrade") - s.keeper.SetUpgradeHandler("test1", func(ctx sdk.Context, plan types.Plan, fromVM module.VersionMap) (module.VersionMap, error) { - return nil, nil + s.keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil }) err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test1", Height: s.ctx.BlockHeight() + 2}}) diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index d4d5ffb57f98..1081d545258d 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -228,17 +228,15 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([] return bz, true } -// IterateDoneUpgrades iterate over all the applied upgrades. -// Callback to get all data, returns true to stop, false to keep reading -func (k Keeper) IterateDoneUpgrades(ctx sdk.Context, cb func(name string, height int64) bool) { - iter := sdk.KVStorePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) +// GetLastCompletedUpgrade returns the last applied upgrade name and height. +func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) { + iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) defer iter.Close() - - stop := false - for ; iter.Valid() && !stop; iter.Next() { - bz := iter.Value() - stop = cb(parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(bz))) + for iter.Valid() { + return parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(iter.Value())) } + + return "", 0 } // parseDoneKey - split upgrade name from the done key diff --git a/x/upgrade/keeper/keeper_test.go b/x/upgrade/keeper/keeper_test.go index c8df562f5de4..767eb53b087c 100644 --- a/x/upgrade/keeper/keeper_test.go +++ b/x/upgrade/keeper/keeper_test.go @@ -232,6 +232,47 @@ func (s *KeeperTestSuite) TestMigrations() { s.Require().Equal(vmBefore["bank"]+1, vm["bank"]) } + +func (s *KeeperTestSuite) TestLastCompletedUpgrade() { + keeper := s.app.UpgradeKeeper + require := s.Require() + + s.T().Log("verify empty name if applied upgrades are empty") + name, height := keeper.GetLastCompletedUpgrade(s.ctx) + require.Equal("", name) + require.Equal(int64(0), height) + + keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil + }) + + keeper.ApplyUpgrade(s.ctx, types.Plan{ + Name: "test0", + Height: 10, + }) + + s.T().Log("verify valid upgrade name and height") + name, height = keeper.GetLastCompletedUpgrade(s.ctx) + require.Equal("test0", name) + require.Equal(int64(10), height) + + keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil + }) + + newCtx := s.ctx.WithBlockHeight(15) + keeper.ApplyUpgrade(newCtx, types.Plan{ + Name: "test1", + Height: 15, + }) + + s.T().Log("verify valid upgrade name and height with multiple upgrades") + name, height = keeper.GetLastCompletedUpgrade(newCtx) + require.Equal("test1", name) + require.Equal(int64(15), height) +} + + func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } From 7f51facf79d0d42e924879ceb7b023660fc11f9b Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 3 Nov 2021 19:57:54 +0530 Subject: [PATCH 05/17] review changes --- x/upgrade/abci.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index ece905e6b724..893fde49fb09 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -23,10 +23,9 @@ import ( func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) - recentPlan, _ := k.GetLastCompletedUpgrade(ctx) - plan, found := k.GetUpgradePlan(ctx) if !found { + recentPlan, _ := k.GetLastCompletedUpgrade(ctx) if recentPlan != "" && !k.HasHandler(recentPlan) { panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", recentPlan)) } @@ -34,7 +33,7 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { return } else { if !k.HasHandler(plan.Name) { - panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", recentPlan)) + panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", plan.Name)) } } From b023d1a858184e021d1b7593511b123716003b30 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Tue, 9 Nov 2021 21:39:35 +0530 Subject: [PATCH 06/17] review changes --- x/upgrade/abci.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 893fde49fb09..3e017e5b9ba4 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -23,18 +23,15 @@ import ( func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) + lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx) + // Throw an error if there is no upgrade handler is registered for last applied upgrade(meaning this is a wrong binary) + if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { + panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) + } + plan, found := k.GetUpgradePlan(ctx) if !found { - recentPlan, _ := k.GetLastCompletedUpgrade(ctx) - if recentPlan != "" && !k.HasHandler(recentPlan) { - panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", recentPlan)) - } - return - } else { - if !k.HasHandler(plan.Name) { - panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", plan.Name)) - } } logger := ctx.Logger() From ea8b276c0c56217e87db3eaaf766f0d2325cdf66 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Tue, 9 Nov 2021 21:51:10 +0530 Subject: [PATCH 07/17] review changes --- x/upgrade/abci.go | 2 +- x/upgrade/keeper/keeper.go | 4 ++-- x/upgrade/keeper/keeper_test.go | 10 ++++------ 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 3e017e5b9ba4..0ce789f15ea3 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -23,7 +23,7 @@ import ( func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) - lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx) + lastAppliedPlan, _ := k.GetNextOrLastUpgrade(ctx) // Throw an error if there is no upgrade handler is registered for last applied upgrade(meaning this is a wrong binary) if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 1081d545258d..dd331d4d5276 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -228,8 +228,8 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([] return bz, true } -// GetLastCompletedUpgrade returns the last applied upgrade name and height. -func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) { +// GetNextOrLastUpgrade returns the last applied upgrade name and height. +func (k Keeper) GetNextOrLastUpgrade(ctx sdk.Context) (string, int64) { iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) defer iter.Close() for iter.Valid() { diff --git a/x/upgrade/keeper/keeper_test.go b/x/upgrade/keeper/keeper_test.go index 767eb53b087c..9e8395cd1cbf 100644 --- a/x/upgrade/keeper/keeper_test.go +++ b/x/upgrade/keeper/keeper_test.go @@ -232,27 +232,26 @@ func (s *KeeperTestSuite) TestMigrations() { s.Require().Equal(vmBefore["bank"]+1, vm["bank"]) } - func (s *KeeperTestSuite) TestLastCompletedUpgrade() { keeper := s.app.UpgradeKeeper require := s.Require() s.T().Log("verify empty name if applied upgrades are empty") - name, height := keeper.GetLastCompletedUpgrade(s.ctx) + name, height := keeper.GetNextOrLastUpgrade(s.ctx) require.Equal("", name) require.Equal(int64(0), height) keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { return vm, nil }) - + keeper.ApplyUpgrade(s.ctx, types.Plan{ Name: "test0", Height: 10, }) s.T().Log("verify valid upgrade name and height") - name, height = keeper.GetLastCompletedUpgrade(s.ctx) + name, height = keeper.GetNextOrLastUpgrade(s.ctx) require.Equal("test0", name) require.Equal(int64(10), height) @@ -267,12 +266,11 @@ func (s *KeeperTestSuite) TestLastCompletedUpgrade() { }) s.T().Log("verify valid upgrade name and height with multiple upgrades") - name, height = keeper.GetLastCompletedUpgrade(newCtx) + name, height = keeper.GetNextOrLastUpgrade(newCtx) require.Equal("test1", name) require.Equal(int64(15), height) } - func TestKeeperTestSuite(t *testing.T) { suite.Run(t, new(KeeperTestSuite)) } From 512a73c391e11e012723d6fee9f76eb57656b804 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 10 Nov 2021 21:02:06 +0530 Subject: [PATCH 08/17] chore: cleanup --- x/upgrade/keeper/keeper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index dd331d4d5276..f42beb6cae25 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -232,7 +232,7 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([] func (k Keeper) GetNextOrLastUpgrade(ctx sdk.Context) (string, int64) { iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) defer iter.Close() - for iter.Valid() { + if iter.Valid() { return parseDoneKey(iter.Key()), int64(binary.BigEndian.Uint64(iter.Value())) } From 23ccc63d75d7a8bf4b4cc5b0fca9da6cc2b8df46 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Thu, 11 Nov 2021 11:56:44 +0530 Subject: [PATCH 09/17] chore: add more tests --- x/upgrade/abci.go | 19 +++++++++++++------ x/upgrade/abci_test.go | 34 ++++++++++++++++++++++++---------- x/upgrade/keeper/keeper.go | 5 +++++ 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 0ce789f15ea3..bbd08cc86896 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -23,14 +23,9 @@ import ( func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) - lastAppliedPlan, _ := k.GetNextOrLastUpgrade(ctx) - // Throw an error if there is no upgrade handler is registered for last applied upgrade(meaning this is a wrong binary) - if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { - panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) - } - plan, found := k.GetUpgradePlan(ctx) if !found { + verifyAppliedHandlerPresent(ctx, k) return } @@ -40,6 +35,8 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { if plan.ShouldExecute(ctx) { // If skip upgrade has been set for current height, we clear the upgrade plan if k.IsSkipHeight(ctx.BlockHeight()) { + verifyAppliedHandlerPresent(ctx, k) + skipUpgradeMsg := fmt.Sprintf("UPGRADE \"%s\" SKIPPED at %d: %s", plan.Name, plan.Height, plan.Info) logger.Info(skipUpgradeMsg) @@ -69,6 +66,8 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { return } + verifyAppliedHandlerPresent(ctx, k) + // if we have a pending upgrade, but it is not yet time, make sure we did not // set the handler already if k.HasHandler(plan.Name) { @@ -78,6 +77,14 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { } } +// checkBinaryVersion throws an error if there is no upgrade handler is registered for last applied upgrade +func verifyAppliedHandlerPresent(ctx sdk.Context, k keeper.Keeper) { + lastAppliedPlan, _ := k.GetNextOrLastUpgrade(ctx) + if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { + panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) + } +} + // BuildUpgradeNeededMsg prints the message that notifies that an upgrade is needed. func BuildUpgradeNeededMsg(plan types.Plan) string { return fmt.Sprintf("UPGRADE \"%s\" NEEDED at %s: %s", plan.Name, plan.DueAt(), plan.Info) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index efab3a868142..a0feb0552a3e 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -412,9 +412,17 @@ func TestDumpUpgradeInfoToFile(t *testing.T) { } func TestBinaryVersion(t *testing.T) { - s := setupTest(t, 10, map[int64]bool{}) + var ( + skipHeight int64 = 15 + ) + s := setupTest(t, 10, map[int64]bool{skipHeight: true}) + + t.Log("verify not panic: no scheduled upgrade or applied upgrade is present") + req := abci.RequestBeginBlock{Header: s.ctx.BlockHeader()} + require.NotPanics(t, func() { + s.module.BeginBlock(s.ctx, req) + }) - t.Log("verify not panic: upgrade handler is present for applied upgrade") s.keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { return vm, nil }) @@ -428,31 +436,37 @@ func TestBinaryVersion(t *testing.T) { Height: 12, }) - newCtx = s.ctx.WithBlockHeight(13) - req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} require.NotPanics(t, func() { s.module.BeginBlock(newCtx, req) }) - t.Log("verify not panic: upgrade handler is present for scheduled upgrade") + // remove last applied upgrade + s.keeper.DeleteUpgradeHandler("test0") + t.Log("verify panic: no upgrade handler is present for last upgrade") + require.Panics(t, func() { + s.module.BeginBlock(newCtx, req) + }) + + t.Log("verify panic: no upgrade handler is present for last upgrade with skip upgrade") s.keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { return vm, nil }) - err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test1", Height: s.ctx.BlockHeight() + 2}}) + err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test1", Height: 15}}) require.Nil(t, err) - newCtx = s.ctx.WithBlockHeight(13) + newCtx = s.ctx.WithBlockHeight(15) req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - require.NotPanics(t, func() { + require.Panics(t, func() { s.module.BeginBlock(newCtx, req) }) t.Log("verify test panic: upgrade handler is not present for scheduled upgrade") - err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: s.ctx.BlockHeight() + 2}}) + err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: skipHeight + 2}}) require.Nil(t, err) - newCtx = s.ctx.WithBlockHeight(13) + newCtx = s.ctx.WithBlockHeight(skipHeight + 2) req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} require.Panics(t, func() { s.module.BeginBlock(newCtx, req) diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index f42beb6cae25..0f24df7727ad 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -59,6 +59,11 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl k.upgradeHandlers[name] = upgradeHandler } +// DeleteUpgradeHandler removes an UpgradeHandler for the given upgrade name +func (k Keeper) DeleteUpgradeHandler(name string) { + delete(k.upgradeHandlers, name) +} + // setProtocolVersion sets the protocol version to state func (k Keeper) setProtocolVersion(ctx sdk.Context, v uint64) { store := ctx.KVStore(k.storeKey) From 1e8e047a99696a47eb6886f20d845008dbcb0d2d Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Fri, 12 Nov 2021 19:02:58 +0530 Subject: [PATCH 10/17] chore: address review changes --- x/upgrade/abci.go | 26 ++++++++++++++------------ x/upgrade/abci_test.go | 3 +++ x/upgrade/keeper/keeper.go | 4 ++-- x/upgrade/keeper/keeper_test.go | 6 +++--- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index bbd08cc86896..ecbabd4541e1 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -12,6 +12,8 @@ import ( "github.com/cosmos/cosmos-sdk/x/upgrade/types" ) +var DowngradeVerified = false + // BeginBlock will check if there is a scheduled plan and if it is ready to be executed. // If the current height is in the provided set of heights to skip, it will skip and clear the upgrade plan. // If it is ready, it will execute it if the handler is installed, and panic/abort otherwise. @@ -24,8 +26,19 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { defer telemetry.ModuleMeasureSince(types.ModuleName, time.Now(), telemetry.MetricKeyBeginBlocker) plan, found := k.GetUpgradePlan(ctx) + + if !DowngradeVerified { + DowngradeVerified = true + lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx) + // verify if we are running valid app version + if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) { + if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { + panic(fmt.Sprintf("Wrong app version, upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) + } + } + } + if !found { - verifyAppliedHandlerPresent(ctx, k) return } @@ -35,7 +48,6 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { if plan.ShouldExecute(ctx) { // If skip upgrade has been set for current height, we clear the upgrade plan if k.IsSkipHeight(ctx.BlockHeight()) { - verifyAppliedHandlerPresent(ctx, k) skipUpgradeMsg := fmt.Sprintf("UPGRADE \"%s\" SKIPPED at %d: %s", plan.Name, plan.Height, plan.Info) logger.Info(skipUpgradeMsg) @@ -66,8 +78,6 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { return } - verifyAppliedHandlerPresent(ctx, k) - // if we have a pending upgrade, but it is not yet time, make sure we did not // set the handler already if k.HasHandler(plan.Name) { @@ -77,14 +87,6 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { } } -// checkBinaryVersion throws an error if there is no upgrade handler is registered for last applied upgrade -func verifyAppliedHandlerPresent(ctx sdk.Context, k keeper.Keeper) { - lastAppliedPlan, _ := k.GetNextOrLastUpgrade(ctx) - if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { - panic(fmt.Sprintf("upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) - } -} - // BuildUpgradeNeededMsg prints the message that notifies that an upgrade is needed. func BuildUpgradeNeededMsg(plan types.Plan) string { return fmt.Sprintf("UPGRADE \"%s\" NEEDED at %s: %s", plan.Name, plan.DueAt(), plan.Info) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index a0feb0552a3e..0156608deb06 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -444,6 +444,7 @@ func TestBinaryVersion(t *testing.T) { // remove last applied upgrade s.keeper.DeleteUpgradeHandler("test0") t.Log("verify panic: no upgrade handler is present for last upgrade") + upgrade.DowngradeVerified = false require.Panics(t, func() { s.module.BeginBlock(newCtx, req) }) @@ -458,6 +459,7 @@ func TestBinaryVersion(t *testing.T) { newCtx = s.ctx.WithBlockHeight(15) req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + upgrade.DowngradeVerified = false require.Panics(t, func() { s.module.BeginBlock(newCtx, req) }) @@ -468,6 +470,7 @@ func TestBinaryVersion(t *testing.T) { newCtx = s.ctx.WithBlockHeight(skipHeight + 2) req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + upgrade.DowngradeVerified = false require.Panics(t, func() { s.module.BeginBlock(newCtx, req) }) diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 0f24df7727ad..c8f423704e4d 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -233,8 +233,8 @@ func (k Keeper) GetUpgradedConsensusState(ctx sdk.Context, lastHeight int64) ([] return bz, true } -// GetNextOrLastUpgrade returns the last applied upgrade name and height. -func (k Keeper) GetNextOrLastUpgrade(ctx sdk.Context) (string, int64) { +// GetLastCompletedUpgrade returns the last applied upgrade name and height. +func (k Keeper) GetLastCompletedUpgrade(ctx sdk.Context) (string, int64) { iter := sdk.KVStoreReversePrefixIterator(ctx.KVStore(k.storeKey), []byte{types.DoneByte}) defer iter.Close() if iter.Valid() { diff --git a/x/upgrade/keeper/keeper_test.go b/x/upgrade/keeper/keeper_test.go index 9e8395cd1cbf..b572a89ac1b4 100644 --- a/x/upgrade/keeper/keeper_test.go +++ b/x/upgrade/keeper/keeper_test.go @@ -237,7 +237,7 @@ func (s *KeeperTestSuite) TestLastCompletedUpgrade() { require := s.Require() s.T().Log("verify empty name if applied upgrades are empty") - name, height := keeper.GetNextOrLastUpgrade(s.ctx) + name, height := keeper.GetLastCompletedUpgrade(s.ctx) require.Equal("", name) require.Equal(int64(0), height) @@ -251,7 +251,7 @@ func (s *KeeperTestSuite) TestLastCompletedUpgrade() { }) s.T().Log("verify valid upgrade name and height") - name, height = keeper.GetNextOrLastUpgrade(s.ctx) + name, height = keeper.GetLastCompletedUpgrade(s.ctx) require.Equal("test0", name) require.Equal(int64(10), height) @@ -266,7 +266,7 @@ func (s *KeeperTestSuite) TestLastCompletedUpgrade() { }) s.T().Log("verify valid upgrade name and height with multiple upgrades") - name, height = keeper.GetNextOrLastUpgrade(newCtx) + name, height = keeper.GetLastCompletedUpgrade(newCtx) require.Equal("test1", name) require.Equal(int64(15), height) } From ecd03d37038535eb65dd47e91a6805ac8f5ceb0a Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Fri, 12 Nov 2021 21:48:12 +0530 Subject: [PATCH 11/17] chore: cleanup --- x/upgrade/abci.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index ecbabd4541e1..cf5cc3ebb55e 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -41,14 +41,12 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { if !found { return } - logger := ctx.Logger() // To make sure clear upgrade is executed at the same block if plan.ShouldExecute(ctx) { // If skip upgrade has been set for current height, we clear the upgrade plan if k.IsSkipHeight(ctx.BlockHeight()) { - skipUpgradeMsg := fmt.Sprintf("UPGRADE \"%s\" SKIPPED at %d: %s", plan.Name, plan.Height, plan.Info) logger.Info(skipUpgradeMsg) From 1f7cc724c6138cfd482406d0b621fa04564d2901 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 17 Nov 2021 21:06:33 +0530 Subject: [PATCH 12/17] chore: refactor tests to table tests --- x/upgrade/abci.go | 8 ++- x/upgrade/abci_test.go | 144 ++++++++++++++++++++++++----------------- 2 files changed, 93 insertions(+), 59 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index cf5cc3ebb55e..7bbee5bf18d9 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -12,6 +12,8 @@ import ( "github.com/cosmos/cosmos-sdk/x/upgrade/types" ) +// DowngradeVerified is a variable that tells if we've already sanity checked that this binary version isn't being used against an old state. +// If so, we dont run that check every beginblock. var DowngradeVerified = false // BeginBlock will check if there is a scheduled plan and if it is ready to be executed. @@ -30,7 +32,11 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { if !DowngradeVerified { DowngradeVerified = true lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx) - // verify if we are running valid app version + // This check will make sure that we are using a valid binary. + // It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade. + // 1. If there is no scheduled upgrade. + // 2. If the plan is not ready. + // 3. If the plan is ready and skip upgrade height is set for current height. if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) { if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { panic(fmt.Sprintf("Wrong app version, upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 0156608deb06..c16a2be8a00c 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -412,66 +412,94 @@ func TestDumpUpgradeInfoToFile(t *testing.T) { } func TestBinaryVersion(t *testing.T) { - var ( - skipHeight int64 = 15 - ) + var skipHeight int64 = 15 s := setupTest(t, 10, map[int64]bool{skipHeight: true}) - t.Log("verify not panic: no scheduled upgrade or applied upgrade is present") - req := abci.RequestBeginBlock{Header: s.ctx.BlockHeader()} - require.NotPanics(t, func() { - s.module.BeginBlock(s.ctx, req) - }) - - s.keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { - return vm, nil - }) - - err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}}) - require.Nil(t, err) - - newCtx := s.ctx.WithBlockHeight(12) - s.keeper.ApplyUpgrade(newCtx, types.Plan{ - Name: "test0", - Height: 12, - }) - - req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - require.NotPanics(t, func() { - s.module.BeginBlock(newCtx, req) - }) - - // remove last applied upgrade - s.keeper.DeleteUpgradeHandler("test0") - t.Log("verify panic: no upgrade handler is present for last upgrade") - upgrade.DowngradeVerified = false - require.Panics(t, func() { - s.module.BeginBlock(newCtx, req) - }) - - t.Log("verify panic: no upgrade handler is present for last upgrade with skip upgrade") - s.keeper.SetUpgradeHandler("test1", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { - return vm, nil - }) - - err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test1", Height: 15}}) - require.Nil(t, err) - - newCtx = s.ctx.WithBlockHeight(15) - req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - upgrade.DowngradeVerified = false - require.Panics(t, func() { - s.module.BeginBlock(newCtx, req) - }) + testCases := []struct { + name string + preRun func() (sdk.Context, abci.RequestBeginBlock) + expectPanic bool + }{ + { + "test not panic: no scheduled upgrade or applied upgrade is present", + func() (sdk.Context, abci.RequestBeginBlock) { + req := abci.RequestBeginBlock{Header: s.ctx.BlockHeader()} + return s.ctx, req + }, + false, + }, + { + "test not panic: upgrade handler is present for last applied upgrade", + func() (sdk.Context, abci.RequestBeginBlock) { + s.keeper.SetUpgradeHandler("test0", func(_ sdk.Context, _ types.Plan, vm module.VersionMap) (module.VersionMap, error) { + return vm, nil + }) + + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test0", Height: s.ctx.BlockHeight() + 2}}) + require.Nil(t, err) + + newCtx := s.ctx.WithBlockHeight(12) + s.keeper.ApplyUpgrade(newCtx, types.Plan{ + Name: "test0", + Height: 12, + }) + + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + false, + }, + { + "test panic: no upgrade handler is present for last upgrade", + func() (sdk.Context, abci.RequestBeginBlock) { + + newCtx := s.ctx.WithBlockHeight(13) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + upgrade.DowngradeVerified = false + + // delete last applied upgrade + s.keeper.DeleteUpgradeHandler("test0") + return newCtx, req + }, + true, + }, + { + "test panic: no upgrade handler is present for last upgrade with skip upgrade", + func() (sdk.Context, abci.RequestBeginBlock) { + upgrade.DowngradeVerified = false + + newCtx := s.ctx.WithBlockHeight(15) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + true, + }, + { + "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) + + newCtx := s.ctx.WithBlockHeight(13) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + true, + }, + } - t.Log("verify test panic: upgrade handler is not present for scheduled upgrade") - err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "Upgrade test", Plan: types.Plan{Name: "test2", Height: skipHeight + 2}}) - require.Nil(t, err) + for _, tc := range testCases { + ctx, req := tc.preRun() + if tc.expectPanic { + require.Panics(t, func() { + s.module.BeginBlock(ctx, req) + }) + } else { + require.NotPanics(t, func() { + s.module.BeginBlock(ctx, req) + }) + } + } - newCtx = s.ctx.WithBlockHeight(skipHeight + 2) - req = abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - upgrade.DowngradeVerified = false - require.Panics(t, func() { - s.module.BeginBlock(newCtx, req) - }) + require.True(t, false) } From 9914bfee562e65acc9227e690e0870b37ade53de Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 17 Nov 2021 21:14:20 +0530 Subject: [PATCH 13/17] chore: add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e3e0868081a..808513bf6d8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (deps) [\#10210](https://github.com/cosmos/cosmos-sdk/pull/10210) Bump Tendermint to [v0.35.0](https://github.com/tendermint/tendermint/releases/tag/v0.35.0). * [\#10486](https://github.com/cosmos/cosmos-sdk/pull/10486) store/cachekv's `Store.Write` conservatively looks up keys, but also uses the [map clearing idiom](https://bencher.orijtech.com/perfclinic/mapclearing/) to reduce the RAM usage, CPU time usage, and garbage collection pressure from clearing maps, instead of allocating new maps. +* [\#10407](https://github.com/cosmos/cosmos-sdk/pull/10407) Add validation to `x/upgrade` module's `BeginBlock` to check accidental binary downgrades ### API Breaking Changes From 5a2f4ae6f150f177eda4eea864141bae01d0ddeb Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 17 Nov 2021 21:35:57 +0530 Subject: [PATCH 14/17] chore: fix test --- x/upgrade/abci_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index c16a2be8a00c..56db2c278255 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -500,6 +500,4 @@ func TestBinaryVersion(t *testing.T) { }) } } - - require.True(t, false) } From 51d7d21f8f0bc3b94dc08f81e32004a4ade2f95d Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Fri, 19 Nov 2021 10:31:26 +0530 Subject: [PATCH 15/17] chore: review changes --- x/upgrade/abci.go | 16 +++++++++++----- x/upgrade/abci_test.go | 4 ++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 7bbee5bf18d9..a31dc4fa71d4 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -12,9 +12,15 @@ import ( "github.com/cosmos/cosmos-sdk/x/upgrade/types" ) -// DowngradeVerified is a variable that tells if we've already sanity checked that this binary version isn't being used against an old state. +// downgradeVerified is a variable that tells if we've already sanity checked that this binary version isn't being used against an old state. // If so, we dont run that check every beginblock. -var DowngradeVerified = false +var downgradeVerified = false + +// ResetDowngradeVerified resets downgradeVerified. +// Note: It should be use only for testing purposes. +func ResetDowngradeVerified() { + downgradeVerified = false +} // BeginBlock will check if there is a scheduled plan and if it is ready to be executed. // If the current height is in the provided set of heights to skip, it will skip and clear the upgrade plan. @@ -29,8 +35,8 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { plan, found := k.GetUpgradePlan(ctx) - if !DowngradeVerified { - DowngradeVerified = true + if !downgradeVerified { + downgradeVerified = true lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx) // This check will make sure that we are using a valid binary. // It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade. @@ -39,7 +45,7 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { // 3. If the plan is ready and skip upgrade height is set for current height. if !found || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) { if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { - panic(fmt.Sprintf("Wrong app version, upgrade handler is missing for %s upgrade plan", lastAppliedPlan)) + panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", ctx.ConsensusParams().Version.AppVersion, lastAppliedPlan)) } } } diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 56db2c278255..c6fb056776aa 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -455,7 +455,7 @@ func TestBinaryVersion(t *testing.T) { newCtx := s.ctx.WithBlockHeight(13) req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - upgrade.DowngradeVerified = false + upgrade.ResetDowngradeVerified() // delete last applied upgrade s.keeper.DeleteUpgradeHandler("test0") @@ -466,7 +466,7 @@ func TestBinaryVersion(t *testing.T) { { "test panic: no upgrade handler is present for last upgrade with skip upgrade", func() (sdk.Context, abci.RequestBeginBlock) { - upgrade.DowngradeVerified = false + upgrade.ResetDowngradeVerified() newCtx := s.ctx.WithBlockHeight(15) req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} From 97fada74c8ed23166567b4fe936c12afe7402f45 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Mon, 10 Jan 2022 11:23:15 +0530 Subject: [PATCH 16/17] chore: review changes --- x/upgrade/abci_test.go | 26 +------------------------- x/upgrade/keeper/keeper.go | 5 ----- 2 files changed, 1 insertion(+), 30 deletions(-) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 3fb029a5c81a..c3f1ea6bd1a8 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -411,6 +411,7 @@ func TestDumpUpgradeInfoToFile(t *testing.T) { require.Nil(err) } +// TODO: add testcase to for `no upgrade handler is present for last applied upgrade`. func TestBinaryVersion(t *testing.T) { var skipHeight int64 = 15 s := setupTest(t, 10, map[int64]bool{skipHeight: true}) @@ -449,31 +450,6 @@ func TestBinaryVersion(t *testing.T) { }, false, }, - { - "test panic: no upgrade handler is present for last upgrade", - func() (sdk.Context, abci.RequestBeginBlock) { - - newCtx := s.ctx.WithBlockHeight(13) - req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - upgrade.ResetDowngradeVerified() - - // delete last applied upgrade - s.keeper.DeleteUpgradeHandler("test0") - return newCtx, req - }, - true, - }, - { - "test panic: no upgrade handler is present for last upgrade with skip upgrade", - func() (sdk.Context, abci.RequestBeginBlock) { - upgrade.ResetDowngradeVerified() - - newCtx := s.ctx.WithBlockHeight(15) - req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - return newCtx, req - }, - true, - }, { "test panic: upgrade needed", func() (sdk.Context, abci.RequestBeginBlock) { diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index c344184cd900..d7408a54acc3 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -60,11 +60,6 @@ func (k Keeper) SetUpgradeHandler(name string, upgradeHandler types.UpgradeHandl k.upgradeHandlers[name] = upgradeHandler } -// DeleteUpgradeHandler removes an UpgradeHandler for the given upgrade name -func (k Keeper) DeleteUpgradeHandler(name string) { - delete(k.upgradeHandlers, name) -} - // setProtocolVersion sets the protocol version to state func (k Keeper) setProtocolVersion(ctx sdk.Context, v uint64) { store := ctx.KVStore(k.storeKey) From 6bae3a7e216e23badba8ecde705709bd9d11d7d4 Mon Sep 17 00:00:00 2001 From: aleem1314 Date: Wed, 12 Jan 2022 18:22:15 +0530 Subject: [PATCH 17/17] chore: move downgradeVerified to keeper --- x/upgrade/abci.go | 14 ++------------ x/upgrade/keeper/keeper.go | 11 +++++++++++ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index a31dc4fa71d4..f390a873d134 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -12,16 +12,6 @@ import ( "github.com/cosmos/cosmos-sdk/x/upgrade/types" ) -// downgradeVerified is a variable that tells if we've already sanity checked that this binary version isn't being used against an old state. -// If so, we dont run that check every beginblock. -var downgradeVerified = false - -// ResetDowngradeVerified resets downgradeVerified. -// Note: It should be use only for testing purposes. -func ResetDowngradeVerified() { - downgradeVerified = false -} - // BeginBlock will check if there is a scheduled plan and if it is ready to be executed. // If the current height is in the provided set of heights to skip, it will skip and clear the upgrade plan. // If it is ready, it will execute it if the handler is installed, and panic/abort otherwise. @@ -35,8 +25,8 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { plan, found := k.GetUpgradePlan(ctx) - if !downgradeVerified { - downgradeVerified = true + if !k.DowngradeVerified() { + k.SetDowngradeVerified(true) lastAppliedPlan, _ := k.GetLastCompletedUpgrade(ctx) // This check will make sure that we are using a valid binary. // It'll panic in these cases if there is no upgrade handler registered for the last applied upgrade. diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index d7408a54acc3..a209777c66ab 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -34,6 +34,7 @@ type Keeper struct { cdc codec.BinaryCodec // App-wide binary codec upgradeHandlers map[string]types.UpgradeHandler // map of plan name to upgrade handler versionSetter xp.ProtocolVersionSetter // implements setting the protocol version field on BaseApp + downgradeVerified bool // tells if we've already sanity checked that this binary version isn't being used against an old state. } // NewKeeper constructs an upgrade Keeper which requires the following arguments: @@ -407,3 +408,13 @@ func (k Keeper) ReadUpgradeInfoFromDisk() (types.Plan, error) { return upgradeInfo, nil } + +// SetDowngradeVerified updates downgradeVerified. +func (k *Keeper) SetDowngradeVerified(v bool) { + k.downgradeVerified = v +} + +// DowngradeVerified returns downgradeVerified. +func (k Keeper) DowngradeVerified() bool { + return k.downgradeVerified +}