feat!: add a delay between quorum and upgrade height (#3560)

Closes #3552

Ready for review but I won't merge until we reach social consensus on
what the upgrade height delay should be.

## Testing

Manually tested the new CLI commands by staring single node script and
then:

```shell
$ celestia-appd query signal upgrade
no upgrade is pending.

$ celestia-appd tx signal signal 3 --from $VALIDATOR --fees $DEFAULT_FEES --broadcast-mode $BROADCAST_MODE --yes

$ celestia-appd tx signal try-upgrade  --from $VALIDATOR --fees $DEFAULT_FEES --broadcast-mode $BROADCAST_MODE --yes

$ celestia-appd query signal upgrade
upgrade is pending to app version 3 at height 151345.
```

---------

Co-authored-by: Sanaz Taheri <[email protected]>

app/app.go

@@ -276,7 +276,7 @@ func New(
 		),
 	)
 
-	app.SignalKeeper = signal.NewKeeper(keys[signaltypes.StoreKey], app.StakingKeeper)
+	app.SignalKeeper = signal.NewKeeper(appCodec, keys[signaltypes.StoreKey], app.StakingKeeper)
 
 	app.IBCKeeper = ibckeeper.NewKeeper(
 		appCodec,
@@ -464,7 +464,7 @@ func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.Respo
 			}
 		}
 		// from v2 to v3 and onwards we use a signalling mechanism
