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
     * [x/stake] \#2412 Added an unbonding validator queue to EndBlock to automatically update validator.Status when finished Unbonding
 
 * Tendermint

client/lcd/certificates.go

@@ -43,7 +43,7 @@ func generateSelfSignedCert(host string) (certBytes []byte, priv *ecdsa.PrivateK
 		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
 		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 		BasicConstraintsValid: true,
-		IsCA: true,
+		IsCA:                  true,
 	}
 	hosts := strings.Split(host, ",")
 	for _, h := range hosts {

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

@@ -93,25 +93,27 @@ 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, false)
-    signInfo.SignedBlocksCounter--
-  else if !previous and val not in block.AbsentValidators:
-    SigningBitArray.Set(val.Address, index, true)
-    signInfo.SignedBlocksCounter++
+  if !previous and val in block.AbsentValidators:
+    MissedBlockBitArray.Set(val.Address, index, true)
+    signInfo.MissedBlocksCounter++
+  else if previous and val not in block.AbsentValidators:
+    MissedBlockBitArray.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
-
+    signInfo.IndexOffset = 0
+    signInfo.MissedBlocksCounter = 0
+    clearMissedBlockBitArray()
     slash & unbond the validator
 
   SigningInfo.Set(val.Address, signInfo)

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,

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.
 
@@ -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
 

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)
 

x/distribution/types/validator_info.go

@@ -19,9 +19,9 @@ func NewValidatorDistInfo(operatorAddr sdk.ValAddress, currentHeight int64) Vali
 	return ValidatorDistInfo{
 		OperatorAddr:            operatorAddr,
 		FeePoolWithdrawalHeight: currentHeight,
-		Pool:           DecCoins{},
-		PoolCommission: DecCoins{},
-		DelAccum:       NewTotalAccum(currentHeight),
+		Pool:                    DecCoins{},
+		PoolCommission:          DecCoins{},
+		DelAccum:                NewTotalAccum(currentHeight),
 	}
 }
 

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

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,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) {
+	// Update the signing info start height or create a new signing info
+	_, found := k.getValidatorSigningInfo(ctx, address)
+	if !found {
+		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

@@ -102,33 +102,34 @@ 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++
 
 	// 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)
-	if previous == signed {
+	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 && !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
-		k.setValidatorSigningBitArray(ctx, consAddr, index, true)
-		signInfo.SignedBlocksCounter++
+	} else if !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 {
+		// Array value has changed from missed to not missed, decrement counter
+		k.setValidatorMissedBlockBitArray(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
@@ -143,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",