Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(x/slashing): Avoid overslashing on redelegation + unbonding in specific situations #20688

Merged
merged 4 commits into from
Jun 24, 2024
Merged

fix(x/slashing): Avoid overslashing on redelegation + unbonding in specific situations #20688

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dfc8d55
avoid overslashing
facundomedica Jun 17, 2024
bc303a6
cl++
facundomedica Jun 17, 2024
2216a3e
lint
facundomedica Jun 17, 2024
e503cb5
some fixes
facundomedica Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion simapp/mint_fn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (

type MintBankKeeper interface {
MintCoins(ctx context.Context, moduleName string, coins sdk.Coins) error
SendCoinsFromModuleToModule(ctx context.Context, senderModule string, recipientModule string, amt sdk.Coins) error
SendCoinsFromModuleToModule(ctx context.Context, senderModule, recipientModule string, amt sdk.Coins) error
}

// ProvideExampleMintFn returns the function used in x/mint's endblocker to mint new tokens.
Expand Down
205 changes: 205 additions & 0 deletions tests/integration/slashing/keeper/slash_redelegation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,208 @@ func fundAccount(t *testing.T, ctx context.Context, bankKeeper bankkeeper.Keeper

require.NoError(t, banktestutil.FundAccount(ctx, bankKeeper, addr, amount))
}

func TestOverSlashing(t *testing.T) {
// slash penalty percentage
slashFraction := "0.50"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prefer an odd value so that you which half was actually used. Seeing some rounding can be interesting, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it to 45 👌


// percentage of (undelegation/(undelegation + redelegation))
undelegationPercentageStr := "0.40"

// setting up
var (
stakingKeeper *stakingkeeper.Keeper
bankKeeper bankkeeper.Keeper
slashKeeper slashingkeeper.Keeper
distrKeeper distributionkeeper.Keeper
authKeeper authkeeper.AccountKeeper
)

app, err := simtestutil.Setup(depinject.Configs(
depinject.Supply(log.NewNopLogger()),
slashing.AppConfig,
), &stakingKeeper, &bankKeeper, &slashKeeper, &distrKeeper, &authKeeper)
require.NoError(t, err)

// get sdk context, staking msg server and bond denom
ctx := app.BaseApp.NewContext(false)
stakingMsgServer := stakingkeeper.NewMsgServerImpl(stakingKeeper)
bondDenom, err := stakingKeeper.BondDenom(ctx)
require.NoError(t, err)

// evilVal will be slashed, goodVal won't be slashed
evilValPubKey := secp256k1.GenPrivKey().PubKey()
goodValPubKey := secp256k1.GenPrivKey().PubKey()

/*
all test accs will delegate to evil val, which evil validator will eventually be slashed

- test acc 1: redelegate -> undelegate full amount
- test acc 2: simple undelegation. intended scenario.
- test acc 3: redelegate -> undelegate some amount

*/

testAcc1 := sdk.AccAddress([]byte("addr1new____________"))
testAcc2 := sdk.AccAddress([]byte("addr2new____________"))
testAcc3 := sdk.AccAddress([]byte("addr3new____________"))

fmt.Println("testAcc1 address:", testAcc1.String())
fmt.Println("testAcc2 address:", testAcc2.String())
fmt.Println("testAcc3 address:", testAcc3.String())
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

// fund all accounts
testCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 1)))
facundomedica marked this conversation as resolved.
Show resolved Hide resolved
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoins)
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoins)
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc3, testCoins)
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

balance1Before := bankKeeper.GetBalance(ctx, testAcc1, bondDenom)
balance2Before := bankKeeper.GetBalance(ctx, testAcc2, bondDenom)
balance3Before := bankKeeper.GetBalance(ctx, testAcc3, bondDenom)

fmt.Println("testAcc1 balance amount:", balance1Before.Amount)
fmt.Println("testAcc2 balance amount:", balance2Before.Amount)
fmt.Println("testAcc3 balance amount:", balance3Before.Amount)

// assert acc 1, 2 and 3 balance
require.Equal(t, testCoins[0].Amount.String(), balance1Before.Amount.String())
require.Equal(t, testCoins[0].Amount.String(), balance2Before.Amount.String())
require.Equal(t, testCoins[0].Amount.String(), balance3Before.Amount.String())

