feat: remove time-based upgrade (#889)

* feat: remove time-based upgrade

* test: fix typo

* docs: add changelog

* test: fix validate func

* chore: revert irrelevant test

* fix: fix typo, chore: remove __debug_bin binary

* test: restore height based plan test

Author: dudong2

CHANGELOG.md

@@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/wasm) [\#850](https://github.com/line/lbm-sdk/pull/850) remove `x/wasm` module in lbm-sdk
 * (log) [\#883](https://github.com/line/lbm-sdk/pull/883) add zerolog based rolling log system
 * (Ostracon) [\#887](https://github.com/line/lbm-sdk/pull/887) apply the changes of vrf location in Ostracon
+* (x/upgrade) [\#889](https://github.com/line/lbm-sdk/pull/889) remove time based upgrade
 
 ### Improvements
 * (cosmovisor) [\#792](https://github.com/line/lbm-sdk/pull/792) Use upstream's cosmovisor

x/upgrade/abci_test.go

@@ -96,7 +96,7 @@ func TestCanOverwriteScheduleUpgrade(t *testing.T) {
 }
 
 func VerifyDoUpgrade(t *testing.T) {
-	t.Log("Verify that a panic happens at the upgrade time/height")
+	t.Log("Verify that a panic happens at the upgrade height")
 	newCtx := s.ctx.WithBlockHeight(s.ctx.BlockHeight() + 1).WithBlockTime(time.Now())
 
 	req := ocabci.RequestBeginBlock{Header: newCtx.BlockHeader()}
@@ -116,7 +116,7 @@ func VerifyDoUpgrade(t *testing.T) {
 }
 
 func VerifyDoUpgradeWithCtx(t *testing.T, newCtx sdk.Context, proposalName string) {
-	t.Log("Verify that a panic happens at the upgrade time/height")
+	t.Log("Verify that a panic happens at the upgrade height")
 	req := ocabci.RequestBeginBlock{Header: newCtx.BlockHeader()}
 	require.Panics(t, func() {
 		s.module.BeginBlock(newCtx, req)

x/upgrade/client/cli/tx.go

@@ -1,9 +1,6 @@
 package cli
 
 import (
-	"fmt"
-	"time"
-
 	"github.com/spf13/cobra"
 
 	"github.com/line/lbm-sdk/client"
@@ -15,11 +12,7 @@ import (
 )
 
 const (
-	// TimeFormat specifies ISO UTC format for submitting the time for a new upgrade proposal
-	TimeFormat = "2006-01-02T15:04:05Z"
-
 	FlagUpgradeHeight = "upgrade-height"
-	FlagUpgradeTime   = "upgrade-time"
 	FlagUpgradeInfo   = "upgrade-info"
 )
 
@@ -36,11 +29,11 @@ func GetTxCmd() *cobra.Command {
 // NewCmdSubmitUpgradeProposal implements a command handler for submitting a software upgrade proposal transaction.
 func NewCmdSubmitUpgradeProposal() *cobra.Command {
 	cmd := &cobra.Command{
-		Use:   "software-upgrade [name] (--upgrade-height [height] | --upgrade-time [time]) (--upgrade-info [info]) [flags]",
+		Use:   "software-upgrade [name] (--upgrade-height [height]) (--upgrade-info [info]) [flags]",
 		Args:  cobra.ExactArgs(1),
 		Short: "Submit a software upgrade proposal",
 		Long: "Submit a software upgrade along with an initial deposit.\n" +
-			"Please specify a unique name and height OR time for the upgrade to take effect.\n" +
+			"Please specify a unique name and height for the upgrade to take effect.\n" +
 			"You may include info to reference a binary download link, in a format compatible with: https://github.com/line/lbm-sdk/tree/master/cosmovisor",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			clientCtx, err := client.GetClientTxContext(cmd)
@@ -76,8 +69,7 @@ func NewCmdSubmitUpgradeProposal() *cobra.Command {
 	cmd.Flags().String(cli.FlagTitle, "", "title of proposal")
 	cmd.Flags().String(cli.FlagDescription, "", "description of proposal")
 	cmd.Flags().String(cli.FlagDeposit, "", "deposit of proposal")
-	cmd.Flags().Int64(FlagUpgradeHeight, 0, "The height at which the upgrade must happen (not to be used together with --upgrade-time)")
-	cmd.Flags().String(FlagUpgradeTime, "", fmt.Sprintf("The time at which the upgrade must happen (ex. %s) (not to be used together with --upgrade-height)", TimeFormat))
+	cmd.Flags().Int64(FlagUpgradeHeight, 0, "The height at which the upgrade must happen")
 	cmd.Flags().String(FlagUpgradeInfo, "", "Optional info for the planned upgrade such as commit hash, etc.")
 
 	return cmd
@@ -153,29 +145,12 @@ func parseArgsToContent(cmd *cobra.Command, name string) (gov.Content, error) {
 		return nil, err
 	}
 
-	timeStr, err := cmd.Flags().GetString(FlagUpgradeTime)
-	if err != nil {
-		return nil, err
-	}
-
-	if height != 0 && len(timeStr) != 0 {
-		return nil, fmt.Errorf("only one of --upgrade-time or --upgrade-height should be specified")
-	}
-
-	var upgradeTime time.Time
-	if len(timeStr) != 0 {
-		upgradeTime, err = time.Parse(TimeFormat, timeStr)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	info, err := cmd.Flags().GetString(FlagUpgradeInfo)
 	if err != nil {
 		return nil, err
 	}
 
-	plan := types.Plan{Name: name, Time: upgradeTime, Height: height, Info: info}
+	plan := types.Plan{Name: name, Height: height, Info: info}
 	content := types.NewSoftwareUpgradeProposal(title, description, plan)
 	return content, nil
 }

x/upgrade/doc.go

@@ -1,7 +1,7 @@
 /*
 Package upgrade provides a Cosmos SDK module that can be used for smoothly upgrading a live Cosmos chain to a
 new software version. It accomplishes this by providing a BeginBlocker hook that prevents the blockchain state
-machine from proceeding once a pre-defined upgrade block time or height has been reached. The module does not prescribe
+machine from proceeding once a pre-defined upgrade block height has been reached. The module does not prescribe
 anything regarding how governance decides to do an upgrade, but just the mechanism for coordinating the upgrade safely.
 Without software support for upgrades, upgrading a live chain is risky because all of the validators need to pause
 their state machines at exactly the same point in the process. If this is not done correctly, there can be state
@@ -21,9 +21,9 @@ perform a migration, but also to identify if this is the old or new version (eg.
 a handler registered for the named upgrade).
 
 Once the release candidate along with an appropriate upgrade handler is frozen,
-we can have a governance vote to approve this upgrade at some future block time
-or block height (e.g. 200000). This is known as an upgrade.Plan. The v0.38.0 code will not know of this
-handler, but will continue to run until block 200000, when the plan kicks in at BeginBlock. It will check
+we can have a governance vote to approve this upgrade at some future block height (e.g. 200000).
+This is known as an upgrade.Plan. The v0.38.0 code will not know of this handler, but will
+continue to run until block 200000, when the plan kicks in at BeginBlock. It will check
 for existence of the handler, and finding it missing, know that it is running the obsolete software,
 and gracefully exit.
 
@@ -55,7 +55,7 @@ should call ScheduleUpgrade to schedule an upgrade and ClearUpgradePlan to cance
 
 # Performing Upgrades
 
-Upgrades can be scheduled at either a predefined block height or time. Once this block height or time is reached, the
+Upgrades can be scheduled at a predefined block height. Once this block height is reached, the
 existing software will cease to process ABCI messages and a new version with code that handles the upgrade must be deployed.
 All upgrades are coordinated by a unique upgrade name that cannot be reused on the same blockchain. In order for the upgrade
 module to know that the upgrade has been safely applied, a handler with the name of the upgrade must be installed.

x/upgrade/keeper/keeper_test.go

@@ -98,16 +98,6 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() {
 			setup:   func() {},
 			expPass: false,
 		},
-		{
-			name: "unsuccessful time schedule: due date in past",
-			plan: types.Plan{
-				Name: "all-good",
-				Info: "some text here",
-				Time: s.ctx.BlockTime(),
-			},
-			setup:   func() {},
-			expPass: false,
-		},
 		{
 			name: "unsuccessful height schedule: due date in past",
 			plan: types.Plan{

x/upgrade/spec/01_concepts.md

@@ -7,7 +7,7 @@ order: 1
 ## Plan
 
 The `x/upgrade` module defines a `Plan` type in which a live upgrade is scheduled
-to occur. A `Plan` can be scheduled at a specific block height or time, but not both.
+to occur. A `Plan` can be scheduled at a specific block height.
 A `Plan` is created once a (frozen) release candidate along with an appropriate upgrade
 `Handler` (see below) is agreed upon, where the `Name` of a `Plan` corresponds to a
 specific `Handler`. Typically, a `Plan` is created through a governance proposal
@@ -52,7 +52,7 @@ type UpgradeHandler func(Context, Plan, VersionMap) (VersionMap, error)
 ```
 
 During each `EndBlock` execution, the `x/upgrade` module checks if there exists a
-`Plan` that should execute (is scheduled at that time or height). If so, the corresponding
+`Plan` that should execute (is scheduled at that height). If so, the corresponding
 `Handler` is executed. If the `Plan` is expected to execute but no `Handler` is registered
 or if the binary was upgraded too early, the node will gracefully panic and exit.
 
@@ -87,7 +87,7 @@ will ensure these `StoreUpgrades` takes place only in planned upgrade handler.
 Typically, a `Plan` is proposed and submitted through governance via a `SoftwareUpgradeProposal`.
 This proposal prescribes to the standard governance process. If the proposal passes,
 the `Plan`, which targets a specific `Handler`, is persisted and scheduled. The
-upgrade can be delayed or hastened by updating the `Plan.Time` in a new proposal.
+upgrade can be delayed or hastened by updating the `Plan.Height` in a new proposal.
 
 ```go
 type SoftwareUpgradeProposal struct {

x/upgrade/spec/README.md

@@ -12,7 +12,7 @@ parent:
 `x/upgrade` is an implementation of a Cosmos SDK module that facilitates smoothly
 upgrading a live Cosmos chain to a new (breaking) software version. It accomplishes this by
 providing a `BeginBlocker` hook that prevents the blockchain state machine from
-proceeding once a pre-defined upgrade block time or height has been reached.
+proceeding once a pre-defined upgrade block height has been reached.
 
 The module does not prescribe anything regarding how governance decides to do an
 upgrade, but just the mechanism for coordinating the upgrade safely. Without software

x/upgrade/types/plan.go

@@ -2,50 +2,36 @@ package types
 
 import (
 	"fmt"
-	"strings"
 
 	sdk "github.com/line/lbm-sdk/types"
 	sdkerrors "github.com/line/lbm-sdk/types/errors"
 )
 
 func (p Plan) String() string {
 	due := p.DueAt()
-	dueUp := strings.ToUpper(due[0:1]) + due[1:]
 	return fmt.Sprintf(`Upgrade Plan
   Name: %s
   %s
-  Info: %s.`, p.Name, dueUp, p.Info)
+  Info: %s.`, p.Name, due, p.Info)
 }
 
 // ValidateBasic does basic validation of a Plan
 func (p Plan) ValidateBasic() error {
-	if !p.Time.IsZero() {
-		return sdkerrors.ErrInvalidRequest.Wrap("time-based upgrades have been deprecated in the SDK")
-	}
 	if p.UpgradedClientState != nil {
 		return sdkerrors.ErrInvalidRequest.Wrap("upgrade logic for IBC has been moved to the IBC module")
 	}
 	if len(p.Name) == 0 {
 		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "name cannot be empty")
 	}
-	if p.Height < 0 {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "height cannot be negative")
-	}
-	if p.Time.Unix() <= 0 && p.Height == 0 {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "must set either time or height")
-	}
-	if p.Time.Unix() > 0 && p.Height != 0 {
-		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "cannot set both time and height")
+	if p.Height <= 0 {
+		return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "height must be greater than 0")
 	}
 
 	return nil
 }
 
 // ShouldExecute returns true if the Plan is ready to execute given the current context
 func (p Plan) ShouldExecute(ctx sdk.Context) bool {
-	if p.Time.Unix() > 0 {
-		return !ctx.BlockTime().Before(p.Time)
-	}
 	if p.Height > 0 {
 		return p.Height <= ctx.BlockHeight()
 	}
@@ -54,5 +40,5 @@ func (p Plan) ShouldExecute(ctx sdk.Context) bool {
 
 // DueAt is a string representation of when this plan is due to be executed
 func (p Plan) DueAt() string {
-	return fmt.Sprintf("height: %d", p.Height)
+	return fmt.Sprintf("Height: %d", p.Height)
 }

x/upgrade/types/plan_test.go

@@ -70,11 +70,6 @@ func TestPlanValid(t *testing.T) {
 				Height: 123450000,
 			},
 		},
-		"time-base upgrade": {
-			p: types.Plan{
-				Time: time.Now(),
-			},
-		},
 		"IBC upgrade": {
 			p: types.Plan{
 				Height:              123450000,
@@ -115,34 +110,6 @@ func TestShouldExecute(t *testing.T) {
 		ctxHeight int64
 		expected  bool
 	}{
-		"past time": {
-			p: types.Plan{
-				Name: "do-good",
-				Info: "some text here",
-				Time: mustParseTime("2019-07-08T11:33:55Z"),
-			},
-			ctxTime:   mustParseTime("2019-07-08T11:32:00Z"),
-			ctxHeight: 100000,
-			expected:  false,
-		},
-		"on time": {
-			p: types.Plan{
-				Name: "do-good",
-				Time: mustParseTime("2019-07-08T11:33:55Z"),
-			},
-			ctxTime:   mustParseTime("2019-07-08T11:33:55Z"),
-			ctxHeight: 100000,
-			expected:  true,
-		},
-		"future time": {
-			p: types.Plan{
-				Name: "do-good",
-				Time: mustParseTime("2019-07-08T11:33:55Z"),
-			},
-			ctxTime:   mustParseTime("2019-07-08T11:33:57Z"),
-			ctxHeight: 100000,
-			expected:  true,
-		},
 		"past height": {
 			p: types.Plan{
 				Name:   "do-good",

x/upgrade/types/proposal_test.go

@@ -26,9 +26,9 @@ func TestContentAccessors(t *testing.T) {
 	}{
 		"upgrade": {
 			p: types.NewSoftwareUpgradeProposal("Title", "desc", types.Plan{
-				Name: "due_time",
-				Info: "https://foo.bar",
-				Time: mustParseTime("2019-07-08T11:33:55Z"),
+				Name:   "due_height",
+				Info:   "https://foo.bar",
+				Height: 99999999999,
 			}),
 			title: "Title",
 			desc:  "desc",