From e099491daa48cf9730f1843d797fe62abf972031 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 18 Oct 2018 21:58:57 +0200 Subject: [PATCH] Merge PR #2526: Distribution fixes from simulation --- cmd/gaia/app/sim_test.go | 2 +- x/distribution/handler.go | 10 +++++-- x/distribution/keeper/delegation.go | 26 ++++++++++++++--- x/distribution/keeper/validator.go | 36 ++++++++++++++++++++++-- x/distribution/types/dec_coin.go | 43 ++++++++++++++++++++++++----- x/distribution/types/errors.go | 11 ++++++-- x/stake/keeper/delegation.go | 1 + x/stake/simulation/invariants.go | 40 +++++++++++++++++++-------- 8 files changed, 138 insertions(+), 31 deletions(-) diff --git a/cmd/gaia/app/sim_test.go b/cmd/gaia/app/sim_test.go index d7dd12343..bf64f24f2 100644 --- a/cmd/gaia/app/sim_test.go +++ b/cmd/gaia/app/sim_test.go @@ -124,7 +124,7 @@ func invariants(app *GaiaApp) []simulation.Invariant { banksim.NonnegativeBalanceInvariant(app.accountMapper), distributionsim.AllInvariants(app.bankKeeper, app.distrKeeper, app.accountMapper), govsim.AllInvariants(), - stakesim.AllInvariants(app.bankKeeper, app.stakeKeeper, app.accountMapper), + stakesim.AllInvariants(app.bankKeeper, app.stakeKeeper, app.distrKeeper, app.accountMapper), slashingsim.AllInvariants(), } } diff --git a/x/distribution/handler.go b/x/distribution/handler.go index 661d7d32a..624589cea 100644 --- a/x/distribution/handler.go +++ b/x/distribution/handler.go @@ -58,7 +58,10 @@ func handleMsgWithdrawDelegatorRewardsAll(ctx sdk.Context, msg types.MsgWithdraw func handleMsgWithdrawDelegatorReward(ctx sdk.Context, msg types.MsgWithdrawDelegatorReward, k keeper.Keeper) sdk.Result { - k.WithdrawDelegationReward(ctx, msg.DelegatorAddr, msg.ValidatorAddr) + err := k.WithdrawDelegationReward(ctx, msg.DelegatorAddr, msg.ValidatorAddr) + if err != nil { + return err.Result() + } tags := sdk.NewTags( tags.Action, tags.ActionWithdrawDelegatorReward, @@ -72,7 +75,10 @@ func handleMsgWithdrawDelegatorReward(ctx sdk.Context, msg types.MsgWithdrawDele func handleMsgWithdrawValidatorRewardsAll(ctx sdk.Context, msg types.MsgWithdrawValidatorRewardsAll, k keeper.Keeper) sdk.Result { - k.WithdrawValidatorRewardsAll(ctx, msg.ValidatorAddr) + err := k.WithdrawValidatorRewardsAll(ctx, msg.ValidatorAddr) + if err != nil { + return err.Result() + } tags := sdk.NewTags( tags.Action, tags.ActionWithdrawValidatorRewardsAll, diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index b7443a0c1..971bd4c14 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -5,6 +5,13 @@ import ( "github.com/cosmos/cosmos-sdk/x/distribution/types" ) +// check whether a delegator distribution info exists +func (k Keeper) HasDelegationDistInfo(ctx sdk.Context, delAddr sdk.AccAddress, + valOperatorAddr sdk.ValAddress) (has bool) { + store := ctx.KVStore(k.storeKey) + return store.Has(GetDelegationDistInfoKey(delAddr, valOperatorAddr)) +} + // get the delegator distribution info func (k Keeper) GetDelegationDistInfo(ctx sdk.Context, delAddr sdk.AccAddress, valOperatorAddr sdk.ValAddress) (ddi types.DelegationDistInfo) { @@ -64,7 +71,11 @@ func (k Keeper) RemoveDelegatorWithdrawAddr(ctx sdk.Context, delAddr, withdrawAd // withdraw all the rewards for a single delegation func (k Keeper) WithdrawDelegationReward(ctx sdk.Context, delegatorAddr sdk.AccAddress, - validatorAddr sdk.ValAddress) { + validatorAddr sdk.ValAddress) sdk.Error { + + if !k.HasDelegationDistInfo(ctx, delegatorAddr, validatorAddr) { + return types.ErrNoDelegationDistInfo(k.codespace) + } height := ctx.BlockHeight() bondedTokens := k.stakeKeeper.TotalPower(ctx) @@ -77,14 +88,17 @@ func (k Keeper) WithdrawDelegationReward(ctx sdk.Context, delegatorAddr sdk.AccA delInfo, valInfo, feePool, withdraw := delInfo.WithdrawRewards(feePool, valInfo, height, bondedTokens, validator.GetTokens(), validator.GetDelegatorShares(), delegation.GetShares(), validator.GetCommission()) - k.SetFeePool(ctx, feePool) k.SetValidatorDistInfo(ctx, valInfo) k.SetDelegationDistInfo(ctx, delInfo) withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, delegatorAddr) - _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, withdraw.TruncateDecimal()) + coinsToAdd, change := withdraw.TruncateDecimal() + feePool.CommunityPool = feePool.CommunityPool.Plus(change) + k.SetFeePool(ctx, feePool) + _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coinsToAdd) if err != nil { panic(err) } + return nil } //___________________________________________________________________________________________ @@ -93,8 +107,12 @@ func (k Keeper) WithdrawDelegationReward(ctx sdk.Context, delegatorAddr sdk.AccA func (k Keeper) WithdrawDelegationRewardsAll(ctx sdk.Context, delegatorAddr sdk.AccAddress) { height := ctx.BlockHeight() withdraw := k.getDelegatorRewardsAll(ctx, delegatorAddr, height) + feePool := k.GetFeePool(ctx) withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, delegatorAddr) - _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, withdraw.TruncateDecimal()) + coinsToAdd, change := withdraw.TruncateDecimal() + feePool.CommunityPool = feePool.CommunityPool.Plus(change) + k.SetFeePool(ctx, feePool) + _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coinsToAdd) if err != nil { panic(err) } diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index ae07d8b3f..bbc2a732e 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -5,6 +5,13 @@ import ( "github.com/cosmos/cosmos-sdk/x/distribution/types" ) +// check whether a validator has distribution info +func (k Keeper) HasValidatorDistInfo(ctx sdk.Context, + operatorAddr sdk.ValAddress) (exists bool) { + store := ctx.KVStore(k.storeKey) + return store.Has(GetValidatorDistInfoKey(operatorAddr)) +} + // get the validator distribution info func (k Keeper) GetValidatorDistInfo(ctx sdk.Context, operatorAddr sdk.ValAddress) (vdi types.ValidatorDistInfo) { @@ -34,7 +41,11 @@ func (k Keeper) RemoveValidatorDistInfo(ctx sdk.Context, valAddr sdk.ValAddress) } // withdrawal all the validator rewards including the commission -func (k Keeper) WithdrawValidatorRewardsAll(ctx sdk.Context, operatorAddr sdk.ValAddress) { +func (k Keeper) WithdrawValidatorRewardsAll(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error { + + if !k.HasValidatorDistInfo(ctx, operatorAddr) { + return types.ErrNoValidatorDistInfo(k.codespace) + } // withdraw self-delegation height := ctx.BlockHeight() @@ -50,11 +61,30 @@ func (k Keeper) WithdrawValidatorRewardsAll(ctx sdk.Context, operatorAddr sdk.Va validator.GetTokens(), validator.GetCommission()) withdraw = withdraw.Plus(commission) k.SetValidatorDistInfo(ctx, valInfo) - k.SetFeePool(ctx, feePool) withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, accAddr) - _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, withdraw.TruncateDecimal()) + truncated, change := withdraw.TruncateDecimal() + feePool.CommunityPool = feePool.CommunityPool.Plus(change) + k.SetFeePool(ctx, feePool) + _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, truncated) if err != nil { panic(err) } + + return nil +} + +func (k Keeper) IterateValidatorDistInfos(ctx sdk.Context, fn func(index int64, distInfo types.ValidatorDistInfo) (stop bool)) { + store := ctx.KVStore(k.storeKey) + iter := sdk.KVStorePrefixIterator(store, ValidatorDistInfoKey) + defer iter.Close() + index := int64(0) + for ; iter.Valid(); iter.Next() { + var vdi types.ValidatorDistInfo + k.cdc.MustUnmarshalBinary(iter.Value(), &vdi) + if fn(index, vdi) { + return + } + index++ + } } diff --git a/x/distribution/types/dec_coin.go b/x/distribution/types/dec_coin.go index 59373976d..9e9313788 100644 --- a/x/distribution/types/dec_coin.go +++ b/x/distribution/types/dec_coin.go @@ -43,9 +43,11 @@ func (coin DecCoin) Minus(coinB DecCoin) DecCoin { return DecCoin{coin.Denom, coin.Amount.Sub(coinB.Amount)} } -// return the decimal coins with trunctated decimals -func (coin DecCoin) TruncateDecimal() sdk.Coin { - return sdk.NewCoin(coin.Denom, coin.Amount.TruncateInt()) +// return the decimal coins with trunctated decimals, and return the change +func (coin DecCoin) TruncateDecimal() (sdk.Coin, DecCoin) { + truncated := coin.Amount.TruncateInt() + change := coin.Amount.Sub(sdk.NewDecFromInt(truncated)) + return sdk.NewCoin(coin.Denom, truncated), DecCoin{coin.Denom, change} } //_______________________________________________________________________ @@ -61,13 +63,16 @@ func NewDecCoins(coins sdk.Coins) DecCoins { return dcs } -// return the coins with trunctated decimals -func (coins DecCoins) TruncateDecimal() sdk.Coins { +// return the coins with trunctated decimals, and return the change +func (coins DecCoins) TruncateDecimal() (sdk.Coins, DecCoins) { + changeSum := DecCoins{} out := make(sdk.Coins, len(coins)) for i, coin := range coins { - out[i] = coin.TruncateDecimal() + truncated, change := coin.TruncateDecimal() + out[i] = truncated + changeSum = changeSum.Plus(DecCoins{change}) } - return out + return out, changeSum } // Plus combines two sets of coins @@ -147,3 +152,27 @@ func (coins DecCoins) QuoDec(d sdk.Dec) DecCoins { } return res } + +// returns the amount of a denom from deccoins +func (coins DecCoins) AmountOf(denom string) sdk.Dec { + switch len(coins) { + case 0: + return sdk.ZeroDec() + case 1: + coin := coins[0] + if coin.Denom == denom { + return coin.Amount + } + return sdk.ZeroDec() + default: + midIdx := len(coins) / 2 // 2:1, 3:1, 4:2 + coin := coins[midIdx] + if denom < coin.Denom { + return coins[:midIdx].AmountOf(denom) + } else if denom == coin.Denom { + return coin.Amount + } else { + return coins[midIdx+1:].AmountOf(denom) + } + } +} diff --git a/x/distribution/types/errors.go b/x/distribution/types/errors.go index 57a3dd73e..605c1b38d 100644 --- a/x/distribution/types/errors.go +++ b/x/distribution/types/errors.go @@ -8,8 +8,9 @@ import ( type CodeType = sdk.CodeType const ( - DefaultCodespace sdk.CodespaceType = 6 - CodeInvalidInput CodeType = 103 + DefaultCodespace sdk.CodespaceType = 6 + CodeInvalidInput CodeType = 103 + CodeNoDistributionInfo CodeType = 104 ) func ErrNilDelegatorAddr(codespace sdk.CodespaceType) sdk.Error { @@ -21,3 +22,9 @@ func ErrNilWithdrawAddr(codespace sdk.CodespaceType) sdk.Error { func ErrNilValidatorAddr(codespace sdk.CodespaceType) sdk.Error { return sdk.NewError(codespace, CodeInvalidInput, "validator address is nil") } +func ErrNoDelegationDistInfo(codespace sdk.CodespaceType) sdk.Error { + return sdk.NewError(codespace, CodeNoDistributionInfo, "no delegation distribution info") +} +func ErrNoValidatorDistInfo(codespace sdk.CodespaceType) sdk.Error { + return sdk.NewError(codespace, CodeNoDistributionInfo, "no validator distribution info") +} diff --git a/x/stake/keeper/delegation.go b/x/stake/keeper/delegation.go index 25ea3d08b..214ae399c 100644 --- a/x/stake/keeper/delegation.go +++ b/x/stake/keeper/delegation.go @@ -551,6 +551,7 @@ func (k Keeper) BeginRedelegation(ctx sdk.Context, delAddr sdk.AccAddress, if err != nil { return types.Redelegation{}, err } + k.OnDelegationCreated(ctx, delAddr, valDstAddr) // create the unbonding delegation minTime, height, completeNow := k.getBeginInfo(ctx, valSrcAddr) diff --git a/x/stake/simulation/invariants.go b/x/stake/simulation/invariants.go index 2866f6292..175338cdf 100644 --- a/x/stake/simulation/invariants.go +++ b/x/stake/simulation/invariants.go @@ -7,6 +7,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/auth" "github.com/cosmos/cosmos-sdk/x/bank" + "github.com/cosmos/cosmos-sdk/x/distribution" "github.com/cosmos/cosmos-sdk/x/mock/simulation" "github.com/cosmos/cosmos-sdk/x/stake" abci "github.com/tendermint/tendermint/abci/types" @@ -14,9 +15,9 @@ import ( // AllInvariants runs all invariants of the stake module. // Currently: total supply, positive power -func AllInvariants(ck bank.Keeper, k stake.Keeper, am auth.AccountMapper) simulation.Invariant { +func AllInvariants(ck bank.Keeper, k stake.Keeper, d distribution.Keeper, am auth.AccountMapper) simulation.Invariant { return func(app *baseapp.BaseApp) error { - err := SupplyInvariants(ck, k, am)(app) + err := SupplyInvariants(ck, k, d, am)(app) if err != nil { return err } @@ -31,19 +32,19 @@ func AllInvariants(ck bank.Keeper, k stake.Keeper, am auth.AccountMapper) simula // SupplyInvariants checks that the total supply reflects all held loose tokens, bonded tokens, and unbonding delegations // nolint: unparam -func SupplyInvariants(ck bank.Keeper, k stake.Keeper, am auth.AccountMapper) simulation.Invariant { +func SupplyInvariants(ck bank.Keeper, k stake.Keeper, d distribution.Keeper, am auth.AccountMapper) simulation.Invariant { return func(app *baseapp.BaseApp) error { ctx := app.NewContext(false, abci.Header{}) pool := k.GetPool(ctx) - loose := sdk.ZeroInt() + loose := sdk.ZeroDec() bonded := sdk.ZeroDec() am.IterateAccounts(ctx, func(acc auth.Account) bool { - loose = loose.Add(acc.GetCoins().AmountOf("steak")) + loose = loose.Add(sdk.NewDecFromInt(acc.GetCoins().AmountOf("steak"))) return false }) k.IterateUnbondingDelegations(ctx, func(_ int64, ubd stake.UnbondingDelegation) bool { - loose = loose.Add(ubd.Balance.Amount) + loose = loose.Add(sdk.NewDecFromInt(ubd.Balance.Amount)) return false }) k.IterateValidators(ctx, func(_ int64, validator sdk.Validator) bool { @@ -51,21 +52,36 @@ func SupplyInvariants(ck bank.Keeper, k stake.Keeper, am auth.AccountMapper) sim case sdk.Bonded: bonded = bonded.Add(validator.GetPower()) case sdk.Unbonding: - loose = loose.Add(validator.GetTokens().RoundInt()) + loose = loose.Add(validator.GetTokens()) case sdk.Unbonded: - loose = loose.Add(validator.GetTokens().RoundInt()) + loose = loose.Add(validator.GetTokens()) } return false }) + feePool := d.GetFeePool(ctx) + + // add community pool + loose = loose.Add(feePool.CommunityPool.AmountOf("steak")) + + // add validator distribution pool + loose = loose.Add(feePool.Pool.AmountOf("steak")) + + // add validator distribution commission and yet-to-be-withdrawn-by-delegators + d.IterateValidatorDistInfos(ctx, func(_ int64, distInfo distribution.ValidatorDistInfo) (stop bool) { + loose = loose.Add(distInfo.Pool.AmountOf("steak")) + loose = loose.Add(distInfo.PoolCommission.AmountOf("steak")) + return false + }) + // Loose tokens should equal coin supply plus unbonding delegations plus tokens on unbonded validators - if pool.LooseTokens.RoundInt64() != loose.Int64() { - return fmt.Errorf("expected loose tokens to equal total steak held by accounts - pool.LooseTokens: %v, sum of account tokens: %v", pool.LooseTokens.RoundInt64(), loose.Int64()) + if !pool.LooseTokens.Equal(loose) { + return fmt.Errorf("expected loose tokens to equal total steak held by accounts - pool.LooseTokens: %v, sum of account tokens: %v", pool.LooseTokens, loose) } // Bonded tokens should equal sum of tokens with bonded validators - if pool.BondedTokens.RoundInt64() != bonded.RoundInt64() { - return fmt.Errorf("expected bonded tokens to equal total steak held by bonded validators - pool.BondedTokens: %v, sum of bonded validator tokens: %v", pool.BondedTokens.RoundInt64(), bonded.RoundInt64()) + if !pool.BondedTokens.Equal(bonded) { + return fmt.Errorf("expected bonded tokens to equal total steak held by bonded validators - pool.BondedTokens: %v, sum of bonded validator tokens: %v", pool.BondedTokens, bonded) } // TODO Inflation check on total supply