Revert "add log for parse issue with upgrade details"

This reverts commit 83ad378.

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 {

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) {

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

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