R4R: Downtime slashing off-by-one-block fix #1805
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1805 +/- ##
===========================================
- Coverage 63.45% 63.44% -0.02%
===========================================
Files 117 117
Lines 6926 6937 +11
===========================================
+ Hits 4395 4401 +6
- Misses 2276 2281 +5
Partials 255 255 |
cwgoes
changed the title
| @@ -57,6 +59,15 @@ func (k Keeper) Validator(ctx sdk.Context, address sdk.AccAddress) sdk.Validator | |||
| return val | |||
| } | |||
|
|
|||
| // get the sdk.validator for a particular pubkey | |||
| func (k Keeper) ValidatorByPubKey(ctx sdk.Context, pubkey crypto.PubKey) sdk.Validator { | |||
| val, found := k.GetValidatorByPubKey(ctx, pubkey) | |||
There was a problem hiding this comment.
Why not use GetValidatorByPubKey? Is it to just avoid using a boolean?
There was a problem hiding this comment.
Not sure I understand the question - ValidatorByPubKey is in the sdk.ValidatorSet interface, which is used by the slashing module.
There was a problem hiding this comment.
Apologies, went through the code and realized the interface was updated.
| @@ -65,6 +65,7 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, pubkey crypto.PubKey, infracti | |||
| } | |||
|
|
|||
| // handle a validator signature, must be called once per validator per block | |||
| // nolint gocyclo | |||
| func (k Keeper) handleValidatorSignature(ctx sdk.Context, pubkey crypto.PubKey, power int64, signed bool) { | |||
There was a problem hiding this comment.
We're planning to actually not send the pubkey in BeginBlock, only the address (tendermint/tendermint#1712). Is that going to be a problem? It means we need to do the lookup by validator address!
There was a problem hiding this comment.
We'll need to change the staking store then, at the moment the secondary index is by raw public key bytes.
There was a problem hiding this comment.
cc @rigelrozanski but AFAIK this should be fine
| k.validatorSet.Revoke(ctx, pubkey) | ||
| signInfo.JailedUntil = ctx.BlockHeader().Time + k.DowntimeUnbondDuration(ctx) | ||
| validator := k.validatorSet.ValidatorByPubKey(ctx, pubkey) | ||
| if validator != nil && !validator.GetRevoked() { |
There was a problem hiding this comment.
shouldn't we be checking this sooner? we are we even bothering fetching and updating the signInfo if he's no longer a validator ?
There was a problem hiding this comment.
Hmm, I'm not sure.
If the validator was just revoked, technically they still should have signed the block in which they were revoked, so I left in the usual accounting logic for that. I think we could safely remove it though.
* Avoid slashing & revoking no longer stored or already revoked validators for downtime * Add testcase * Update PENDING.md
Closes #1797
The SDK previously made the incorrect assumption that
abci.RequestBeginBlock.SigningValidatorshad signatures on the current block - in fact, it had signatures on the previous block.This PR ensures that we don't slash/revoke nonexistent validators (as happened on gaia-7001a) and don't slash/revoke validators that are already revoked.
docs/)PENDING.mdcmd/gaiaandexamples/For Admin Use: