From e986f0382a64898984c8ab79baba9ef0577028fa Mon Sep 17 00:00:00 2001 From: 170210 <85928898+170210@users.noreply.github.com> Date: Wed, 27 Mar 2024 11:10:10 +0900 Subject: [PATCH 1/2] fix: fetch a failed proposal tally from proposal.FinalTallyResult in the gprc query (#1304) * fix(x/gov): grpc query tally for failed proposal (#19725) * fix: fix conflict Signed-off-by: 170210 * chore: update CHANGELOG.md Signed-off-by: 170210 --------- Signed-off-by: 170210 Co-authored-by: David Tumcharoen --- CHANGELOG.md | 1 + x/gov/keeper/grpc_query.go | 2 +- x/gov/keeper/grpc_query_test.go | 28 ++++++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cbb973ea6..590e0d2ff5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (client) [\#1303](https://github.com/Finschia/finschia-sdk/pull/1303) fix possible overflow in BuildUnsignedTx * (types) [\#1299](https://github.com/Finschia/finschia-sdk/pull/1299) add missing nil checks * (x/staking) [\#1301](https://github.com/Finschia/finschia-sdk/pull/1301) Use bytes instead of string comparison in delete validator queue (backport cosmos/cosmos-sdk#12303) +* (x/gov) [\#1304](https://github.com/Finschia/finschia-sdk/pull/1304) fetch a failed proposal tally from proposal.FinalTallyResult in the gprc query ### Removed diff --git a/x/gov/keeper/grpc_query.go b/x/gov/keeper/grpc_query.go index 80a86cab85..226f4221e6 100644 --- a/x/gov/keeper/grpc_query.go +++ b/x/gov/keeper/grpc_query.go @@ -264,7 +264,7 @@ func (q Keeper) TallyResult(c context.Context, req *types.QueryTallyResultReques case proposal.Status == types.StatusDepositPeriod: tallyResult = types.EmptyTallyResult() - case proposal.Status == types.StatusPassed || proposal.Status == types.StatusRejected: + case proposal.Status == types.StatusPassed || proposal.Status == types.StatusRejected || proposal.Status == types.StatusFailed: tallyResult = proposal.FinalTallyResult default: diff --git a/x/gov/keeper/grpc_query_test.go b/x/gov/keeper/grpc_query_test.go index dae0206801..73aa23e555 100644 --- a/x/gov/keeper/grpc_query_test.go +++ b/x/gov/keeper/grpc_query_test.go @@ -4,6 +4,7 @@ import ( gocontext "context" "fmt" "strconv" + "time" "github.com/Finschia/finschia-sdk/simapp" sdk "github.com/Finschia/finschia-sdk/types" @@ -798,6 +799,33 @@ func (suite *KeeperTestSuite) TestGRPCQueryTally() { }, true, }, + { + "proposal status failed", + func() { + propTime := time.Now() + proposal := types.Proposal{ + ProposalId: 1, + Status: types.StatusFailed, + FinalTallyResult: types.TallyResult{ + Yes: sdk.NewInt(4), + Abstain: sdk.NewInt(1), + No: sdk.NewInt(0), + NoWithVeto: sdk.NewInt(0), + }, + SubmitTime: propTime, + VotingStartTime: propTime, + VotingEndTime: propTime, + } + app.GovKeeper.SetProposal(ctx, proposal) + + req = &types.QueryTallyResultRequest{ProposalId: proposal.ProposalId} + + expRes = &types.QueryTallyResultResponse{ + Tally: proposal.FinalTallyResult, + } + }, + true, + }, } for _, testCase := range testCases { From 05ff4ea74f6c947ffdcfae969456fd57c839bc6b Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Wed, 27 Mar 2024 11:36:00 +0900 Subject: [PATCH 2/2] fix: add validation for potential slashing evasion during re-delegation (#1306) * Add slash redelegation test * Add validation * Update CHANGELOG * Fix for lint * Fix for lint --- CHANGELOG.md | 1 + simapp/app.go | 16 ++ x/slashing/keeper/slash_redelegation_test.go | 151 +++++++++++++++++++ x/staking/keeper/slash.go | 39 ++++- 4 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 x/slashing/keeper/slash_redelegation_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 590e0d2ff5..0d95306b37 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (types) [\#1299](https://github.com/Finschia/finschia-sdk/pull/1299) add missing nil checks * (x/staking) [\#1301](https://github.com/Finschia/finschia-sdk/pull/1301) Use bytes instead of string comparison in delete validator queue (backport cosmos/cosmos-sdk#12303) * (x/gov) [\#1304](https://github.com/Finschia/finschia-sdk/pull/1304) fetch a failed proposal tally from proposal.FinalTallyResult in the gprc query +* (x/staking) [\#1306](https://github.com/Finschia/finschia-sdk/pull/1306) add validation for potential slashing evasion during re-delegation ### Removed diff --git a/simapp/app.go b/simapp/app.go index c3e1f44cfe..a86dd1c611 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "path/filepath" + "time" "github.com/gorilla/mux" "github.com/spf13/cast" @@ -597,6 +598,21 @@ func (app *SimApp) GetTxConfig() client.TxConfig { return MakeTestEncodingConfig().TxConfig } +// NextBlock starts a new block. +func (app *SimApp) NextBlock(ctx sdk.Context, jumpTime time.Duration) sdk.Context { + app.EndBlock(abci.RequestEndBlock{Height: ctx.BlockHeight()}) + + app.Commit() + + newBlockTime := ctx.BlockTime().Add(jumpTime) + nextHeight := ctx.BlockHeight() + 1 + newHeader := tmproto.Header{Height: nextHeight, Time: newBlockTime} + + app.BeginBlock(abci.RequestBeginBlock{Header: newHeader}) + + return app.NewUncachedContext(false, newHeader) +} + // AppCodec returns SimApp's app codec. // // NOTE: This is solely to be used for testing purposes as it may be desirable diff --git a/x/slashing/keeper/slash_redelegation_test.go b/x/slashing/keeper/slash_redelegation_test.go new file mode 100644 index 0000000000..15d004fa59 --- /dev/null +++ b/x/slashing/keeper/slash_redelegation_test.go @@ -0,0 +1,151 @@ +package keeper_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + + "github.com/Finschia/finschia-sdk/crypto/keys/secp256k1" + "github.com/Finschia/finschia-sdk/simapp" + sdk "github.com/Finschia/finschia-sdk/types" + bankkeeper "github.com/Finschia/finschia-sdk/x/bank/keeper" + banktestutil "github.com/Finschia/finschia-sdk/x/bank/testutil" + distributionkeeper "github.com/Finschia/finschia-sdk/x/distribution/keeper" + stakingkeeper "github.com/Finschia/finschia-sdk/x/staking/keeper" + stakingtypes "github.com/Finschia/finschia-sdk/x/staking/types" +) + +func TestSlashRedelegation(t *testing.T) { + // setting up + app := simapp.Setup(false) + + stakingKeeper := app.StakingKeeper + bankKeeper := app.BankKeeper + slashKeeper := app.SlashingKeeper + distrKeeper := app.DistrKeeper + + // get sdk context, staking msg server and bond denom + ctx := app.BaseApp.NewContext(false, tmproto.Header{Height: app.LastBlockHeight() + 1}) + stakingMsgServer := stakingkeeper.NewMsgServerImpl(stakingKeeper) + bondDenom := stakingKeeper.BondDenom(ctx) + + // evilVal will be slashed, goodVal won't be slashed + evilValPubKey := secp256k1.GenPrivKey().PubKey() + goodValPubKey := secp256k1.GenPrivKey().PubKey() + + // both test acc 1 and 2 delegated to evil val, both acc should be slashed when evil val is slashed + // test acc 1 use the "undelegation after redelegation" trick (redelegate to good val and then undelegate) to avoid slashing + // test acc 2 only undelegate from evil val + testAcc1 := sdk.AccAddress([]byte("addr1_______________")) + testAcc2 := sdk.AccAddress([]byte("addr2_______________")) + + // fund acc 1 and acc 2 + testCoins := sdk.NewCoins(sdk.NewCoin(bondDenom, stakingKeeper.TokensFromConsensusPower(ctx, 10))) + err := banktestutil.FundAccount(bankKeeper, ctx, testAcc1, testCoins) + require.NoError(t, err) + err = banktestutil.FundAccount(bankKeeper, ctx, testAcc2, testCoins) + require.NoError(t, err) + + 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()) + + // creating evil val + evilValAddr := sdk.ValAddress(evilValPubKey.Address()) + err = banktestutil.FundAccount(bankKeeper, ctx, sdk.AccAddress(evilValAddr), testCoins) + require.NoError(t, err) + createValMsg1, _ := stakingtypes.NewMsgCreateValidator( + evilValAddr, evilValPubKey, testCoins[0], stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)), sdk.OneInt()) + _, err = stakingMsgServer.CreateValidator(sdk.WrapSDKContext(ctx), createValMsg1) + require.NoError(t, err) + + // creating good val + goodValAddr := sdk.ValAddress(goodValPubKey.Address()) + err = banktestutil.FundAccount(bankKeeper, ctx, sdk.AccAddress(goodValAddr), testCoins) + require.NoError(t, err) + createValMsg2, _ := stakingtypes.NewMsgCreateValidator( + goodValAddr, goodValPubKey, testCoins[0], stakingtypes.Description{Details: "test"}, stakingtypes.NewCommissionRates(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)), sdk.OneInt()) + _, err = stakingMsgServer.CreateValidator(sdk.WrapSDKContext(ctx), createValMsg2) + require.NoError(t, err) + + // next block, commit height 1, move to height 2 + // acc 1 and acc 2 delegate to evil val + ctx = app.NextBlock(ctx, time.Duration(1)) + require.NoError(t, err) + + // Acc 2 delegate + delMsg := stakingtypes.NewMsgDelegate(testAcc2, evilValAddr, testCoins[0]) + _, err = stakingMsgServer.Delegate(sdk.WrapSDKContext(ctx), delMsg) + require.NoError(t, err) + + // Acc 1 delegate + delMsg = stakingtypes.NewMsgDelegate(testAcc1, evilValAddr, testCoins[0]) + _, err = stakingMsgServer.Delegate(sdk.WrapSDKContext(ctx), delMsg) + require.NoError(t, err) + + // next block, commit height 2, move to height 3 + // with the new delegations, evil val increases in voting power and commit byzantine behavior at height 3 consensus + // at the same time, acc 1 and acc 2 withdraw delegation from evil val + ctx = app.NextBlock(ctx, time.Duration(1)) + require.NoError(t, err) + + evilVal, found := stakingKeeper.GetValidator(ctx, evilValAddr) + require.True(t, found) + + evilPower := stakingKeeper.TokensToConsensusPower(ctx, evilVal.Tokens) + + // Acc 1 redelegate from evil val to good val + redelMsg := stakingtypes.NewMsgBeginRedelegate(testAcc1, evilValAddr, goodValAddr, testCoins[0]) + _, err = stakingMsgServer.BeginRedelegate(sdk.WrapSDKContext(ctx), redelMsg) + require.NoError(t, err) + + // Acc 1 undelegate from good val + undelMsg := stakingtypes.NewMsgUndelegate(testAcc1, goodValAddr, testCoins[0]) + _, err = stakingMsgServer.Undelegate(sdk.WrapSDKContext(ctx), undelMsg) + require.NoError(t, err) + + // Acc 2 undelegate from evil val + undelMsg = stakingtypes.NewMsgUndelegate(testAcc2, evilValAddr, testCoins[0]) + _, err = stakingMsgServer.Undelegate(sdk.WrapSDKContext(ctx), undelMsg) + require.NoError(t, err) + + // next block, commit height 3, move to height 4 + // Slash evil val for byzantine behavior at height 3 consensus, + // at which acc 1 and acc 2 still contributed to evil val voting power + // even tho they undelegate at block 3, the valset update is applied after committed block 3 when height 3 consensus already passes + ctx = app.NextBlock(ctx, time.Duration(1)) + require.NoError(t, err) + + // slash evil val with slash factor = 0.9, leaving only 10% of stake after slashing + evilVal, _ = stakingKeeper.GetValidator(ctx, evilValAddr) + evilValConsAddr, err := evilVal.GetConsAddr() + require.NoError(t, err) + + slashKeeper.Slash(ctx, evilValConsAddr, sdk.MustNewDecFromStr("0.9"), evilPower, 3) + + // assert invariant to make sure we conduct slashing correctly + _, 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) + + // one eternity later + ctx = app.NextBlock(ctx, time.Duration(1000000000000000000)) + ctx = app.NextBlock(ctx, time.Duration(1)) + + // confirm that account 1 and account 2 has been slashed, and the slash amount is correct + balance1AfterSlashing := bankKeeper.GetBalance(ctx, testAcc1, bondDenom) + balance2AfterSlashing := bankKeeper.GetBalance(ctx, testAcc2, bondDenom) + + require.Equal(t, balance1AfterSlashing.Amount.Mul(sdk.NewIntFromUint64(10)).String(), balance1Before.Amount.String()) + require.Equal(t, balance2AfterSlashing.Amount.Mul(sdk.NewIntFromUint64(10)).String(), balance2Before.Amount.String()) +} diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index 4c6aed6d0b..904c9c5898 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -248,9 +248,46 @@ func (k Keeper) SlashRedelegation(ctx sdk.Context, srcValidator types.Validator, slashAmount := slashAmountDec.TruncateInt() totalSlashAmount = totalSlashAmount.Add(slashAmount) + validatorDstAddress, err := sdk.ValAddressFromBech32(redelegation.ValidatorDstAddress) + if err != nil { + panic(err) + } + // Handle undelegation after redelegation + // Prioritize slashing unbondingDelegation than delegation + unbondingDelegation, found := k.GetUnbondingDelegation(ctx, sdk.MustAccAddressFromBech32(redelegation.DelegatorAddress), validatorDstAddress) + if found { + for i, entry := range unbondingDelegation.Entries { + // slash with the amount of `slashAmount` if possible, else slash all unbonding token + unbondingSlashAmount := sdk.MinInt(slashAmount, entry.Balance) + + switch { + // There's no token to slash + case unbondingSlashAmount.IsZero(): + continue + // If unbonding started before this height, stake didn't contribute to infraction + case entry.CreationHeight < infractionHeight: + continue + // Unbonding delegation no longer eligible for slashing, skip it + case entry.IsMature(now): + continue + // Slash the unbonding delegation + default: + // update remaining slashAmount + slashAmount = slashAmount.Sub(unbondingSlashAmount) + + notBondedBurnedAmount = notBondedBurnedAmount.Add(unbondingSlashAmount) + entry.Balance = entry.Balance.Sub(unbondingSlashAmount) + unbondingDelegation.Entries[i] = entry + k.SetUnbondingDelegation(ctx, unbondingDelegation) + } + } + } + + // Slash the moved delegation + // Unbond from target validator sharesToUnbond := slashFactor.Mul(entry.SharesDst) - if sharesToUnbond.IsZero() { + if sharesToUnbond.IsZero() || slashAmount.IsZero() { continue }