From f6e92df3fae1a281624f62b9d97794dcb72861f6 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 11 Oct 2018 22:48:00 +0200 Subject: [PATCH 01/20] Create signing info struct on validator bond hook --- x/slashing/hooks.go | 16 +++++++++++++++- x/slashing/keeper.go | 3 +-- x/slashing/keeper_test.go | 17 +++++++---------- x/slashing/test_common.go | 1 + 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/x/slashing/hooks.go b/x/slashing/hooks.go index 701a6b2cd..a2e6c506a 100644 --- a/x/slashing/hooks.go +++ b/x/slashing/hooks.go @@ -1,11 +1,25 @@ package slashing import ( + "time" + sdk "github.com/cosmos/cosmos-sdk/types" ) -// Create a new slashing period when a validator is bonded func (k Keeper) onValidatorBonded(ctx sdk.Context, address sdk.ConsAddress) { + // Create a new signing info if necessary + _, found := k.getValidatorSigningInfo(ctx, address) + if !found { + signingInfo := ValidatorSigningInfo{ + StartHeight: ctx.BlockHeight(), + IndexOffset: 0, + JailedUntil: time.Unix(0, 0), + SignedBlocksCounter: 0, + } + k.setValidatorSigningInfo(ctx, address, signingInfo) + } + + // Create a new slashing period when a validator is bonded slashingPeriod := ValidatorSlashingPeriod{ ValidatorAddr: address, StartHeight: ctx.BlockHeight(), diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 94dd9f0d0..9cd7076cb 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -101,8 +101,7 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p // Will use the 0-value default signing info if not present, except for start height signInfo, found := k.getValidatorSigningInfo(ctx, consAddr) if !found { - // If this validator has never been seen before, construct a new SigningInfo with the correct start height - signInfo = NewValidatorSigningInfo(height, 0, time.Unix(0, 0), 0) + panic(fmt.Sprintf("Expected signing info for validator %s but not found", consAddr)) } index := signInfo.IndexOffset % k.SignedBlocksWindow(ctx) signInfo.IndexOffset++ diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index cf67af192..23a3ae2de 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -28,7 +28,6 @@ func TestHandleDoubleSign(t *testing.T) { ctx, ck, sk, _, keeper := createTestInput(t) // validator added pre-genesis ctx = ctx.WithBlockHeight(-1) - sk = sk.WithHooks(keeper.Hooks()) amtInt := int64(100) operatorAddr, val, amt := addrs[0], pks[0], sdk.NewInt(amtInt) got := stake.NewHandler(sk)(ctx, newTestMsgCreateValidator(operatorAddr, val, amt)) @@ -69,7 +68,6 @@ func TestSlashingPeriodCap(t *testing.T) { // initial setup ctx, ck, sk, _, keeper := createTestInput(t) - sk = sk.WithHooks(keeper.Hooks()) amtInt := int64(100) operatorAddr, amt := addrs[0], sdk.NewInt(amtInt) valConsPubKey, valConsAddr := pks[0], pks[0].Address() @@ -135,7 +133,6 @@ func TestHandleAbsentValidator(t *testing.T) { // initial setup ctx, ck, sk, _, keeper := createTestInput(t) - sk = sk.WithHooks(keeper.Hooks()) amtInt := int64(100) addr, val, amt := addrs[0], pks[0], sdk.NewInt(amtInt) sh := stake.NewHandler(sk) @@ -146,14 +143,12 @@ func TestHandleAbsentValidator(t *testing.T) { keeper.AddValidators(ctx, validatorUpdates) require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.Sub(amt)}}) require.True(t, sdk.NewDecFromInt(amt).Equal(sk.Validator(ctx, addr).GetPower())) + // will exist since the validator has been bonded info, found := keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) - require.False(t, found) + require.True(t, found) require.Equal(t, int64(0), info.StartHeight) require.Equal(t, int64(0), info.IndexOffset) require.Equal(t, int64(0), info.SignedBlocksCounter) - // default time.Time value - var blankTime time.Time - require.Equal(t, blankTime, info.JailedUntil) height := int64(0) // 1000 first blocks OK @@ -292,6 +287,11 @@ func TestHandleNewValidator(t *testing.T) { ctx, ck, sk, _, keeper := createTestInput(t) addr, val, amt := addrs[0], pks[0], int64(100) sh := stake.NewHandler(sk) + + // 1000 first blocks not a validator + ctx = ctx.WithBlockHeight(keeper.SignedBlocksWindow(ctx) + 1) + + // Validator created got := sh(ctx, newTestMsgCreateValidator(addr, val, sdk.NewInt(amt))) require.True(t, got.IsOK()) validatorUpdates := stake.EndBlocker(ctx, sk) @@ -299,9 +299,6 @@ func TestHandleNewValidator(t *testing.T) { require.Equal(t, ck.GetCoins(ctx, sdk.AccAddress(addr)), sdk.Coins{{sk.GetParams(ctx).BondDenom, initCoins.SubRaw(amt)}}) require.Equal(t, sdk.NewDec(amt), sk.Validator(ctx, addr).GetPower()) - // 1000 first blocks not a validator - ctx = ctx.WithBlockHeight(keeper.SignedBlocksWindow(ctx) + 1) - // Now a validator, for two blocks keeper.handleValidatorSignature(ctx, val.Address(), 100, true) ctx = ctx.WithBlockHeight(keeper.SignedBlocksWindow(ctx) + 2) diff --git a/x/slashing/test_common.go b/x/slashing/test_common.go index 7c97a8537..25140c824 100644 --- a/x/slashing/test_common.go +++ b/x/slashing/test_common.go @@ -84,6 +84,7 @@ func createTestInput(t *testing.T) (sdk.Context, bank.Keeper, stake.Keeper, para } require.Nil(t, err) keeper := NewKeeper(cdc, keySlashing, sk, params.Getter(), DefaultCodespace) + sk = sk.WithHooks(keeper.Hooks()) return ctx, ck, sk, params.Setter(), keeper } From b55f29f72d48355e848af75db0475b749dab8686 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 11 Oct 2018 22:58:20 +0200 Subject: [PATCH 02/20] Update PENDING.md --- PENDING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/PENDING.md b/PENDING.md index dbae0cc2c..1ced472c7 100644 --- a/PENDING.md +++ b/PENDING.md @@ -69,6 +69,7 @@ BREAKING CHANGES * [x/staking] \#2244 staking now holds a consensus-address-index instead of a consensus-pubkey-index * [x/staking] \#2236 more distribution hooks for distribution * [x/stake] \#2394 Split up UpdateValidator into distinct state transitions applied only in EndBlock + * [x/slashing] \#2480 Fix signing info handling bugs & faulty slashing * Tendermint * Update tendermint version from v0.23.0 to v0.25.0, notable changes From 5fd7297e259865452f16eadba4249086165aae72 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 12 Oct 2018 00:04:44 +0200 Subject: [PATCH 03/20] Count missed blocks instead of signed blocks --- x/slashing/handler_test.go | 2 +- x/slashing/hooks.go | 2 +- x/slashing/keeper.go | 24 +++++++++++++----------- x/slashing/keeper_test.go | 14 +++++++------- x/slashing/signing_info.go | 16 ++++++++-------- x/slashing/signing_info_test.go | 12 ++++++------ x/slashing/tick_test.go | 2 +- 7 files changed, 37 insertions(+), 35 deletions(-) diff --git a/x/slashing/handler_test.go b/x/slashing/handler_test.go index 059b19059..79092aebb 100644 --- a/x/slashing/handler_test.go +++ b/x/slashing/handler_test.go @@ -53,7 +53,7 @@ func TestJailedValidatorDelegations(t *testing.T) { StartHeight: int64(0), IndexOffset: int64(0), JailedUntil: time.Unix(0, 0), - SignedBlocksCounter: int64(0), + MissedBlocksCounter: int64(0), } slashingKeeper.setValidatorSigningInfo(ctx, consAddr, newInfo) diff --git a/x/slashing/hooks.go b/x/slashing/hooks.go index a2e6c506a..808260830 100644 --- a/x/slashing/hooks.go +++ b/x/slashing/hooks.go @@ -14,7 +14,7 @@ func (k Keeper) onValidatorBonded(ctx sdk.Context, address sdk.ConsAddress) { StartHeight: ctx.BlockHeight(), IndexOffset: 0, JailedUntil: time.Unix(0, 0), - SignedBlocksCounter: 0, + MissedBlocksCounter: 0, } k.setValidatorSigningInfo(ctx, address, signingInfo) } diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 9cd7076cb..23e30fe1e 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -110,23 +110,25 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p // 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.getValidatorSigningBitArray(ctx, consAddr, index) - if previous == signed { + missed := !signed + if previous == missed { // Array value at this index has not changed, no need to update counter - } else if previous && !signed { - // Array value has changed from signed to unsigned, decrement counter - k.setValidatorSigningBitArray(ctx, consAddr, index, false) - signInfo.SignedBlocksCounter-- - } else if !previous && signed { - // Array value has changed from unsigned to signed, increment counter + } else if !previous && missed { + // Array value has changed from not missed to missed, increment counter k.setValidatorSigningBitArray(ctx, consAddr, index, true) - signInfo.SignedBlocksCounter++ + signInfo.MissedBlocksCounter++ + } else if previous && !missed { + // Array value has changed from missed to not missed, decrement counter + k.setValidatorSigningBitArray(ctx, consAddr, index, false) + signInfo.MissedBlocksCounter-- } - if !signed { - logger.Info(fmt.Sprintf("Absent validator %s at height %d, %d signed, threshold %d", addr, height, signInfo.SignedBlocksCounter, k.MinSignedPerWindow(ctx))) + if missed { + logger.Info(fmt.Sprintf("Absent validator %s at height %d, %d missed, threshold %d", addr, height, signInfo.MissedBlocksCounter, k.MinSignedPerWindow(ctx))) } minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx) - if height > minHeight && signInfo.SignedBlocksCounter < k.MinSignedPerWindow(ctx) { + maxMissed := k.SignedBlocksWindow(ctx) - k.MinSignedPerWindow(ctx) + if height > minHeight && signInfo.MissedBlocksCounter > maxMissed { validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr) if validator != nil && !validator.GetJailed() { // Downtime confirmed: slash and jail the validator diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 23a3ae2de..32469b6d3 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -148,7 +148,7 @@ func TestHandleAbsentValidator(t *testing.T) { require.True(t, found) require.Equal(t, int64(0), info.StartHeight) require.Equal(t, int64(0), info.IndexOffset) - require.Equal(t, int64(0), info.SignedBlocksCounter) + require.Equal(t, int64(0), info.MissedBlocksCounter) height := int64(0) // 1000 first blocks OK @@ -159,7 +159,7 @@ func TestHandleAbsentValidator(t *testing.T) { info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, int64(0), info.StartHeight) - require.Equal(t, keeper.SignedBlocksWindow(ctx), info.SignedBlocksCounter) + require.Equal(t, int64(0), info.MissedBlocksCounter) // 500 blocks missed for ; height < keeper.SignedBlocksWindow(ctx)+(keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)); height++ { @@ -169,7 +169,7 @@ func TestHandleAbsentValidator(t *testing.T) { info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, int64(0), info.StartHeight) - require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx), info.SignedBlocksCounter) + require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx), info.MissedBlocksCounter) // validator should be bonded still validator, _ := sk.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(val)) @@ -183,7 +183,7 @@ func TestHandleAbsentValidator(t *testing.T) { info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, int64(0), info.StartHeight) - require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)-1, info.SignedBlocksCounter) + require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)+1, info.MissedBlocksCounter) // end block stake.EndBlocker(ctx, sk) @@ -204,7 +204,7 @@ func TestHandleAbsentValidator(t *testing.T) { info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, int64(0), info.StartHeight) - require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)-2, info.SignedBlocksCounter) + require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)+2, info.MissedBlocksCounter) // end block stake.EndBlocker(ctx, sk) @@ -246,7 +246,7 @@ func TestHandleAbsentValidator(t *testing.T) { require.True(t, found) require.Equal(t, height, info.StartHeight) // we've missed 2 blocks more than the maximum - require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)-2, info.SignedBlocksCounter) + require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)+2, info.MissedBlocksCounter) // validator should not be immediately jailed again height++ @@ -308,7 +308,7 @@ func TestHandleNewValidator(t *testing.T) { require.True(t, found) require.Equal(t, keeper.SignedBlocksWindow(ctx)+1, info.StartHeight) require.Equal(t, int64(2), info.IndexOffset) - require.Equal(t, int64(1), info.SignedBlocksCounter) + require.Equal(t, int64(1), info.MissedBlocksCounter) require.Equal(t, time.Unix(0, 0).UTC(), info.JailedUntil) // validator should be bonded still, should not have been jailed or slashed diff --git a/x/slashing/signing_info.go b/x/slashing/signing_info.go index 1adf49abc..64f47838a 100644 --- a/x/slashing/signing_info.go +++ b/x/slashing/signing_info.go @@ -48,25 +48,25 @@ func (k Keeper) setValidatorSigningBitArray(ctx sdk.Context, address sdk.ConsAdd } // Construct a new `ValidatorSigningInfo` struct -func NewValidatorSigningInfo(startHeight int64, indexOffset int64, jailedUntil time.Time, signedBlocksCounter int64) ValidatorSigningInfo { +func NewValidatorSigningInfo(startHeight int64, indexOffset int64, jailedUntil time.Time, missedBlocksCounter int64) ValidatorSigningInfo { return ValidatorSigningInfo{ StartHeight: startHeight, IndexOffset: indexOffset, JailedUntil: jailedUntil, - SignedBlocksCounter: signedBlocksCounter, + MissedBlocksCounter: missedBlocksCounter, } } // Signing info for a validator type ValidatorSigningInfo struct { - StartHeight int64 `json:"start_height"` // height at which validator was first a candidate OR was unjailed - IndexOffset int64 `json:"index_offset"` // index offset into signed block bit array - JailedUntil time.Time `json:"jailed_until"` // timestamp validator cannot be unjailed until - SignedBlocksCounter int64 `json:"signed_blocks_counter"` // signed blocks counter (to avoid scanning the array every time) + StartHeight int64 `json:"start_height"` // height at which validator was first a candidate OR was unjailed + IndexOffset int64 `json:"index_offset"` // index offset into signed block bit array + JailedUntil time.Time `json:"jailed_until"` // timestamp validator cannot be unjailed until + MissedBlocksCounter int64 `json:"misseded_blocks_counter"` // missed blocks counter (to avoid scanning the array every time) } // Return human readable signing info func (i ValidatorSigningInfo) HumanReadableString() string { - return fmt.Sprintf("Start height: %d, index offset: %d, jailed until: %v, signed blocks counter: %d", - i.StartHeight, i.IndexOffset, i.JailedUntil, i.SignedBlocksCounter) + return fmt.Sprintf("Start height: %d, index offset: %d, jailed until: %v, missed blocks counter: %d", + i.StartHeight, i.IndexOffset, i.JailedUntil, i.MissedBlocksCounter) } diff --git a/x/slashing/signing_info_test.go b/x/slashing/signing_info_test.go index 7aff0da95..1b22d7461 100644 --- a/x/slashing/signing_info_test.go +++ b/x/slashing/signing_info_test.go @@ -17,7 +17,7 @@ func TestGetSetValidatorSigningInfo(t *testing.T) { StartHeight: int64(4), IndexOffset: int64(3), JailedUntil: time.Unix(2, 0), - SignedBlocksCounter: int64(10), + MissedBlocksCounter: int64(10), } keeper.setValidatorSigningInfo(ctx, sdk.ConsAddress(addrs[0]), newInfo) info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(addrs[0])) @@ -25,14 +25,14 @@ func TestGetSetValidatorSigningInfo(t *testing.T) { require.Equal(t, info.StartHeight, int64(4)) require.Equal(t, info.IndexOffset, int64(3)) require.Equal(t, info.JailedUntil, time.Unix(2, 0).UTC()) - require.Equal(t, info.SignedBlocksCounter, int64(10)) + require.Equal(t, info.MissedBlocksCounter, int64(10)) } func TestGetSetValidatorSigningBitArray(t *testing.T) { ctx, _, _, _, keeper := createTestInput(t) - signed := keeper.getValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) - require.False(t, signed) // treat empty key as unsigned + missed := keeper.getValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) + require.False(t, missed) // treat empty key as not missed keeper.setValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0, true) - signed = keeper.getValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) - require.True(t, signed) // now should be signed + missed = keeper.getValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) + require.True(t, missed) // now should be missed } diff --git a/x/slashing/tick_test.go b/x/slashing/tick_test.go index a98d97c54..c85d4f4cb 100644 --- a/x/slashing/tick_test.go +++ b/x/slashing/tick_test.go @@ -45,7 +45,7 @@ func TestBeginBlocker(t *testing.T) { require.Equal(t, ctx.BlockHeight(), info.StartHeight) require.Equal(t, int64(1), info.IndexOffset) require.Equal(t, time.Unix(0, 0).UTC(), info.JailedUntil) - require.Equal(t, int64(1), info.SignedBlocksCounter) + require.Equal(t, int64(0), info.MissedBlocksCounter) height := int64(0) From 8c2c9dba8a9450298e61ac77607ed39dff64bed6 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 12 Oct 2018 00:15:58 +0200 Subject: [PATCH 04/20] Update block height on validator bonding --- client/lcd/lcd_test.go | 1 - x/slashing/hooks.go | 12 +++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/client/lcd/lcd_test.go b/client/lcd/lcd_test.go index 6eef4afd0..4a999b066 100644 --- a/client/lcd/lcd_test.go +++ b/client/lcd/lcd_test.go @@ -713,7 +713,6 @@ func TestUnjail(t *testing.T) { tests.WaitForHeight(4, port) require.Equal(t, true, signingInfo.IndexOffset > 0) require.Equal(t, time.Unix(0, 0).UTC(), signingInfo.JailedUntil) - require.Equal(t, true, signingInfo.SignedBlocksCounter > 0) } func TestProposalsQuery(t *testing.T) { diff --git a/x/slashing/hooks.go b/x/slashing/hooks.go index 808260830..3a5ddd6fd 100644 --- a/x/slashing/hooks.go +++ b/x/slashing/hooks.go @@ -7,17 +7,19 @@ import ( ) func (k Keeper) onValidatorBonded(ctx sdk.Context, address sdk.ConsAddress) { - // Create a new signing info if necessary - _, found := k.getValidatorSigningInfo(ctx, address) - if !found { - signingInfo := ValidatorSigningInfo{ + // Update the signing info start height or create a new signing info + signingInfo, found := k.getValidatorSigningInfo(ctx, address) + if found { + signingInfo.StartHeight = ctx.BlockHeight() + } else { + signingInfo = ValidatorSigningInfo{ StartHeight: ctx.BlockHeight(), IndexOffset: 0, JailedUntil: time.Unix(0, 0), MissedBlocksCounter: 0, } - k.setValidatorSigningInfo(ctx, address, signingInfo) } + k.setValidatorSigningInfo(ctx, address, signingInfo) // Create a new slashing period when a validator is bonded slashingPeriod := ValidatorSlashingPeriod{ From 6e4a8d9e166925d0abea7b8dbf3a25923580aa5b Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 12 Oct 2018 01:04:57 +0200 Subject: [PATCH 05/20] Update spec --- client/lcd/lcd_test.go | 1 + docs/spec/slashing/begin-block.md | 14 +++++++------- docs/spec/slashing/state.md | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/client/lcd/lcd_test.go b/client/lcd/lcd_test.go index 4a999b066..1a7aa7ffe 100644 --- a/client/lcd/lcd_test.go +++ b/client/lcd/lcd_test.go @@ -713,6 +713,7 @@ func TestUnjail(t *testing.T) { tests.WaitForHeight(4, port) require.Equal(t, true, signingInfo.IndexOffset > 0) require.Equal(t, time.Unix(0, 0).UTC(), signingInfo.JailedUntil) + require.Equal(t, true, signingInfo.MissedBlocksCounter == 0) } func TestProposalsQuery(t *testing.T) { diff --git a/docs/spec/slashing/begin-block.md b/docs/spec/slashing/begin-block.md index 375e19185..fb7d04e5d 100644 --- a/docs/spec/slashing/begin-block.md +++ b/docs/spec/slashing/begin-block.md @@ -96,20 +96,20 @@ for val in block.Validators: previous = SigningBitArray.Get(val.Address, index) // update counter if array has changed - if previous and val in block.AbsentValidators: - SigningBitArray.Set(val.Address, index, false) - signInfo.SignedBlocksCounter-- - else if !previous and val not in block.AbsentValidators: + if !previous and val in block.AbsentValidators: SigningBitArray.Set(val.Address, index, true) - signInfo.SignedBlocksCounter++ + signInfo.MissedBlocksCounter++ + else if previous and val not in block.AbsentValidators: + SigningBitArray.Set(val.Address, index, false) + signInfo.MissedBlocksCounter-- // else previous == val not in block.AbsentValidators, no change // validator must be active for at least SIGNED_BLOCKS_WINDOW // before they can be automatically unbonded for failing to be // included in 50% of the recent LastCommits minHeight = signInfo.StartHeight + SIGNED_BLOCKS_WINDOW - minSigned = SIGNED_BLOCKS_WINDOW / 2 - if height > minHeight AND signInfo.SignedBlocksCounter < minSigned: + maxMissed = SIGNED_BLOCKS_WINDOW / 2 + if height > minHeight AND signInfo.MissedBlocksCounter > maxMissed: signInfo.JailedUntil = block.Time + DOWNTIME_UNBOND_DURATION slash & unbond the validator diff --git a/docs/spec/slashing/state.md b/docs/spec/slashing/state.md index ae426db7b..fe6f39f5c 100644 --- a/docs/spec/slashing/state.md +++ b/docs/spec/slashing/state.md @@ -40,7 +40,7 @@ type ValidatorSigningInfo struct { IndexOffset int64 // Offset into the signed block bit array JailedUntilHeight int64 // Block height until which the validator is jailed, // or sentinel value of 0 for not jailed - SignedBlocksCounter int64 // Running counter of signed blocks + MissedBlocksCounter int64 // Running counter of missed blocks } ``` @@ -49,7 +49,7 @@ Where: * `StartHeight` is set to the height that the candidate became an active validator (with non-zero voting power). * `IndexOffset` is incremented each time the candidate was a bonded validator in a block (and may have signed a precommit or not). * `JailedUntil` is set whenever the candidate is jailed due to downtime -* `SignedBlocksCounter` is a counter kept to avoid unnecessary array reads. `SignedBlocksBitArray.Sum() == SignedBlocksCounter` always. +* `MissedBlocksCounter` is a counter kept to avoid unnecessary array reads. `MissedBlocksBitArray.Sum() == MissedBlocksCounter` always. ## Slashing Period From a83535aef34443180c7769a3a2ed7ad04b0ef7ca Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 12 Oct 2018 21:09:23 +0200 Subject: [PATCH 06/20] Greater than to greater than or equal to --- docs/spec/slashing/begin-block.md | 2 +- x/slashing/keeper.go | 2 +- x/slashing/keeper_test.go | 85 +++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 2 deletions(-) diff --git a/docs/spec/slashing/begin-block.md b/docs/spec/slashing/begin-block.md index fb7d04e5d..2d03fb6a7 100644 --- a/docs/spec/slashing/begin-block.md +++ b/docs/spec/slashing/begin-block.md @@ -109,7 +109,7 @@ for val in block.Validators: // included in 50% of the recent LastCommits minHeight = signInfo.StartHeight + SIGNED_BLOCKS_WINDOW maxMissed = SIGNED_BLOCKS_WINDOW / 2 - if height > minHeight AND signInfo.MissedBlocksCounter > maxMissed: + if height >= minHeight AND signInfo.MissedBlocksCounter > maxMissed: signInfo.JailedUntil = block.Time + DOWNTIME_UNBOND_DURATION slash & unbond the validator diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 23e30fe1e..1e0725ae8 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -128,7 +128,7 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p } minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx) maxMissed := k.SignedBlocksWindow(ctx) - k.MinSignedPerWindow(ctx) - if height > minHeight && signInfo.MissedBlocksCounter > maxMissed { + if height >= minHeight && signInfo.MissedBlocksCounter > maxMissed { validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr) if validator != nil && !validator.GetJailed() { // Downtime confirmed: slash and jail the validator diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 32469b6d3..9dcaae6e5 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -364,3 +364,88 @@ func TestHandleAlreadyJailed(t *testing.T) { require.Equal(t, amtInt-1, validator.GetTokens().RoundInt64()) } + +// Test a validator dipping in and out of the validator set +// Ensure that missed blocks are tracked correctly +func TestValidatorDippingInAndOut(t *testing.T) { + + // initial setup + ctx, _, sk, _, keeper := createTestInput(t) + params := sk.GetParams(ctx) + params.MaxValidators = 1 + sk.SetParams(ctx, params) + amtInt := int64(100) + addr, val, amt := addrs[0], pks[0], sdk.NewInt(amtInt) + sh := stake.NewHandler(sk) + got := sh(ctx, newTestMsgCreateValidator(addr, val, amt)) + require.True(t, got.IsOK()) + validatorUpdates := stake.EndBlocker(ctx, sk) + keeper.AddValidators(ctx, validatorUpdates) + + // 100 first blocks OK + height := int64(0) + for ; height < int64(100); height++ { + ctx = ctx.WithBlockHeight(height) + keeper.handleValidatorSignature(ctx, val.Address(), amtInt, true) + } + + // validator kicked out of validator set + newAmt := int64(101) + got = sh(ctx, newTestMsgCreateValidator(addrs[1], pks[1], sdk.NewInt(newAmt))) + require.True(t, got.IsOK()) + validatorUpdates = stake.EndBlocker(ctx, sk) + require.Equal(t, 2, len(validatorUpdates)) + keeper.AddValidators(ctx, validatorUpdates) + validator, _ := sk.GetValidator(ctx, addr) + require.Equal(t, sdk.Unbonding, validator.Status) + + // 600 more blocks happened + height = int64(700) + ctx = ctx.WithBlockHeight(height) + + // validator added back in + got = sh(ctx, newTestMsgDelegate(sdk.AccAddress(addrs[2]), addrs[0], sdk.NewInt(2))) + require.True(t, got.IsOK()) + validatorUpdates = stake.EndBlocker(ctx, sk) + require.Equal(t, 2, len(validatorUpdates)) + validator, _ = sk.GetValidator(ctx, addr) + require.Equal(t, sdk.Bonded, validator.Status) + newAmt = int64(102) + + // validator misses a block + keeper.handleValidatorSignature(ctx, val.Address(), newAmt, false) + height++ + + // shouldn't be jailed/kicked yet + validator, _ = sk.GetValidator(ctx, addr) + require.Equal(t, sdk.Bonded, validator.Status) + + // validator misses 500 blocks + for ; height < int64(1202); height++ { + ctx = ctx.WithBlockHeight(height) + keeper.handleValidatorSignature(ctx, val.Address(), newAmt, false) + } + + // should not yet be jailed & kicked + stake.EndBlocker(ctx, sk) + validator, _ = sk.GetValidator(ctx, addr) + require.Equal(t, sdk.Bonded, validator.Status) + + // validator signs 500 blocks + for ; height < int64(1701); height++ { + ctx = ctx.WithBlockHeight(height) + keeper.handleValidatorSignature(ctx, val.Address(), newAmt, true) + } + + // should have exceeded threshold + signingInfo, found := keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) + require.True(t, found) + require.Equal(t, int64(700), signingInfo.StartHeight) + require.Equal(t, int64(1101), signingInfo.IndexOffset) + require.Equal(t, int64(501), signingInfo.MissedBlocksCounter) + + // should be jailed & kicked + stake.EndBlocker(ctx, sk) + validator, _ = sk.GetValidator(ctx, addr) + require.Equal(t, sdk.Unbonding, validator.Status) +} From bdfb10f5511768e6f4e38604465d59261f89f9ea Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 12 Oct 2018 21:10:55 +0200 Subject: [PATCH 07/20] Clarify testcase --- x/slashing/keeper_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 9dcaae6e5..6b53eaf8c 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -366,7 +366,8 @@ func TestHandleAlreadyJailed(t *testing.T) { } // Test a validator dipping in and out of the validator set -// Ensure that missed blocks are tracked correctly +// Ensure that missed blocks are tracked correctly and that +// the start height of the signing info is reset correctly func TestValidatorDippingInAndOut(t *testing.T) { // initial setup From 1ff2e865a8ce845eed0e433a1bfe4c040947fc69 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 12 Oct 2018 21:15:39 +0200 Subject: [PATCH 08/20] Back to greater than --- docs/spec/slashing/begin-block.md | 2 +- x/slashing/keeper.go | 2 +- x/slashing/keeper_test.go | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/spec/slashing/begin-block.md b/docs/spec/slashing/begin-block.md index 2d03fb6a7..fb7d04e5d 100644 --- a/docs/spec/slashing/begin-block.md +++ b/docs/spec/slashing/begin-block.md @@ -109,7 +109,7 @@ for val in block.Validators: // included in 50% of the recent LastCommits minHeight = signInfo.StartHeight + SIGNED_BLOCKS_WINDOW maxMissed = SIGNED_BLOCKS_WINDOW / 2 - if height >= minHeight AND signInfo.MissedBlocksCounter > maxMissed: + if height > minHeight AND signInfo.MissedBlocksCounter > maxMissed: signInfo.JailedUntil = block.Time + DOWNTIME_UNBOND_DURATION slash & unbond the validator diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 1e0725ae8..23e30fe1e 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -128,7 +128,7 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p } minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx) maxMissed := k.SignedBlocksWindow(ctx) - k.MinSignedPerWindow(ctx) - if height >= minHeight && signInfo.MissedBlocksCounter > maxMissed { + if height > minHeight && signInfo.MissedBlocksCounter > maxMissed { validator := k.validatorSet.ValidatorByConsAddr(ctx, consAddr) if validator != nil && !validator.GetJailed() { // Downtime confirmed: slash and jail the validator diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 6b53eaf8c..2efc8dd84 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -421,8 +421,8 @@ func TestValidatorDippingInAndOut(t *testing.T) { validator, _ = sk.GetValidator(ctx, addr) require.Equal(t, sdk.Bonded, validator.Status) - // validator misses 500 blocks - for ; height < int64(1202); height++ { + // validator misses 501 blocks + for ; height < int64(1203); height++ { ctx = ctx.WithBlockHeight(height) keeper.handleValidatorSignature(ctx, val.Address(), newAmt, false) } @@ -433,7 +433,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { require.Equal(t, sdk.Bonded, validator.Status) // validator signs 500 blocks - for ; height < int64(1701); height++ { + for ; height < int64(1702); height++ { ctx = ctx.WithBlockHeight(height) keeper.handleValidatorSignature(ctx, val.Address(), newAmt, true) } @@ -442,7 +442,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { signingInfo, found := keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, int64(700), signingInfo.StartHeight) - require.Equal(t, int64(1101), signingInfo.IndexOffset) + require.Equal(t, int64(1102), signingInfo.IndexOffset) require.Equal(t, int64(501), signingInfo.MissedBlocksCounter) // should be jailed & kicked From fa372d3b82b3fe668315a348f155b7d5d8f01344 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 21:08:56 +0200 Subject: [PATCH 09/20] Test correct JailedUntil --- x/slashing/keeper_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 7b1ee24bc..52defda8d 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -151,6 +151,7 @@ func TestHandleAbsentValidator(t *testing.T) { require.Equal(t, int64(0), info.StartHeight) require.Equal(t, int64(0), info.IndexOffset) require.Equal(t, int64(0), info.MissedBlocksCounter) + require.Equal(t, time.Unix(0, 0).UTC(), info.JailedUntil) height := int64(0) // 1000 first blocks OK From 6e0f3d6baf106eca44c736b03e75da9b292c3ad0 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 21:11:32 +0200 Subject: [PATCH 10/20] Update function names to clarify semantics --- x/slashing/keeper.go | 6 +++--- x/slashing/signing_info.go | 4 ++-- x/slashing/signing_info_test.go | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 61cfbb15c..68cbd90b0 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -110,17 +110,17 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p // Update signed block bit array & counter // 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.getValidatorSigningBitArray(ctx, consAddr, index) + previous := k.getValidatorMissedBlockBitArray(ctx, consAddr, index) missed := !signed if previous == missed { // Array value at this index has not changed, no need to update counter } else if !previous && missed { // Array value has changed from not missed to missed, increment counter - k.setValidatorSigningBitArray(ctx, consAddr, index, true) + k.setValidatorMissedBlockBitArray(ctx, consAddr, index, true) signInfo.MissedBlocksCounter++ } else if previous && !missed { // Array value has changed from missed to not missed, decrement counter - k.setValidatorSigningBitArray(ctx, consAddr, index, false) + k.setValidatorMissedBlockBitArray(ctx, consAddr, index, false) signInfo.MissedBlocksCounter-- } diff --git a/x/slashing/signing_info.go b/x/slashing/signing_info.go index 64f47838a..88e00fd67 100644 --- a/x/slashing/signing_info.go +++ b/x/slashing/signing_info.go @@ -28,7 +28,7 @@ func (k Keeper) setValidatorSigningInfo(ctx sdk.Context, address sdk.ConsAddress } // Stored by *validator* address (not operator address) -func (k Keeper) getValidatorSigningBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64) (signed bool) { +func (k Keeper) getValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64) (signed bool) { store := ctx.KVStore(k.storeKey) bz := store.Get(GetValidatorSigningBitArrayKey(address, index)) if bz == nil { @@ -41,7 +41,7 @@ func (k Keeper) getValidatorSigningBitArray(ctx sdk.Context, address sdk.ConsAdd } // Stored by *validator* address (not operator address) -func (k Keeper) setValidatorSigningBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64, signed bool) { +func (k Keeper) setValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64, signed bool) { store := ctx.KVStore(k.storeKey) bz := k.cdc.MustMarshalBinary(signed) store.Set(GetValidatorSigningBitArrayKey(address, index), bz) diff --git a/x/slashing/signing_info_test.go b/x/slashing/signing_info_test.go index f5c8d6ea3..c5ffc8bf1 100644 --- a/x/slashing/signing_info_test.go +++ b/x/slashing/signing_info_test.go @@ -30,9 +30,9 @@ func TestGetSetValidatorSigningInfo(t *testing.T) { func TestGetSetValidatorSigningBitArray(t *testing.T) { ctx, _, _, _, keeper := createTestInput(t, DefaultParams()) - missed := keeper.getValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) + missed := keeper.getValidatorMissedBlockBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) require.False(t, missed) // treat empty key as not missed - keeper.setValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0, true) - missed = keeper.getValidatorSigningBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) + keeper.setValidatorMissedBlockBitArray(ctx, sdk.ConsAddress(addrs[0]), 0, true) + missed = keeper.getValidatorMissedBlockBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) require.True(t, missed) // now should be missed } From 096a8bb9ec68141d6b3eb974bcac30340789997e Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 21:20:37 +0200 Subject: [PATCH 11/20] Further clarification of nomenclature --- x/slashing/keys.go | 17 +++++++++++------ x/slashing/signing_info.go | 25 +++++++++++++++++-------- x/slashing/signing_info_test.go | 2 +- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/x/slashing/keys.go b/x/slashing/keys.go index ec453cedc..8f4fecc6c 100644 --- a/x/slashing/keys.go +++ b/x/slashing/keys.go @@ -9,10 +9,10 @@ import ( // key prefix bytes var ( - ValidatorSigningInfoKey = []byte{0x01} // Prefix for signing info - ValidatorSigningBitArrayKey = []byte{0x02} // Prefix for signature bit array - ValidatorSlashingPeriodKey = []byte{0x03} // Prefix for slashing period - AddrPubkeyRelationKey = []byte{0x04} // Prefix for address-pubkey relation + ValidatorSigningInfoKey = []byte{0x01} // Prefix for signing info + ValidatorMissedBlockBitArrayKey = []byte{0x02} // Prefix for missed block bit array + ValidatorSlashingPeriodKey = []byte{0x03} // Prefix for slashing period + AddrPubkeyRelationKey = []byte{0x04} // Prefix for address-pubkey relation ) // stored by *Tendermint* address (not operator address) @@ -21,10 +21,15 @@ func GetValidatorSigningInfoKey(v sdk.ConsAddress) []byte { } // stored by *Tendermint* address (not operator address) -func GetValidatorSigningBitArrayKey(v sdk.ConsAddress, i int64) []byte { +func GetValidatorMissedBlockBitArrayPrefixKey(v sdk.ConsAddress) []byte { + return append(ValidatorMissedBlockBitArrayKey, v.Bytes()...) +} + +// stored by *Tendermint* address (not operator address) +func GetValidatorMissedBlockBitArrayKey(v sdk.ConsAddress, i int64) []byte { b := make([]byte, 8) binary.LittleEndian.PutUint64(b, uint64(i)) - return append(ValidatorSigningBitArrayKey, append(v.Bytes(), b...)...) + return append(GetValidatorMissedBlockBitArrayPrefixKey(v), b...) } // stored by *Tendermint* address (not operator address) diff --git a/x/slashing/signing_info.go b/x/slashing/signing_info.go index 88e00fd67..d921f9d7e 100644 --- a/x/slashing/signing_info.go +++ b/x/slashing/signing_info.go @@ -28,23 +28,32 @@ func (k Keeper) setValidatorSigningInfo(ctx sdk.Context, address sdk.ConsAddress } // Stored by *validator* address (not operator address) -func (k Keeper) getValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64) (signed bool) { +func (k Keeper) getValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64) (missed bool) { store := ctx.KVStore(k.storeKey) - bz := store.Get(GetValidatorSigningBitArrayKey(address, index)) + bz := store.Get(GetValidatorMissedBlockBitArrayKey(address, index)) if bz == nil { - // lazy: treat empty key as unsigned - signed = false + // lazy: treat empty key as not missed + missed = false return } - k.cdc.MustUnmarshalBinary(bz, &signed) + k.cdc.MustUnmarshalBinary(bz, &missed) return } // Stored by *validator* address (not operator address) -func (k Keeper) setValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64, signed bool) { +func (k Keeper) setValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress, index int64, missed bool) { store := ctx.KVStore(k.storeKey) - bz := k.cdc.MustMarshalBinary(signed) - store.Set(GetValidatorSigningBitArrayKey(address, index), bz) + bz := k.cdc.MustMarshalBinary(missed) + store.Set(GetValidatorMissedBlockBitArrayKey(address, index), bz) +} + +// Stored by *validator* address (not operator address) +func (k Keeper) clearValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.ConsAddress) { + store := ctx.KVStore(k.storeKey) + iter := sdk.KVStorePrefixIterator(store, GetValidatorMissedBlockBitArrayPrefixKey(address)) + for ; iter.Valid(); iter.Next() { + store.Delete(iter.Key()) + } } // Construct a new `ValidatorSigningInfo` struct diff --git a/x/slashing/signing_info_test.go b/x/slashing/signing_info_test.go index c5ffc8bf1..15863ebc7 100644 --- a/x/slashing/signing_info_test.go +++ b/x/slashing/signing_info_test.go @@ -28,7 +28,7 @@ func TestGetSetValidatorSigningInfo(t *testing.T) { require.Equal(t, info.MissedBlocksCounter, int64(10)) } -func TestGetSetValidatorSigningBitArray(t *testing.T) { +func TestGetSetValidatorMissedBlockBitArray(t *testing.T) { ctx, _, _, _, keeper := createTestInput(t, DefaultParams()) missed := keeper.getValidatorMissedBlockBitArray(ctx, sdk.ConsAddress(addrs[0]), 0) require.False(t, missed) // treat empty key as not missed From 9ec35a0ae4e151e68c5696b5f665c3718ec127ea Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 21:44:23 +0200 Subject: [PATCH 12/20] Clear array when validator is jailed --- x/slashing/hooks.go | 4 +--- x/slashing/keeper.go | 4 ++++ x/slashing/keeper_test.go | 44 +++++++++++++++++++++++++++------------ 3 files changed, 36 insertions(+), 16 deletions(-) diff --git a/x/slashing/hooks.go b/x/slashing/hooks.go index 3a5ddd6fd..3710e4072 100644 --- a/x/slashing/hooks.go +++ b/x/slashing/hooks.go @@ -9,9 +9,7 @@ import ( func (k Keeper) onValidatorBonded(ctx sdk.Context, address sdk.ConsAddress) { // Update the signing info start height or create a new signing info signingInfo, found := k.getValidatorSigningInfo(ctx, address) - if found { - signingInfo.StartHeight = ctx.BlockHeight() - } else { + if !found { signingInfo = ValidatorSigningInfo{ StartHeight: ctx.BlockHeight(), IndexOffset: 0, diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 68cbd90b0..73734198d 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -144,6 +144,10 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p k.validatorSet.Slash(ctx, consAddr, distributionHeight, power, k.SlashFractionDowntime(ctx)) k.validatorSet.Jail(ctx, consAddr) signInfo.JailedUntil = ctx.BlockHeader().Time.Add(k.DowntimeUnbondDuration(ctx)) + // We need to reset the counter & array so that the validator won't be immediately slashed for downtime upon rebonding. + signInfo.MissedBlocksCounter = 0 + signInfo.IndexOffset = 0 + k.clearValidatorMissedBlockBitArray(ctx, consAddr) } else { // Validator was (a) not found or (b) already jailed, don't slash logger.Info(fmt.Sprintf("Validator %s would have been slashed for downtime, but was either not found in store or already jailed", diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 52defda8d..4299bb7cc 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -380,6 +380,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { sk.SetParams(ctx, params) amtInt := int64(100) addr, val, amt := addrs[0], pks[0], sdk.NewInt(amtInt) + consAddr := sdk.ConsAddress(addr) sh := stake.NewHandler(sk) got := sh(ctx, newTestMsgCreateValidator(addr, val, amt)) require.True(t, got.IsOK()) @@ -425,31 +426,48 @@ func TestValidatorDippingInAndOut(t *testing.T) { require.Equal(t, sdk.Bonded, validator.Status) // validator misses 501 blocks - for ; height < int64(1203); height++ { + for ; height < int64(1201); height++ { ctx = ctx.WithBlockHeight(height) keeper.handleValidatorSignature(ctx, val.Address(), newAmt, false) } - // should not yet be jailed & kicked + // should now be jailed & kicked + stake.EndBlocker(ctx, sk) + validator, _ = sk.GetValidator(ctx, addr) + require.Equal(t, sdk.Unbonding, validator.Status) + + signInfo, found := keeper.getValidatorSigningInfo(ctx, consAddr) + require.True(t, found) + require.Equal(t, int64(0), signInfo.MissedBlocksCounter) + require.Equal(t, int64(0), signInfo.IndexOffset) + // array should be cleared + for offset := int64(0); offset < keeper.SignedBlocksWindow(ctx); offset++ { + missed := keeper.getValidatorMissedBlockBitArray(ctx, consAddr, offset) + require.False(t, missed) + } + + // some blocks pass + height = int64(5000) + ctx = ctx.WithBlockHeight(height) + + // validator rejoins and starts signing again + sk.Unjail(ctx, consAddr) + keeper.handleValidatorSignature(ctx, val.Address(), newAmt, true) + + // validator should not be kicked stake.EndBlocker(ctx, sk) validator, _ = sk.GetValidator(ctx, addr) require.Equal(t, sdk.Bonded, validator.Status) - // validator signs 500 blocks - for ; height < int64(1702); height++ { + // validator misses 501 blocks + for height = int64(5001); height < int64(5503); height++ { ctx = ctx.WithBlockHeight(height) - keeper.handleValidatorSignature(ctx, val.Address(), newAmt, true) + keeper.handleValidatorSignature(ctx, val.Address(), newAmt, false) } - // should have exceeded threshold - signingInfo, found := keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) - require.True(t, found) - require.Equal(t, int64(700), signingInfo.StartHeight) - require.Equal(t, int64(1102), signingInfo.IndexOffset) - require.Equal(t, int64(501), signingInfo.MissedBlocksCounter) - - // should be jailed & kicked + // validator should now be jailed & kicked stake.EndBlocker(ctx, sk) validator, _ = sk.GetValidator(ctx, addr) require.Equal(t, sdk.Unbonding, validator.Status) + } From ec53a14b57ea7b81968a5a73d273e80deaf1ba06 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 21:51:26 +0200 Subject: [PATCH 13/20] Fix testcases --- x/slashing/keeper_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 4299bb7cc..d1cc44408 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -186,7 +186,8 @@ func TestHandleAbsentValidator(t *testing.T) { info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, int64(0), info.StartHeight) - require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)+1, info.MissedBlocksCounter) + // counter now reset to zero + require.Equal(t, int64(0), info.MissedBlocksCounter) // end block stake.EndBlocker(ctx, sk) @@ -207,7 +208,7 @@ func TestHandleAbsentValidator(t *testing.T) { info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, int64(0), info.StartHeight) - require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)+2, info.MissedBlocksCounter) + require.Equal(t, int64(1), info.MissedBlocksCounter) // end block stake.EndBlocker(ctx, sk) @@ -248,8 +249,8 @@ func TestHandleAbsentValidator(t *testing.T) { info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) require.Equal(t, height, info.StartHeight) - // we've missed 2 blocks more than the maximum - require.Equal(t, keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx)+2, info.MissedBlocksCounter) + // we've missed 2 blocks more than the maximum, so the counter was reset to 0 at 1 block more and is now 1 + require.Equal(t, int64(1), info.MissedBlocksCounter) // validator should not be immediately jailed again height++ From 83f7a4cb7b56c845eb355f521d4d72a0ecfbdf2d Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 23:01:29 +0200 Subject: [PATCH 14/20] Bugfix; update slashing spec --- docs/spec/slashing/begin-block.md | 10 ++++++---- docs/spec/slashing/hooks.md | 11 +++++++++++ docs/spec/slashing/state.md | 8 ++++---- docs/spec/slashing/transactions.md | 4 ---- x/slashing/handler.go | 6 +----- x/slashing/hooks.go | 2 +- x/slashing/keeper_test.go | 4 ++-- 7 files changed, 25 insertions(+), 20 deletions(-) diff --git a/docs/spec/slashing/begin-block.md b/docs/spec/slashing/begin-block.md index fb7d04e5d..43691b38d 100644 --- a/docs/spec/slashing/begin-block.md +++ b/docs/spec/slashing/begin-block.md @@ -93,14 +93,14 @@ for val in block.Validators: index := signInfo.IndexOffset % SIGNED_BLOCKS_WINDOW signInfo.IndexOffset++ - previous = SigningBitArray.Get(val.Address, index) + previous = MissedBlockBitArray.Get(val.Address, index) // update counter if array has changed if !previous and val in block.AbsentValidators: - SigningBitArray.Set(val.Address, index, true) + MissedBlockBitArray.Set(val.Address, index, true) signInfo.MissedBlocksCounter++ else if previous and val not in block.AbsentValidators: - SigningBitArray.Set(val.Address, index, false) + MissedBlockBitArray.Set(val.Address, index, false) signInfo.MissedBlocksCounter-- // else previous == val not in block.AbsentValidators, no change @@ -111,7 +111,9 @@ for val in block.Validators: maxMissed = SIGNED_BLOCKS_WINDOW / 2 if height > minHeight AND signInfo.MissedBlocksCounter > maxMissed: signInfo.JailedUntil = block.Time + DOWNTIME_UNBOND_DURATION - + signInfo.IndexOffset = 0 + signInfo.MissedBlocksCounter = 0 + clearMissedBlockBitArray() slash & unbond the validator SigningInfo.Set(val.Address, signInfo) diff --git a/docs/spec/slashing/hooks.md b/docs/spec/slashing/hooks.md index 36dde61f9..1888c1d2f 100644 --- a/docs/spec/slashing/hooks.md +++ b/docs/spec/slashing/hooks.md @@ -12,6 +12,17 @@ and `SlashedSoFar` of `0`: ``` onValidatorBonded(address sdk.ValAddress) + signingInfo, found = getValidatorSigningInfo(address) + if !found { + signingInfo = ValidatorSigningInfo { + StartHeight : CurrentHeight, + IndexOffset : 0, + JailedUntil : time.Unix(0, 0), + MissedBloskCounter : 0 + } + setValidatorSigningInfo(signingInfo) + } + slashingPeriod = SlashingPeriod{ ValidatorAddr : address, StartHeight : CurrentHeight, diff --git a/docs/spec/slashing/state.md b/docs/spec/slashing/state.md index fe6f39f5c..9a9a188ff 100644 --- a/docs/spec/slashing/state.md +++ b/docs/spec/slashing/state.md @@ -17,18 +17,18 @@ Information about validator activity is tracked in a `ValidatorSigningInfo`. It is indexed in the store as follows: - SigningInfo: ` 0x01 | ValTendermintAddr -> amino(valSigningInfo)` -- SigningBitArray: ` 0x02 | ValTendermintAddr | LittleEndianUint64(signArrayIndex) -> VarInt(didSign)` +- MissedBlocksBitArray: ` 0x02 | ValTendermintAddr | LittleEndianUint64(signArrayIndex) -> VarInt(didMiss)` The first map allows us to easily lookup the recent signing info for a validator, according to the Tendermint validator address. The second map acts as -a bit-array of size `SIGNED_BLOCKS_WINDOW` that tells us if the validator signed for a given index in the bit-array. +a bit-array of size `SIGNED_BLOCKS_WINDOW` that tells us if the validator missed the block for a given index in the bit-array. The index in the bit-array is given as little endian uint64. The result is a `varint` that takes on `0` or `1`, where `0` indicates the -validator did not sign the corresponding block, and `1` indicates they did. +validator did not miss (did sign) the corresponding block, and `1` indicates they missed the block (did not sign). -Note that the SigningBitArray is not explicitly initialized up-front. Keys are +Note that the MissedBlocksBitArray is not explicitly initialized up-front. Keys are added as we progress through the first `SIGNED_BLOCKS_WINDOW` blocks for a newly bonded validator. diff --git a/docs/spec/slashing/transactions.md b/docs/spec/slashing/transactions.md index be33ee096..c33e96047 100644 --- a/docs/spec/slashing/transactions.md +++ b/docs/spec/slashing/transactions.md @@ -25,10 +25,6 @@ handleMsgUnjail(tx TxUnjail) if block time < info.JailedUntil fail with "Validator still jailed, cannot unjail until period has expired" - // Update the start height so the validator won't be immediately unbonded again - info.StartHeight = BlockHeight - setValidatorSigningInfo(info) - validator.Jailed = false setValidator(validator) diff --git a/x/slashing/handler.go b/x/slashing/handler.go index 740166d2a..701577080 100644 --- a/x/slashing/handler.go +++ b/x/slashing/handler.go @@ -46,11 +46,7 @@ func handleMsgUnjail(ctx sdk.Context, msg MsgUnjail, k Keeper) sdk.Result { return ErrValidatorJailed(k.codespace).Result() } - // update the starting height so the validator can't be immediately jailed - // again - info.StartHeight = ctx.BlockHeight() - k.setValidatorSigningInfo(ctx, consAddr, info) - + // unjail the validator k.validatorSet.Unjail(ctx, consAddr) tags := sdk.NewTags("action", []byte("unjail"), "validator", []byte(msg.ValidatorAddr.String())) diff --git a/x/slashing/hooks.go b/x/slashing/hooks.go index 3710e4072..669536928 100644 --- a/x/slashing/hooks.go +++ b/x/slashing/hooks.go @@ -16,8 +16,8 @@ func (k Keeper) onValidatorBonded(ctx sdk.Context, address sdk.ConsAddress) { JailedUntil: time.Unix(0, 0), MissedBlocksCounter: 0, } + k.setValidatorSigningInfo(ctx, address, signingInfo) } - k.setValidatorSigningInfo(ctx, address, signingInfo) // Create a new slashing period when a validator is bonded slashingPeriod := ValidatorSlashingPeriod{ diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index d1cc44408..0af3936b2 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -245,10 +245,10 @@ func TestHandleAbsentValidator(t *testing.T) { pool = sk.GetPool(ctx) require.Equal(t, amtInt-slashAmt-secondSlashAmt, pool.BondedTokens.RoundInt64()) - // validator start height should have been changed + // validator start height should not have been changed info, found = keeper.getValidatorSigningInfo(ctx, sdk.ConsAddress(val.Address())) require.True(t, found) - require.Equal(t, height, info.StartHeight) + require.Equal(t, int64(0), info.StartHeight) // we've missed 2 blocks more than the maximum, so the counter was reset to 0 at 1 block more and is now 1 require.Equal(t, int64(1), info.MissedBlocksCounter) From e3fa9820ec2b22e7469ab7f8ff10835e59cbc41f Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 23:06:46 +0200 Subject: [PATCH 15/20] Fix linter warning --- x/slashing/hooks.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/slashing/hooks.go b/x/slashing/hooks.go index 669536928..ed07bc0c7 100644 --- a/x/slashing/hooks.go +++ b/x/slashing/hooks.go @@ -8,9 +8,9 @@ import ( func (k Keeper) onValidatorBonded(ctx sdk.Context, address sdk.ConsAddress) { // Update the signing info start height or create a new signing info - signingInfo, found := k.getValidatorSigningInfo(ctx, address) + _, found := k.getValidatorSigningInfo(ctx, address) if !found { - signingInfo = ValidatorSigningInfo{ + signingInfo := ValidatorSigningInfo{ StartHeight: ctx.BlockHeight(), IndexOffset: 0, JailedUntil: time.Unix(0, 0), From 7bf8a41463aa79f7ec9efcf04871f4762cd52777 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 23:09:16 +0200 Subject: [PATCH 16/20] Fix typo --- x/slashing/signing_info.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x/slashing/signing_info.go b/x/slashing/signing_info.go index d921f9d7e..e8795cd00 100644 --- a/x/slashing/signing_info.go +++ b/x/slashing/signing_info.go @@ -68,10 +68,10 @@ func NewValidatorSigningInfo(startHeight int64, indexOffset int64, jailedUntil t // Signing info for a validator type ValidatorSigningInfo struct { - StartHeight int64 `json:"start_height"` // height at which validator was first a candidate OR was unjailed - IndexOffset int64 `json:"index_offset"` // index offset into signed block bit array - JailedUntil time.Time `json:"jailed_until"` // timestamp validator cannot be unjailed until - MissedBlocksCounter int64 `json:"misseded_blocks_counter"` // missed blocks counter (to avoid scanning the array every time) + StartHeight int64 `json:"start_height"` // height at which validator was first a candidate OR was unjailed + IndexOffset int64 `json:"index_offset"` // index offset into signed block bit array + JailedUntil time.Time `json:"jailed_until"` // timestamp validator cannot be unjailed until + MissedBlocksCounter int64 `json:"missed_blocks_counter"` // missed blocks counter (to avoid scanning the array every time) } // Return human readable signing info From c0c5e293a8c53cdadd55a5975f0bc742f75fb9c7 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 23:09:56 +0200 Subject: [PATCH 17/20] iter.Close() --- x/slashing/signing_info.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/slashing/signing_info.go b/x/slashing/signing_info.go index e8795cd00..8c35693f1 100644 --- a/x/slashing/signing_info.go +++ b/x/slashing/signing_info.go @@ -54,6 +54,7 @@ func (k Keeper) clearValidatorMissedBlockBitArray(ctx sdk.Context, address sdk.C for ; iter.Valid(); iter.Next() { store.Delete(iter.Key()) } + iter.Close() } // Construct a new `ValidatorSigningInfo` struct From f9877508e5f2af5d3e5d0ec235502cdbbddd70d1 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 15 Oct 2018 23:58:39 +0200 Subject: [PATCH 18/20] Address @rigelrozanski comments --- x/slashing/keeper.go | 9 +++++---- x/slashing/keeper_test.go | 13 +++++++++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 73734198d..861391ac2 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -112,16 +112,17 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p // That way we avoid needing to read/write the whole array each time previous := k.getValidatorMissedBlockBitArray(ctx, consAddr, index) missed := !signed - if previous == missed { - // Array value at this index has not changed, no need to update counter - } else if !previous && missed { + switch { + case !previous && missed: // Array value has changed from not missed to missed, increment counter k.setValidatorMissedBlockBitArray(ctx, consAddr, index, true) signInfo.MissedBlocksCounter++ - } else if previous && !missed { + case previous && !missed: // Array value has changed from missed to not missed, decrement counter k.setValidatorMissedBlockBitArray(ctx, consAddr, index, false) signInfo.MissedBlocksCounter-- + default: + // Array value at this index has not changed, no need to update counter } if missed { diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 0af3936b2..6ad5c3792 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -375,6 +375,7 @@ func TestHandleAlreadyJailed(t *testing.T) { func TestValidatorDippingInAndOut(t *testing.T) { // initial setup + // keeperTestParams set the SignedBlocksWindow to 1000 and MaxMissedBlocksPerWindow to 500 ctx, _, sk, _, keeper := createTestInput(t, keeperTestParams()) params := sk.GetParams(ctx) params.MaxValidators = 1 @@ -426,8 +427,9 @@ func TestValidatorDippingInAndOut(t *testing.T) { validator, _ = sk.GetValidator(ctx, addr) require.Equal(t, sdk.Bonded, validator.Status) - // validator misses 501 blocks - for ; height < int64(1201); height++ { + // validator misses 500 more blocks, 501 total + latest := height + for ; height < latest+500; height++ { ctx = ctx.WithBlockHeight(height) keeper.handleValidatorSignature(ctx, val.Address(), newAmt, false) } @@ -437,6 +439,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { validator, _ = sk.GetValidator(ctx, addr) require.Equal(t, sdk.Unbonding, validator.Status) + // check all the signing information signInfo, found := keeper.getValidatorSigningInfo(ctx, consAddr) require.True(t, found) require.Equal(t, int64(0), signInfo.MissedBlocksCounter) @@ -454,14 +457,16 @@ func TestValidatorDippingInAndOut(t *testing.T) { // validator rejoins and starts signing again sk.Unjail(ctx, consAddr) keeper.handleValidatorSignature(ctx, val.Address(), newAmt, true) + height++ - // validator should not be kicked + // validator should not be kicked since we reset counter/array when it was jailed stake.EndBlocker(ctx, sk) validator, _ = sk.GetValidator(ctx, addr) require.Equal(t, sdk.Bonded, validator.Status) // validator misses 501 blocks - for height = int64(5001); height < int64(5503); height++ { + latest = height + for ; height < latest+501; height++ { ctx = ctx.WithBlockHeight(height) keeper.handleValidatorSignature(ctx, val.Address(), newAmt, false) } From 7d46b8b50c2895f2cd871df9aae1b3549f2d0fc5 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 16 Oct 2018 19:27:16 +0200 Subject: [PATCH 19/20] Alphanumeric parameter keys --- x/distribution/keeper/key.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x/distribution/keeper/key.go b/x/distribution/keeper/key.go index 2e5989081..466d83c24 100644 --- a/x/distribution/keeper/key.go +++ b/x/distribution/keeper/key.go @@ -13,9 +13,9 @@ var ( ProposerKey = []byte{0x04} // key for storing the proposer operator address // params store - ParamStoreKeyCommunityTax = []byte("community-tax") - ParamStoreKeyBaseProposerReward = []byte("base-proposer-reward") - ParamStoreKeyBonusProposerReward = []byte("bonus-proposer-reward") + ParamStoreKeyCommunityTax = []byte("communitytax") + ParamStoreKeyBaseProposerReward = []byte("baseproposerreward") + ParamStoreKeyBonusProposerReward = []byte("bonusproposerreward") ) const ( From 0d1104330f48df912529a7714f07d2d086fa1319 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 16 Oct 2018 19:42:51 +0200 Subject: [PATCH 20/20] Fix merge rename --- x/slashing/keeper_test.go | 6 +++--- x/stake/handler_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 77d0ebcf8..caf1cf3da 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -296,7 +296,7 @@ func TestHandleNewValidator(t *testing.T) { ctx = ctx.WithBlockHeight(keeper.SignedBlocksWindow(ctx) + 1) // Validator created - got := sh(ctx, newTestMsgCreateValidator(addr, val, sdk.NewInt(amt))) + got := sh(ctx, NewTestMsgCreateValidator(addr, val, sdk.NewInt(amt))) require.True(t, got.IsOK()) validatorUpdates := stake.EndBlocker(ctx, sk) keeper.AddValidators(ctx, validatorUpdates) @@ -384,7 +384,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { addr, val, amt := addrs[0], pks[0], sdk.NewInt(amtInt) consAddr := sdk.ConsAddress(addr) sh := stake.NewHandler(sk) - got := sh(ctx, newTestMsgCreateValidator(addr, val, amt)) + got := sh(ctx, NewTestMsgCreateValidator(addr, val, amt)) require.True(t, got.IsOK()) validatorUpdates := stake.EndBlocker(ctx, sk) keeper.AddValidators(ctx, validatorUpdates) @@ -398,7 +398,7 @@ func TestValidatorDippingInAndOut(t *testing.T) { // validator kicked out of validator set newAmt := int64(101) - got = sh(ctx, newTestMsgCreateValidator(addrs[1], pks[1], sdk.NewInt(newAmt))) + got = sh(ctx, NewTestMsgCreateValidator(addrs[1], pks[1], sdk.NewInt(newAmt))) require.True(t, got.IsOK()) validatorUpdates = stake.EndBlocker(ctx, sk) require.Equal(t, 2, len(validatorUpdates)) diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index 083bb9a91..8755c91b0 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -847,11 +847,11 @@ func TestConflictingRedelegation(t *testing.T) { keeper.SetParams(ctx, params) // create the validators - msgCreateValidator := newTestMsgCreateValidator(validatorAddr, keep.PKs[0], 10) + msgCreateValidator := NewTestMsgCreateValidator(validatorAddr, keep.PKs[0], 10) got := handleMsgCreateValidator(ctx, msgCreateValidator, keeper) require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator") - msgCreateValidator = newTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 10) + msgCreateValidator = NewTestMsgCreateValidator(validatorAddr2, keep.PKs[1], 10) got = handleMsgCreateValidator(ctx, msgCreateValidator, keeper) require.True(t, got.IsOK(), "expected no error on runMsgCreateValidator")