From 4b7f6efd873d56ea4c4076abacc8b8347a6be328 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 25 Jul 2018 04:12:48 +0200 Subject: [PATCH] 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 | 1 + examples/democoin/mock/validator.go | 5 ++++ types/stake.go | 5 ++-- x/slashing/keeper.go | 19 +++++++++---- x/slashing/keeper_test.go | 43 +++++++++++++++++++++++++++++ x/stake/keeper/sdk_types.go | 11 ++++++++ 6 files changed, 77 insertions(+), 7 deletions(-) diff --git a/PENDING.md b/PENDING.md index 3977dab58..e1010eb99 100644 --- a/PENDING.md +++ b/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 diff --git a/examples/democoin/mock/validator.go b/examples/democoin/mock/validator.go index c3d01b170..0ef0a2391 100644 --- a/examples/democoin/mock/validator.go +++ b/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() diff --git a/types/stake.go b/types/stake.go index e48577c0b..4e3cf38a3 100644 --- a/types/stake.go +++ b/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) diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 3d721f4a4..cd4e69be1 100644 --- a/x/slashing/keeper.go +++ b/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 diff --git a/x/slashing/keeper_test.go b/x/slashing/keeper_test.go index 71ac3b099..fc60c5029 100644 --- a/x/slashing/keeper_test.go +++ b/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()) + +} diff --git a/x/stake/keeper/sdk_types.go b/x/stake/keeper/sdk_types.go index 280320649..9c3311474 100644 --- a/x/stake/keeper/sdk_types.go +++ b/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)