Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/upgrade: remove support for time based upgrades #8849

Merged
merged 15 commits into from
Mar 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#8363](https://github.com/cosmos/cosmos-sdk/pull/8363) Addresses no longer have a fixed 20-byte length. From the SDK modules' point of view, any 1-255 bytes-long byte array is a valid address.
* [\#8346](https://github.com/cosmos/cosmos-sdk/pull/8346) All CLI `tx` commands generate ServiceMsgs by default. Graceful Amino support has been added to ServiceMsgs to support signing legacy Msgs.
* (crypto/ed25519) [\#8690] Adopt zip1215 ed2559 verification rules.
* [\#8849](https://github.com/cosmos/cosmos-sdk/pull/8849) Upgrade module no longer supports time based upgrades.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we want to add a Deprecated section and add this to that

alessio marked this conversation as resolved.
Show resolved Hide resolved


### API Breaking Changes
Expand Down
2 changes: 2 additions & 0 deletions buf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ lint:
breaking:
use:
- FILE
except:
- FIELD_NO_DELETE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we had some comments about this line indicating that this still requires reserving the name and number. Could you add quickly in a separate PR @technicallyty ?

ignore:
- tendermint
- gogoproto
Expand Down
1 change: 0 additions & 1 deletion docs/core/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -7362,7 +7362,6 @@ Plan specifies information about a planned upgrade and when it should occur.
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| `name` | [string](#string) | | Sets the name for the upgrade. This name will be used by the upgraded version of the software to apply any special "on-upgrade" commands during the first BeginBlock method after the upgrade is applied. It is also used to detect whether a software version can handle a given upgrade. If no upgrade handler with this name has been set in the software, it will be assumed that the software is out-of-date when the upgrade Time or Height is reached and the software will exit. |
| `time` | [google.protobuf.Timestamp](#google.protobuf.Timestamp) | | The time after which the upgrade must be performed. Leave set to its zero value to use a pre-defined Height instead. |
| `height` | [int64](#int64) | | The height at which the upgrade must be performed. Only used if Time is not set. |
| `info` | [string](#string) | | Any application specific upgrade info to be included on-chain such as a git commit that validators could automatically upgrade to |

Expand Down
7 changes: 4 additions & 3 deletions proto/cosmos/upgrade/v1beta1/upgrade.proto
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ message Plan {
// reached and the software will exit.
string name = 1;

// The time after which the upgrade must be performed.
// Leave set to its zero value to use a pre-defined Height instead.
google.protobuf.Timestamp time = 2 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false];
// Time based upgrades have been deprecated. Time based upgrade logic
// has been removed from the SDK.
reserved 2;
reserved "time";

// The height at which the upgrade must be performed.
// Only used if Time is not set.
Expand Down
1 change: 0 additions & 1 deletion x/gov/legacy/v040/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ func migrateContent(oldContent v036gov.Content) *codectypes.Any {
Title: oldContent.Title,
Plan: v040upgrade.Plan{
Name: oldContent.Plan.Name,
Time: oldContent.Plan.Time,
Height: oldContent.Plan.Height,
Info: oldContent.Plan.Info,
},
Expand Down
3 changes: 1 addition & 2 deletions x/gov/legacy/v040/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ func TestMigrate(t *testing.T) {
"plan": {
"height": "123",
"info": "foo_upgrade_info",
"name": "foo_upgrade_name",
"time": "0001-01-01T00:00:00Z"
"name": "foo_upgrade_name"
},
"title": "foo_software_upgrade"
},
Expand Down
45 changes: 9 additions & 36 deletions x/upgrade/abci_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,36 +68,13 @@ func TestRequireName(t *testing.T) {
require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err)
}

func TestRequireFutureTime(t *testing.T) {
s := setupTest(10, map[int64]bool{})
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Time: s.ctx.BlockHeader().Time}})
require.NotNil(t, err)
require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err)
}

func TestRequireFutureBlock(t *testing.T) {
s := setupTest(10, map[int64]bool{})
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight()}})
require.NotNil(t, err)
require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err)
}

func TestCantSetBothTimeAndHeight(t *testing.T) {
s := setupTest(10, map[int64]bool{})
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Time: time.Now(), Height: s.ctx.BlockHeight() + 1}})
require.NotNil(t, err)
require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err)
}

func TestDoTimeUpgrade(t *testing.T) {
s := setupTest(10, map[int64]bool{})
t.Log("Verify can schedule an upgrade")
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Time: time.Now()}})
require.Nil(t, err)

VerifyDoUpgrade(t)
}

func TestDoHeightUpgrade(t *testing.T) {
s := setupTest(10, map[int64]bool{})
t.Log("Verify can schedule an upgrade")
Expand All @@ -119,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 := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
Expand All @@ -137,7 +114,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 := abci.RequestBeginBlock{Header: newCtx.BlockHeader()}
require.Panics(t, func() {
s.module.BeginBlock(newCtx, req)
Expand Down Expand Up @@ -195,7 +172,7 @@ func VerifyCleared(t *testing.T, newCtx sdk.Context) {
func TestCanClear(t *testing.T) {
s := setupTest(10, map[int64]bool{})
t.Log("Verify upgrade is scheduled")
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Time: time.Now()}})
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: s.ctx.BlockHeight() + 100}})
require.Nil(t, err)

