Merge PR #1805: Downtime slashing off-by-one-block fix

* Avoid slashing & revoking no longer stored or already revoked validators for downtime
* Add testcase
* Update PENDING.md

PENDING.md

@@ -45,4 +45,5 @@ IMPROVEMENTS
 
 BUG FIXES
 *  \#1666 Add intra-tx counter to the genesis validators
+*  \#1797 Fix off-by-one error in slashing for downtime
 *  \#1787 Fixed bug where Tally fails due to revoked/unbonding validator

examples/democoin/mock/validator.go

@@ -87,6 +87,11 @@ func (vs *ValidatorSet) Validator(ctx sdk.Context, addr sdk.AccAddress) sdk.Vali
 	return nil
 }
 
+// ValidatorByPubKey implements sdk.ValidatorSet
+func (vs *ValidatorSet) ValidatorByPubKey(ctx sdk.Context, pubkey crypto.PubKey) sdk.Validator {
+	panic("not implemented")
+}
+
 // TotalPower implements sdk.ValidatorSet
 func (vs *ValidatorSet) TotalPower(ctx sdk.Context) sdk.Rat {
 	res := sdk.ZeroRat()

types/stake.go

@@ -66,8 +66,9 @@ type ValidatorSet interface {
 	IterateValidatorsBonded(Context,
 		func(index int64, validator Validator) (stop bool))
 
-	Validator(Context, AccAddress) Validator // get a particular validator by owner AccAddress
-	TotalPower(Context) Rat                  // total power of the validator set
+	Validator(Context, AccAddress) Validator            // get a particular validator by owner AccAddress
+	ValidatorByPubKey(Context, crypto.PubKey) Validator // get a particular validator by signing PubKey
+	TotalPower(Context) Rat                             // total power of the validator set
 
 	// slash the validator and delegators of the validator, specifying offence height, offence power, and slash fraction
 	Slash(Context, crypto.PubKey, int64, int64, Rat)

x/slashing/keeper.go

@@ -65,6 +65,7 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, pubkey crypto.PubKey, infracti
 }
 
 // handle a validator signature, must be called once per validator per block
+// nolint gocyclo
 func (k Keeper) handleValidatorSignature(ctx sdk.Context, pubkey crypto.PubKey, power int64, signed bool) {
 	logger := ctx.Logger().With("module", "x/slashing")
 	height := ctx.BlockHeight()
@@ -101,11 +102,19 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, pubkey crypto.PubKey,
 	}
 	minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx)
 	if height > minHeight && signInfo.SignedBlocksCounter < k.MinSignedPerWindow(ctx) {
-		// Downtime confirmed, slash, revoke, and jail the validator
-		logger.Info(fmt.Sprintf("Validator %s past min height of %d and below signed blocks threshold of %d", pubkey.Address(), minHeight, k.MinSignedPerWindow(ctx)))
-		k.validatorSet.Slash(ctx, pubkey, height, power, k.SlashFractionDowntime(ctx))
-		k.validatorSet.Revoke(ctx, pubkey)
-		signInfo.JailedUntil = ctx.BlockHeader().Time + k.DowntimeUnbondDuration(ctx)
+		validator := k.validatorSet.ValidatorByPubKey(ctx, pubkey)
+		if validator != nil && !validator.GetRevoked() {
+			// Downtime confirmed, slash, revoke, and jail the validator
+			logger.Info(fmt.Sprintf("Validator %s past min height of %d and below signed blocks threshold of %d",
+				pubkey.Address(), minHeight, k.MinSignedPerWindow(ctx)))
+			k.validatorSet.Slash(ctx, pubkey, height, power, k.SlashFractionDowntime(ctx))
+			k.validatorSet.Revoke(ctx, pubkey)
+			signInfo.JailedUntil = ctx.BlockHeader().Time + k.DowntimeUnbondDuration(ctx)
+		} else {
+			// Validator was (a) not found or (b) already revoked, don't slash
+			logger.Info(fmt.Sprintf("Validator %s would have been slashed for downtime, but was either not found in store or already revoked",
+				pubkey.Address()))
+		}
 	}
 
 	// Set the updated signing info

x/slashing/keeper_test.go

@@ -195,3 +195,46 @@ func TestHandleNewValidator(t *testing.T) {
 	pool := sk.GetPool(ctx)
 	require.Equal(t, int64(100), pool.BondedTokens.RoundInt64())
 }
+
+// Test a revoked validator being "down" twice
+// Ensure that they're only slashed once
+func TestHandleAlreadyRevoked(t *testing.T) {
+
+	// initial setup
+	ctx, _, sk, _, keeper := createTestInput(t)
+	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())
+	stake.EndBlocker(ctx, sk)
+
+	// 1000 first blocks OK
+	height := int64(0)
+	for ; height < keeper.SignedBlocksWindow(ctx); height++ {
+		ctx = ctx.WithBlockHeight(height)
+		keeper.handleValidatorSignature(ctx, val, amtInt, true)
+	}
+
+	// 501 blocks missed
+	for ; height < keeper.SignedBlocksWindow(ctx)+(keeper.SignedBlocksWindow(ctx)-keeper.MinSignedPerWindow(ctx))+1; height++ {
+		ctx = ctx.WithBlockHeight(height)
+		keeper.handleValidatorSignature(ctx, val, amtInt, false)
+	}
+
+	// validator should have been revoked and slashed
+	validator, _ := sk.GetValidatorByPubKey(ctx, val)
+	require.Equal(t, sdk.Unbonded, validator.GetStatus())
+
+	// validator should have been slashed
+	require.Equal(t, int64(amtInt-1), validator.GetTokens().RoundInt64())
+
+	// another block missed
+	ctx = ctx.WithBlockHeight(height)
+	keeper.handleValidatorSignature(ctx, val, amtInt, false)
+
+	// validator should not have been slashed twice
+	validator, _ = sk.GetValidatorByPubKey(ctx, val)
+	require.Equal(t, int64(amtInt-1), validator.GetTokens().RoundInt64())
+
+}

x/stake/keeper/sdk_types.go

@@ -3,6 +3,8 @@ package keeper
 import (
 	"fmt"
 
+	"github.com/tendermint/tendermint/crypto"
+
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/stake/types"
 )
@@ -57,6 +59,15 @@ func (k Keeper) Validator(ctx sdk.Context, address sdk.AccAddress) sdk.Validator
 	return val
 }
 
+// get the sdk.validator for a particular pubkey
+func (k Keeper) ValidatorByPubKey(ctx sdk.Context, pubkey crypto.PubKey) sdk.Validator {
+	val, found := k.GetValidatorByPubKey(ctx, pubkey)
+	if !found {
+		return nil
+	}
+	return val
+}
+
 // total power from the bond
 func (k Keeper) TotalPower(ctx sdk.Context) sdk.Rat {
 	pool := k.GetPool(ctx)