-	} else if shouldUpgrade, newVersion := app.SignalKeeper.ShouldUpgrade(); shouldUpgrade {
+	} else if shouldUpgrade, newVersion := app.SignalKeeper.ShouldUpgrade(ctx); shouldUpgrade {
 		// Version changes must be increasing. Downgrades are not permitted
 		if newVersion > currentVersion {
 			app.SetAppVersion(ctx, newVersion)

app/module/configurator_test.go

@@ -37,7 +37,7 @@ func TestConfigurator(t *testing.T) {
 		stateStore.MountStoreWithDB(storeKey, storetypes.StoreTypeIAVL, db)
 		require.NoError(t, stateStore.LoadLatestVersion())
 
-		keeper := signal.NewKeeper(storeKey, nil)
+		keeper := signal.NewKeeper(config.Codec, storeKey, nil)
 		require.NotNil(t, keeper)
 		upgradeModule := signal.NewAppModule(keeper)
 		manager, err := module.NewManager([]module.VersionedModule{

proto/celestia/signal/v1/query.proto

@@ -2,6 +2,7 @@ syntax = "proto3";
 package celestia.signal.v1;
 
 import "google/api/annotations.proto";
+import "celestia/signal/v1/upgrade.proto";
 
 option go_package = "github.com/celestiaorg/celestia-app/x/signal/types";
 
@@ -13,6 +14,13 @@ service Query {
       returns (QueryVersionTallyResponse) {
     option (google.api.http).get = "/signal/v1/tally/{version}";
   }
+
+  // GetUpgrade enables a client to query for upgrade information if an upgrade is pending.
+  // The response will be empty if no upgrade is pending.
+  rpc GetUpgrade(QueryGetUpgradeRequest)
+      returns (QueryGetUpgradeResponse) {
+    option (google.api.http).get = "/signal/v1/upgrade";
+  }
 }
 
 // QueryVersionTallyRequest is the request type for the VersionTally query.
@@ -24,3 +32,11 @@ message QueryVersionTallyResponse {
   uint64 threshold_power = 2;
   uint64 total_voting_power = 3;
 }
+
+// QueryGetUpgradeRequest is the request type for the GetUpgrade query.
+message QueryGetUpgradeRequest {}
+
+// QueryGetUpgradeResponse is the response type for the GetUpgrade query.
+message QueryGetUpgradeResponse {
+  Upgrade upgrade = 1;
+}

proto/celestia/signal/v1/upgrade.proto

@@ -0,0 +1,15 @@
+syntax = "proto3";
+package celestia.signal.v1;
+
+option go_package = "github.com/celestiaorg/celestia-app/x/signal/types";
+
+// Upgrade is a type that represents a network upgrade.
+message Upgrade {
+	// AppVersion is the app version that has received a quorum of validators to
+	// signal for it.
+	uint64 app_version = 1;
+
+	// UpgradeHeight is the height at which the network should upgrade to the
+	// AppVersion.
+	int64 upgrade_height = 2;
+}

x/signal/cli/cli_test.go

@@ -38,3 +38,10 @@ func (s *CLITestSuite) TestCmdQueryTally() {
 	s.Require().Contains(output.String(), "threshold_power")
 	s.Require().Contains(output.String(), "total_voting_power")
 }
+
+func (s *CLITestSuite) TestCmdGetUpgrade() {
+	cmd := cli.CmdGetUpgrade()
+	output, err := testutil.ExecTestCLICmd(s.ctx.Context, cmd, []string{})
+	s.Require().NoError(err)
+	s.Require().Contains(output.String(), "No upgrade is pending.")
+}

x/signal/cli/query.go

@@ -21,6 +21,7 @@ func GetQueryCmd() *cobra.Command {
 	}
 
 	cmd.AddCommand(CmdQueryTally())
+	cmd.AddCommand(CmdGetUpgrade())
 	return cmd
 }
 
@@ -54,3 +55,32 @@ func CmdQueryTally() *cobra.Command {
 	flags.AddQueryFlagsToCmd(cmd)
 	return cmd
 }
+
+func CmdGetUpgrade() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:     "upgrade",
+		Short:   "Query for the upgrade information if an upgrade is pending",
+		Args:    cobra.NoArgs,
+		Example: "upgrade",
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			clientCtx, err := client.GetClientQueryContext(cmd)
+			if err != nil {
+				return err
+			}
+
+			queryClient := types.NewQueryClient(clientCtx)
+			resp, err := queryClient.GetUpgrade(cmd.Context(), &types.QueryGetUpgradeRequest{})
+			if err != nil {
+				return err
+			}
+
+			if resp.Upgrade != nil {
+				return clientCtx.PrintString(fmt.Sprintf("An upgrade is pending to app version %d at height %d.\n", resp.Upgrade.AppVersion, resp.Upgrade.UpgradeHeight))
+			}
+			return clientCtx.PrintString("No upgrade is pending.\n")
+		},
+	}
+
+	flags.AddQueryFlagsToCmd(cmd)
+	return cmd
+}

x/signal/integration_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/celestiaorg/celestia-app/v2/app"
 	testutil "github.com/celestiaorg/celestia-app/v2/test/util"
+	"github.com/celestiaorg/celestia-app/v2/x/signal"
 	"github.com/celestiaorg/celestia-app/v2/x/signal/types"
 	"github.com/stretchr/testify/require"
 
@@ -54,7 +55,28 @@ func TestUpgradeIntegration(t *testing.T) {
 	_, err = app.SignalKeeper.TryUpgrade(ctx, nil)
 	require.NoError(t, err)
 
-	shouldUpgrade, version := app.SignalKeeper.ShouldUpgrade()
+	// Verify that if a subsequent call to TryUpgrade is made, it returns an
+	// error because an upgrade is already pending.
+	_, err = app.SignalKeeper.TryUpgrade(ctx, nil)
+	require.Error(t, err)
+	require.ErrorIs(t, err, types.ErrUpgradePending)
+
+	// Verify that if a validator tries to change their signal version, it
+	// returns an error because an upgrade is pending.
+	_, err = app.SignalKeeper.SignalVersion(ctx, &types.MsgSignalVersion{
+		ValidatorAddress: valAddr.String(),
+		Version:          3,
+	})
+	require.Error(t, err)
+	require.ErrorIs(t, err, types.ErrUpgradePending)
+
+	shouldUpgrade, version := app.SignalKeeper.ShouldUpgrade(ctx)
+	require.False(t, shouldUpgrade)
+	require.EqualValues(t, 0, version)
+
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + signal.DefaultUpgradeHeightDelay)
+
+	shouldUpgrade, version = app.SignalKeeper.ShouldUpgrade(ctx)
 	require.True(t, shouldUpgrade)
 	require.EqualValues(t, 2, version)
 }

x/signal/keeper.go

