R4R: Fix signing info handling bugs & faulty slashing

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

client/lcd/lcd_test.go

@@ -713,7 +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.SignedBlocksCounter > 0)
+	require.Equal(t, true, signingInfo.MissedBlocksCounter == 0)
 }
 
 func TestProposalsQuery(t *testing.T) {

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

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
 

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)
 

x/slashing/hooks.go

@@ -1,11 +1,27 @@
 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) {
+	// 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)
+
+	// Create a new slashing period when a validator is bonded
 	slashingPeriod := ValidatorSlashingPeriod{
 		ValidatorAddr: address,
 		StartHeight:   ctx.BlockHeight(),

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++
@@ -111,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

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)
+	require.Equal(t, int64(0), info.MissedBlocksCounter)
 	height := int64(0)
 
 	// 1000 first blocks OK
@@ -164,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++ {
@@ -174,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))
@@ -188,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)
@@ -209,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)
@@ -251,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++
@@ -292,16 +287,18 @@ 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)
 	keeper.AddValidators(ctx, validatorUpdates)
 	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)
@@ -311,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
@@ -367,3 +364,89 @@ 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 and that
+// the start height of the signing info is reset 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 501 blocks
+	for ; height < int64(1203); 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(1702); 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(1102), 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)
+}

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