From bc51fa93b678a66f4cc98b6db3ed0ae908fba77c Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Sat, 8 Dec 2018 07:18:04 -0800 Subject: [PATCH] Fix updateValidatorDistInfoFromPool (#3046) Fixes regression introduced by #2984. Continuiation of #3033 , which didn't fix the simulation issues. (candidate) Complete solution for #3019, 9002 halt bug. From #2984, it isn't sufficient to take the fee pool rewards of a validator. Since we don't track delegator accums (as we do with validator accums), and because onValidatorModified >updateValidatorDistInfoFromPool is also being called upon delegation updates (or at least I believe this is the reason), it is necessary to also withdraw self delegation. TODO: I don't think self-delegation should be required to be modified here... consider using a delegation hook to do the self-delegation withdraw part instead, e.g. splitting the updateValidatorDistInfoFromPool function into two. It might not result in cleaner code, however. Think hard. --- Makefile | 2 + cmd/gaia/cmd/gaiakeyutil/main.go | 51 +++++++++++++++++++++++++ types/decimal.go | 5 +++ x/distribution/keeper/hooks.go | 2 +- x/distribution/keeper/validator.go | 19 ++++++--- x/distribution/keeper/validator_test.go | 22 +++++++---- x/distribution/simulation/invariants.go | 5 ++- 7 files changed, 91 insertions(+), 15 deletions(-) create mode 100644 cmd/gaia/cmd/gaiakeyutil/main.go diff --git a/Makefile b/Makefile index e1c69b103795..7d5a0790c36f 100644 --- a/Makefile +++ b/Makefile @@ -53,6 +53,7 @@ else go build $(BUILD_FLAGS) -o build/gaiad ./cmd/gaia/cmd/gaiad go build $(BUILD_FLAGS) -o build/gaiacli ./cmd/gaia/cmd/gaiacli go build $(BUILD_FLAGS) -o build/gaiareplay ./cmd/gaia/cmd/gaiareplay + go build $(BUILD_FLAGS) -o build/gaiakeyutil ./cmd/gaia/cmd/gaiakeyutil endif build-linux: @@ -85,6 +86,7 @@ install: check-ledger update_gaia_lite_docs go install $(BUILD_FLAGS) ./cmd/gaia/cmd/gaiad go install $(BUILD_FLAGS) ./cmd/gaia/cmd/gaiacli go install $(BUILD_FLAGS) ./cmd/gaia/cmd/gaiareplay + go install $(BUILD_FLAGS) ./cmd/gaia/cmd/gaiakeyutil install_examples: go install $(BUILD_FLAGS) ./docs/examples/basecoin/cmd/basecoind diff --git a/cmd/gaia/cmd/gaiakeyutil/main.go b/cmd/gaia/cmd/gaiakeyutil/main.go new file mode 100644 index 000000000000..53320e8233d5 --- /dev/null +++ b/cmd/gaia/cmd/gaiakeyutil/main.go @@ -0,0 +1,51 @@ +package main + +import ( + "encoding/hex" + "fmt" + "os" + + "github.com/tendermint/tendermint/libs/bech32" +) + +var bech32Prefixes = []string{"cosmos", "cosmospub", "cosmosvaloper", "cosmosvaloperpub", "cosmosvalcons", "cosmosvalconspub"} + +func main() { + if len(os.Args) < 2 { + fmt.Println("Must specify an input string") + } + arg := os.Args[1] + runFromBech32(arg) + runFromHex(arg) +} + +// Print info from bech32. +func runFromBech32(bech32str string) { + hrp, bz, err := bech32.DecodeAndConvert(bech32str) + if err != nil { + fmt.Println("Not a valid bech32 string") + return + } + fmt.Println("Bech32 parse:") + fmt.Printf("Human readible part: %v\nBytes (hex): %X\n", + hrp, + bz, + ) +} + +func runFromHex(hexaddr string) { + bz, err := hex.DecodeString(hexaddr) + if err != nil { + fmt.Println("Not a valid hex string") + return + } + fmt.Println("Hex parse:") + fmt.Println("Bech32 formats:") + for _, prefix := range bech32Prefixes { + bech32Addr, err := bech32.ConvertAndEncode(prefix, bz) + if err != nil { + panic(err) + } + fmt.Println(" - " + bech32Addr) + } +} diff --git a/types/decimal.go b/types/decimal.go index b4bd44fb27b0..5e0f8d267027 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -400,6 +400,11 @@ func (d Dec) TruncateInt() Int { return NewIntFromBigInt(chopPrecisionAndTruncateNonMutative(d.Int)) } +// TruncateDec truncates the decimals from the number and returns a Dec +func (d Dec) TruncateDec() Dec { + return NewDecFromBigInt(chopPrecisionAndTruncateNonMutative(d.Int)) +} + //___________________________________________________________________________________ // reuse nil values diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 260ee2e77cdb..e9d31b8a2340 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -32,7 +32,7 @@ func (k Keeper) onValidatorModified(ctx sdk.Context, valAddr sdk.ValAddress) { // (dist info), but without actually withdrawing the rewards. This does not // need to happen during the genesis block. if ctx.BlockHeight() > 0 { - if err := k.updateValidatorDistInfoFromPool(ctx, valAddr); err != nil { + if err := k.takeValidatorFeePoolRewards(ctx, valAddr); err != nil { panic(err) } } diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index 951ba280a584..4ce0834a711b 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -91,27 +91,36 @@ func (k Keeper) GetValidatorAccum(ctx sdk.Context, operatorAddr sdk.ValAddress) return accum, nil } -// updateValidatorDistInfoFromPool updates the validator's distribution info +// takeValidatorFeePoolRewards updates the validator's distribution info // from the global fee pool without withdrawing any rewards. This will be called // from a onValidatorModified hook. -func (k Keeper) updateValidatorDistInfoFromPool(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error { +func (k Keeper) takeValidatorFeePoolRewards(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error { if !k.HasValidatorDistInfo(ctx, operatorAddr) { return types.ErrNoValidatorDistInfo(k.codespace) } + accAddr := sdk.AccAddress(operatorAddr.Bytes()) + + // withdraw reward for self-delegation + if k.HasDelegationDistInfo(ctx, accAddr, operatorAddr) { + fp, vi, di, withdraw := + k.withdrawDelegationReward(ctx, accAddr, operatorAddr) + k.SetFeePool(ctx, fp) + k.SetValidatorDistInfo(ctx, vi) + k.SetDelegationDistInfo(ctx, di) + k.WithdrawToDelegator(ctx, fp, accAddr, withdraw) + } + // withdrawal validator commission rewards valInfo := k.GetValidatorDistInfo(ctx, operatorAddr) wc := k.GetWithdrawContext(ctx, operatorAddr) valInfo, feePool := valInfo.TakeFeePoolRewards(wc) - k.SetFeePool(ctx, feePool) k.SetValidatorDistInfo(ctx, valInfo) - return nil } // withdrawal all the validator rewards including the commission func (k Keeper) WithdrawValidatorRewardsAll(ctx sdk.Context, operatorAddr sdk.ValAddress) sdk.Error { - if !k.HasValidatorDistInfo(ctx, operatorAddr) { return types.ErrNoValidatorDistInfo(k.codespace) } diff --git a/x/distribution/keeper/validator_test.go b/x/distribution/keeper/validator_test.go index 62638a4d2289..fc9331959ce3 100644 --- a/x/distribution/keeper/validator_test.go +++ b/x/distribution/keeper/validator_test.go @@ -13,7 +13,7 @@ func TestWithdrawValidatorRewardsAllNoDelegator(t *testing.T) { stakeHandler := stake.NewHandler(sk) denom := sk.GetParams(ctx).BondDenom - //first make a validator + // first make a validator msgCreateValidator := stake.NewTestMsgCreateValidator(valOpAddr1, valConsPk1, 10) got := stakeHandler(ctx, msgCreateValidator) require.True(t, got.IsOK(), "expected msg to be ok, got %v", got) @@ -107,17 +107,20 @@ func TestWithdrawValidatorRewardsAllMultipleValidator(t *testing.T) { stakeHandler := stake.NewHandler(sk) denom := sk.GetParams(ctx).BondDenom - //make some validators with different commissions + // Make some validators with different commissions. + // Bond 10 of 100 with 0.1 commission. msgCreateValidator := stake.NewTestMsgCreateValidatorWithCommission( valOpAddr1, valConsPk1, 10, sdk.NewDecWithPrec(1, 1)) got := stakeHandler(ctx, msgCreateValidator) require.True(t, got.IsOK(), "expected msg to be ok, got %v", got) + // Bond 50 of 100 with 0.2 commission. msgCreateValidator = stake.NewTestMsgCreateValidatorWithCommission( valOpAddr2, valConsPk2, 50, sdk.NewDecWithPrec(2, 1)) got = stakeHandler(ctx, msgCreateValidator) require.True(t, got.IsOK(), "expected msg to be ok, got %v", got) + // Bond 40 of 100 with 0.3 commission. msgCreateValidator = stake.NewTestMsgCreateValidatorWithCommission( valOpAddr3, valConsPk3, 40, sdk.NewDecWithPrec(3, 1)) got = stakeHandler(ctx, msgCreateValidator) @@ -125,22 +128,27 @@ func TestWithdrawValidatorRewardsAllMultipleValidator(t *testing.T) { _ = sk.ApplyAndReturnValidatorSetUpdates(ctx) - // allocate 100 denom of fees + // Allocate 1000 denom of fees. feeInputs := sdk.NewInt(1000) fck.SetCollectedFees(sdk.Coins{sdk.NewCoin(denom, feeInputs)}) require.Equal(t, feeInputs, fck.GetCollectedFees(ctx).AmountOf(denom)) + // Collect proposer reward for 100% of votes. keeper.AllocateTokens(ctx, sdk.OneDec(), valConsAddr1) - // withdraw validator reward + // Withdraw validator reward. ctx = ctx.WithBlockHeight(1) keeper.WithdrawValidatorRewardsAll(ctx, valOpAddr1) amt := accMapper.GetAccount(ctx, valAccAddr1).GetCoins().AmountOf(denom) feesInNonProposer := sdk.NewDecFromInt(feeInputs).Mul(sdk.NewDecWithPrec(95, 2)) feesInProposer := sdk.NewDecFromInt(feeInputs).Mul(sdk.NewDecWithPrec(5, 2)) - expRes := sdk.NewDec(90). // orig tokens (100 - 10) - Add(feesInNonProposer.Quo(sdk.NewDec(10))). // validator 1 has 1/10 total power - Add(feesInProposer). + // NOTE: the non-proposer rewards (95) and proposer rewards (50) add up to + // 145. During computation, this is further split into 130.5 and 14.5, + // which is the non-commission and commission respectively, but the + // commission is for self so the result is just 145. + expRes := sdk.NewDec(90). // orig tokens (100) - bonded (10) + Add(feesInNonProposer.Quo(sdk.NewDec(10))). // validator 1 has 1/10 total power (non-proposer rewards = 95) + Add(feesInProposer). // (proposer rewards = 50) TruncateInt() require.True(sdk.IntEq(t, expRes, amt)) } diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index bf808905af89..2f954d6e1204 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -101,9 +101,10 @@ func DelAccumInvariants(k distr.Keeper, sk distr.StakeKeeper) simulation.Invaria delegation := sk.Delegation(ctx, ddi.DelegatorAddr, ddi.ValOperatorAddr) accum := ddi.GetDelAccum(height, delegation.GetShares()) if accum.IsPositive() { - logDelAccums += fmt.Sprintf("\n\t\tdel: %v, accum: %v", + logDelAccums += fmt.Sprintf("\n\t\tdel: %v, accum: %v, power: %v", ddi.DelegatorAddr.String(), - accum.String()) + accum.String(), + delegation.GetShares()) } } return false