fmt.Println("initial balance for accounts: ", testCoins[0])
fmt.Println("slash percentage: ", slashFraction)
slashPercentage := math.LegacyMustNewDecFromStr(slashFraction)

penalty := testCoins[0].Amount.ToLegacyDec().Mul(slashPercentage).TruncateInt()
postSlash := testCoins[0].Amount.Sub(penalty)
fmt.Println("expected balance after slash: ", sdk.NewCoin(bondDenom, postSlash))
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

// create evil val
evilValAddr := sdk.ValAddress(evilValPubKey.Address())
fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(evilValAddr), testCoins)
createValMsg1, _ := stakingtypes.NewMsgCreateValidator(
evilValAddr.String(), evilValPubKey, testCoins[0], stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
_, err = stakingMsgServer.CreateValidator(ctx, createValMsg1)
require.NoError(t, err)

// create good val 1
goodValAddr := sdk.ValAddress(goodValPubKey.Address())
fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(goodValAddr), testCoins)
createValMsg2, _ := stakingtypes.NewMsgCreateValidator(
goodValAddr.String(), goodValPubKey, testCoins[0], stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDecWithPrec(5, 1), math.LegacyNewDec(0)), math.OneInt())
_, err = stakingMsgServer.CreateValidator(ctx, createValMsg2)
require.NoError(t, err)

// next block
ctx = ctx.WithBlockHeight(app.LastBlockHeight() + 1).WithHeaderInfo(header.Info{Height: app.LastBlockHeight() + 1})
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
require.NoError(t, err)

// delegate all accs to evil val
delMsg := stakingtypes.NewMsgDelegate(testAcc1.String(), evilValAddr.String(), testCoins[0])
_, err = stakingMsgServer.Delegate(ctx, delMsg)
require.NoError(t, err)

delMsg = stakingtypes.NewMsgDelegate(testAcc2.String(), evilValAddr.String(), testCoins[0])
_, err = stakingMsgServer.Delegate(ctx, delMsg)
require.NoError(t, err)

delMsg = stakingtypes.NewMsgDelegate(testAcc3.String(), evilValAddr.String(), testCoins[0])
_, err = stakingMsgServer.Delegate(ctx, delMsg)
require.NoError(t, err)

// next block
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
require.NoError(t, err)

// evilValAddr done something bad
misbehaveHeight := ctx.BlockHeader().Height
fmt.Println("evilValAddr misbehaved in height: ", misbehaveHeight)
evilVal, err := stakingKeeper.GetValidator(ctx, evilValAddr)
require.NoError(t, err)

evilValConsAddr, err := evilVal.GetConsAddr()
require.NoError(t, err)

evilPower := stakingKeeper.TokensToConsensusPower(ctx, evilVal.Tokens)

// next block
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
require.NoError(t, err)

// acc 1: redelegate to goodval1 and undelegate FULL amount
redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1.String(), evilValAddr.String(), goodValAddr.String(), testCoins[0])
_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
require.NoError(t, err)
undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoins[0])
_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
require.NoError(t, err)

// acc 2: undelegate full amount
undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoins[0])
_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
require.NoError(t, err)

// acc 3: redelegate to goodval1 and undelegate some amount
redelMsg = stakingtypes.NewMsgBeginRedelegate(testAcc3.String(), evilValAddr.String(), goodValAddr.String(), testCoins[0])
_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
require.NoError(t, err)

undelegationPercentage := math.LegacyMustNewDecFromStr(undelegationPercentageStr)
undelegationAmountDec := math.LegacyNewDecFromInt(testCoins[0].Amount).Mul(undelegationPercentage)
amountToUndelegate := undelegationAmountDec.TruncateInt()

// next block
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
require.NoError(t, err)

portionofTestCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, amountToUndelegate))
fmt.Println("Undelegation amount:", portionofTestCoins[0].Amount)

undelMsg = stakingtypes.NewMsgUndelegate(testAcc3.String(), goodValAddr.String(), portionofTestCoins[0])
_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
require.NoError(t, err)

// next block
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1))
require.NoError(t, err)

// slash the evil val
err = slashKeeper.Slash(ctx, evilValConsAddr, math.LegacyMustNewDecFromStr(slashFraction), evilPower, misbehaveHeight)
facundomedica marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(t, err)

