diff --git a/simapp/mint_fn.go b/simapp/mint_fn.go index e34aca38f496..9f2ee70ca57b 100644 --- a/simapp/mint_fn.go +++ b/simapp/mint_fn.go @@ -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. diff --git a/tests/integration/slashing/keeper/slash_redelegation_test.go b/tests/integration/slashing/keeper/slash_redelegation_test.go index d50a012765e5..ad6dd9abd57a 100644 --- a/tests/integration/slashing/keeper/slash_redelegation_test.go +++ b/tests/integration/slashing/keeper/slash_redelegation_test.go @@ -2,7 +2,6 @@ package keeper_test import ( "context" - "fmt" "testing" "time" @@ -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) 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) @@ -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) @@ -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) _, err = stakingMsgServer.Undelegate(ctx, undelMsg) require.NoError(t, err) @@ -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 { @@ -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) + 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()) +} diff --git a/x/staking/CHANGELOG.md b/x/staking/CHANGELOG.md index cfae69ea4fce..1a44d827c8e7 100644 --- a/x/staking/CHANGELOG.md +++ b/x/staking/CHANGELOG.md @@ -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. diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index 167c1da0a036..ae4f1334f977 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -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) + if err != nil { + return math.ZeroInt(), err + } + + if sharesToUnbond.IsZero() { continue }