Allow minor releases before the block height (#320)

## Describe your changes and provide context
- If a pending minor release is running and it's before the block
height, apply the upgrade
- If a pending release has been applied and before the block height,
don't panic
- If a release is past the block height, regardless of type, panic 

One can pass an upgradeType to info as follows
```
seid tx gov submit-proposal software-upgrade ... --upgrade-info '{"upgradeType":"minor"}'
```

## Testing performed to validate your change
- Unit Tests
- Local Testing:
- Passed minor upgrade proposal in future block, and verified I could
upgrade before that block
- Passed minor upgrade proposal in future block, and verified it panics
if not upgraded at that block
- Passed major upgrade proposal in future block, and verified I could
NOT upgrade before that block
- Skip height continues to work for major and minor releases (a
skip-height applies to the _deadline_ block for minor)

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,49 +53,81 @@ 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)
 		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.
+	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)
+}

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 {

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"`
 }
 

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")
+}

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)
+			}
+		})
+	}
+}