Fixes staking hooks safety issues #12578

danwt · 2022-07-14T13:43:21Z

This PR fixes

Incorrect code in staking module CompleteUnbonding can cause permanent loss or minting of funds. interchain-security#229
validatorUnbondingCanComplete must take into account (re)bonded validators interchain-security#232

This PR must fix

SDK changes break simulator interchain-security#227 <-- must fix this before merging

~~This PR additionally fixes~~

Validator.UnbondingHeight meaning is inconsistent, misused and not useful. #12486 <--- this is addressed
Make SDK staking hook changes robust to order of staking.EndBlock and CanComplete interchain-security#233 <-- I think this should be left as future work

* fix bug breaking ModuleAccountInvariants * set UnbondingOnHold to false explicitly * Fixes staking hooks safety issues (#12578) Co-authored-by: Daniel T <30197399+danwt@users.noreply.github.com>

mpoke · 2022-08-01T10:16:43Z

x/staking/keeper/val_state_change.go

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


Why pass validator.UnbondingTime and validator.UnbondingHeight? Can't we just retrieve them in DeleteValidatorQueue since we pass validator anyway?

mpoke · 2022-08-01T10:21:09Z

x/staking/keeper/validator.go

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


Removing keyHeight <= blockHeight (and anything related to it) should be part of a separate PR that is opened on the main branch of the SDK since it's not Interchain Security specific.

mpoke · 2022-08-01T10:21:36Z

x/staking/keeper/validator.go

@@ -432,6 +433,8 @@ func (k Keeper) UnbondAllMatureValidators(ctx sdk.Context) {
 				}

 				if !val.IsUnbonding() {
+					fmt.Println("status is ", val.Status)


mpoke · 2022-08-01T10:22:09Z

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,


No need to change the signature of this function.

mpoke · 2022-08-01T10:24:31Z

x/staking/keeper/unbonding.go

+		return true, nil
+	}
+
+	if val.UnbondingTime.After(ctx.BlockTime()) {


I agree with removing the IsMature function and replace it with the actual check, but we shouldn't get rid of the v.UnbondingHeight < currentHeight since this is a general fix for the SDK (see my other comment).

mpoke · 2022-08-01T10:37:34Z

x/staking/keeper/val_state_change.go

-	// trigger hook
-	consAddr, err := validator.GetConsAddr()
-	if err != nil {
-		return validator, err
-	}


Why move this block?

mpoke · 2022-08-01T10:41:03Z

x/staking/keeper/val_state_change.go

+	validator.UnbondingHeight = 0
+	validator.UnbondingTime = time.Time{}
+	validator.UnbondingOnHold = false
+	validator.UnbondingId = 0


Why are these needed? For example, a validator unbonding cannot complete without validator.UnbondingOnHold == false.

mpoke · 2022-08-01T10:45:41Z

x/staking/keeper/val_state_change.go

 	validator = validator.UpdateStatus(types.Bonded)
+	validator.UnbondingHeight = 0
+	validator.UnbondingTime = time.Time{}
+	validator.UnbondingOnHold = false


This should be moved to unbondingToBonded, since it's the only case where validator.UnbondingOnHold could be true without being set to false by the CCV module (via the validatorUnbondingCanComplete function).

mpoke · 2022-08-01T10:48:25Z

x/staking/keeper/val_state_change.go

+	validator.UnbondingHeight = 0
+	validator.UnbondingTime = time.Time{}


Why are these needed now and they were not needed before?

mpoke · 2022-08-01T10:53:30Z

proto/cosmos/staking/v1beta1/staking.proto

  bool unbonding_on_hold = 12;
+  // unique id, used to distinguish unbond->rebond->unbond executions
+  uint64 unbonding_id = 13;


I think the solution described here would be cleaner without adding an extra field to the Validator struct, i.e., validatorUnbondingCanComplete just sets val.UnbondingOnHold to false and let the logic in UnbondAllMatureValidators move validators to UNBONDED. @jtremback what do you think?

Actually it is not sufficient, since a validator that goes through unbond-rebond-unbond, can have its val.UnbondingOnHold set to true by the CCV module (on the second unbond) and then have this flag reset to false by a call to validatorUnbondingCanComplete for the first unbond.

A solution could be for val.UnbondingOnHold to be a counter instead of a bool, i.e.,

every call to PutUnbondingOnHold increments it;

every call to UnbondingCanComplete decrements it;

a validator can move to UNBONDED only if val.UnbondingOnHold == 0

Like this we allow for these functions to be used by multiple modules (in case it's needed in the future).

* feat: store ABCI validator updates in transient store * fix test build * change transient key name * add UnbondingDelegationEntryCreated hook * add id to UnbondingDelegationEntry * changes to add simple version of staking ccv hooks * ubde id to string * rough draft of more efficient technique * change hook api and do some clean ups * use ByEntry index and keep stopped entries in Entries array * correct error convention * add comment * some cleanups * comment cleanup * finish hooking up hooks * get the tests to pass * proof of concept with embedded mock hooks * first unit test of CCV hooks * fix forgotten pointer bug * move hook mocks into own file * added test for completing stopped unbonding early * added staking hooks template * correct file and module names * clean up and fix import error * move staking hooks template to types * fix hooks after merge * fix silly proto bug * feat: Add AfterValidatorDowntime hook in slashing module (#10938) Create a slashing hook that allows external modules to register an execution when a validator has downtime. Implementation details: * Call hook in HandleValidatorSignature (x/slashing/keeper/infractions.go) which updates validator SignInfo data * Defer hook execution in case it also wants to update validator SignInfo data * Add methods to update SignInfo to slashing keeper interface(/x/slashing/types/expected_keepers.go) * update: Remove slashing module hooks (#11425) * update: Remove slashing module hooks Hooks are not required anymore to implement the slashing for downtime in CCV. The logic is now using the staking keeper interface definition from the slashing module. The SDK changes are the following: - /x/slashing/keeper/infractions.go - remove hook calls and don't update validators missed blocks when jailed - /x/slashing/types/expected_keepers.go - remove `AfterValidatorDowntime` hook interface and add `IsJailed()` method to staking interface definition - /x/staking/keeper/validator.go - implement `IsJailed()` method * fix last details * Finish staking hooks merge (#11677) * allow stopping and completing of redelegations * refactor to remove BeforeUnbondingDelegationEntryComplete hook and notes for validator unbonding * WIP rough draft of validator unbonding hooks * add many of marius's suggested changes * More review changes * unbonding delegation tests pass * WIP adding redelegation tests * WIP redelegation tests work * unbondingDelegation and redelegation tests pass and cleanup * WIP validator unbonding tests almost pass * tests for all new functionality pass * fix index deleting logic * clean up TODOs * fix small logic bug * fix slashing tests * Rename statements containing 'UnbondingOp' to 'Unbond' in code, docs and proto files Co-authored-by: Simon <simon.ntz@gmail.com> * feat: enable double-signing evidence in Interchain-Security (#11921) * Add a `InfractionType` enum to Slash function arguments * Remove pubkey condition in HandleEquivocation * Update docs/core/proto-docs.md Co-authored-by: billy rennekamp <billy.rennekamp@gmail.com> * Update proto/cosmos/staking/v1beta1/staking.proto Co-authored-by: billy rennekamp <billy.rennekamp@gmail.com> * add a possible solution to the evidence module issue Co-authored-by: billy rennekamp <billy.rennekamp@gmail.com> * chore: remove direct reliance on staking from slashing (backport #12177) (#12181) * 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 <30197399+danwt@users.noreply.github.com> * Revert "fix: make ModuleAccountInvariants pass for IS SDK changes (#12554)" (#12782) This reverts commit 67c8163. * fix: make ModuleAccountInvariants pass for IS SDK changes (#12783) * fix bug breaking ModuleAccountInvariants * set UnbondingOnHold to false explicitly * fix: [Interchain Security] `validatorUnbondingCanComplete` must take into account (re)bonded validators (#12796) * replace val.UnbondingOnHold w/ UnbondingOnHoldRefCount * add UnbondingOnHoldRefCount for undel and red (for consistency) * improve comments * improve TestValidatorUnbondingOnHold test * ret error if UnbondingOnHoldRefCount is negative * adding extra validator unbonding test * change OnHold() def * fix: [Interchain Security] Fix leak in unbonding hooks (#12849) * remove leak for UBDEs and REDEs * remove leak for val unbondings * docs: [Interchain Security] update spec (#12848) * updating staking spec * clarify code * fix typo * store ValidatorUpdates in normal store (#12845) * Update x/slashing/keeper/signing_info.go Co-authored-by: Simon Noetzlin <simon.ntz@gmail.com> * Update x/staking/keeper/val_state_change.go * Update x/staking/keeper/val_state_change.go * Update x/slashing/keeper/infractions.go Co-authored-by: Simon Noetzlin <simon.ntz@gmail.com> * Update x/staking/keeper/val_state_change.go * Update x/staking/keeper/val_state_change.go * fix compile errors * remove stakingtypes.TStoreKey * fix: decrease minimums for genesis parameters (#13106) * Update genesis.go * Update genesis.go Co-authored-by: Federico Kunze <federico.kunze94@gmail.com> Co-authored-by: Aditya Sripal <adityasripal@gmail.com> Co-authored-by: Simon Noetzlin <simon.ntz@gmail.com> Co-authored-by: billy rennekamp <billy.rennekamp@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Marius Poke <marius.poke@posteo.de> Co-authored-by: Daniel T <30197399+danwt@users.noreply.github.com> Co-authored-by: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com>

* fix bug breaking ModuleAccountInvariants * set UnbondingOnHold to false explicitly * Fixes staking hooks safety issues (#12578) Co-authored-by: Daniel T <30197399+danwt@users.noreply.github.com>

Daniel added 5 commits July 14, 2022 11:40

Adds unbonding_id to validator proto

1dbf873

Reset validator unbonding fields when becoming unbonded or bonded

135a3fb

Fix unbonding id to validator

8f21c56

Ignore KeyHeight in validator unbonding completion

bcd789a

Correctly delete rebonded validators from queue

7c48c6c

danwt requested review from aaronc and alexanderbez as code owners July 14, 2022 13:43

github-actions bot added the C:x/staking label Jul 14, 2022

danwt changed the title ~~Fixes CCV safety issue~~ Fixes staking hooks safety issues Jul 14, 2022

This was referenced Jul 14, 2022

fix: [Interchain Security] Add equality to validator.IsMature #12537

Closed

fix: make ModuleAccountInvariants pass for IS SDK changes #12554

Merged

alexanderbez merged commit 7ec4e9e into cosmos:interchain-security-bug Jul 17, 2022

mpoke reviewed Aug 1, 2022

View reviewed changes

mpoke mentioned this pull request Aug 1, 2022

fix: make ModuleAccountInvariants pass for IS SDK changes #12783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes staking hooks safety issues #12578

Fixes staking hooks safety issues #12578

danwt commented Jul 14, 2022 •

edited by mpoke

Loading

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

mpoke Aug 1, 2022

		validator.UnbondingHeight = 0
		validator.UnbondingTime = time.Time{}

Fixes staking hooks safety issues #12578

Fixes staking hooks safety issues #12578

Conversation

danwt commented Jul 14, 2022 • edited by mpoke Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwt commented Jul 14, 2022 •

edited by mpoke

Loading