@@ -6,11 +6,17 @@ import (
 
 	sdkmath "cosmossdk.io/math"
 	"github.com/celestiaorg/celestia-app/v2/x/signal/types"
+	"github.com/cosmos/cosmos-sdk/codec"
 	storetypes "github.com/cosmos/cosmos-sdk/store/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
+// DefaultUpgradeHeightDelay is the number of blocks after a quorum has been
+// reached that the chain should upgrade to the new version. Assuming a block
+// interval of 12 seconds, this is 7 days.
+const DefaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks.
+
 // Keeper implements the MsgServer and QueryServer interfaces
 var (
 	_ types.MsgServer   = &Keeper{}
@@ -30,26 +36,26 @@ func Threshold(_ uint64) sdk.Dec {
 }
 
 type Keeper struct {
+	// binaryCodec is used to marshal and unmarshal data from the store.
+	binaryCodec codec.BinaryCodec
+
 	// storeKey is key that is used to fetch the signal store from the multi
 	// store.
 	storeKey storetypes.StoreKey
 
-	// quorumVersion is the version that has received a quorum of validators
-	// to signal for it. This variable is relevant just for the scope of the
-	// lifetime of the block.
-	quorumVersion uint64
-
 	// stakingKeeper is used to fetch validators to calculate the total power
 	// signalled to a version.
 	stakingKeeper StakingKeeper
 }
 
 // NewKeeper returns a signal keeper.
 func NewKeeper(
+	binaryCodec codec.BinaryCodec,
 	storeKey storetypes.StoreKey,
 	stakingKeeper StakingKeeper,
 ) Keeper {
 	return Keeper{
+		binaryCodec:   binaryCodec,
 		storeKey:      storeKey,
 		stakingKeeper: stakingKeeper,
 	}
@@ -58,6 +64,11 @@ func NewKeeper(
 // SignalVersion is a method required by the MsgServer interface.
 func (k Keeper) SignalVersion(ctx context.Context, req *types.MsgSignalVersion) (*types.MsgSignalVersionResponse, error) {
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
+
+	if k.IsUpgradePending(sdkCtx) {
+		return &types.MsgSignalVersionResponse{}, types.ErrUpgradePending.Wrapf("can not signal version")
+	}
+
 	valAddr, err := sdk.ValAddressFromBech32(req.ValidatorAddress)
 	if err != nil {
 		return nil, err
@@ -66,7 +77,7 @@ func (k Keeper) SignalVersion(ctx context.Context, req *types.MsgSignalVersion)
 	// The signalled version can not be less than the current version.
 	currentVersion := sdkCtx.BlockHeader().Version.App
 	if req.Version < currentVersion {
-		return nil, types.ErrInvalidVersion.Wrapf("signalled version %d, current version %d", req.Version, currentVersion)
+		return nil, types.ErrInvalidSignalVersion.Wrapf("signalled version %d, current version %d", req.Version, currentVersion)
 	}
 
 	_, found := k.stakingKeeper.GetValidator(sdkCtx, valAddr)
@@ -78,16 +89,28 @@ func (k Keeper) SignalVersion(ctx context.Context, req *types.MsgSignalVersion)
 	return &types.MsgSignalVersionResponse{}, nil
 }
 
-// TryUpgrade is a method required by the MsgServer interface.
-// It tallies the voting power that has voted on each version.
-// If one version has quorum, it is set as the quorum version
-// which the application can use as signal to upgrade to that version.
+// TryUpgrade is a method required by the MsgServer interface. It tallies the
+// voting power that has voted on each version. If one version has reached a
+// quorum, an upgrade is persisted to the store. The upgrade is used by the
+// application later when it is time to upgrade to that version.
 func (k *Keeper) TryUpgrade(ctx context.Context, _ *types.MsgTryUpgrade) (*types.MsgTryUpgradeResponse, error) {
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
+
+	if k.IsUpgradePending(sdkCtx) {
+		return &types.MsgTryUpgradeResponse{}, types.ErrUpgradePending.Wrapf("can not try upgrade")
+	}
+
 	threshold := k.GetVotingPowerThreshold(sdkCtx)
 	hasQuorum, version := k.TallyVotingPower(sdkCtx, threshold.Int64())
 	if hasQuorum {
-		k.quorumVersion = version
+		if version <= sdkCtx.BlockHeader().Version.App {
+			return &types.MsgTryUpgradeResponse{}, types.ErrInvalidUpgradeVersion.Wrapf("can not upgrade to version %v because it is less than or equal to current version %v", version, sdkCtx.BlockHeader().Version.App)
+		}
+		upgrade := types.Upgrade{
+			AppVersion:    version,
+			UpgradeHeight: sdkCtx.BlockHeader().Height + DefaultUpgradeHeightDelay,
+		}
+		k.setUpgrade(sdkCtx, upgrade)
 	}
 	return &types.MsgTryUpgradeResponse{}, nil
 }
@@ -99,7 +122,7 @@ func (k Keeper) VersionTally(ctx context.Context, req *types.QueryVersionTallyRe
 	totalVotingPower := k.stakingKeeper.GetLastTotalPower(sdkCtx)
 	currentVotingPower := sdk.NewInt(0)
 	store := sdkCtx.KVStore(k.storeKey)
-	iterator := store.Iterator(nil, nil)
+	iterator := store.Iterator(types.FirstSignalKey, nil)
 	defer iterator.Close()
 	for ; iterator.Valid(); iterator.Next() {
 		valAddress := sdk.ValAddress(iterator.Key())
@@ -135,7 +158,7 @@ func (k Keeper) DeleteValidatorVersion(ctx sdk.Context, valAddress sdk.ValAddres
 func (k Keeper) TallyVotingPower(ctx sdk.Context, threshold int64) (bool, uint64) {
 	versionToPower := make(map[uint64]int64)
 	store := ctx.KVStore(k.storeKey)
-	iterator := store.Iterator(nil, nil)
+	iterator := store.Iterator(types.FirstSignalKey, nil)
 	defer iterator.Close()
 	for ; iterator.Valid(); iterator.Next() {
 		valAddress := sdk.ValAddress(iterator.Key())
@@ -171,23 +194,33 @@ func (k Keeper) GetVotingPowerThreshold(ctx sdk.Context) sdkmath.Int {
 	return thresholdFraction.MulInt(totalVotingPower).Ceil().TruncateInt()
 }
 
-// ShouldUpgrade returns true if the signalling mechanism has concluded
-// that the network is ready to upgrade. It also returns the version
-// that the node should upgrade to.
-func (k *Keeper) ShouldUpgrade() (bool, uint64) {
-	return k.quorumVersion != 0, k.quorumVersion
+// ShouldUpgrade returns whether the signalling mechanism has concluded that the
+// network is ready to upgrade and the version to upgrade to. It returns false
+// and 0 if no version has reached quorum.
+func (k *Keeper) ShouldUpgrade(ctx sdk.Context) (isQuorumVersion bool, version uint64) {
+	upgrade, ok := k.getUpgrade(ctx)
+	if !ok {
+		return false, 0
+	}
+
+	hasUpgradeHeightBeenReached := ctx.BlockHeight() >= upgrade.UpgradeHeight
+	if hasUpgradeHeightBeenReached {
+		return true, upgrade.AppVersion
+	}
+	return false, 0
 }
 
 // ResetTally resets the tally after a version change. It iterates over the
-// store and deletes all versions. It also resets the quorumVersion to 0.
+// store and deletes all versions. It also resets the quorumVersion and
+// upgradeHeight to 0.
 func (k *Keeper) ResetTally(ctx sdk.Context) {
 	store := ctx.KVStore(k.storeKey)
 	iterator := store.Iterator(nil, nil)
 	defer iterator.Close()
+	// delete the value in the upgrade key and all signals.
 	for ; iterator.Valid(); iterator.Next() {
 		store.Delete(iterator.Key())
 	}
-	k.quorumVersion = 0
 }
 
 func VersionToBytes(version uint64) []byte {
@@ -197,3 +230,41 @@ func VersionToBytes(version uint64) []byte {
 func VersionFromBytes(version []byte) uint64 {
 	return binary.BigEndian.Uint64(version)
 }
+
+// GetUpgrade returns the current upgrade information.
+func (k Keeper) GetUpgrade(ctx context.Context, _ *types.QueryGetUpgradeRequest) (*types.QueryGetUpgradeResponse, error) {
+	upgrade, ok := k.getUpgrade(sdk.UnwrapSDKContext(ctx))
+	if !ok {
+		return &types.QueryGetUpgradeResponse{}, nil
+	}
+	return &types.QueryGetUpgradeResponse{Upgrade: &upgrade}, nil
+}
+
+// IsUpgradePending returns true if an app version has reached quorum and the
+// chain should upgrade to the app version at the upgrade height. While the
+// keeper has an upgrade pending the SignalVersion and TryUpgrade messages will
+// be rejected.
+func (k *Keeper) IsUpgradePending(ctx sdk.Context) bool {
+	_, ok := k.getUpgrade(ctx)
+	return ok
+}
+
+// getUpgrade returns the current upgrade information from the store.
+// If an upgrade is found, it returns the upgrade object and true.
+// If no upgrade is found, it returns an empty upgrade object and false.
+func (k *Keeper) getUpgrade(ctx sdk.Context) (upgrade types.Upgrade, ok bool) {
+	store := ctx.KVStore(k.storeKey)
+	value := store.Get(types.UpgradeKey)
+	if value == nil {
+		return types.Upgrade{}, false
+	}
+	k.binaryCodec.MustUnmarshal(value, &upgrade)
+	return upgrade, true
+}
+
+// setUpgrade sets the upgrade in the store.
+func (k *Keeper) setUpgrade(ctx sdk.Context, upgrade types.Upgrade) {
+	store := ctx.KVStore(k.storeKey)
+	value := k.binaryCodec.MustMarshal(&upgrade)
+	store.Set(types.UpgradeKey, value)
+}