feat!: add a delay between quorum and upgrade height

app/app.go

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

x/signal/integration_test.go

@@ -54,7 +54,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() + defaultUpgradeHeightDelay)
+
+	shouldUpgrade, version = app.SignalKeeper.ShouldUpgrade(ctx)
 	require.True(t, shouldUpgrade)
 	require.EqualValues(t, 2, version)
 }

x/signal/keeper.go

@@ -18,6 +18,11 @@ var (
 
 	// defaultSignalThreshold is 5/6 or approximately 83.33%
 	defaultSignalThreshold = sdk.NewDec(5).Quo(sdk.NewDec(6))
+
+	// 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 48 hours.
+	defaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks.
 )
 
 // Threshold is the fraction of voting power that is required
@@ -35,10 +40,13 @@ type Keeper struct {
 	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.
+	// to signal for it. It is set to 0 if no version has reached quorum.
 	quorumVersion uint64
 
+	// upgradeHeight is the height at which the network should upgrade to the
+	// quorumVersion. It is set to 0 if no version has reached quorum.
+	upgradeHeight int64
+
 	// stakingKeeper is used to fetch validators to calculate the total power
 	// signalled to a version.
 	stakingKeeper StakingKeeper
@@ -57,6 +65,10 @@ func NewKeeper(
 
 // SignalVersion is a method required by the MsgServer interface.
 func (k Keeper) SignalVersion(ctx context.Context, req *types.MsgSignalVersion) (*types.MsgSignalVersionResponse, error) {
+	if k.isUpgradePending() {
+		return &types.MsgSignalVersionResponse{}, types.ErrUpgradePending.Wrapf("can not signal version")
+	}
+
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
 	valAddr, err := sdk.ValAddressFromBech32(req.ValidatorAddress)
 	if err != nil {
@@ -84,11 +96,19 @@ func (k Keeper) SignalVersion(ctx context.Context, req *types.MsgSignalVersion)
 // If one version has quorum, it is set as the quorum version
 // which the application can use as signal to upgrade to that version.
 func (k *Keeper) TryUpgrade(ctx context.Context, _ *types.MsgTryUpgrade) (*types.MsgTryUpgradeResponse, error) {
+	if k.isUpgradePending() {
+		return &types.MsgTryUpgradeResponse{}, types.ErrUpgradePending.Wrapf("can not try upgrade")
+	}
+
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
 	threshold := k.GetVotingPowerThreshold(sdkCtx)
 	hasQuorum, version := k.TallyVotingPower(sdkCtx, threshold.Int64())
-	if hasQuorum {
+	if hasQuorum && 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)
+	}
+	if hasQuorum && version > sdkCtx.BlockHeader().Version.App {
 		k.quorumVersion = version
+		k.upgradeHeight = sdkCtx.BlockHeight() + defaultUpgradeHeightDelay
 	}
 	return &types.MsgTryUpgradeResponse{}, nil
 }
@@ -172,15 +192,22 @@ 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) {
+	hasQuorumVersion := k.quorumVersion != 0
+	hasUpgradeHeightBeenReached := ctx.BlockHeight() >= k.upgradeHeight
+
+	if hasQuorumVersion && hasUpgradeHeightBeenReached {
+		return true, k.quorumVersion
+	}
+	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)
@@ -189,6 +216,7 @@ func (k *Keeper) ResetTally(ctx sdk.Context) {
 		store.Delete(iterator.Key())
 	}
 	k.quorumVersion = 0
+	k.upgradeHeight = 0
 }
 
 func VersionToBytes(version uint64) []byte {
@@ -198,3 +226,10 @@ func VersionToBytes(version uint64) []byte {
 func VersionFromBytes(version []byte) uint64 {
 	return binary.BigEndian.Uint64(version)
 }
+
+// isUpgradePending returns true if a version has reached quorum and the chain
+// will upgrade to quorumVersion at upgradeHeight. While the keeper has an
+// upgrade pending actions like SignalVersion and TryUpgrade will be rejected.
+func (k *Keeper) isUpgradePending() bool {
+	return k.quorumVersion != 0 && k.upgradeHeight != 0
+}

x/signal/keeper_test.go

@@ -24,6 +24,8 @@ import (
 	tmdb "github.com/tendermint/tm-db"
 )
 
+const defaultUpgradeHeightDelay = int64(7 * 24 * 60 * 60 / 12) // 7 days * 24 hours * 60 minutes * 60 seconds / 12 seconds per block = 50,400 blocks.
+
 func TestGetVotingPowerThreshold(t *testing.T) {
 	bigInt := big.NewInt(0)
 	bigInt.SetString("23058430092136939509", 10)
@@ -158,7 +160,7 @@ func TestTallyingLogic(t *testing.T) {
 
 	_, err = upgradeKeeper.TryUpgrade(goCtx, &types.MsgTryUpgrade{})
 	require.NoError(t, err)
-	shouldUpgrade, version := upgradeKeeper.ShouldUpgrade()
+	shouldUpgrade, version := upgradeKeeper.ShouldUpgrade(ctx)
 	require.False(t, shouldUpgrade)
 	require.Equal(t, uint64(0), version)
 
@@ -171,9 +173,19 @@ func TestTallyingLogic(t *testing.T) {
 
 	_, err = upgradeKeeper.TryUpgrade(goCtx, &types.MsgTryUpgrade{})
 	require.NoError(t, err)
-	shouldUpgrade, version = upgradeKeeper.ShouldUpgrade()
-	require.True(t, shouldUpgrade)
+
+	shouldUpgrade, version = upgradeKeeper.ShouldUpgrade(ctx)
+	require.False(t, shouldUpgrade) // should be false because upgrade height hasn't been reached.
+	require.Equal(t, uint64(0), version)
+
+	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + defaultUpgradeHeightDelay)
+
+	shouldUpgrade, version = upgradeKeeper.ShouldUpgrade(ctx)
+	require.True(t, shouldUpgrade) // should be true because upgrade height has been reached.
 	require.Equal(t, uint64(2), version)
+
+	upgradeKeeper.ResetTally(ctx)
+
 	// update the version to 2
 	ctx = ctx.WithBlockHeader(tmproto.Header{
 		Version: tmversion.Consensus{
@@ -188,6 +200,16 @@ func TestTallyingLogic(t *testing.T) {
 		Version:          3,
 	})
 	require.NoError(t, err)
+	_, err = upgradeKeeper.SignalVersion(goCtx, &types.MsgSignalVersion{
+		ValidatorAddress: testutil.ValAddrs[1].String(),
+		Version:          2,
+	})
+	require.NoError(t, err)
+	_, err = upgradeKeeper.SignalVersion(goCtx, &types.MsgSignalVersion{
+		ValidatorAddress: testutil.ValAddrs[2].String(),
+		Version:          2,
+	})
+	require.NoError(t, err)
 
 	res, err = upgradeKeeper.VersionTally(goCtx, &types.QueryVersionTallyRequest{
 		Version: 2,
@@ -286,6 +308,49 @@ func TestResetTally(t *testing.T) {
 	assert.Equal(t, uint64(0), resp.VotingPower)
 }
 
+func TestTryUpgrade(t *testing.T) {
+	t.Run("should return an error if an upgrade is already pending", func(t *testing.T) {
+		upgradeKeeper, ctx, _ := setup(t)
+		goCtx := sdk.WrapSDKContext(ctx)
+
+		_, err := upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[0].String(), Version: 2})
+		require.NoError(t, err)
+		_, err = upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[1].String(), Version: 2})
+		require.NoError(t, err)
+		_, err = upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[2].String(), Version: 2})
+		require.NoError(t, err)
+		_, err = upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[3].String(), Version: 2})
+		require.NoError(t, err)
+
+		// This TryUpgrade should succeed.
+		_, err = upgradeKeeper.TryUpgrade(goCtx, &types.MsgTryUpgrade{})
+		require.NoError(t, err)
+
+		// This TryUpgrade should fail because an upgrade is pending.
+		_, err = upgradeKeeper.TryUpgrade(goCtx, &types.MsgTryUpgrade{})
+		require.Error(t, err)
+		require.ErrorIs(t, err, types.ErrUpgradePending)
+	})
+
+	t.Run("should return an error if quorum version is less than or equal to the current version", func(t *testing.T) {
+		upgradeKeeper, ctx, _ := setup(t)
+		goCtx := sdk.WrapSDKContext(ctx)
+
+		_, err := upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[0].String(), Version: 1})
+		require.NoError(t, err)
+		_, err = upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[1].String(), Version: 1})
+		require.NoError(t, err)
+		_, err = upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[2].String(), Version: 1})
+		require.NoError(t, err)
+		_, err = upgradeKeeper.SignalVersion(ctx, &types.MsgSignalVersion{ValidatorAddress: testutil.ValAddrs[3].String(), Version: 1})
+		require.NoError(t, err)
+
+		_, err = upgradeKeeper.TryUpgrade(goCtx, &types.MsgTryUpgrade{})
+		require.Error(t, err)
+		require.ErrorIs(t, err, types.ErrInvalidUpgradeVersion)
+	})
+}
+
 func setup(t *testing.T) (signal.Keeper, sdk.Context, *mockStakingKeeper) {
 	signalStore := sdk.NewKVStoreKey(types.StoreKey)
 	db := tmdb.NewMemDB()

x/signal/types/errors.go

@@ -4,4 +4,8 @@ import (
 	"cosmossdk.io/errors"
 )
 
-var ErrInvalidVersion = errors.Register(ModuleName, 1, "signalled version must be either the current version or one greater")
+var (
+	ErrInvalidVersion        = errors.Register(ModuleName, 1, "signalled version must be either the current version or one greater")
+	ErrUpgradePending        = errors.Register(ModuleName, 2, "upgrade is already pending")
+	ErrInvalidUpgradeVersion = errors.Register(ModuleName, 3, "invalid upgrade version")
+)