fix: [Interchain Security] validatorUnbondingCanComplete must take into account (re)bonded validators

docs/core/proto-docs.md

@@ -6489,7 +6489,7 @@ RedelegationEntry defines a redelegation object with relevant metadata.
 | `initial_balance` | [string](#string) |  | initial_balance defines the initial balance when redelegation started. |
 | `shares_dst` | [string](#string) |  | shares_dst is the amount of destination-validator shares created by redelegation. |
 | `unbonding_id` | [uint64](#uint64) |  | Incrementing id that uniquely identifies this entry |
-| `unbonding_on_hold` | [bool](#bool) |  | True if this entry's unbonding has been stopped by an external module |
+| `unbonding_on_hold_ref_count` | [int64](#int64) |  | Strictly positive if this entry's unbonding has been stopped by external modules |
 
 
 
@@ -6565,7 +6565,7 @@ UnbondingDelegationEntry defines an unbonding object with relevant metadata.
 | `initial_balance` | [string](#string) |  | initial_balance defines the tokens initially scheduled to receive at completion. |
 | `balance` | [string](#string) |  | balance defines the tokens to receive at completion. |
 | `unbonding_id` | [uint64](#uint64) |  | Incrementing id that uniquely identifies this entry |
-| `unbonding_on_hold` | [bool](#bool) |  | True if this entry's unbonding has been stopped by an external module |
+| `unbonding_on_hold_ref_count` | [int64](#int64) |  | Strictly positive if this entry's unbonding has been stopped by external modules |
 
 
 
@@ -6613,7 +6613,7 @@ multiplied by exchange rate.
 | `unbonding_time` | [google.protobuf.Timestamp](#google.protobuf.Timestamp) |  | unbonding_time defines, if unbonding, the min time for the validator to complete unbonding. |
 | `commission` | [Commission](#cosmos.staking.v1beta1.Commission) |  | commission defines the commission parameters. |
 | `min_self_delegation` | [string](#string) |  | min_self_delegation is the validator's self declared minimum self delegation. |
-| `unbonding_on_hold` | [bool](#bool) |  | True if this validator's unbonding has been stopped by an external module |
+| `unbonding_on_hold_ref_count` | [int64](#int64) |  | Strictly positive if this validator's unbonding has been stopped by external modules |
 
 
 

proto/cosmos/staking/v1beta1/staking.proto

@@ -118,8 +118,8 @@ message Validator {
     (gogoproto.nullable)   = false
   ];
 
-  // True if this validator's unbonding has been stopped by an external module
-  bool unbonding_on_hold = 12;
+  // Strictly positive if this validator's unbonding has been stopped by external modules
+  int64 unbonding_on_hold_ref_count = 12;
 }
 
 // BondStatus is the status of a validator.
@@ -233,8 +233,8 @@ message UnbondingDelegationEntry {
   // Incrementing id that uniquely identifies this entry
   uint64 unbonding_id = 5;
 
-  // True if this entry's unbonding has been stopped by an external module
-  bool unbonding_on_hold = 6;
+  // Strictly positive if this entry's unbonding has been stopped by external modules
+  int64 unbonding_on_hold_ref_count = 6;
 }
 
 // RedelegationEntry defines a redelegation object with relevant metadata.
@@ -260,8 +260,8 @@ message RedelegationEntry {
   // Incrementing id that uniquely identifies this entry
   uint64 unbonding_id = 5;
 
-  // True if this entry's unbonding has been stopped by an external module
-  bool unbonding_on_hold = 6;
+  // Strictly positive if this entry's unbonding has been stopped by external modules
+  int64 unbonding_on_hold_ref_count = 6;
 }
 
 // Redelegation contains the list of a particular delegator's redelegating bonds

x/staking/keeper/delegation.go

@@ -786,7 +786,7 @@ func (k Keeper) CompleteUnbonding(ctx sdk.Context, delAddr sdk.AccAddress, valAd
 	// loop through all the entries and try to complete unbonding mature entries
 	for i := 0; i < len(ubd.Entries); i++ {
 		entry := ubd.Entries[i]
-		if entry.IsMature(ctxTime) && !entry.UnbondingOnHold {
+		if entry.IsMature(ctxTime) && !entry.OnHold() {
 			// Proceed with unbonding
 			ubd.RemoveEntry(int64(i))
 			i--
@@ -890,7 +890,7 @@ func (k Keeper) CompleteRedelegation(
 	// loop through all the entries and try to complete mature redelegation entries
 	for i := 0; i < len(red.Entries); i++ {
 		entry := red.Entries[i]
-		if entry.IsMature(ctxTime) && !entry.UnbondingOnHold {
+		if entry.IsMature(ctxTime) && !entry.OnHold() {
 			red.RemoveEntry(int64(i))
 			i--
 

x/staking/keeper/slash.go

@@ -176,7 +176,7 @@ func (k Keeper) SlashUnbondingDelegation(ctx sdk.Context, unbondingDelegation ty
 			continue
 		}
 
-		if entry.IsMature(now) && !entry.UnbondingOnHold {
+		if entry.IsMature(now) && !entry.OnHold() {
 			// Unbonding delegation no longer eligible for slashing, skip it
 			continue
 		}
@@ -229,7 +229,7 @@ func (k Keeper) SlashRedelegation(ctx sdk.Context, srcValidator types.Validator,
 			continue
 		}
 
-		if entry.IsMature(now) && !entry.UnbondingOnHold {
+		if entry.IsMature(now) && !entry.OnHold() {
 			// Redelegation no longer eligible for slashing, skip it
 			continue
 		}

x/staking/keeper/unbonding.go

@@ -4,6 +4,7 @@ import (
 	"encoding/binary"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 	"github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
@@ -205,8 +206,10 @@ func redelegationEntryArrayIndex(red types.Redelegation, id uint64) (index int,
 	return 0, false
 }
 
-// UnbondingCanComplete allows a stopped unbonding operation such as an
-// unbonding delegation, a redelegation, or a validator unbonding to complete
+// UnbondingCanComplete allows a stopped unbonding operation, such as an
+// unbonding delegation, a redelegation, or a validator unbonding to complete.
+// In order for the unbonding operation with `id` to eventually complete, every call
+// to PutUnbondingOnHold(id) must be matched by a call to UnbondingCanComplete(id).
 // ----------------------------------------------------------------------------------------
 
 func (k Keeper) UnbondingCanComplete(ctx sdk.Context, id uint64) error {
@@ -250,11 +253,18 @@ func (k Keeper) unbondingDelegationEntryCanComplete(ctx sdk.Context, id uint64)
 		return false, nil
 	}
 
+	if ubd.Entries[i].UnbondingOnHoldRefCount <= 0 {
+		return true,
+			sdkerrors.Wrapf(
+				types.ErrUnbondingOnHoldRefCountNegative,
+				"undelegation unbondingId(%d), expecting UnbondingOnHoldRefCount > 0, got %T",
+				id, ubd.Entries[i].UnbondingOnHoldRefCount,
+			)
+	}
+	ubd.Entries[i].UnbondingOnHoldRefCount--
+
 	// Check if entry is matured.
-	if !ubd.Entries[i].IsMature(ctx.BlockHeader().Time) {
-		// If not matured, set onHold to false
-		ubd.Entries[i].UnbondingOnHold = false
-	} else {
+	if !ubd.Entries[i].OnHold() && ubd.Entries[i].IsMature(ctx.BlockHeader().Time) {
 		// If matured, complete it.
 		delegatorAddress, err := sdk.AccAddressFromBech32(ubd.DelegatorAddress)
 		if err != nil {
@@ -301,10 +311,17 @@ func (k Keeper) redelegationEntryCanComplete(ctx sdk.Context, id uint64) (found
 		return false, nil
 	}
 
-	if !red.Entries[i].IsMature(ctx.BlockHeader().Time) {
-		// If not matured, set onHold to false
-		red.Entries[i].UnbondingOnHold = false
-	} else {
+	if red.Entries[i].UnbondingOnHoldRefCount <= 0 {
+		return true,
+			sdkerrors.Wrapf(
+				types.ErrUnbondingOnHoldRefCountNegative,
+				"redelegation unbondingId(%d), expecting UnbondingOnHoldRefCount > 0, got %T",
+				id, red.Entries[i].UnbondingOnHoldRefCount,
+			)
+	}
+	red.Entries[i].UnbondingOnHoldRefCount--
+
+	if !red.Entries[i].OnHold() && red.Entries[i].IsMature(ctx.BlockHeader().Time) {
 		// If matured, complete it.
 		// Remove entry
 		red.RemoveEntry(int64(i))
@@ -329,24 +346,24 @@ func (k Keeper) validatorUnbondingCanComplete(ctx sdk.Context, id uint64) (found
 		return false, nil
 	}
 
-	if !val.IsMature(ctx.BlockTime(), ctx.BlockHeight()) {
-		val.UnbondingOnHold = false
-		k.SetValidator(ctx, val)
-	} else {
-		// If unbonding is mature complete it
-		val = k.UnbondingToUnbonded(ctx, val)
-		if val.GetDelegatorShares().IsZero() {
-			k.RemoveValidator(ctx, val.GetOperator())
-		}
-
-		k.DeleteUnbondingIndex(ctx, id)
+	if val.UnbondingOnHoldRefCount <= 0 {
+		return true,
+			sdkerrors.Wrapf(
+				types.ErrUnbondingOnHoldRefCountNegative,
+				"val(%s), expecting UnbondingOnHoldRefCount > 0, got %T",
+				val.OperatorAddress, val.UnbondingOnHoldRefCount,
+			)
 	}
+	val.UnbondingOnHoldRefCount--
+	k.SetValidator(ctx, val)
 
 	return true, nil
 }
 
-// PutUnbondingOnHold allows an external module to stop an unbonding operation such as an
-// unbonding delegation, a redelegation, or a validator unbonding
+// PutUnbondingOnHold allows an external module to stop an unbonding operation,
+// such as an unbonding delegation, a redelegation, or a validator unbonding.
+// In order for the unbonding operation with `id` to eventually complete, every call
+// to PutUnbondingOnHold(id) must be matched by a call to UnbondingCanComplete(id).
 // ----------------------------------------------------------------------------------------
 func (k Keeper) PutUnbondingOnHold(ctx sdk.Context, id uint64) error {
 	found := k.putUnbondingDelegationEntryOnHold(ctx, id)
@@ -379,8 +396,7 @@ func (k Keeper) putUnbondingDelegationEntryOnHold(ctx sdk.Context, id uint64) (f
 		return false
 	}
 
-	ubd.Entries[i].UnbondingOnHold = true
-
+	ubd.Entries[i].UnbondingOnHoldRefCount++
 	k.SetUnbondingDelegation(ctx, ubd)
 
 	return true
@@ -397,8 +413,7 @@ func (k Keeper) putRedelegationEntryOnHold(ctx sdk.Context, id uint64) (found bo
 		return false
 	}
 
-	red.Entries[i].UnbondingOnHold = true
-
+	red.Entries[i].UnbondingOnHoldRefCount++
 	k.SetRedelegation(ctx, red)
 
 	return true
@@ -410,8 +425,7 @@ func (k Keeper) putValidatorOnHold(ctx sdk.Context, id uint64) (found bool) {
 		return false
 	}
 
-	val.UnbondingOnHold = true
-
+	val.UnbondingOnHoldRefCount++
 	k.SetValidator(ctx, val)
 
 	return true

x/staking/keeper/unbonding_test.go

@@ -138,9 +138,9 @@ func doRedelegation(
 }
 
 func doValidatorUnbonding(
-	t *testing.T, app *simapp.SimApp, ctx sdk.Context, addrDels []sdk.AccAddress, addrVals []sdk.ValAddress, hookCalled *bool,
+	t *testing.T, app *simapp.SimApp, ctx sdk.Context, addrVal sdk.ValAddress, hookCalled *bool,
 ) (validator types.Validator) {
-	validator, found := app.StakingKeeper.GetValidator(ctx, addrVals[0])
+	validator, found := app.StakingKeeper.GetValidator(ctx, addrVal)
 	require.True(t, found)
 	// Check that status is bonded
 	require.Equal(t, types.BondStatus(3), validator.Status)
@@ -157,62 +157,80 @@ func doValidatorUnbonding(
 	return validator
 }
 
-func TestValidatorUnbondingOnHold1(t *testing.T) {
+func TestValidatorUnbondingOnHold(t *testing.T) {
 	var hookCalled bool
 	var ubdeID uint64
-	app, ctx, _, addrDels, addrVals := setup(t, &hookCalled, &ubdeID)
-	validator := doValidatorUnbonding(t, app, ctx, addrDels, addrVals, &hookCalled)
+	var ubdeIDs []uint64
+	app, ctx, _, _, addrVals := setup(t, &hookCalled, &ubdeID)
 
-	completionTime := validator.UnbondingTime
+	// Start unbonding first validator
+	validator1 := doValidatorUnbonding(t, app, ctx, addrVals[0], &hookCalled)
+	ubdeIDs = append(ubdeIDs, ubdeID)
 
-	// CONSUMER CHAIN'S UNBONDING PERIOD ENDS - BUT UNBONDING CANNOT COMPLETE
-	err := app.StakingKeeper.UnbondingCanComplete(ctx, ubdeID)
-	require.NoError(t, err)
+	// Reset hookCalled flag
+	hookCalled = false
 
-	// Check that unbonding is not complete
-	validator, found := app.StakingKeeper.GetValidator(ctx, addrVals[0])
-	require.True(t, found)
-	// Check that status is unbonding
-	require.Equal(t, types.BondStatus(2), validator.Status)
+	// Start unbonding second validator
+	validator2 := doValidatorUnbonding(t, app, ctx, addrVals[1], &hookCalled)
+	ubdeIDs = append(ubdeIDs, ubdeID)
 
-	// PROVIDER CHAIN'S UNBONDING PERIOD ENDS - STOPPED UNBONDING CAN NOW COMPLETE
-	ctx = ctx.WithBlockTime(completionTime)
-	app.StakingKeeper.UnbondAllMatureValidators(ctx)
+	// Check that there are two unbonding operations
+	require.Equal(t, 2, len(ubdeIDs))
 
-	validator, found = app.StakingKeeper.GetValidator(ctx, addrVals[0])
-	require.True(t, found)
-	// Check that status is unbonded
-	require.Equal(t, types.BondStatus(1), validator.Status)
-}
-
-func TestValidatorUnbondingOnHold2(t *testing.T) {
-	var hookCalled bool
-	var ubdeID uint64
-	app, ctx, _, addrDels, addrVals := setup(t, &hookCalled, &ubdeID)
-	validator := doValidatorUnbonding(t, app, ctx, addrDels, addrVals, &hookCalled)
+	// Check that both validators have same unbonding time
+	require.Equal(t, validator1.UnbondingTime, validator2.UnbondingTime)
 
-	completionTime := validator.UnbondingTime
-	completionHeight := validator.UnbondingHeight
+	completionTime := validator1.UnbondingTime
+	completionHeight := validator1.UnbondingHeight
 
 	// PROVIDER CHAIN'S UNBONDING PERIOD ENDS - BUT UNBONDING CANNOT COMPLETE
 	ctx = ctx.WithBlockTime(completionTime.Add(time.Duration(1)))
 	ctx = ctx.WithBlockHeight(completionHeight + 1)
 	app.StakingKeeper.UnbondAllMatureValidators(ctx)
 
-	// Check that unbonding is not complete
-	validator, found := app.StakingKeeper.GetValidator(ctx, addrVals[0])
+	// Check that unbonding is not complete for both validators
+	validator1, found := app.StakingKeeper.GetValidator(ctx, addrVals[0])
 	require.True(t, found)
-	// Check that status is unbonding
-	require.Equal(t, types.BondStatus(2), validator.Status)
+	require.Equal(t, types.Unbonding, validator1.Status)
+	validator2, found = app.StakingKeeper.GetValidator(ctx, addrVals[1])
+	require.True(t, found)
+	require.Equal(t, types.Unbonding, validator2.Status)
+	unbondingVals := app.StakingKeeper.GetUnbondingValidators(ctx, completionTime, completionHeight)
+	require.Equal(t, 2, len(unbondingVals))
+	require.Equal(t, validator1.OperatorAddress, unbondingVals[0])
+	require.Equal(t, validator2.OperatorAddress, unbondingVals[1])
 
 	// CONSUMER CHAIN'S UNBONDING PERIOD ENDS - STOPPED UNBONDING CAN NOW COMPLETE
-	err := app.StakingKeeper.UnbondingCanComplete(ctx, ubdeID)
+	err := app.StakingKeeper.UnbondingCanComplete(ctx, ubdeIDs[0])
 	require.NoError(t, err)
 
-	validator, found = app.StakingKeeper.GetValidator(ctx, addrVals[0])
+	// Try again to unbond validators
+	app.StakingKeeper.UnbondAllMatureValidators(ctx)
+
+	// Check that unbonding is complete for validator1, but not for validator2
+	validator1, found = app.StakingKeeper.GetValidator(ctx, addrVals[0])
+	require.True(t, found)
+	require.Equal(t, types.Unbonded, validator1.Status)
+	validator2, found = app.StakingKeeper.GetValidator(ctx, addrVals[1])
+	require.True(t, found)
+	require.Equal(t, types.Unbonding, validator2.Status)
+	unbondingVals = app.StakingKeeper.GetUnbondingValidators(ctx, completionTime, completionHeight)
+	require.Equal(t, 1, len(unbondingVals))
+	require.Equal(t, validator2.OperatorAddress, unbondingVals[0])
+
+	// Unbonding for validator2 can complete
+	err = app.StakingKeeper.UnbondingCanComplete(ctx, ubdeIDs[1])
+	require.NoError(t, err)
+
+	// Try again to unbond validators
+	app.StakingKeeper.UnbondAllMatureValidators(ctx)
+
+	// Check that unbonding is complete for validator2
+	validator2, found = app.StakingKeeper.GetValidator(ctx, addrVals[1])
 	require.True(t, found)
-	// Check that status is unbonded
-	require.Equal(t, types.BondStatus(1), validator.Status)
+	require.Equal(t, types.Unbonded, validator2.Status)
+	unbondingVals = app.StakingKeeper.GetUnbondingValidators(ctx, completionTime, completionHeight)
+	require.Equal(t, 0, len(unbondingVals))
 }
 
 func TestRedelegationOnHold1(t *testing.T) {

x/staking/keeper/validator.go

@@ -394,8 +394,6 @@ func (k Keeper) ValidatorQueueIterator(ctx sdk.Context, endTime time.Time, endHe
 // UnbondAllMatureValidators unbonds all the mature unbonding validators that
 // have finished their unbonding period.
 func (k Keeper) UnbondAllMatureValidators(ctx sdk.Context) {
-	store := ctx.KVStore(k.storeKey)
-
 	blockTime := ctx.BlockTime()
 	blockHeight := ctx.BlockHeight()
 
@@ -434,15 +432,15 @@ func (k Keeper) UnbondAllMatureValidators(ctx sdk.Context) {
 				if !val.IsUnbonding() {
 					panic("unexpected validator in unbonding queue; status was not unbonding")
 				}
-				if !val.UnbondingOnHold {
+				if val.UnbondingOnHoldRefCount == 0 {
 					val = k.UnbondingToUnbonded(ctx, val)
 					if val.GetDelegatorShares().IsZero() {
 						k.RemoveValidator(ctx, val.GetOperator())
 					}
+					// remove validator from queue
+					k.DeleteValidatorQueue(ctx, val)
 				}
 			}
-
-			store.Delete(key)
 		}
 	}
 }