fix: make ModuleAccountInvariants pass for IS SDK changes (#12554)

* fix bug breaking ModuleAccountInvariants * set UnbondingOnHold to false explicitly * Fixes staking hooks safety issues (#12578) Co-authored-by: Daniel T <[email protected]>
cosmos · Jul 19, 2022 · 67c8163 · 67c8163
1 parent c783aea
commit 67c8163
Show file tree

Hide file tree

Showing 14 changed files with 864 additions and 810 deletions.
diff --git a/docs/core/proto-docs.md b/docs/core/proto-docs.md
@@ -6613,7 +6613,8 @@ 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` | [bool](#bool) |  | false iff unbonding is allowed to complete in staking EndBlock |
+| `unbonding_id` | [uint64](#uint64) |  | unique id, used to distinguish unbond->rebond->unbond executions |
 
 
 

diff --git a/proto/cosmos/staking/v1beta1/staking.proto b/proto/cosmos/staking/v1beta1/staking.proto
@@ -117,9 +117,10 @@ message Validator {
     (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
     (gogoproto.nullable)   = false
   ];
-
-  // True if this validator's unbonding has been stopped by an external module
+  // false iff unbonding is allowed to complete in staking EndBlock
   bool unbonding_on_hold = 12;
+  // unique id, used to distinguish unbond->rebond->unbond executions
+  uint64 unbonding_id = 13;
 }
 
 // BondStatus is the status of a validator.

diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go
@@ -432,10 +432,10 @@ func (k Keeper) SetRedelegationEntry(ctx sdk.Context,
 	red, found := k.GetRedelegation(ctx, delegatorAddr, validatorSrcAddr, validatorDstAddr)
 	id := k.IncrementUnbondingId(ctx)
 	if found {
-		red.AddEntry(creationHeight, minTime, balance, sharesDst, false, id)
+		red.AddEntry(creationHeight, minTime, balance, sharesDst, id)
 	} else {
 		red = types.NewRedelegation(delegatorAddr, validatorSrcAddr,
-			validatorDstAddr, creationHeight, minTime, balance, sharesDst, false, id)
+			validatorDstAddr, creationHeight, minTime, balance, sharesDst, id)
 	}
 
 	k.SetRedelegation(ctx, red)
@@ -703,9 +703,9 @@ func (k Keeper) Unbond(
 	return amount, nil
 }
 
-// getBeginInfo returns the completion time and height of a redelegation, along
-// with a boolean signaling if the redelegation is complete based on the source
-// validator.
+// getBeginInfo returns the completion time and creation height of a
+// redelegation, along with a boolean signaling if the redelegation is
+// complete based on the source validator.
 func (k Keeper) getBeginInfo(
 	ctx sdk.Context, valSrcAddr sdk.ValAddress,
 ) (completionTime time.Time, height int64, completeNow bool) {
@@ -785,7 +785,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]
+		entry := ubd.Entries[i]
 		if entry.IsMature(ctxTime) && !entry.UnbondingOnHold {
 			// Proceed with unbonding
 			ubd.RemoveEntry(int64(i))
@@ -889,7 +889,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]
+		entry := red.Entries[i]
 		if entry.IsMature(ctxTime) && !entry.UnbondingOnHold {
 			red.RemoveEntry(int64(i))
 			i--

diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go
@@ -607,7 +607,7 @@ func TestGetRedelegationsFromSrcValidator(t *testing.T) {
 
 	rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 0,
 		time.Unix(0, 0), sdk.NewInt(5),
-		sdk.NewDec(5), false, 1)
+		sdk.NewDec(5), 1)
 
 	// set and retrieve a record
 	app.StakingKeeper.SetRedelegation(ctx, rd)
@@ -634,7 +634,7 @@ func TestRedelegation(t *testing.T) {
 
 	rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 0,
 		time.Unix(0, 0).UTC(), sdk.NewInt(5),
-		sdk.NewDec(5), false, 1)
+		sdk.NewDec(5), 1)
 
 	// test shouldn't have and redelegations
 	has := app.StakingKeeper.HasReceivingRedelegation(ctx, addrDels[0], addrVals[1])

diff --git a/x/staking/keeper/querier.go b/x/staking/keeper/querier.go
@@ -514,7 +514,6 @@ func RedelegationsToRedelegationResponses(
 				entry.SharesDst,
 				entry.InitialBalance,
 				val.TokensFromShares(entry.SharesDst).TruncateInt(),
-				entry.UnbondingOnHold,
 				entry.UnbondingId,
 			)
 		}

diff --git a/x/staking/keeper/slash_test.go b/x/staking/keeper/slash_test.go
@@ -131,7 +131,7 @@ func TestSlashRedelegation(t *testing.T) {
 	// set a redelegation with an expiration timestamp beyond which the
 	// redelegation shouldn't be slashed
 	rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 0,
-		time.Unix(5, 0), sdk.NewInt(10), sdk.NewDec(10), false, 1)
+		time.Unix(5, 0), sdk.NewInt(10), sdk.NewDec(10), 1)
 
 	app.StakingKeeper.SetRedelegation(ctx, rd)
 
@@ -390,7 +390,7 @@ func TestSlashWithRedelegation(t *testing.T) {
 	// set a redelegation
 	rdTokens := app.StakingKeeper.TokensFromConsensusPower(ctx, 6)
 	rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 11,
-		time.Unix(0, 0), rdTokens, rdTokens.ToDec(), false, 1)
+		time.Unix(0, 0), rdTokens, rdTokens.ToDec(), 1)
 	app.StakingKeeper.SetRedelegation(ctx, rd)
 
 	// set the associated delegation
@@ -544,7 +544,7 @@ func TestSlashBoth(t *testing.T) {
 	rdATokens := app.StakingKeeper.TokensFromConsensusPower(ctx, 6)
 	rdA := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 11,
 		time.Unix(0, 0), rdATokens,
-		rdATokens.ToDec(), false, 1)
+		rdATokens.ToDec(), 1)
 	app.StakingKeeper.SetRedelegation(ctx, rdA)
 
 	// set the associated delegation

diff --git a/x/staking/keeper/unbonding.go b/x/staking/keeper/unbonding.go
@@ -323,22 +323,33 @@ func (k Keeper) redelegationEntryCanComplete(ctx sdk.Context, id uint64) (found
 	return true, nil
 }
 
+// WARNING: precondition:
+// Safety only guaranteed if this method is called AFTER staking.EndBlock
 func (k Keeper) validatorUnbondingCanComplete(ctx sdk.Context, id uint64) (found bool, err error) {
 	val, found := k.GetValidatorByUnbondingId(ctx, id)
+
 	if !found {
+		// validator can never be deleted before unbonding
+		// even if it is slashed to 0, so we always expect
+		// to find it.
 		return false, nil
 	}
 
-	if !val.IsMature(ctx.BlockTime(), ctx.BlockHeight()) {
+	if val.UnbondingId != id {
+		// validator already rebonded
+		return true, nil
+	}
+
+	if val.UnbondingTime.After(ctx.BlockTime()) {
+		// validator cannot have already been dequeued by EndBlock
 		val.UnbondingOnHold = false
 		k.SetValidator(ctx, val)
 	} else {
-		// If unbonding is mature complete it
-		val = k.UnbondingToUnbonded(ctx, val)
+		// Validator is mature. Unbond it.
 		if val.GetDelegatorShares().IsZero() {
 			k.RemoveValidator(ctx, val.GetOperator())
 		}
-
+		val = k.UnbondingToUnbonded(ctx, val)
 		k.DeleteUnbondingIndex(ctx, id)
 	}
 

diff --git a/x/staking/keeper/val_state_change.go b/x/staking/keeper/val_state_change.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"sort"
+	"time"
 
 	gogotypes "github.com/gogo/protobuf/types"
 	abci "github.com/tendermint/tendermint/abci/types"
@@ -273,7 +274,7 @@ func (k Keeper) unbondedToBonded(ctx sdk.Context, validator types.Validator) (ty
 // UnbondingToUnbonded switches a validator from unbonding state to unbonded state
 func (k Keeper) UnbondingToUnbonded(ctx sdk.Context, validator types.Validator) types.Validator {
 	if !validator.IsUnbonding() {
-		panic(fmt.Sprintf("bad state transition unbondingToBonded, validator: %v\n", validator))
+		panic(fmt.Sprintf("bad state transition unbondingToUnbonded, validator: %v\n", validator))
 	}
 
 	return k.CompleteUnbondingValidator(ctx, validator)
@@ -306,15 +307,19 @@ func (k Keeper) bondValidator(ctx sdk.Context, validator types.Validator) (types
 	// delete the validator by power index, as the key will change
 	k.DeleteValidatorByPowerIndex(ctx, validator)
 
+	// delete from queue if present
+	k.DeleteValidatorQueue(ctx, validator, validator.UnbondingTime, validator.UnbondingHeight)
+
 	validator = validator.UpdateStatus(types.Bonded)
+	validator.UnbondingHeight = 0
+	validator.UnbondingTime = time.Time{}
+	validator.UnbondingOnHold = false
+	validator.UnbondingId = 0
 
 	// save the now bonded validator record to the two referenced stores
 	k.SetValidator(ctx, validator)
 	k.SetValidatorByPowerIndex(ctx, validator)
 
-	// delete from queue if present
-	k.DeleteValidatorQueue(ctx, validator)
-
 	// trigger hook
 	consAddr, err := validator.GetConsAddr()
 	if err != nil {
@@ -337,27 +342,29 @@ func (k Keeper) BeginUnbondingValidator(ctx sdk.Context, validator types.Validat
 		panic(fmt.Sprintf("should not already be unbonded or unbonding, validator: %v\n", validator))
 	}
 
+	consAddr, err := validator.GetConsAddr()
+	if err != nil {
+		return validator, err
+	}
+
 	validator = validator.UpdateStatus(types.Unbonding)
 
 	// set the unbonding completion time and completion height appropriately
 	validator.UnbondingTime = ctx.BlockHeader().Time.Add(params.UnbondingTime)
 	validator.UnbondingHeight = ctx.BlockHeader().Height
 
+	id := k.IncrementUnbondingId(ctx)
+	validator.UnbondingId = id
+
 	// save the now unbonded validator record and power index
 	k.SetValidator(ctx, validator)
 	k.SetValidatorByPowerIndex(ctx, validator)
 
 	// Adds to unbonding validator queue
 	k.InsertUnbondingValidatorQueue(ctx, validator)
 
-	// trigger hook
-	consAddr, err := validator.GetConsAddr()
-	if err != nil {
-		return validator, err
-	}
 	k.AfterValidatorBeginUnbonding(ctx, consAddr, validator.GetOperator())
 
-	id := k.IncrementUnbondingId(ctx)
 	k.SetValidatorByUnbondingIndex(ctx, validator, id)
 
 	k.AfterUnbondingInitiated(ctx, id)
@@ -368,6 +375,10 @@ func (k Keeper) BeginUnbondingValidator(ctx sdk.Context, validator types.Validat
 // perform all the store operations for when a validator status becomes unbonded
 func (k Keeper) CompleteUnbondingValidator(ctx sdk.Context, validator types.Validator) types.Validator {
 	validator = validator.UpdateStatus(types.Unbonded)
+	validator.UnbondingHeight = 0
+	validator.UnbondingTime = time.Time{}
+	validator.UnbondingOnHold = false
+	validator.UnbondingId = 0
 	k.SetValidator(ctx, validator)
 
 	return validator

diff --git a/x/staking/keeper/validator.go b/x/staking/keeper/validator.go
@@ -367,8 +367,9 @@ func (k Keeper) DeleteValidatorQueueTimeSlice(ctx sdk.Context, endTime time.Time
 
 // DeleteValidatorQueue removes a validator by address from the unbonding queue
 // indexed by a given height and time.
-func (k Keeper) DeleteValidatorQueue(ctx sdk.Context, val types.Validator) {
-	addrs := k.GetUnbondingValidators(ctx, val.UnbondingTime, val.UnbondingHeight)
+func (k Keeper) DeleteValidatorQueue(ctx sdk.Context, val types.Validator,
+	unbondingTime time.Time, unbondingHeight int64) {
+	addrs := k.GetUnbondingValidators(ctx, unbondingTime, unbondingHeight)
 	newAddrs := []string{}
 
 	for _, addr := range addrs {
@@ -378,14 +379,14 @@ func (k Keeper) DeleteValidatorQueue(ctx sdk.Context, val types.Validator) {
 	}
 
 	if len(newAddrs) == 0 {
-		k.DeleteValidatorQueueTimeSlice(ctx, val.UnbondingTime, val.UnbondingHeight)
+		k.DeleteValidatorQueueTimeSlice(ctx, unbondingTime, unbondingHeight)
 	} else {
-		k.SetUnbondingValidatorsQueue(ctx, val.UnbondingTime, val.UnbondingHeight, newAddrs)
+		k.SetUnbondingValidatorsQueue(ctx, unbondingTime, unbondingHeight, newAddrs)
 	}
 }
 
 // ValidatorQueueIterator returns an interator ranging over validators that are
-// unbonding whose unbonding completion occurs at the given height and time.
+// unbonding whose unbonding completion occurs not before a given time.
 func (k Keeper) ValidatorQueueIterator(ctx sdk.Context, endTime time.Time, endHeight int64) sdk.Iterator {
 	store := ctx.KVStore(k.storeKey)
 	return store.Iterator(types.ValidatorQueueKey, sdk.InclusiveEndBytes(types.GetValidatorQueueKey(endTime, endHeight)))
@@ -409,15 +410,15 @@ func (k Keeper) UnbondAllMatureValidators(ctx sdk.Context) {
 
 	for ; unbondingValIterator.Valid(); unbondingValIterator.Next() {
 		key := unbondingValIterator.Key()
-		keyTime, keyHeight, err := types.ParseValidatorQueueKey(key)
+		keyTime, _, err := types.ParseValidatorQueueKey(key)
 		if err != nil {
 			panic(fmt.Errorf("failed to parse unbonding key: %w", err))
 		}
 
 		// All addresses for the given key have the same unbonding height and time.
 		// We only unbond if the height and time are less than the current height
 		// and time.
-		if keyHeight <= blockHeight && (keyTime.Before(blockTime) || keyTime.Equal(blockTime)) {
+		if keyTime.Before(blockTime) || keyTime.Equal(blockTime) {
 			addrs := types.ValAddresses{}
 			k.cdc.MustUnmarshal(unbondingValIterator.Value(), &addrs)
 
@@ -432,6 +433,8 @@ func (k Keeper) UnbondAllMatureValidators(ctx sdk.Context) {
 				}
 
 				if !val.IsUnbonding() {
+					fmt.Println("status is ", val.Status)
+
 					panic("unexpected validator in unbonding queue; status was not unbonding")
 				}
 				if !val.UnbondingOnHold {

diff --git a/x/staking/simulation/decoder_test.go b/x/staking/simulation/decoder_test.go
@@ -40,7 +40,7 @@ func TestDecodeStore(t *testing.T) {
 	require.NoError(t, err)
 	del := types.NewDelegation(delAddr1, valAddr1, sdk.OneDec())
 	ubd := types.NewUnbondingDelegation(delAddr1, valAddr1, 15, bondTime, sdk.OneInt(), 1)
-	red := types.NewRedelegation(delAddr1, valAddr1, valAddr1, 12, bondTime, sdk.OneInt(), sdk.OneDec(), false, 0)
+	red := types.NewRedelegation(delAddr1, valAddr1, valAddr1, 12, bondTime, sdk.OneInt(), sdk.OneDec(), 0)
 
 	kvPairs := kv.Pairs{
 		Pairs: []kv.Pair{

diff --git a/x/staking/types/delegation.go b/x/staking/types/delegation.go
@@ -94,11 +94,12 @@ func (d Delegations) String() (out string) {
 
 func NewUnbondingDelegationEntry(creationHeight int64, completionTime time.Time, balance sdk.Int, id uint64) UnbondingDelegationEntry {
 	return UnbondingDelegationEntry{
-		CreationHeight: creationHeight,
-		CompletionTime: completionTime,
-		InitialBalance: balance,
-		Balance:        balance,
-		UnbondingId:    id,
+		CreationHeight:  creationHeight,
+		CompletionTime:  completionTime,
+		InitialBalance:  balance,
+		Balance:         balance,
+		UnbondingId:     id,
+		UnbondingOnHold: false,
 	}
 }
 
@@ -209,14 +210,14 @@ func (ubds UnbondingDelegations) String() (out string) {
 	return strings.TrimSpace(out)
 }
 
-func NewRedelegationEntry(creationHeight int64, completionTime time.Time, balance sdk.Int, sharesDst sdk.Dec, onHold bool, id uint64) RedelegationEntry {
+func NewRedelegationEntry(creationHeight int64, completionTime time.Time, balance sdk.Int, sharesDst sdk.Dec, id uint64) RedelegationEntry {
 	return RedelegationEntry{
 		CreationHeight:  creationHeight,
 		CompletionTime:  completionTime,
 		InitialBalance:  balance,
 		SharesDst:       sharesDst,
-		UnbondingOnHold: onHold,
 		UnbondingId:     id,
+		UnbondingOnHold: false,
 	}
 }
 
@@ -234,21 +235,21 @@ func (e RedelegationEntry) IsMature(currentTime time.Time) bool {
 //nolint:interfacer
 func NewRedelegation(
 	delegatorAddr sdk.AccAddress, validatorSrcAddr, validatorDstAddr sdk.ValAddress,
-	creationHeight int64, minTime time.Time, balance sdk.Int, sharesDst sdk.Dec, onHold bool, unbondingId uint64,
+	creationHeight int64, minTime time.Time, balance sdk.Int, sharesDst sdk.Dec, unbondingId uint64,
 ) Redelegation {
 	return Redelegation{
 		DelegatorAddress:    delegatorAddr.String(),
 		ValidatorSrcAddress: validatorSrcAddr.String(),
 		ValidatorDstAddress: validatorDstAddr.String(),
 		Entries: []RedelegationEntry{
-			NewRedelegationEntry(creationHeight, minTime, balance, sharesDst, onHold, unbondingId),
+			NewRedelegationEntry(creationHeight, minTime, balance, sharesDst, unbondingId),
 		},
 	}
 }
 
 // AddEntry - append entry to the unbonding delegation
-func (red *Redelegation) AddEntry(creationHeight int64, minTime time.Time, balance sdk.Int, sharesDst sdk.Dec, onHold bool, unbondingId uint64) {
-	entry := NewRedelegationEntry(creationHeight, minTime, balance, sharesDst, onHold, unbondingId)
+func (red *Redelegation) AddEntry(creationHeight int64, minTime time.Time, balance sdk.Int, sharesDst sdk.Dec, unbondingId uint64) {
+	entry := NewRedelegationEntry(creationHeight, minTime, balance, sharesDst, unbondingId)
 	red.Entries = append(red.Entries, entry)
 }
 
@@ -374,9 +375,9 @@ func NewRedelegationResponse(
 
 // NewRedelegationEntryResponse creates a new RedelegationEntryResponse instance.
 func NewRedelegationEntryResponse(
-	creationHeight int64, completionTime time.Time, sharesDst sdk.Dec, initialBalance, balance sdk.Int, onHold bool, id uint64) RedelegationEntryResponse {
+	creationHeight int64, completionTime time.Time, sharesDst sdk.Dec, initialBalance, balance sdk.Int, id uint64) RedelegationEntryResponse {
 	return RedelegationEntryResponse{
-		RedelegationEntry: NewRedelegationEntry(creationHeight, completionTime, initialBalance, sharesDst, onHold, id),
+		RedelegationEntry: NewRedelegationEntry(creationHeight, completionTime, initialBalance, sharesDst, id),
 		Balance:           balance,
 	}
 }