err = s.handler(s.ctx, &types.CancelSoftwareUpgradeProposal{Title: "cancel"})
Expand All @@ -206,11 +183,12 @@ func TestCanClear(t *testing.T) {

func TestCantApplySameUpgradeTwice(t *testing.T) {
s := setupTest(10, map[int64]bool{})
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Time: time.Now()}})
height := s.ctx.BlockHeader().Height + 1
err := s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: height}})
require.Nil(t, err)
VerifyDoUpgrade(t)
t.Log("Verify an executed upgrade \"test\" can't be rescheduled")
err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Time: time.Now()}})
err = s.handler(s.ctx, &types.SoftwareUpgradeProposal{Title: "prop", Plan: types.Plan{Name: "test", Height: height}})
require.NotNil(t, err)
require.True(t, errors.Is(sdkerrors.ErrInvalidRequest, err), err)
}
Expand All @@ -225,20 +203,15 @@ func TestNoSpuriousUpgrades(t *testing.T) {
}

func TestPlanStringer(t *testing.T) {
ti, err := time.Parse(time.RFC3339, "2020-01-01T00:00:00Z")
require.Nil(t, err)
require.Equal(t, `Upgrade Plan
Name: test
Time: 2020-01-01T00:00:00Z
Info: .`, types.Plan{Name: "test", Time: ti}.String())
require.Equal(t, `Upgrade Plan
Name: test
Height: 100
Info: .`, types.Plan{Name: "test", Height: 100}.String())
Info: .`, types.Plan{Name: "test", Height: 100, Info: ""}.String())

require.Equal(t, fmt.Sprintf(`Upgrade Plan
Name: test
Height: 100
Info: .`), types.Plan{Name: "test", Height: 100}.String())
Info: .`), types.Plan{Name: "test", Height: 100, Info: ""}.String())
}

func VerifyNotDone(t *testing.T, newCtx sdk.Context, name string) {
Expand Down
33 changes: 4 additions & 29 deletions x/upgrade/client/cli/tx.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
package cli

import (
"fmt"
"time"

"github.com/spf13/cobra"

"github.com/cosmos/cosmos-sdk/client"
Expand All @@ -16,11 +13,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"
)

Expand All @@ -37,11 +30,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/cosmos/cosmos-sdk/tree/master/cosmovisor",
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
Expand Down Expand Up @@ -84,8 +77,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
Expand Down Expand Up @@ -168,29 +160,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
}
12 changes: 1 addition & 11 deletions x/upgrade/client/rest/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package rest

import (
"net/http"
"time"

"github.com/cosmos/cosmos-sdk/client/tx"

Expand Down Expand Up @@ -32,7 +31,6 @@ type PlanRequest struct {
Deposit sdk.Coins `json:"deposit" yaml:"deposit"`
UpgradeName string `json:"upgrade_name" yaml:"upgrade_name"`
UpgradeHeight int64 `json:"upgrade_height" yaml:"upgrade_height"`
UpgradeTime string `json:"upgrade_time" yaml:"upgrade_time"`
UpgradeInfo string `json:"upgrade_info" yaml:"upgrade_info"`
}

Expand Down Expand Up @@ -76,15 +74,7 @@ func newPostPlanHandler(clientCtx client.Context) http.HandlerFunc {
return
}

var t time.Time
if req.UpgradeTime != "" {
t, err = time.Parse(time.RFC3339, req.UpgradeTime)
if rest.CheckBadRequestError(w, err) {
return
}
}

plan := types.Plan{Name: req.UpgradeName, Time: t, Height: req.UpgradeHeight, Info: req.UpgradeInfo}
plan := types.Plan{Name: req.UpgradeName, Height: req.UpgradeHeight, Info: req.UpgradeInfo}
content := types.NewSoftwareUpgradeProposal(req.Title, req.Description, plan)
msg, err := govtypes.NewMsgSubmitProposal(content, req.Deposit, fromAddr)
if rest.CheckBadRequestError(w, err) {
Expand Down
10 changes: 5 additions & 5 deletions x/upgrade/doc.go
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for thinking about this file and the SPEC 👍

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.

Expand Down Expand Up @@ -54,7 +54,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.
Expand Down
6 changes: 1 addition & 5 deletions x/upgrade/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,7 @@ func (k Keeper) ScheduleUpgrade(ctx sdk.Context, plan types.Plan) error {
return err
}

if plan.Time.Unix() > 0 {
if !plan.Time.After(ctx.BlockHeader().Time) {
return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "upgrade cannot be scheduled in the past")
}
} else if plan.Height <= ctx.BlockHeight() {
if plan.Height <= ctx.BlockHeight() {
return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "upgrade cannot be scheduled in the past")
}

Expand Down
20 changes: 0 additions & 20 deletions x/upgrade/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,6 @@ func (s *KeeperTestSuite) TestScheduleUpgrade() {
setup func()
expPass bool
}{
{
name: "successful time schedule",
plan: types.Plan{
Name: "all-good",
Info: "some text here",
Time: s.ctx.BlockTime().Add(time.Hour),
},
setup: func() {},
expPass: true,
},
{
name: "successful height schedule",
plan: types.Plan{
Expand Down Expand Up @@ -107,16 +97,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{
Expand Down
7 changes: 3 additions & 4 deletions x/upgrade/spec/01_concepts.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -30,7 +30,6 @@ for more info.
```go
type Plan struct {
Name string
Time Time
Height int64
Info string
}
Expand All @@ -52,7 +51,7 @@ type UpgradeHandler func(Context, Plan)
```

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.

Expand Down Expand Up @@ -88,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 {
Expand Down
2 changes: 1 addition & 1 deletion x/upgrade/spec/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading