update: Remove slashing module hooks #11425

sainoe · 2022-03-21T15:11:14Z

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

Description

Closes: #XXXX

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

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

tac0turtle · 2022-03-21T15:59:04Z

@sainoe does this need a review? I couldn't find this code on master so it seems its only for the interchain security branch?

sainoe · 2022-03-21T16:12:28Z

@marbar3778 yes it's only for the IS branch and @AdityaSripal @mpoke will review it.

jtremback · 2022-03-21T16:16:07Z

x/slashing/keeper/infractions.go

@@ -35,7 +35,8 @@ func (k Keeper) HandleValidatorSignature(ctx sdk.Context, addr cryptotypes.Addre
 	// This counter just tracks the sum of the bit array
 	// That way we avoid needing to read/write the whole array each time
 	previous := k.GetValidatorMissedBlockBitArray(ctx, consAddr, index)
-	missed := !signed
+	missed := !signed && !k.sk.IsJailed(ctx, consAddr) // don't update missed blocks when validator's jailed


What effect (if any) will this have on the operation of this code in the single chain (non IS) case?

In non IS cases, jailed validators won't have their missed blocks counter updated anymore. This was a possible scenario due to the TM validator update delay.

This needs to be much clearer, and the functionality probably needs to be different as well.

I agree. IMO we shouldn't overload the missed flag. Also, we shouldn't change the behavior for non-CCV validators.

In non IS cases, jailed validators won't have their missed blocks counter updated anymore. This was a possible scenario due to the TM validator update delay.

This is true, but only for 1-2 blocks, not enough to accumulate another downtime window. And it is irrelevant anyway due to the check at line 80.

If this tiny effect in non IS case isn't wanted, we can just falsify the IsJailed function in the staking module. It was added for CCV so it wouldn't have any negative impact.

mpoke · 2022-03-22T18:24:21Z

x/staking/keeper/validator.go

@@ -445,3 +445,12 @@ func (k Keeper) UnbondAllMatureValidators(ctx sdk.Context) {
 		}
 	}
 }
+
+func (k Keeper) IsJailed(ctx sdk.Context, addr sdk.ConsAddress) bool {


The name of the function is overloaded. There is already a function with the same name in staking/types/validator.go. Do we actually need this function?

Yes we do need this function it satisfies the StakingKeeper interface in x/slashing/types/expected_keepers.go. However we can change its name.

sunnya97 · 2022-03-22T18:26:38Z

Why remove the hooks? There may be non standard modules in other repos that are making use of these hooks

mpoke · 2022-03-22T18:31:05Z

Why remove the hooks? There may be non standard modules in other repos that are making use of these hooks

The only hook removed is AfterValidatorDowntime() that we added before for CCV and we decided we no longer need. I don't think such a hook is really useful for other modules. The name of the PR may be a bit misleading :)

* 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 <[email protected]> * 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 <[email protected]> * Update proto/cosmos/staking/v1beta1/staking.proto Co-authored-by: billy rennekamp <[email protected]> * add a possible solution to the evidence module issue Co-authored-by: billy rennekamp <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Aditya Sripal <[email protected]> Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: billy rennekamp <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Marius Poke <[email protected]> Co-authored-by: Daniel T <[email protected]> Co-authored-by: Shawn Marshall-Spitzbart <[email protected]>

* 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

sainoe requested review from aaronc and alexanderbez as code owners March 21, 2022 15:11

github-actions bot added C:x/slashing C:x/staking labels Mar 21, 2022

amaury1093 assigned tac0turtle Mar 21, 2022

tac0turtle unassigned tac0turtle Mar 21, 2022

tac0turtle assigned jtremback Mar 21, 2022

jtremback reviewed Mar 21, 2022

View reviewed changes

mpoke self-requested a review March 22, 2022 18:09

mpoke reviewed Mar 22, 2022

View reviewed changes

fix last details

087ae06

jtremback merged commit 29c717a into cosmos:interchain-security-rebase Mar 23, 2022

sainoe deleted the interchain-security-rebase branch May 2, 2022 08:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update: Remove slashing module hooks #11425

update: Remove slashing module hooks #11425

sainoe commented Mar 21, 2022

tac0turtle commented Mar 21, 2022

sainoe commented Mar 21, 2022

jtremback Mar 21, 2022

sainoe Mar 21, 2022

jtremback Mar 22, 2022

mpoke Mar 22, 2022

mpoke Mar 22, 2022

sainoe Mar 23, 2022

mpoke Mar 22, 2022

sainoe Mar 23, 2022

sunnya97 commented Mar 22, 2022

mpoke commented Mar 22, 2022

update: Remove slashing module hooks #11425

update: Remove slashing module hooks #11425

Conversation

sainoe commented Mar 21, 2022

Description

Author Checklist

Reviewers Checklist

tac0turtle commented Mar 21, 2022

sainoe commented Mar 21, 2022

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

sunnya97 commented Mar 22, 2022

mpoke commented Mar 22, 2022