diff --git a/x/upgrade/abci.go b/x/upgrade/abci.go index bfb7ff964..e0791bc0c 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.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)) } } } - if !found { + if !planFound { return } @@ -53,41 +53,40 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { }, ) - // To make sure clear upgrade is executed at the same block + // 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()) { - 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) + } + applyUpgrade(k, ctx, plan) + return + } - 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) + details, err := plan.UpgradeDetails() + if err != nil { + ctx.Logger().Error("failed to parse upgrade details", "err", err) + } - panic(upgradeMsg) + // 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 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 { + ctx.Logger().Info(BuildUpgradeScheduledMsg(plan)) + } } - // 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) return } - // if we have a pending upgrade, but it is not yet time, make sure we did not - // set the handler already + // 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) @@ -95,7 +94,40 @@ func BeginBlocker(k keeper.Keeper, ctx sdk.Context, _ abci.RequestBeginBlock) { } } +// 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. + 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) + 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. 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) +} diff --git a/x/upgrade/abci_test.go b/x/upgrade/abci_test.go index d9af90d70..daf5ab53a 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 { @@ -254,6 +256,35 @@ 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(height, 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 minor upgrade plan") + VerifySet(t, map[int64]bool{planHeight: true}) + + newCtx = newCtx.WithBlockHeight(height) + require.NotPanics(t, func() { + s.module.BeginBlock(newCtx, req) + }) + + VerifyNotDone(t, s.ctx, name) +} + func TestSkipUpgradeSkippingAll(t *testing.T) { var ( skipOne int64 = 11 @@ -462,6 +493,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: 105, Info: minorUpgradeInfo}, + }) + 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) { + 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..6eebd0bd9 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 has types.Plan.Info value Info string `json:"info,omitempty"` } diff --git a/x/upgrade/types/plan.go b/x/upgrade/types/plan.go index 9e4bc85ab..6104e883c 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" @@ -45,3 +47,28 @@ 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, 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{}, err + } + return details, nil +} + +// 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..b138fdc0d 100644 --- a/x/upgrade/types/plan_test.go +++ b/x/upgrade/types/plan_test.go @@ -154,3 +154,84 @@ func TestShouldExecute(t *testing.T) { }) } } + +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: "minor release with extra fields", + plan: types.Plan{ + Info: `{"upgradeType":"minor","extra":true}`, + }, + 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) + } + }) + } +}