From 7092914018d65d8392a6906399e0ba734c9b15c8 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 13 Oct 2023 17:16:22 +0200 Subject: [PATCH] add tests --- testutil/keeper/expectations.go | 63 +++++ x/ccv/provider/keeper/double_vote.go | 5 +- x/ccv/provider/keeper/misbehaviour.go | 7 +- x/ccv/provider/keeper/punish_validator.go | 67 +++-- .../provider/keeper/punish_validator_test.go | 233 +++++++++--------- 5 files changed, 212 insertions(+), 163 deletions(-) diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index 7814fe0fcf..4702e688e2 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -3,6 +3,7 @@ package keeper import ( time "time" + math "cosmossdk.io/math" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" conntypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" @@ -156,3 +157,65 @@ func GetMocksForSendIBCPacket(ctx sdk.Context, mocks MockedKeepers, channelID st ).Return(uint64(888), nil).Times(times), } } + +func GetMocksForSlashValidator( + ctx sdk.Context, + mocks MockedKeepers, + validator stakingtypes.Validator, + consAddr sdk.ConsAddress, + undelegations []stakingtypes.UnbondingDelegation, + redelegations []stakingtypes.Redelegation, + powerReduction math.Int, + slashFraction math.LegacyDec, + currentPower, + expectedInfractionHeight, + expectedSlashPower int64, +) []*gomock.Call { + return []*gomock.Call{ + mocks.MockStakingKeeper.EXPECT(). + GetUnbondingDelegationsFromValidator(ctx, validator.GetOperator()). + Return(undelegations), + mocks.MockStakingKeeper.EXPECT(). + GetRedelegationsFromSrcValidator(ctx, validator.GetOperator()). + Return(redelegations), + mocks.MockStakingKeeper.EXPECT(). + GetLastValidatorPower(ctx, validator.GetOperator()). + Return(currentPower), + mocks.MockStakingKeeper.EXPECT(). + PowerReduction(ctx). + Return(powerReduction), + mocks.MockStakingKeeper.EXPECT(). + SlashUnbondingDelegation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn( + func(_ sdk.Context, undelegation stakingtypes.UnbondingDelegation, _ int64, _ sdk.Dec) math.Int { + sum := sdk.NewInt(0) + for _, r := range undelegation.Entries { + if r.IsMature(ctx.BlockTime()) { + continue + } + sum = sum.Add(sdk.NewInt(r.InitialBalance.Int64())) + } + return sum + }).AnyTimes(), + mocks.MockStakingKeeper.EXPECT(). + SlashRedelegation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn( + func(_ sdk.Context, _ stakingtypes.Validator, redelegation stakingtypes.Redelegation, _ int64, _ sdk.Dec) math.Int { + sum := sdk.NewInt(0) + for _, r := range redelegation.Entries { + if r.IsMature(ctx.BlockTime()) { + continue + } + sum = sum.Add(sdk.NewInt(r.InitialBalance.Int64())) + } + return sum + }).AnyTimes(), + mocks.MockSlashingKeeper.EXPECT(). + SlashFractionDoubleSign(ctx). + Return(slashFraction), + mocks.MockStakingKeeper.EXPECT(). + SlashWithInfractionReason(ctx, consAddr, expectedInfractionHeight, expectedSlashPower, slashFraction, stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN). + Times(1), + } + +} diff --git a/x/ccv/provider/keeper/double_vote.go b/x/ccv/provider/keeper/double_vote.go index 2b550c39aa..9a160d412a 100644 --- a/x/ccv/provider/keeper/double_vote.go +++ b/x/ccv/provider/keeper/double_vote.go @@ -35,10 +35,7 @@ func (k Keeper) HandleConsumerDoubleVoting( types.NewConsumerConsAddress(sdk.ConsAddress(evidence.VoteA.ValidatorAddress.Bytes())), ) - if err := k.SlashValidator(ctx, providerAddr); err != nil { - return err - } - if err := k.JailAndTombstoneValidator(ctx, providerAddr); err != nil { + if err := k.PunishValidator(ctx, providerAddr); err != nil { return err } diff --git a/x/ccv/provider/keeper/misbehaviour.go b/x/ccv/provider/keeper/misbehaviour.go index 2f34e5d1c5..c9a8a0849e 100644 --- a/x/ccv/provider/keeper/misbehaviour.go +++ b/x/ccv/provider/keeper/misbehaviour.go @@ -47,14 +47,11 @@ func (k Keeper) HandleConsumerMisbehaviour(ctx sdk.Context, misbehaviour ibctmty misbehaviour.Header1.Header.ChainID, types.NewConsumerConsAddress(sdk.ConsAddress(v.Address.Bytes())), ) - err := k.SlashValidator(ctx, providerAddr) - if err != nil { - errors = append(errors, err) - } - err = k.JailAndTombstoneValidator(ctx, providerAddr) + err := k.PunishValidator(ctx, providerAddr) if err != nil { errors = append(errors, err) } + provAddrs = append(provAddrs, providerAddr) } diff --git a/x/ccv/provider/keeper/punish_validator.go b/x/ccv/provider/keeper/punish_validator.go index 06eac14a1f..0f8b9fd8e6 100644 --- a/x/ccv/provider/keeper/punish_validator.go +++ b/x/ccv/provider/keeper/punish_validator.go @@ -14,8 +14,7 @@ import ( "github.com/cosmos/interchain-security/v3/x/ccv/provider/types" ) -// JailAndTombstoneValidator jails and tombstones the validator with the given provider consensus address -func (k Keeper) JailAndTombstoneValidator(ctx sdk.Context, providerAddr types.ProviderConsAddress) error { +func (k Keeper) PunishValidator(ctx sdk.Context, providerAddr types.ProviderConsAddress) error { validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerAddr.ToSdkConsAddr()) if !found { return errorsmod.Wrapf(slashingtypes.ErrNoValidatorForAddress, "provider consensus address: %s", providerAddr.String()) @@ -29,20 +28,48 @@ func (k Keeper) JailAndTombstoneValidator(ctx sdk.Context, providerAddr types.Pr return fmt.Errorf("validator is tombstoned. provider consensus address: %s", providerAddr.String()) } + k.SlashValidator(ctx, validator) + k.JailAndTombstoneValidator(ctx, validator) + + return nil +} + +// JailAndTombstoneValidator jails and tombstones the validator with the given provider consensus address +func (k Keeper) JailAndTombstoneValidator(ctx sdk.Context, validator stakingtypes.Validator) { + consAdrr, err := validator.GetConsAddr() + if err != nil { + panic(err) + } + // jail validator if not already if !validator.IsJailed() { - k.stakingKeeper.Jail(ctx, providerAddr.ToSdkConsAddr()) + k.stakingKeeper.Jail(ctx, consAdrr) } - k.slashingKeeper.JailUntil(ctx, providerAddr.ToSdkConsAddr(), evidencetypes.DoubleSignJailEndTime) + k.slashingKeeper.JailUntil(ctx, consAdrr, evidencetypes.DoubleSignJailEndTime) // Tombstone the validator so that we cannot slash the validator more than once // Note that we cannot simply use the fact that a validator is jailed to avoid slashing more than once // because then a validator could i) perform an equivocation, ii) get jailed (e.g., through downtime) // and in such a case the validator would not get slashed when we call `SlashValidator`. - k.slashingKeeper.Tombstone(ctx, providerAddr.ToSdkConsAddr()) + k.slashingKeeper.Tombstone(ctx, consAdrr) +} - return nil +// SlashValidator slashes the given validator +func (k Keeper) SlashValidator(ctx sdk.Context, validator stakingtypes.Validator) { + undelegations := k.stakingKeeper.GetUnbondingDelegationsFromValidator(ctx, validator.GetOperator()) + redelegations := k.stakingKeeper.GetRedelegationsFromSrcValidator(ctx, validator.GetOperator()) + lastPower := k.stakingKeeper.GetLastValidatorPower(ctx, validator.GetOperator()) + powerReduction := k.stakingKeeper.PowerReduction(ctx) + totalPower := k.ComputePowerToSlash(ctx, validator, undelegations, redelegations, lastPower, powerReduction) + slashFraction := k.slashingKeeper.SlashFractionDoubleSign(ctx) + + consAdrr, err := validator.GetConsAddr() + if err != nil { + panic(err) + } + + k.stakingKeeper.SlashWithInfractionReason(ctx, consAdrr, 0, totalPower, slashFraction, stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN) } // ComputePowerToSlash computes the power to be slashed based on the tokens in non-matured `undelegations` and @@ -75,31 +102,3 @@ func (k Keeper) ComputePowerToSlash(ctx sdk.Context, validator stakingtypes.Vali return power + undelegationsAndRedelegationsInPower } - -// SlashValidator slashes validator with `providerAddr` -func (k Keeper) SlashValidator(ctx sdk.Context, providerAddr types.ProviderConsAddress) error { - validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerAddr.ToSdkConsAddr()) - if !found { - return errorsmod.Wrapf(slashingtypes.ErrNoValidatorForAddress, "provider consensus address: %s", providerAddr.String()) - } - - // check if the validator is unbonded to prevent panicking when slashing (see cosmos/cosmos-sdk/blob/v0.47.5/x/staking/keeper/slash.go#L61) - if validator.IsUnbonded() { - return fmt.Errorf("validator is unbonded. provider consensus address: %s", providerAddr.String()) - } - - // check if the validator is already tombstoned to avoid slashing a validator more than once - if k.slashingKeeper.IsTombstoned(ctx, providerAddr.ToSdkConsAddr()) { - return fmt.Errorf("validator is tombstoned. provider consensus address: %s", providerAddr.String()) - } - - undelegations := k.stakingKeeper.GetUnbondingDelegationsFromValidator(ctx, validator.GetOperator()) - redelegations := k.stakingKeeper.GetRedelegationsFromSrcValidator(ctx, validator.GetOperator()) - lastPower := k.stakingKeeper.GetLastValidatorPower(ctx, validator.GetOperator()) - powerReduction := k.stakingKeeper.PowerReduction(ctx) - totalPower := k.ComputePowerToSlash(ctx, validator, undelegations, redelegations, lastPower, powerReduction) - slashFraction := k.slashingKeeper.SlashFractionDoubleSign(ctx) - - k.stakingKeeper.SlashWithInfractionReason(ctx, providerAddr.ToSdkConsAddr(), 0, totalPower, slashFraction, stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN) - return nil -} diff --git a/x/ccv/provider/keeper/punish_validator_test.go b/x/ccv/provider/keeper/punish_validator_test.go index cce8749710..5c13e3a274 100644 --- a/x/ccv/provider/keeper/punish_validator_test.go +++ b/x/ccv/provider/keeper/punish_validator_test.go @@ -10,7 +10,6 @@ import ( "cosmossdk.io/math" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" sdk "github.com/cosmos/cosmos-sdk/types" evidencetypes "github.com/cosmos/cosmos-sdk/x/evidence/types" @@ -18,19 +17,36 @@ import ( tmtypes "github.com/cometbft/cometbft/types" - cryptotestutil "github.com/cosmos/interchain-security/v3/testutil/crypto" testkeeper "github.com/cosmos/interchain-security/v3/testutil/keeper" "github.com/cosmos/interchain-security/v3/x/ccv/provider/types" ) // TestJailAndTombstoneValidator tests that the jailing of a validator is only executed // under the conditions that the validator is neither unbonded, nor jailed, nor tombstoned. -func TestJailAndTombstoneValidator(t *testing.T) { - providerConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(7842334).ProviderConsAddress() +func TestPunishValidator(t *testing.T) { + pubKey, _ := cryptocodec.FromTmPubKeyInterface(tmtypes.NewMockPV().PrivKey.PubKey()) + + // manually build a validator instead of using `stakingtypes.NewValidator` + // to guarantee that the validator is bonded is jailed for testing + + validator, err := stakingtypes.NewValidator( + sdk.ValAddress(pubKey.Address().Bytes()), + pubKey, + stakingtypes.NewDescription("", "", "", "", ""), + ) + require.NoError(t, err) + + slashFraction, _ := sdk.NewDecFromStr("0.5") + consAddr, _ := validator.GetConsAddr() + providerConsAddr := types.NewProviderConsAddress(consAddr) + + fmt.Println(validator.GetOperator()) + testCases := []struct { name string provAddr types.ProviderConsAddress expectedCalls func(sdk.Context, testkeeper.MockedKeepers, types.ProviderConsAddress) []*gomock.Call + expErr bool }{ { "unfound validator", @@ -47,6 +63,7 @@ func TestJailAndTombstoneValidator(t *testing.T) { ).Times(1), } }, + true, }, { "unbonded validator", @@ -62,6 +79,7 @@ func TestJailAndTombstoneValidator(t *testing.T) { ).Times(1), } }, + true, }, { "tombstoned validator", @@ -80,6 +98,7 @@ func TestJailAndTombstoneValidator(t *testing.T) { ).Times(1), } }, + true, }, { "jailed validator", @@ -87,23 +106,46 @@ func TestJailAndTombstoneValidator(t *testing.T) { func(ctx sdk.Context, mocks testkeeper.MockedKeepers, provAddr types.ProviderConsAddress, ) []*gomock.Call { - return []*gomock.Call{ + validator.Jailed = true + validator.Status = stakingtypes.Unbonding + gc := append([]*gomock.Call{ mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr( ctx, providerConsAddr.ToSdkConsAddr()).Return( - stakingtypes.Validator{Jailed: true}, true, + validator, true, ).Times(1), mocks.MockSlashingKeeper.EXPECT().IsTombstoned( ctx, providerConsAddr.ToSdkConsAddr()).Return( false, ).Times(1), + }, + testkeeper.GetMocksForSlashValidator( + ctx, + mocks, + validator, + consAddr, + []stakingtypes.UnbondingDelegation{}, + []stakingtypes.Redelegation{}, + sdk.NewInt(2), + slashFraction, + int64(1), + int64(0), + int64(1), + )..., + ) + + gc = append( + gc, mocks.MockSlashingKeeper.EXPECT().JailUntil( ctx, providerConsAddr.ToSdkConsAddr(), evidencetypes.DoubleSignJailEndTime). Times(1), mocks.MockSlashingKeeper.EXPECT().Tombstone( ctx, providerConsAddr.ToSdkConsAddr()). Times(1), - } + ) + return gc + }, + false, }, { "bonded validator", @@ -111,15 +153,35 @@ func TestJailAndTombstoneValidator(t *testing.T) { func(ctx sdk.Context, mocks testkeeper.MockedKeepers, provAddr types.ProviderConsAddress, ) []*gomock.Call { - return []*gomock.Call{ + validator.Jailed = false + validator.Status = stakingtypes.Bonded + gc := append([]*gomock.Call{ mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr( ctx, providerConsAddr.ToSdkConsAddr()).Return( - stakingtypes.Validator{Status: stakingtypes.Bonded}, true, + validator, true, ).Times(1), mocks.MockSlashingKeeper.EXPECT().IsTombstoned( ctx, providerConsAddr.ToSdkConsAddr()).Return( false, ).Times(1), + }, + testkeeper.GetMocksForSlashValidator( + ctx, + mocks, + validator, + consAddr, + []stakingtypes.UnbondingDelegation{}, + []stakingtypes.Redelegation{}, + sdk.NewInt(2), + slashFraction, + int64(1), + int64(0), + int64(1), + )..., + ) + + gc = append( + gc, mocks.MockStakingKeeper.EXPECT().Jail( ctx, providerConsAddr.ToSdkConsAddr()). Times(1), @@ -129,22 +191,29 @@ func TestJailAndTombstoneValidator(t *testing.T) { mocks.MockSlashingKeeper.EXPECT().Tombstone( ctx, providerConsAddr.ToSdkConsAddr()). Times(1), - } + ) + return gc }, + false, }, } for _, tc := range testCases { - providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx( - t, testkeeper.NewInMemKeeperParams(t)) - - // Setup expected mock calls - gomock.InOrder(tc.expectedCalls(ctx, mocks, tc.provAddr)...) - - // Execute method and assert expected mock calls - providerKeeper.JailAndTombstoneValidator(ctx, tc.provAddr) - - ctrl.Finish() + t.Run(tc.name, func(t *testing.T) { + providerKeeper, ctx, _, mocks := testkeeper.GetProviderKeeperAndCtx( + t, testkeeper.NewInMemKeeperParams(t)) + + // Setup expected mock calls + gomock.InOrder(tc.expectedCalls(ctx, mocks, tc.provAddr)...) + + // Execute method and assert expected mock calls + err := providerKeeper.PunishValidator(ctx, tc.provAddr) + if tc.expErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) } } @@ -357,26 +426,16 @@ func TestSlashValidator(t *testing.T) { testkeeper.NewInMemProviderKeeper(keeperParams, mocks) pubKey, _ := cryptocodec.FromTmPubKeyInterface(tmtypes.NewMockPV().PrivKey.PubKey()) - pkAny, _ := codectypes.NewAnyWithValue(pubKey) - - // manually build a validator instead of using `stakingtypes.NewValidator` to guarantee that the validator is bonded - validator := stakingtypes.Validator{ - OperatorAddress: sdk.ValAddress(pubKey.Address().Bytes()).String(), - ConsensusPubkey: pkAny, - Jailed: false, - Status: stakingtypes.Bonded, - Tokens: sdk.ZeroInt(), - DelegatorShares: sdk.ZeroDec(), - Description: stakingtypes.Description{}, - UnbondingHeight: int64(0), - UnbondingTime: time.Unix(0, 0).UTC(), - Commission: stakingtypes.NewCommission(sdk.ZeroDec(), sdk.ZeroDec(), sdk.ZeroDec()), - MinSelfDelegation: math.OneInt(), - UnbondingOnHoldRefCount: 0, - } + + validator, err := stakingtypes.NewValidator( + sdk.ValAddress(pubKey.Address().Bytes()), + pubKey, + stakingtypes.NewDescription("", "", "", "", ""), + ) + require.NoError(t, err) + validator.Status = stakingtypes.Bonded consAddr, _ := validator.GetConsAddr() - providerAddr := types.NewProviderConsAddress(consAddr) // we create 1000 tokens worth of undelegations, 750 of them are non-matured // we also create 1000 tokens worth of redelegations, 750 of them are non-matured @@ -400,86 +459,20 @@ func TestSlashValidator(t *testing.T) { expectedInfractionHeight := int64(0) expectedSlashPower := int64(3750) - expectedCalls := []*gomock.Call{ - mocks.MockStakingKeeper.EXPECT(). - GetValidatorByConsAddr(ctx, gomock.Any()). - Return(validator, true), - mocks.MockSlashingKeeper.EXPECT(). - IsTombstoned(ctx, consAddr). - Return(false), - mocks.MockStakingKeeper.EXPECT(). - GetUnbondingDelegationsFromValidator(ctx, validator.GetOperator()). - Return(undelegations), - mocks.MockStakingKeeper.EXPECT(). - GetRedelegationsFromSrcValidator(ctx, validator.GetOperator()). - Return(redelegations), - mocks.MockStakingKeeper.EXPECT(). - GetLastValidatorPower(ctx, validator.GetOperator()). - Return(currentPower), - mocks.MockStakingKeeper.EXPECT(). - PowerReduction(ctx). - Return(powerReduction), - mocks.MockStakingKeeper.EXPECT(). - SlashUnbondingDelegation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - DoAndReturn( - func(_ sdk.Context, undelegation stakingtypes.UnbondingDelegation, _ int64, _ sdk.Dec) math.Int { - sum := sdk.NewInt(0) - for _, r := range undelegation.Entries { - if r.IsMature(ctx.BlockTime()) { - continue - } - sum = sum.Add(sdk.NewInt(r.InitialBalance.Int64())) - } - return sum - }).AnyTimes(), - mocks.MockStakingKeeper.EXPECT(). - SlashRedelegation(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - DoAndReturn( - func(_ sdk.Context, _ stakingtypes.Validator, redelegation stakingtypes.Redelegation, _ int64, _ sdk.Dec) math.Int { - sum := sdk.NewInt(0) - for _, r := range redelegation.Entries { - if r.IsMature(ctx.BlockTime()) { - continue - } - sum = sum.Add(sdk.NewInt(r.InitialBalance.Int64())) - } - return sum - }).AnyTimes(), - mocks.MockSlashingKeeper.EXPECT(). - SlashFractionDoubleSign(ctx). - Return(slashFraction), - mocks.MockStakingKeeper.EXPECT(). - SlashWithInfractionReason(ctx, consAddr, expectedInfractionHeight, expectedSlashPower, slashFraction, stakingtypes.Infraction_INFRACTION_DOUBLE_SIGN). - Times(1), - } - - gomock.InOrder(expectedCalls...) - keeper.SlashValidator(ctx, providerAddr) -} - -// TestSlashValidatorDoesNotSlashIfValidatorIsUnbonded asserts that `SlashValidator` does not call -// the staking module's `Slash` method if the validator to be slashed is unbonded -func TestSlashValidatorDoesNotSlashIfValidatorIsUnbonded(t *testing.T) { - keeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) - defer ctrl.Finish() - - keeperParams := testkeeper.NewInMemKeeperParams(t) - testkeeper.NewInMemProviderKeeper(keeperParams, mocks) - - pubKey, _ := cryptocodec.FromTmPubKeyInterface(tmtypes.NewMockPV().PrivKey.PubKey()) - - // validator is initially unbonded - validator, _ := stakingtypes.NewValidator(pubKey.Address().Bytes(), pubKey, stakingtypes.Description{}) - - consAddr, _ := validator.GetConsAddr() - providerAddr := types.NewProviderConsAddress(consAddr) - - expectedCalls := []*gomock.Call{ - mocks.MockStakingKeeper.EXPECT(). - GetValidatorByConsAddr(ctx, gomock.Any()). - Return(validator, true), - } - - gomock.InOrder(expectedCalls...) - keeper.SlashValidator(ctx, providerAddr) + gomock.InOrder( + testkeeper.GetMocksForSlashValidator( + ctx, + mocks, + validator, + consAddr, + undelegations, + redelegations, + powerReduction, + slashFraction, + currentPower, + expectedInfractionHeight, + expectedSlashPower, + )..., + ) + keeper.SlashValidator(ctx, validator) }