From dd3778cf0de271aa33c9590f8e142825ce565db8 Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Thu, 28 Sep 2023 12:59:05 -0400 Subject: [PATCH 01/12] allow minor releases before the block height --- x/upgrade/abci.go | 21 +++++++--- x/upgrade/abci_test.go | 75 +++++++++++++++++++++++++++++++++++ x/upgrade/keeper/keeper.go | 4 +- x/upgrade/types/plan.go | 28 ++++++++++++- x/upgrade/types/plan_test.go | 76 +++++++++++++++++++++++++++++++++++- 5 files changed, 193 insertions(+), 11 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index bfb7ff964..7816adb3f 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) - plan, found := k.GetUpgradePlan(ctx) + plan, planFound := k.GetUpgradePlan(ctx) if !k.DowngradeVerified() { k.SetDowngradeVerified(true) @@ -33,14 +33,14 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { // 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 !planFound || !plan.MustExecute(ctx) || (plan.MustExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) { if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", ctx.ConsensusParams().Version.AppVersion, lastAppliedPlan)) } } } - if !found { + if !planFound { return } @@ -53,8 +53,8 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { }, ) - // To make sure clear upgrade is executed at the same block - if plan.ShouldExecute(ctx) { + // If the plan's block height has passed, then it must be the executed version + if plan.MustExecute(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) @@ -65,6 +65,7 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { return } + // If we don't have an upgrade handler for this upgrade name, then we need to shutdown if !k.HasHandler(plan.Name) { // Write the upgrade info to disk. The UpgradeStoreLoader uses this info to perform or skip // store migrations. @@ -86,7 +87,15 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { return } - // if we have a pending upgrade, but it is not yet time, make sure we did not + // if minor release pending, it is allowed to run before the scheduled update height + if plan.UpgradeDetails().IsMinorRelease() && k.HasHandler(plan.Name) { + ctx.Logger().Info(fmt.Sprintf("applying minor upgrade \"%s\" at %s", plan.Name, plan.DueAt())) + ctx = ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) + k.ApplyUpgrade(ctx, plan) + return + } + + // if we have a pending non-minor upgrade, but it is not yet time, make sure we did not // set the handler already if k.HasHandler(plan.Name) { downgradeMsg := fmt.Sprintf("BINARY UPDATED BEFORE TRIGGER! UPGRADE \"%s\" - in binary but not executed on chain", plan.Name) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index d9af90d70..19bbbde8e 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -35,6 +35,8 @@ type TestSuite struct { ctx sdk.Context } +const minorUpgradeInfo = `{"upgradeType":"minor"}` + var s TestSuite func setupTest(height int64, skip map[int64]bool) TestSuite { @@ -462,6 +464,79 @@ func TestBinaryVersion(t *testing.T) { }, true, }, + { + "test not panic: minor version upgrade in future and not applied", + func() (sdk.Context, abci.RequestBeginBlock) { + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{ + Title: "Upgrade test", + Plan: types.Plan{Name: "test2", Height: 13, Info: minorUpgradeInfo}, + }) + require.NoError(t, err) + + newCtx := s.ctx.WithBlockHeight(12) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + false, + }, + { + "test panic: minor version upgrade is due", + func() (sdk.Context, abci.RequestBeginBlock) { + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{ + Title: "Upgrade test", + Plan: types.Plan{Name: "test2", Height: 13, Info: minorUpgradeInfo}, + }) + require.NoError(t, err) + + newCtx := s.ctx.WithBlockHeight(13) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + true, + }, + { + "test not panic: minor upgrade is in future and already applied", + func() (sdk.Context, abci.RequestBeginBlock) { + s.keeper.SetUpgradeHandler("test3", 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: "test3", Height: s.ctx.BlockHeight() + 10, Info: minorUpgradeInfo}, + }) + require.NoError(t, err) + + newCtx := s.ctx.WithBlockHeight(12) + s.keeper.ApplyUpgrade(newCtx, types.Plan{ + Name: "test3", + Height: 12, + }) + + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + false, + }, + { + "test not panic: minor upgrade should apply", + func() (sdk.Context, abci.RequestBeginBlock) { + s.keeper.SetUpgradeHandler("test4", 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: "test4", Height: s.ctx.BlockHeight() + 10, Info: minorUpgradeInfo}, + }) + require.NoError(t, err) + + newCtx := s.ctx.WithBlockHeight(12) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + false, + }, } for _, tc := range testCases { diff --git a/x/upgrade/keeper/keeper.go b/x/upgrade/keeper/keeper.go index 479d5b538..c9d7e2b9b 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -385,7 +385,7 @@ func (k Keeper) DumpUpgradeInfoToDisk(height int64, name string) error { // Deprecated: DumpUpgradeInfoWithInfoToDisk writes upgrade information to UpgradeInfoFileName. // `info` should be provided and contain Plan.Info data in order to support -// auto download functionality by cosmovisor and other tools using upgarde-info.json +// auto download functionality by cosmovisor and other tools using upgrade-info.json // (GetUpgradeInfoPath()) file. func (k Keeper) DumpUpgradeInfoWithInfoToDisk(height int64, name string, info string) error { upgradeInfoFilePath, err := k.GetUpgradeInfoPath() @@ -457,7 +457,7 @@ type upgradeInfo struct { Name string `json:"name,omitempty"` // Height has types.Plan.Height value Height int64 `json:"height,omitempty"` - // Height has types.Plan.Height value + // Info is a string, but represents Info string `json:"info,omitempty"` } diff --git a/x/upgrade/types/plan.go b/x/upgrade/types/plan.go index 9e4bc85ab..6d630c6eb 100644 --- a/x/upgrade/types/plan.go +++ b/x/upgrade/types/plan.go @@ -1,7 +1,9 @@ package types import ( + "encoding/json" "fmt" + "strings" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" @@ -33,8 +35,8 @@ func (p Plan) ValidateBasic() error { return nil } -// ShouldExecute returns true if the Plan is ready to execute given the current context -func (p Plan) ShouldExecute(ctx sdk.Context) bool { +// MustExecute returns true if the Plan is ready to execute given the current context +func (p Plan) MustExecute(ctx sdk.Context) bool { if p.Height > 0 { return p.Height <= ctx.BlockHeight() } @@ -45,3 +47,25 @@ func (p Plan) ShouldExecute(ctx sdk.Context) bool { func (p Plan) DueAt() string { return fmt.Sprintf("height: %d", p.Height) } + +// UpgradeDetails is a struct that represents the details of an upgrade +// This is held in the Info object of an upgrade Plan +type UpgradeDetails struct { + UpgradeType string `json:"upgradeType"` +} + +// UpgradeDetails parses and returns a details struct from the Info field of a Plan +// The upgrade.pb.go is generated from proto, so this is separated here +func (p Plan) UpgradeDetails() UpgradeDetails { + var details UpgradeDetails + if err := json.Unmarshal([]byte(p.Info), &details); err != nil { + // invalid json, assume no upgrade details + return UpgradeDetails{} + } + return details +} + +// IsMinorRelease returns true if the upgrade is a minor release +func (ud UpgradeDetails) IsMinorRelease() bool { + return strings.EqualFold(ud.UpgradeType, "minor") +} diff --git a/x/upgrade/types/plan_test.go b/x/upgrade/types/plan_test.go index bbd969eae..4cd5a3a46 100644 --- a/x/upgrade/types/plan_test.go +++ b/x/upgrade/types/plan_test.go @@ -149,8 +149,82 @@ func TestShouldExecute(t *testing.T) { tc := tc // copy to local variable for scopelint t.Run(name, func(t *testing.T) { ctx := sdk.NewContext(nil, tmproto.Header{Height: tc.ctxHeight, Time: tc.ctxTime}, false, log.NewNopLogger()) - should := tc.p.ShouldExecute(ctx) + should := tc.p.MustExecute(ctx) assert.Equal(t, tc.expected, should) }) } } + +func TestUpgradeDetails(t *testing.T) { + tests := []struct { + name string + plan types.Plan + want types.UpgradeDetails + wantErr bool + }{ + { + name: "valid upgrade details", + plan: types.Plan{ + Info: `{"upgradeType":"minor"}`, + }, + want: types.UpgradeDetails{ + UpgradeType: "minor", + }, + }, + { + name: "invalid json in Info", + plan: types.Plan{ + Info: `{upgradeType:"minor"}`, + }, + want: types.UpgradeDetails{}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := test.plan.UpgradeDetails() + if got != test.want { + t.Errorf("UpgradeDetails() = %v, want %v", got, test.want) + } + }) + } +} + +func TestIsMinorRelease(t *testing.T) { + tests := []struct { + name string + plan types.Plan + want bool + }{ + { + name: "minor release", + plan: types.Plan{ + Info: `{"upgradeType":"minor"}`, + }, + want: true, + }, + { + name: "not a minor release", + plan: types.Plan{ + Info: `{"upgradeType":"major"}`, + }, + want: false, + }, + { + name: "default to major release", + plan: types.Plan{ + Info: "", + }, + want: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ud := test.plan.UpgradeDetails() + if got := ud.IsMinorRelease(); got != test.want { + t.Errorf("IsMinorRelease() = %v, want %v", got, test.want) + } + }) + } +} From 80fe944b90e3f653ea813d8e061ebeaef09af4f8 Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Thu, 28 Sep 2023 16:10:44 -0400 Subject: [PATCH 02/12] avoid renaming shouldexecute --- x/upgrade/abci.go | 79 ++++++++++++++++++++++-------------- x/upgrade/types/plan.go | 4 +- x/upgrade/types/plan_test.go | 2 +- 3 files changed, 51 insertions(+), 34 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 7816adb3f..a6c16b46d 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -33,7 +33,7 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { // 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 !planFound || !plan.MustExecute(ctx) || (plan.MustExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) { + if !planFound || !plan.ShouldExecute(ctx) || (plan.ShouldExecute(ctx) && k.IsSkipHeight(ctx.BlockHeight())) { if lastAppliedPlan != "" && !k.HasHandler(lastAppliedPlan) { panic(fmt.Sprintf("Wrong app version %d, upgrade handler is missing for %s upgrade plan", ctx.ConsensusParams().Version.AppVersion, lastAppliedPlan)) } @@ -54,54 +54,71 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { ) // If the plan's block height has passed, then it must be the executed version - if plan.MustExecute(ctx) { + 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) - ctx.Logger().Info(skipUpgradeMsg) - - // Clear the upgrade plan at current height - k.ClearUpgradePlan(ctx) + skipUpgrade(k, ctx, plan) return } // If we don't have an upgrade handler for this upgrade name, then we need to shutdown if !k.HasHandler(plan.Name) { - // Write the upgrade info to disk. The UpgradeStoreLoader uses this info to perform or skip - // store migrations. - err := k.DumpUpgradeInfoWithInfoToDisk(ctx.BlockHeight(), plan.Name, plan.Info) - if err != nil { - panic(fmt.Errorf("unable to write upgrade info to filesystem: %s", err.Error())) - } + panicUpgradeNeeded(k, ctx, plan) + } - upgradeMsg := BuildUpgradeNeededMsg(plan) - // We don't have an upgrade handler for this upgrade name, meaning this software is out of date so shutdown - ctx.Logger().Error(upgradeMsg) + applyUpgrade(k, ctx, plan) + return + } - panic(upgradeMsg) - } - // We have an upgrade handler for this upgrade name, so apply the upgrade - ctx.Logger().Info(fmt.Sprintf("applying upgrade \"%s\" at %s", plan.Name, plan.DueAt())) - ctx = ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) - k.ApplyUpgrade(ctx, plan) + // if not yet upgraded to future release, exit + if !k.HasHandler(plan.Name) { return } - // if minor release pending, it is allowed to run before the scheduled update height - if plan.UpgradeDetails().IsMinorRelease() && k.HasHandler(plan.Name) { - ctx.Logger().Info(fmt.Sprintf("applying minor upgrade \"%s\" at %s", plan.Name, plan.DueAt())) - ctx = ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) - k.ApplyUpgrade(ctx, plan) + // if running a future minor release, apply the upgrade + if plan.UpgradeDetails().IsMinorRelease() { + // if the pending upgrade is skipped, don't apply it + if k.IsSkipHeight(plan.Height) { + skipUpgrade(k, ctx, plan) + return + } + applyUpgrade(k, ctx, plan) return } // if we have a pending non-minor upgrade, but it is not yet time, make sure we did not // set the handler already - if k.HasHandler(plan.Name) { - downgradeMsg := fmt.Sprintf("BINARY UPDATED BEFORE TRIGGER! UPGRADE \"%s\" - in binary but not executed on chain", plan.Name) - ctx.Logger().Error(downgradeMsg) - panic(downgradeMsg) + downgradeMsg := fmt.Sprintf("BINARY UPDATED BEFORE TRIGGER! UPGRADE \"%s\" - in binary but not executed on chain", plan.Name) + ctx.Logger().Error(downgradeMsg) + panic(downgradeMsg) +} + +func panicUpgradeNeeded(k keeper.Keeper, ctx sdk.Context, plan types.Plan) { + // Write the upgrade info to disk. The UpgradeStoreLoader uses this info to perform or skip + // store migrations. + err := k.DumpUpgradeInfoWithInfoToDisk(ctx.BlockHeight(), plan.Name, plan.Info) + if err != nil { + panic(fmt.Errorf("unable to write upgrade info to filesystem: %s", err.Error())) } + + upgradeMsg := BuildUpgradeNeededMsg(plan) + // We don't have an upgrade handler for this upgrade name, meaning this software is out of date so shutdown + ctx.Logger().Error(upgradeMsg) + + panic(upgradeMsg) +} + +func applyUpgrade(k keeper.Keeper, ctx sdk.Context, plan types.Plan) { + ctx.Logger().Info(fmt.Sprintf("applying upgrade \"%s\" at %s", plan.Name, plan.DueAt())) + ctx = ctx.WithBlockGasMeter(sdk.NewInfiniteGasMeter()) + k.ApplyUpgrade(ctx, plan) +} + +// skipUpgrade logs a message that the upgrade has been skipped and clears the upgrade plan. +func skipUpgrade(k keeper.Keeper, ctx sdk.Context, plan types.Plan) { + skipUpgradeMsg := fmt.Sprintf("UPGRADE \"%s\" SKIPPED at %d: %s", plan.Name, plan.Height, plan.Info) + ctx.Logger().Info(skipUpgradeMsg) + k.ClearUpgradePlan(ctx) } // BuildUpgradeNeededMsg prints the message that notifies that an upgrade is needed. diff --git a/x/upgrade/types/plan.go b/x/upgrade/types/plan.go index 6d630c6eb..358064c47 100644 --- a/x/upgrade/types/plan.go +++ b/x/upgrade/types/plan.go @@ -35,8 +35,8 @@ func (p Plan) ValidateBasic() error { return nil } -// MustExecute returns true if the Plan is ready to execute given the current context -func (p Plan) MustExecute(ctx sdk.Context) bool { +// ShouldExecute returns true if the Plan is ready to execute given the current context +func (p Plan) ShouldExecute(ctx sdk.Context) bool { if p.Height > 0 { return p.Height <= ctx.BlockHeight() } diff --git a/x/upgrade/types/plan_test.go b/x/upgrade/types/plan_test.go index 4cd5a3a46..c667275df 100644 --- a/x/upgrade/types/plan_test.go +++ b/x/upgrade/types/plan_test.go @@ -149,7 +149,7 @@ func TestShouldExecute(t *testing.T) { tc := tc // copy to local variable for scopelint t.Run(name, func(t *testing.T) { ctx := sdk.NewContext(nil, tmproto.Header{Height: tc.ctxHeight, Time: tc.ctxTime}, false, log.NewNopLogger()) - should := tc.p.MustExecute(ctx) + should := tc.p.ShouldExecute(ctx) assert.Equal(t, tc.expected, should) }) } From e45700f24048dbc4b519cd65ac55d4a97994e4b2 Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Fri, 29 Sep 2023 09:56:38 -0400 Subject: [PATCH 03/12] invert the premature upgrade logic --- x/upgrade/abci.go | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index a6c16b46d..a4c3364dd 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -54,45 +54,49 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { ) // If the plan's block height has passed, then it must be the executed version + // All major and minor releases are REQUIRED to execute on the scheduled block height if plan.ShouldExecute(ctx) { // If skip upgrade has been set for current height, we clear the upgrade plan if k.IsSkipHeight(ctx.BlockHeight()) { skipUpgrade(k, ctx, plan) return } - // If we don't have an upgrade handler for this upgrade name, then we need to shutdown if !k.HasHandler(plan.Name) { panicUpgradeNeeded(k, ctx, plan) } - applyUpgrade(k, ctx, plan) return } - // if not yet upgraded to future release, exit - if !k.HasHandler(plan.Name) { - return - } - - // if running a future minor release, apply the upgrade + // If running a pending minor release, apply the upgrade if handler is present + // Minor releases are allowed to run before the scheduled upgrade height, but not required to. if plan.UpgradeDetails().IsMinorRelease() { // if the pending upgrade is skipped, don't apply it if k.IsSkipHeight(plan.Height) { skipUpgrade(k, ctx, plan) return } + // if not yet present, then emit a scheduled log (every 100 blocks, to reduce logs) + if !k.HasHandler(plan.Name) { + if ctx.BlockHeight()%100 == 0 { + ctx.Logger().Info(BuildUpgradeScheduledMsg(plan)) + } + return + } applyUpgrade(k, ctx, plan) return } - // if we have a pending non-minor upgrade, but it is not yet time, make sure we did not - // set the handler already - downgradeMsg := fmt.Sprintf("BINARY UPDATED BEFORE TRIGGER! UPGRADE \"%s\" - in binary but not executed on chain", plan.Name) - ctx.Logger().Error(downgradeMsg) - panic(downgradeMsg) + // if we have a handler for a non-minor upgrade, that means it updated too early and must stop + if k.HasHandler(plan.Name) { + downgradeMsg := fmt.Sprintf("BINARY UPDATED BEFORE TRIGGER! UPGRADE \"%s\" - in binary but not executed on chain", plan.Name) + ctx.Logger().Error(downgradeMsg) + panic(downgradeMsg) + } } +// panicUpgradeNeeded shuts down the node and prints a message that the upgrade needs to be applied. func panicUpgradeNeeded(k keeper.Keeper, ctx sdk.Context, plan types.Plan) { // Write the upgrade info to disk. The UpgradeStoreLoader uses this info to perform or skip // store migrations. @@ -102,7 +106,6 @@ func panicUpgradeNeeded(k keeper.Keeper, ctx sdk.Context, plan types.Plan) { } upgradeMsg := BuildUpgradeNeededMsg(plan) - // We don't have an upgrade handler for this upgrade name, meaning this software is out of date so shutdown ctx.Logger().Error(upgradeMsg) panic(upgradeMsg) @@ -125,3 +128,8 @@ func skipUpgrade(k keeper.Keeper, ctx sdk.Context, plan types.Plan) { func BuildUpgradeNeededMsg(plan types.Plan) string { return fmt.Sprintf("UPGRADE \"%s\" NEEDED at %s: %s", plan.Name, plan.DueAt(), plan.Info) } + +// BuildUpgradeScheduledMsg prints upgrade scheduled message +func BuildUpgradeScheduledMsg(plan types.Plan) string { + return fmt.Sprintf("UPGRADE \"%s\" SCHEDULED at %s: %s", plan.Name, plan.DueAt(), plan.Info) +} From a755a868ceb336a188795e615d5c838a1ea40233 Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Fri, 29 Sep 2023 10:22:45 -0400 Subject: [PATCH 04/12] add coverage for skip --- x/upgrade/abci_test.go | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 19bbbde8e..3361548ee 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -256,6 +256,38 @@ func TestContains(t *testing.T) { require.False(t, s.keeper.IsSkipHeight(4)) } +func TestSkipUpgradeSkipMinor(t *testing.T) { + var ( + planHeight int64 = 15 + height int64 = 10 + name = "minor" + ) + s := setupTest(10, map[int64]bool{planHeight: true}) + + newCtx := s.ctx + + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{ + Name: name, + Height: planHeight, + Info: minorUpgradeInfo, + }}) + require.NoError(t, err) + + t.Log("Verify if skip upgrade flag clears upgrade plan in both cases") + VerifySet(t, map[int64]bool{planHeight: true}) + + newCtx = newCtx.WithBlockHeight(height) + require.NotPanics(t, func() { + s.module.BeginBlock(newCtx, req) + }) + + // To ensure verification is being done only after both upgrades are cleared + t.Log("Verify if both proposals are cleared") + VerifyCleared(t, s.ctx) + VerifyNotDone(t, s.ctx, name) +} + func TestSkipUpgradeSkippingAll(t *testing.T) { var ( skipOne int64 = 11 @@ -469,11 +501,11 @@ func TestBinaryVersion(t *testing.T) { func() (sdk.Context, abci.RequestBeginBlock) { err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{ Title: "Upgrade test", - Plan: types.Plan{Name: "test2", Height: 13, Info: minorUpgradeInfo}, + Plan: types.Plan{Name: "test2", Height: 105, Info: minorUpgradeInfo}, }) require.NoError(t, err) - newCtx := s.ctx.WithBlockHeight(12) + newCtx := s.ctx.WithBlockHeight(100) req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} return newCtx, req }, From 86a4cffaf77357ae79d1f821c2d0c651d679475e Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Fri, 29 Sep 2023 10:26:51 -0400 Subject: [PATCH 05/12] cleanup --- x/upgrade/abci_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index 3361548ee..ef68a98ee 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -262,7 +262,7 @@ func TestSkipUpgradeSkipMinor(t *testing.T) { height int64 = 10 name = "minor" ) - s := setupTest(10, map[int64]bool{planHeight: true}) + s := setupTest(height, map[int64]bool{planHeight: true}) newCtx := s.ctx @@ -274,7 +274,7 @@ func TestSkipUpgradeSkipMinor(t *testing.T) { }}) require.NoError(t, err) - t.Log("Verify if skip upgrade flag clears upgrade plan in both cases") + t.Log("Verify if skip upgrade flag clears minor upgrade plan") VerifySet(t, map[int64]bool{planHeight: true}) newCtx = newCtx.WithBlockHeight(height) @@ -282,8 +282,7 @@ func TestSkipUpgradeSkipMinor(t *testing.T) { s.module.BeginBlock(newCtx, req) }) - // To ensure verification is being done only after both upgrades are cleared - t.Log("Verify if both proposals are cleared") + t.Log("Verify minor proposal is cleared") VerifyCleared(t, s.ctx) VerifyNotDone(t, s.ctx, name) } From b158bc27d27523243b123b6f8a7a68dccd6db0f5 Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Fri, 29 Sep 2023 10:30:28 -0400 Subject: [PATCH 06/12] fix comment --- 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 c9d7e2b9b..6eebd0bd9 100644 --- a/x/upgrade/keeper/keeper.go +++ b/x/upgrade/keeper/keeper.go @@ -457,7 +457,7 @@ type upgradeInfo struct { Name string `json:"name,omitempty"` // Height has types.Plan.Height value Height int64 `json:"height,omitempty"` - // Info is a string, but represents + // Info has types.Plan.Info value Info string `json:"info,omitempty"` } From 5fd21424554714a8b9ca96bf5e5eb4c0b2d8de0a Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Fri, 29 Sep 2023 11:53:08 -0400 Subject: [PATCH 07/12] add an extra-field test --- x/upgrade/types/plan_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/x/upgrade/types/plan_test.go b/x/upgrade/types/plan_test.go index c667275df..94dc1bc65 100644 --- a/x/upgrade/types/plan_test.go +++ b/x/upgrade/types/plan_test.go @@ -203,6 +203,13 @@ func TestIsMinorRelease(t *testing.T) { }, want: true, }, + { + name: "minor release with extra fields", + plan: types.Plan{ + Info: `{"upgradeType":"minor","extra":true}`, + }, + want: true, + }, { name: "not a minor release", plan: types.Plan{ From 34395ca83bc11d29ff3650b43a88eb749610ee1f Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Mon, 2 Oct 2023 11:04:27 -0400 Subject: [PATCH 08/12] avoid clearing or applying upgrades before block height --- x/upgrade/abci.go | 9 +-------- x/upgrade/abci_test.go | 2 -- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index a4c3364dd..ea111a790 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -72,19 +72,12 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { // If running a pending minor release, apply the upgrade if handler is present // Minor releases are allowed to run before the scheduled upgrade height, but not required to. if plan.UpgradeDetails().IsMinorRelease() { - // if the pending upgrade is skipped, don't apply it - if k.IsSkipHeight(plan.Height) { - skipUpgrade(k, ctx, plan) - return - } // if not yet present, then emit a scheduled log (every 100 blocks, to reduce logs) - if !k.HasHandler(plan.Name) { + if !k.HasHandler(plan.Name) && !k.IsSkipHeight(plan.Height) { if ctx.BlockHeight()%100 == 0 { ctx.Logger().Info(BuildUpgradeScheduledMsg(plan)) } - return } - applyUpgrade(k, ctx, plan) return } diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index ef68a98ee..daf5ab53a 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -282,8 +282,6 @@ func TestSkipUpgradeSkipMinor(t *testing.T) { s.module.BeginBlock(newCtx, req) }) - t.Log("Verify minor proposal is cleared") - VerifyCleared(t, s.ctx) VerifyNotDone(t, s.ctx, name) } From 83ad3787c7c8aa8e977b9770d61efce67daa7fc6 Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Mon, 2 Oct 2023 15:04:04 -0400 Subject: [PATCH 09/12] add log for parse issue with upgrade details --- x/upgrade/abci.go | 28 +++++++++++++++++++--------- x/upgrade/abci_test.go | 15 +++++++++++++++ x/upgrade/types/plan.go | 10 ++++++---- x/upgrade/types/plan_test.go | 24 ++++++++++++++++++------ 4 files changed, 58 insertions(+), 19 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index ea111a790..4442a89a5 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -12,14 +12,20 @@ import ( abci "github.com/tendermint/tendermint/abci/types" ) -// 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. -// If the plan is not ready, it will ensure the handler is not registered too early (and abort otherwise). +// BeginBlocker checks for a scheduled upgrade plan and determines whether it's time to execute the upgrade. +// If DowngradeVerified is false, it sets it to true and performs a check to ensure that the binary version is correct. +// If no plan is found, or the plan should not execute yet, it returns early. +// If the plan is set to be skipped at the current block height, it clears the upgrade plan. +// If the plan should execute and a handler for the upgrade is present, it applies the upgrade. +// If no handler is found, it panics indicating that an upgrade is needed. +// For minor releases that are pending, if a handler is not found and the plan height is not set to be skipped, +// it logs a scheduled upgrade message every 100 blocks. +// If a handler for a non-minor upgrade is found before the plan should execute, it panics indicating that the binary +// has been updated too early. // -// The purpose is to ensure the binary is switched EXACTLY at the desired block, and to allow -// a migration to be executed if needed upon this switch (migration defined in the new binary) -// skipUpgradeHeightArray is a set of block heights for which the upgrade must be skipped +// The purpose of BeginBlocker is to ensure that the upgrade binary is switched at the exact block height specified +// in the plan, and to coordinate the execution of any necessary migrations defined in the new binary. +// 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) @@ -65,13 +71,17 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { if !k.HasHandler(plan.Name) { panicUpgradeNeeded(k, ctx, plan) } - applyUpgrade(k, ctx, plan) return } + details, err := plan.UpgradeDetails() + if err != nil { + ctx.Logger().Error("error parsing upgrade info", "err", err.Error()) + } + // If running a pending minor release, apply the upgrade if handler is present // Minor releases are allowed to run before the scheduled upgrade height, but not required to. - if plan.UpgradeDetails().IsMinorRelease() { + if details.IsMinorRelease() { // if not yet present, then emit a scheduled log (every 100 blocks, to reduce logs) if !k.HasHandler(plan.Name) && !k.IsSkipHeight(plan.Height) { if ctx.BlockHeight()%100 == 0 { diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index daf5ab53a..eb044f20d 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -508,6 +508,21 @@ func TestBinaryVersion(t *testing.T) { }, false, }, + { + "test not panic: upgrade with bad json in info field is in future", + func() (sdk.Context, abci.RequestBeginBlock) { + err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{ + Title: "Upgrade test", + Plan: types.Plan{Name: "test2", Height: 105, Info: "bad json"}, + }) + require.NoError(t, err) + + newCtx := s.ctx.WithBlockHeight(100) + req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} + return newCtx, req + }, + false, + }, { "test panic: minor version upgrade is due", func() (sdk.Context, abci.RequestBeginBlock) { diff --git a/x/upgrade/types/plan.go b/x/upgrade/types/plan.go index 358064c47..a5e5286f5 100644 --- a/x/upgrade/types/plan.go +++ b/x/upgrade/types/plan.go @@ -56,13 +56,15 @@ type UpgradeDetails struct { // UpgradeDetails parses and returns a details struct from the Info field of a Plan // The upgrade.pb.go is generated from proto, so this is separated here -func (p Plan) UpgradeDetails() UpgradeDetails { +func (p Plan) UpgradeDetails() (UpgradeDetails, error) { + if p.Info == "" { + return UpgradeDetails{}, nil + } var details UpgradeDetails if err := json.Unmarshal([]byte(p.Info), &details); err != nil { - // invalid json, assume no upgrade details - return UpgradeDetails{} + return UpgradeDetails{}, err } - return details + return details, nil } // IsMinorRelease returns true if the upgrade is a minor release diff --git a/x/upgrade/types/plan_test.go b/x/upgrade/types/plan_test.go index 94dc1bc65..8c0aca21e 100644 --- a/x/upgrade/types/plan_test.go +++ b/x/upgrade/types/plan_test.go @@ -176,25 +176,32 @@ func TestUpgradeDetails(t *testing.T) { plan: types.Plan{ Info: `{upgradeType:"minor"}`, }, - want: types.UpgradeDetails{}, + want: types.UpgradeDetails{}, + wantErr: true, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := test.plan.UpgradeDetails() + got, err := test.plan.UpgradeDetails() if got != test.want { t.Errorf("UpgradeDetails() = %v, want %v", got, test.want) } + if test.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } }) } } func TestIsMinorRelease(t *testing.T) { tests := []struct { - name string - plan types.Plan - want bool + name string + plan types.Plan + want bool + wantErr bool }{ { name: "minor release", @@ -228,10 +235,15 @@ func TestIsMinorRelease(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ud := test.plan.UpgradeDetails() + ud, err := test.plan.UpgradeDetails() if got := ud.IsMinorRelease(); got != test.want { t.Errorf("IsMinorRelease() = %v, want %v", got, test.want) } + if test.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } }) } } From 46be65191e2dad9481350002058ce53b34145c5a Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Mon, 2 Oct 2023 15:36:28 -0400 Subject: [PATCH 10/12] Revert "add log for parse issue with upgrade details" This reverts commit 83ad3787c7c8aa8e977b9770d61efce67daa7fc6. --- x/upgrade/abci.go | 28 +++++++++------------------- x/upgrade/abci_test.go | 15 --------------- x/upgrade/types/plan.go | 10 ++++------ x/upgrade/types/plan_test.go | 24 ++++++------------------ 4 files changed, 19 insertions(+), 58 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index 4442a89a5..ea111a790 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -12,20 +12,14 @@ import ( abci "github.com/tendermint/tendermint/abci/types" ) -// BeginBlocker checks for a scheduled upgrade plan and determines whether it's time to execute the upgrade. -// If DowngradeVerified is false, it sets it to true and performs a check to ensure that the binary version is correct. -// If no plan is found, or the plan should not execute yet, it returns early. -// If the plan is set to be skipped at the current block height, it clears the upgrade plan. -// If the plan should execute and a handler for the upgrade is present, it applies the upgrade. -// If no handler is found, it panics indicating that an upgrade is needed. -// For minor releases that are pending, if a handler is not found and the plan height is not set to be skipped, -// it logs a scheduled upgrade message every 100 blocks. -// If a handler for a non-minor upgrade is found before the plan should execute, it panics indicating that the binary -// has been updated too early. +// 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. +// If the plan is not ready, it will ensure the handler is not registered too early (and abort otherwise). // -// The purpose of BeginBlocker is to ensure that the upgrade binary is switched at the exact block height specified -// in the plan, and to coordinate the execution of any necessary migrations defined in the new binary. -// skipUpgradeHeightArray is a set of block heights for which the upgrade must be skipped. +// The purpose is to ensure the binary is switched EXACTLY at the desired block, and to allow +// a migration to be executed if needed upon this switch (migration defined in the new binary) +// 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) @@ -71,17 +65,13 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { if !k.HasHandler(plan.Name) { panicUpgradeNeeded(k, ctx, plan) } + applyUpgrade(k, ctx, plan) return } - details, err := plan.UpgradeDetails() - if err != nil { - ctx.Logger().Error("error parsing upgrade info", "err", err.Error()) - } - // If running a pending minor release, apply the upgrade if handler is present // Minor releases are allowed to run before the scheduled upgrade height, but not required to. - if details.IsMinorRelease() { + if plan.UpgradeDetails().IsMinorRelease() { // if not yet present, then emit a scheduled log (every 100 blocks, to reduce logs) if !k.HasHandler(plan.Name) && !k.IsSkipHeight(plan.Height) { if ctx.BlockHeight()%100 == 0 { diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index eb044f20d..daf5ab53a 100644 --- a/x/upgrade/abci_test.go +++ b/x/upgrade/abci_test.go @@ -508,21 +508,6 @@ func TestBinaryVersion(t *testing.T) { }, false, }, - { - "test not panic: upgrade with bad json in info field is in future", - func() (sdk.Context, abci.RequestBeginBlock) { - err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{ - Title: "Upgrade test", - Plan: types.Plan{Name: "test2", Height: 105, Info: "bad json"}, - }) - require.NoError(t, err) - - newCtx := s.ctx.WithBlockHeight(100) - req := abci.RequestBeginBlock{Header: newCtx.BlockHeader()} - return newCtx, req - }, - false, - }, { "test panic: minor version upgrade is due", func() (sdk.Context, abci.RequestBeginBlock) { diff --git a/x/upgrade/types/plan.go b/x/upgrade/types/plan.go index a5e5286f5..358064c47 100644 --- a/x/upgrade/types/plan.go +++ b/x/upgrade/types/plan.go @@ -56,15 +56,13 @@ type UpgradeDetails struct { // UpgradeDetails parses and returns a details struct from the Info field of a Plan // The upgrade.pb.go is generated from proto, so this is separated here -func (p Plan) UpgradeDetails() (UpgradeDetails, error) { - if p.Info == "" { - return UpgradeDetails{}, nil - } +func (p Plan) UpgradeDetails() UpgradeDetails { var details UpgradeDetails if err := json.Unmarshal([]byte(p.Info), &details); err != nil { - return UpgradeDetails{}, err + // invalid json, assume no upgrade details + return UpgradeDetails{} } - return details, nil + return details } // IsMinorRelease returns true if the upgrade is a minor release diff --git a/x/upgrade/types/plan_test.go b/x/upgrade/types/plan_test.go index 8c0aca21e..94dc1bc65 100644 --- a/x/upgrade/types/plan_test.go +++ b/x/upgrade/types/plan_test.go @@ -176,32 +176,25 @@ func TestUpgradeDetails(t *testing.T) { plan: types.Plan{ Info: `{upgradeType:"minor"}`, }, - want: types.UpgradeDetails{}, - wantErr: true, + want: types.UpgradeDetails{}, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got, err := test.plan.UpgradeDetails() + got := test.plan.UpgradeDetails() if got != test.want { t.Errorf("UpgradeDetails() = %v, want %v", got, test.want) } - if test.wantErr { - require.Error(t, err) - } else { - require.NoError(t, err) - } }) } } func TestIsMinorRelease(t *testing.T) { tests := []struct { - name string - plan types.Plan - want bool - wantErr bool + name string + plan types.Plan + want bool }{ { name: "minor release", @@ -235,15 +228,10 @@ func TestIsMinorRelease(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ud, err := test.plan.UpgradeDetails() + ud := test.plan.UpgradeDetails() if got := ud.IsMinorRelease(); got != test.want { t.Errorf("IsMinorRelease() = %v, want %v", got, test.want) } - if test.wantErr { - require.Error(t, err) - } else { - require.NoError(t, err) - } }) } } From f07c1dd88a5e9690cd4586a36c4ecb2d7c417909 Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Mon, 2 Oct 2023 15:39:22 -0400 Subject: [PATCH 11/12] narrow the scope of log change --- x/upgrade/abci.go | 7 ++++++- x/upgrade/types/plan.go | 6 +++--- x/upgrade/types/plan_test.go | 4 ++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index ea111a790..e0791bc0c 100644 --- a/x/upgrade/abci.go +++ b/x/upgrade/abci.go @@ -69,9 +69,14 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { return } + details, err := plan.UpgradeDetails() + if err != nil { + ctx.Logger().Error("failed to parse upgrade details", "err", err) + } + // If running a pending minor release, apply the upgrade if handler is present // Minor releases are allowed to run before the scheduled upgrade height, but not required to. - if plan.UpgradeDetails().IsMinorRelease() { + if details.IsMinorRelease() { // if not yet present, then emit a scheduled log (every 100 blocks, to reduce logs) if !k.HasHandler(plan.Name) && !k.IsSkipHeight(plan.Height) { if ctx.BlockHeight()%100 == 0 { diff --git a/x/upgrade/types/plan.go b/x/upgrade/types/plan.go index 358064c47..74031b1ca 100644 --- a/x/upgrade/types/plan.go +++ b/x/upgrade/types/plan.go @@ -56,13 +56,13 @@ type UpgradeDetails struct { // UpgradeDetails parses and returns a details struct from the Info field of a Plan // The upgrade.pb.go is generated from proto, so this is separated here -func (p Plan) UpgradeDetails() UpgradeDetails { +func (p Plan) UpgradeDetails() (UpgradeDetails, error) { var details UpgradeDetails if err := json.Unmarshal([]byte(p.Info), &details); err != nil { // invalid json, assume no upgrade details - return UpgradeDetails{} + return UpgradeDetails{}, err } - return details + return details, nil } // IsMinorRelease returns true if the upgrade is a minor release diff --git a/x/upgrade/types/plan_test.go b/x/upgrade/types/plan_test.go index 94dc1bc65..b138fdc0d 100644 --- a/x/upgrade/types/plan_test.go +++ b/x/upgrade/types/plan_test.go @@ -182,7 +182,7 @@ func TestUpgradeDetails(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := test.plan.UpgradeDetails() + got, _ := test.plan.UpgradeDetails() if got != test.want { t.Errorf("UpgradeDetails() = %v, want %v", got, test.want) } @@ -228,7 +228,7 @@ func TestIsMinorRelease(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ud := test.plan.UpgradeDetails() + ud, _ := test.plan.UpgradeDetails() if got := ud.IsMinorRelease(); got != test.want { t.Errorf("IsMinorRelease() = %v, want %v", got, test.want) } From 73b405dbb125ae69d9dd6ec21309f496934f60eb Mon Sep 17 00:00:00 2001 From: Steven Landers Date: Mon, 2 Oct 2023 15:41:37 -0400 Subject: [PATCH 12/12] handle explicit empty info case (normal) --- x/upgrade/types/plan.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x/upgrade/types/plan.go b/x/upgrade/types/plan.go index 74031b1ca..6104e883c 100644 --- a/x/upgrade/types/plan.go +++ b/x/upgrade/types/plan.go @@ -57,6 +57,9 @@ type UpgradeDetails struct { // UpgradeDetails parses and returns a details struct from the Info field of a Plan // The upgrade.pb.go is generated from proto, so this is separated here func (p Plan) UpgradeDetails() (UpgradeDetails, error) { + if p.Info == "" { + return UpgradeDetails{}, nil + } var details UpgradeDetails if err := json.Unmarshal([]byte(p.Info), &details); err != nil { // invalid json, assume no upgrade details