R4R: Offset validator set power calculations by one block #2255
Conversation
cwgoes
changed the title
|
Also note case of double-signing the "extra block" at the end due to NextValSet offset - what happens? Validator no longer stored in SDK state as bonded. Panic? Freebie? |
x/slashing/slashing_period.go
Outdated
| @@ -68,7 +68,7 @@ func (k Keeper) unmarshalSlashingPeriodKeyValue(key []byte, value []byte) Valida | |||
| var slashingPeriodValue ValidatorSlashingPeriodValue | |||
| k.cdc.MustUnmarshalBinary(value, &slashingPeriodValue) | |||
| address := sdk.ConsAddress(key[1 : 1+sdk.AddrLen]) | |||
| startHeight := int64(binary.LittleEndian.Uint64(key[1+sdk.AddrLen : 1+sdk.AddrLen+8])) | |||
| startHeight := int64(binary.LittleEndian.Uint64(key[1+sdk.AddrLen:1+sdk.AddrLen+8]) - 1) | |||
There was a problem hiding this comment.
Should this be - 1 -> - ValidatorDelay ?
x/stake/genesis.go
Outdated
| // We need to pretend to be "one block before genesis" so that e.g. slashing periods | ||
| // are correctly initialized for the validator set one-block offset - the first TM block | ||
| // is at height 0, so state updates applied from genesis.json are in block -1. | ||
| ctx = ctx.WithBlockHeight(-1) |
x/slashing/params.go
Outdated
| // For example, if this is 0, the validator set at the end of a block will sign the next block, or | ||
| // if this is 1, the validator set at the end of a block will sign the block after the next. | ||
| // Constant as this should not change without a hard fork. | ||
| ValidatorUpdateDelay int64 = 1 |
|
Thanks @rigelrozanski - I agree on making |
|
(won't merge until a Tendermint-team review as per your mention) |
| // Note that this *can* result in a negative "distributionHeight", up to -ValidatorUpdateDelay, | ||
| // i.e. at the end of the pre-genesis block (none) = at the beginning of the genesis block. | ||
| // That's fine since this is just used to filter unbonding delegations & redelegations. | ||
| distributionHeight := infractionHeight - stake.ValidatorUpdateDelay |
There was a problem hiding this comment.
I probably need to dig deeper into the slashing mechanics to confirm this, but the infractionHeight is the actual height at which the validator double signed, and is the height at which we want to know the validator set. Are we tracking validator sets differently here? If the double signing was at height H, here we're looking at the validator set at height H-1
| // Note that this *can* result in a negative "distributionHeight" up to -ValidatorUpdateDelay-1, | ||
| // i.e. at the end of the pre-genesis block (none) = at the beginning of the genesis block. | ||
| // That's fine since this is just used to filter unbonding delegations & redelegations. | ||
| distributionHeight := height - stake.ValidatorUpdateDelay - 1 |
There was a problem hiding this comment.
height - 1 is correct, but same comment as before around also subtracting the update delay.
Use correct offsets in slashing to account for what voting power signed which block. This has changed with
NextValSetin Tendermint 0.24, and I think it was previously wrong for slashing-for-downtime usingLastCommit.Votes.distributionHeightmeans at-end-of-block and test thisCloses #1789
Dependent on #2219
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/)Added entries in
PENDING.mdwith issue #rereviewed
Files changedin the github PR explorerFor Admin Use: