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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Aug 5, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b352143
replace val.UnbondingOnHold w/ UnbondingOnHoldRefCount
mpoke Aug 1, 2022
ac12d67
add UnbondingOnHoldRefCount for undel and red (for consistency)
mpoke Aug 2, 2022
04dc36b
improve comments
mpoke Aug 2, 2022
af6ee93
improve TestValidatorUnbondingOnHold test
mpoke Aug 2, 2022
db20718
ret error if UnbondingOnHoldRefCount is negative
mpoke Aug 3, 2022
c33200b
adding extra validator unbonding test
mpoke Aug 5, 2022
4f7e70e
change OnHold() def
mpoke Aug 5, 2022
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
6 changes: 3 additions & 3 deletions docs/core/proto-docs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |



Expand Down Expand Up @@ -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 |



Expand Down Expand Up @@ -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 |



Expand Down
12 changes: 6 additions & 6 deletions proto/cosmos/staking/v1beta1/staking.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions x/staking/keeper/delegation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick- why not call it UnbondingOnHold()?

Copy link
Author

Choose a reason for hiding this comment

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

My idea was to keep it consistent with IsMature (it is not UnbondingIsMature). The Unbonding Delegation Entry is matured or on hold. But I don't have a strong position about it. @jtremback Let me know if I should change to UnbondingOnHold().

// Proceed with unbonding
ubd.RemoveEntry(int64(i))
i--
Expand Down Expand Up @@ -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--

Expand Down
4 changes: 2 additions & 2 deletions x/staking/keeper/slash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
49 changes: 19 additions & 30 deletions x/staking/keeper/unbonding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,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 {
Expand Down Expand Up @@ -250,11 +252,10 @@ func (k Keeper) unbondingDelegationEntryCanComplete(ctx sdk.Context, id uint64)
return false, nil
}

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 {
Expand Down Expand Up @@ -301,10 +302,9 @@ 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 {
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))
Expand All @@ -329,24 +329,16 @@ 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this get removed? Doesn't it remove functionality from this function? Where does

		// If unbonding is mature complete it
		val = k.UnbondingToUnbonded(ctx, val)
		if val.GetDelegatorShares().IsZero() {
			k.RemoveValidator(ctx, val.GetOperator())
		}

		k.DeleteUnbondingIndex(ctx, id)

happen now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it is also happening in UnbondAllMatureValidators, but I'm not sure that's enough, because there it is only triggered when unbonding on the provider completes, while it is trigger here when unbonding on the consumer completes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the logic might now be broken if unbonding completes on the provider chain first, but I think the unit tests test for that so I'm confused.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, looks like they don't. I don't remember why not.

Copy link
Author

Choose a reason for hiding this comment

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

@jtremback UnbondAllMatureValidators is called in every staking.EndBlock(). As long as a mature validator val (that is on hold due to Interchain Security, i.e., val.UnbondingOnHoldRefCount > 0) is not removed from the unbonding validator queue, then once UnbondingCanComplete is called enough times to decrement val.UnbondingOnHoldRefCount to 0, the next call to staking.EndBlock() will move val to unbonded.

Copy link
Author

@mpoke mpoke Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Notice that for UBDEs and REDEs, we cannot remove the code from unbondingDelegationEntryCanComplete() and redelegationEntryCanComplete(), respectively. This is because in staking.EndBlock(), mature unbonding delegations and redelegations are removed from their respective queues, see DequeueAllMatureUBDQueue and DequeueAllMatureRedelegationQueue here.

// If unbonding is mature complete it
val = k.UnbondingToUnbonded(ctx, val)
if val.GetDelegatorShares().IsZero() {
k.RemoveValidator(ctx, val.GetOperator())
}

k.DeleteUnbondingIndex(ctx, id)
}
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)
Expand Down Expand Up @@ -379,8 +371,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
Expand All @@ -397,8 +388,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
Expand All @@ -410,8 +400,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
Expand Down
94 changes: 56 additions & 38 deletions x/staking/keeper/unbonding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

What triggers this to be called IRL?

Copy link
Author

Choose a reason for hiding this comment

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

The next block. At any given time, app.StakingKeeper.UnbondAllMatureValidators will be eventually called again as long as the chain is producing new blocks (i.e., liveness is not violated).


// 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) {
Expand Down
8 changes: 3 additions & 5 deletions x/staking/keeper/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't .OnHold() doing this elsewhere?

Copy link
Author

Choose a reason for hiding this comment

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

OnHold() is defined for unbonding delegation entries and redelegation entries. It is useful there since we use it in multiple places in the code, similarly to the IsMature(). For validators, there is no IsMature(). Also, this is the only place we check 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)
}
}
}
Expand Down
Loading