From fadec4d5db28b1cce3a00a6a20e96b09b90054a4 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 | 49 +++++++++++++++++++++++++++++ examples/democoin/mock/validator.go | 5 +++ types/stake.go | 5 +-- x/slashing/keeper.go | 19 ++++++++--- x/slashing/keeper_test.go | 44 ++++++++++++++++++++++++++ x/stake/keeper/sdk_types.go | 11 +++++++ 6 files changed, 126 insertions(+), 7 deletions(-) create mode 100644 PENDING.md diff --git a/PENDING.md b/PENDING.md new file mode 100644 index 000000000..e1010eb99 --- /dev/null +++ b/PENDING.md @@ -0,0 +1,49 @@ +## PENDING + +BREAKING CHANGES +* [baseapp] Msgs are no longer run on CheckTx, removed `ctx.IsCheckTx()` +* [x/gov] CLI flag changed from `proposalID` to `proposal-id` +* [x/stake] Fixed the period check for the inflation calculation +* [x/stake] Inflation doesn't use rationals in calculation (performance boost) +* [baseapp] NewBaseApp constructor now takes sdk.TxDecoder as argument instead of wire.Codec +* [x/auth] Default TxDecoder can be found in `x/auth` rather than baseapp +* \#1606 The following CLI commands have been switched to use `--from` + * `gaiacli stake create-validator --address-validator` + * `gaiacli stake edit-validator --address-validator` + * `gaiacli stake delegate --address-delegator` + * `gaiacli stake unbond begin --address-delegator` + * `gaiacli stake unbond complete --address-delegator` + * `gaiacli stake redelegate begin --address-delegator` + * `gaiacli stake redelegate complete --address-delegator` + * `gaiacli stake unrevoke [validator-address]` + * `gaiacli gov submit-proposal --proposer` + * `gaiacli gov deposit --depositer` + * `gaiacli gov vote --voter` +* [x/gov] Added tags sub-package, changed tags to use dash-case + +FEATURES +* [lcd] Can now query governance proposals by ProposalStatus +* [x/mock/simulation] Randomized simulation framework + * Modules specify invariants and operations, preferably in an x/[module]/simulation package + * Modules can test random combinations of their own operations + * Applications can integrate operations and invariants from modules together for an integrated simulation +* [baseapp] Initialize validator set on ResponseInitChain +* [cosmos-sdk-cli] Added support for cosmos-sdk-cli tool under cosmos-sdk/cmd + * This allows SDK users to initialize a new project repository. +* [tests] Remotenet commands for AWS (awsnet) + +IMPROVEMENTS +* [baseapp] Allow any alphanumeric character in route +* [cli] Improve error messages for all txs when the account doesn't exist +* [tools] Remove `rm -rf vendor/` from `make get_vendor_deps` +* [x/auth] Recover ErrorOutOfGas panic in order to set sdk.Result attributes correctly +* [x/stake] Add revoked to human-readable validator +* [tests] Add tests to example apps in docs +* [x/gov] Votes on a proposal can now be queried +* [x/bank] Unit tests are now table-driven +* [tests] Fixes ansible scripts to work with AWS too + +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 84d41d488..208636de9 100644 --- a/examples/democoin/mock/validator.go +++ b/examples/democoin/mock/validator.go @@ -82,6 +82,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 eb3f66082..c5e03e0d7 100644 --- a/types/stake.go +++ b/types/stake.go @@ -65,8 +65,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 9f1ff205b..648f9eaf9 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -61,6 +61,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() @@ -97,11 +98,19 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, pubkey crypto.PubKey, } minHeight := signInfo.StartHeight + SignedBlocksWindow if height > minHeight && signInfo.SignedBlocksCounter < MinSignedPerWindow { - // 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, MinSignedPerWindow)) - k.validatorSet.Slash(ctx, pubkey, height, power, SlashFractionDowntime) - k.validatorSet.Revoke(ctx, pubkey) - signInfo.JailedUntil = ctx.BlockHeader().Time + DowntimeUnbondDuration + 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, MinSignedPerWindow)) + k.validatorSet.Slash(ctx, pubkey, height, power, SlashFractionDowntime) + k.validatorSet.Revoke(ctx, pubkey) + signInfo.JailedUntil = ctx.BlockHeader().Time + DowntimeUnbondDuration + } 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 6d1e52e28..d85db77c0 100644 --- a/x/slashing/keeper_test.go +++ b/x/slashing/keeper_test.go @@ -197,3 +197,47 @@ 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 < SignedBlocksWindow; height++ { + ctx = ctx.WithBlockHeight(height) + keeper.handleValidatorSignature(ctx, val, amtInt, true) + } + + // 501 blocks missed + for ; height < SignedBlocksWindow+(SignedBlocksWindow-MinSignedPerWindow)+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 + slashAmt := sdk.NewRat(amtInt).Mul(SlashFractionDowntime).RoundInt64() + require.Equal(t, int64(amtInt)-slashAmt, validator.Tokens.RoundInt64()) // TODO replace w/ .GetTokens() + + // 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)-slashAmt, validator.Tokens.RoundInt64()) // TODO replace w/ .GetTokens() + +} 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)