// assert invariants
_, stop := stakingkeeper.AllInvariants(stakingKeeper)(ctx)
require.False(t, stop)
_, stop = bankkeeper.AllInvariants(bankKeeper)(ctx)
require.False(t, stop)
_, stop = distributionkeeper.AllInvariants(distrKeeper)(ctx)
require.False(t, stop)

// fastforward few block (and time) to complete redelegations and unbondings
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1000000000000000000))
require.NoError(t, err)
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1000000000000000000))
require.NoError(t, err)
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1000000000000000000))
require.NoError(t, err)
facundomedica marked this conversation as resolved.
Show resolved Hide resolved

// we check all accounts should be slashed with the equal amount, and they should end up with same balance including staked amount
stakedAcc1, err := stakingKeeper.GetDelegatorBonded(ctx, testAcc1)
require.NoError(t, err)
stakedAcc2, err := stakingKeeper.GetDelegatorBonded(ctx, testAcc2)
require.NoError(t, err)
stakedAcc3, err := stakingKeeper.GetDelegatorBonded(ctx, testAcc3)
require.NoError(t, err)

balance1AfterSlashing := bankKeeper.GetBalance(ctx, testAcc1, bondDenom).Add(sdk.NewCoin(bondDenom, stakedAcc1))
balance2AfterSlashing := bankKeeper.GetBalance(ctx, testAcc2, bondDenom).Add(sdk.NewCoin(bondDenom, stakedAcc2))
balance3AfterSlashing := bankKeeper.GetBalance(ctx, testAcc3, bondDenom).Add(sdk.NewCoin(bondDenom, stakedAcc3))
fmt.Println("testAcc1 balance amount:", balance1AfterSlashing.Amount, "staked amount:", stakedAcc1)
fmt.Println("testAcc2 balance amount:", balance2AfterSlashing.Amount, "staked amount:", stakedAcc2)
fmt.Println("testAcc3 balance amount:", balance3AfterSlashing.Amount, "staked amount:", stakedAcc3)

require.Equal(t, balance1AfterSlashing.Amount, balance2AfterSlashing.Amount)
require.Equal(t, balance2AfterSlashing.Amount, balance3AfterSlashing.Amount)
facundomedica marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test case.
There some more scenarios with multiple re-/undelegations that are not covered, yet. Maybe you can add them here, too.

For bonus points:

  • assert bondend and unbonded amounts separately to see how much of each was slashed
  • cover validator status unbonded
  • run into sharesToUnbond.GT(delegation.Shares)
  • (personal preference) refactor to a table test
// setup account and validators
	specs := map[string]struct {
		slashFrac math.LegacyDec
		del      []math.Int
		redel    []math.Int
		undel    []math.Int
		expBonded  math.Int
		expUnbonded  math.Int
	}{
		"": {},
	}
	for name, spec := range specs {
		t.Run(name, func(t *testing.T) {
// run with cacheContext ...
		})
	}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look but I think I won't be able to make it a table test as afaict we need more than one delegation for this to happen

4 changes: 4 additions & 0 deletions x/staking/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ Ref: https://keepachangelog.com/en/1.0.0/

## [Unreleased]

### Bug Fixes

* [#20688](https://github.com/cosmos/cosmos-sdk/pull/20688) Avoid overslashing unbonding delegations after a redelegation.

### Features

* [#19537](https://github.com/cosmos/cosmos-sdk/pull/19537) Changing `MinCommissionRate` in `MsgUpdateParams` now updates the minimum commission rate for all validators.
Expand Down
16 changes: 14 additions & 2 deletions x/staking/keeper/slash.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,20 @@ func (k Keeper) SlashRedelegation(ctx context.Context, srcValidator types.Valida

// Slash the moved delegation
// Unbond from target validator
sharesToUnbond := slashFactor.Mul(entry.SharesDst)
if sharesToUnbond.IsZero() || slashAmount.IsZero() {
if slashAmount.IsZero() {
continue
}

dstVal, err := k.GetValidator(ctx, valDstAddr)
if err != nil {
return math.ZeroInt(), err
}
sharesToUnbond, err := dstVal.SharesFromTokensTruncated(slashAmount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood this correct, then you calculate the shares for the remaining slash amount. The slash amount may be (partially) filled from slashing unbonding delegations first. This looks correct to me and fixes the issue. Good work 👍

if err != nil {
return math.ZeroInt(), err
}

if sharesToUnbond.IsZero() {
continue
}

Expand Down
Loading