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 all 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
214 changes: 197 additions & 17 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 @@ -2,7 +2,6 @@ package keeper_test

import (
"context"
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -66,30 +65,30 @@ func TestSlashRedelegation(t *testing.T) {
testAcc2 := sdk.AccAddress([]byte("addr2_______________"))

// fund acc 1 and acc 2
testCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 10)))
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoins)
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoins)
testCoin := sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 10))
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoin)
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoin)
Comment on lines +68 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure consistent funding logic across test cases.

The function fundAccount is used multiple times to set up initial conditions for the test accounts. Consider refactoring to a helper function that initializes multiple accounts to reduce redundancy and improve readability.


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

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

// creating evil val
evilValAddr := sdk.ValAddress(evilValPubKey.Address())
fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(evilValAddr), testCoins)
fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(evilValAddr), testCoin)
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())
evilValAddr.String(), evilValPubKey, testCoin, 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)

// creating good val
goodValAddr := sdk.ValAddress(goodValPubKey.Address())
fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(goodValAddr), testCoins)
fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(goodValAddr), testCoin)
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())
goodValAddr.String(), goodValPubKey, testCoin, 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)

Expand All @@ -100,12 +99,12 @@ func TestSlashRedelegation(t *testing.T) {
require.NoError(t, err)

// Acc 2 delegate
delMsg := stakingtypes.NewMsgDelegate(testAcc2.String(), evilValAddr.String(), testCoins[0])
delMsg := stakingtypes.NewMsgDelegate(testAcc2.String(), evilValAddr.String(), testCoin)
_, err = stakingMsgServer.Delegate(ctx, delMsg)
require.NoError(t, err)

// Acc 1 delegate
delMsg = stakingtypes.NewMsgDelegate(testAcc1.String(), evilValAddr.String(), testCoins[0])
delMsg = stakingtypes.NewMsgDelegate(testAcc1.String(), evilValAddr.String(), testCoin)
_, err = stakingMsgServer.Delegate(ctx, delMsg)
require.NoError(t, err)

Expand All @@ -119,20 +118,19 @@ func TestSlashRedelegation(t *testing.T) {
require.NoError(t, err)

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

// Acc 1 redelegate from evil val to good val
redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1.String(), evilValAddr.String(), goodValAddr.String(), testCoins[0])
redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1.String(), evilValAddr.String(), goodValAddr.String(), testCoin)
_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
require.NoError(t, err)

// Acc 1 undelegate from good val
undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoins[0])
undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoin)
_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
require.NoError(t, err)

// Acc 2 undelegate from evil val
undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoins[0])
undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoin)
Comment on lines +123 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Consider adding more detailed assertions post-redelegation and undelegation.

The current test file tests/integration/slashing/keeper/slash_redelegation_test.go contains assertions for errors but lacks specific checks on the state of delegations after redelegation and undelegation operations.

  • Add assertions to verify the state of delegations to ensure they reflect the expected changes accurately.
Analysis chain

Consider adding more detailed assertions post-redelegation and undelegation.

After performing redelegation and undelegation operations, it would be beneficial to add assertions checking the state of the delegations to ensure they reflect the expected changes accurately.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the state of delegations after redelegation and undelegation operations.
rg --type go 'GetDelegation' tests/integration/slashing/keeper

Length of output: 62

Script:

#!/bin/bash
# Description: Verify the presence of assertions or checks on the state of delegations after redelegation and undelegation operations.
# Search for assertion methods or patterns in the test file.
rg --type go 'require\..*' tests/integration/slashing/keeper/slash_redelegation_test.go

Length of output: 2151

_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
require.NoError(t, err)

Expand Down Expand Up @@ -175,7 +173,7 @@ func TestSlashRedelegation(t *testing.T) {
require.Equal(t, balance2AfterSlashing.Amount.Mul(math.NewIntFromUint64(10)).String(), balance2Before.Amount.String())
}

func fundAccount(t *testing.T, ctx context.Context, bankKeeper bankkeeper.Keeper, authKeeper authkeeper.AccountKeeper, addr sdk.AccAddress, amount sdk.Coins) {
func fundAccount(t *testing.T, ctx context.Context, bankKeeper bankkeeper.Keeper, authKeeper authkeeper.AccountKeeper, addr sdk.AccAddress, amount ...sdk.Coin) {
t.Helper()

if authKeeper.GetAccount(ctx, addr) == nil {
Expand All @@ -185,3 +183,185 @@ 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.45"

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

// 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____________"))

// fund all accounts
testCoin := sdk.NewCoin(bondDenom, math.NewInt(1_000_000))
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc1, testCoin)
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc2, testCoin)
fundAccount(t, ctx, bankKeeper, authKeeper, testAcc3, testCoin)

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

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

// create evil val
evilValAddr := sdk.ValAddress(evilValPubKey.Address())
fundAccount(t, ctx, bankKeeper, authKeeper, sdk.AccAddress(evilValAddr), testCoin)
createValMsg1, _ := stakingtypes.NewMsgCreateValidator(
evilValAddr.String(), evilValPubKey, testCoin, 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), testCoin)
createValMsg2, _ := stakingtypes.NewMsgCreateValidator(
goodValAddr.String(), goodValPubKey, testCoin, 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(), testCoin)
_, err = stakingMsgServer.Delegate(ctx, delMsg)
require.NoError(t, err)

delMsg = stakingtypes.NewMsgDelegate(testAcc2.String(), evilValAddr.String(), testCoin)
_, err = stakingMsgServer.Delegate(ctx, delMsg)
require.NoError(t, err)

delMsg = stakingtypes.NewMsgDelegate(testAcc3.String(), evilValAddr.String(), testCoin)
_, 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
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(), testCoin)
_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
require.NoError(t, err)
undelMsg := stakingtypes.NewMsgUndelegate(testAcc1.String(), goodValAddr.String(), testCoin)
_, err = stakingMsgServer.Undelegate(ctx, undelMsg)
require.NoError(t, err)

// acc 2: undelegate full amount
undelMsg = stakingtypes.NewMsgUndelegate(testAcc2.String(), evilValAddr.String(), testCoin)
_, 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(), testCoin)
_, err = stakingMsgServer.BeginRedelegate(ctx, redelMsg)
require.NoError(t, err)

undelegationPercentage := math.LegacyMustNewDecFromStr(undelegationPercentageStr)
undelegationAmountDec := math.LegacyNewDecFromInt(testCoin.Amount).Mul(undelegationPercentage)
amountToUndelegate := undelegationAmountDec.TruncateInt()

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

portionofTestCoins := sdk.NewCoin(bondDenom, amountToUndelegate)
undelMsg = stakingtypes.NewMsgUndelegate(testAcc3.String(), goodValAddr.String(), portionofTestCoins)
_, 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 2 blocks to complete redelegations and unbondings
for i := 0; i < 2; i++ {
ctx, err = simtestutil.NextBlock(app, ctx, time.Duration(1000000000000000000))
require.NoError(t, err)
}

// 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))

require.Equal(t, "550000stake", balance1AfterSlashing.String())
require.Equal(t, "550000stake", balance2AfterSlashing.String())
require.Equal(t, "550000stake", balance3AfterSlashing.String())
}
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