From da12165e14d4f9bea597e29ae5cf04f368c81b4b Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 22 Feb 2019 13:02:27 +0100 Subject: [PATCH 01/50] Add altered random number generation --- x/mock/simulation/rand_util.go | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/x/mock/simulation/rand_util.go b/x/mock/simulation/rand_util.go index a57775ecbd22..4301781cd685 100644 --- a/x/mock/simulation/rand_util.go +++ b/x/mock/simulation/rand_util.go @@ -1,6 +1,7 @@ package simulation import ( + "fmt" "math/big" "math/rand" "time" @@ -37,14 +38,37 @@ func RandStringOfLength(r *rand.Rand, n int) string { } // Generate a random amount +// Note: The range of RandomAmount includes max, and is, in fact, biased to return max. func RandomAmount(r *rand.Rand, max sdk.Int) sdk.Int { - return sdk.NewInt(int64(r.Intn(int(max.Int64())))) + // return sdk.NewInt(int64(r.Intn(int(max.Int64())))) + max2 := big.NewInt(0).Mul(max.BigInt(), big.NewInt(2)) + randInt := big.NewInt(0).Rand(r, max2) + if randInt.Cmp(max.BigInt()) > 0 { + randInt = max.BigInt() + } + result := sdk.NewIntFromBigInt(randInt) + // Sanity + if result.GT(max) { + panic(fmt.Sprintf("%v > %v", result, max)) + } + return result } // RandomDecAmount generates a random decimal amount +// Note: The range of RandomDecAmount includes max, and is, in fact, biased to return max. func RandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec { - randInt := big.NewInt(0).Rand(r, max.Int) - return sdk.NewDecFromBigIntWithPrec(randInt, sdk.Precision) + // randInt := big.NewInt(0).Rand(r, max.Int) + max2 := big.NewInt(0).Mul(max.Int, big.NewInt(2)) + randInt := big.NewInt(0).Rand(r, max2) + if randInt.Cmp(max.Int) > 0 { + randInt = max.Int + } + result := sdk.NewDecFromBigIntWithPrec(randInt, sdk.Precision) + // Sanity + if result.GT(max) { + panic(fmt.Sprintf("%v > %v", result, max)) + } + return result } // RandomSetGenesis wraps mock.RandomSetGenesis, but using simulation accounts From b7d1d696b54b69041c21a9f9fb7235690753d262 Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 12:32:06 -0800 Subject: [PATCH 02/50] WIP Allow undelegation of more coins than delegated; Add more validity checks --- docs/spec/auth/05_vesting.md | 6 ++- types/coin.go | 2 - x/auth/account.go | 5 ++- x/bank/keeper.go | 74 ++++++++++++++++++++++++++++++------ x/bank/msgs.go | 8 ++-- 5 files changed, 74 insertions(+), 21 deletions(-) diff --git a/docs/spec/auth/05_vesting.md b/docs/spec/auth/05_vesting.md index a0c64fdd5e95..4b435f9f6cd9 100644 --- a/docs/spec/auth/05_vesting.md +++ b/docs/spec/auth/05_vesting.md @@ -251,10 +251,12 @@ func DelegateCoins(t Time, from Account, amount Coins) { ### Undelegating For a vesting account attempting to undelegate `D` coins, the following is performed: +NOTE: `DV < D` and `(DV + DF) < D` may be possible due to quirks in the rounding of +delegation/undelegation logic. -1. Verify `(DV + DF) >= D > 0` (this is simply a sanity check) +1. Verify `D > 0` 2. Compute `X := min(DF, D)` (portion of `D` that should become free, prioritizing free coins) -3. Compute `Y := D - X` (portion of `D` that should remain vesting) +3. Compute `Y := min(DV, D - X)` (portion of `D` that should remain vesting) 4. Set `DF -= X` 5. Set `DV -= Y` 6. Set `BC += D` diff --git a/types/coin.go b/types/coin.go index 1122c859b6e5..9fa1db0a6faa 100644 --- a/types/coin.go +++ b/types/coin.go @@ -389,8 +389,6 @@ func (coins Coins) AmountOf(denom string) Int { // IsAllPositive returns true if there is at least one coin and all currencies // have a positive value. -// -// TODO: Remove once unsigned integers are used. func (coins Coins) IsAllPositive() bool { if len(coins) == 0 { return false diff --git a/x/auth/account.go b/x/auth/account.go index 0983565ecc9a..6f10fa080d0e 100644 --- a/x/auth/account.go +++ b/x/auth/account.go @@ -295,12 +295,13 @@ func (bva *BaseVestingAccount) TrackUndelegation(amount sdk.Coins) { panic("undelegation attempt with zero coins") } delegatedFree := bva.DelegatedFree.AmountOf(coin.Denom) + delegatedVesting := bva.DelegatedVesting.AmountOf(coin.Denom) // compute x and y per the specification, where: // X := min(DF, D) - // Y := D - X + // Y := min(D - X, DV) x := sdk.MinInt(delegatedFree, coin.Amount) - y := coin.Amount.Sub(x) + y := sdk.MinInt(delegatedVesting, coin.Amount.Sub(x)) if !x.IsZero() { xCoin := sdk.NewCoin(coin.Denom, x) diff --git a/x/bank/keeper.go b/x/bank/keeper.go index de605f25a853..81db06387ddf 100644 --- a/x/bank/keeper.go +++ b/x/bank/keeper.go @@ -47,7 +47,13 @@ func NewBaseKeeper(ak auth.AccountKeeper, } // SetCoins sets the coins at the addr. -func (keeper BaseKeeper) SetCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) sdk.Error { +func (keeper BaseKeeper) SetCoins( + ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, +) sdk.Error { + + if !amt.IsValid() { + return sdk.ErrInvalidCoins(amt.String()) + } return setCoins(ctx, keeper.ak, addr, amt) } @@ -56,6 +62,9 @@ func (keeper BaseKeeper) SubtractCoins( ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, ) (sdk.Coins, sdk.Tags, sdk.Error) { + if !amt.IsValid() { + return nil, nil, sdk.ErrInvalidCoins(amt.String()) + } return subtractCoins(ctx, keeper.ak, addr, amt) } @@ -64,6 +73,9 @@ func (keeper BaseKeeper) AddCoins( ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, ) (sdk.Coins, sdk.Tags, sdk.Error) { + if !amt.IsValid() { + return nil, nil, sdk.ErrInvalidCoins(amt.String()) + } return addCoins(ctx, keeper.ak, addr, amt) } @@ -78,14 +90,27 @@ func (keeper BaseKeeper) InputOutputCoins( // DelegateCoins performs delegation by deducting amt coins from an account with // address addr. For vesting accounts, delegations amounts are tracked for both // vesting and vested coins. -func (keeper BaseKeeper) DelegateCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) { +func (keeper BaseKeeper) DelegateCoins( + ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, +) (sdk.Tags, sdk.Error) { + + if !amt.IsValid() { + return nil, sdk.ErrInvalidCoins(amt.String()) + } return delegateCoins(ctx, keeper.ak, addr, amt) } // UndelegateCoins performs undelegation by crediting amt coins to an account with // address addr. For vesting accounts, undelegation amounts are tracked for both // vesting and vested coins. -func (keeper BaseKeeper) UndelegateCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) { +// If any of the undelegation amounts are negative, an error is returned. +func (keeper BaseKeeper) UndelegateCoins( + ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins, +) (sdk.Tags, sdk.Error) { + + if !amt.IsValid() { + return nil, sdk.ErrInvalidCoins(amt.String()) + } return undelegateCoins(ctx, keeper.ak, addr, amt) } @@ -126,6 +151,10 @@ func NewBaseSendKeeper(ak auth.AccountKeeper, func (keeper BaseSendKeeper) SendCoins( ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins, ) (sdk.Tags, sdk.Error) { + + if !amt.IsValid() { + return nil, sdk.ErrInvalidCoins(amt.String()) + } return sendCoins(ctx, keeper.ak, fromAddr, toAddr, amt) } @@ -188,6 +217,9 @@ func getCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress) sdk.C } func setCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) sdk.Error { + if !amt.IsValid() { + return sdk.ErrInvalidCoins(amt.String()) + } acc := am.GetAccount(ctx, addr) if acc == nil { acc = am.NewAccountWithAddress(ctx, addr) @@ -218,6 +250,11 @@ func setAccount(ctx sdk.Context, ak auth.AccountKeeper, acc auth.Account) { // // CONTRACT: If the account is a vesting account, the amount has to be spendable. func subtractCoins(ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Tags, sdk.Error) { + + if !amt.IsValid() { + return nil, nil, sdk.ErrInvalidCoins(amt.String()) + } + oldCoins, spendableCoins := sdk.Coins{}, sdk.Coins{} acc := getAccount(ctx, ak, addr) @@ -244,6 +281,11 @@ func subtractCoins(ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress, // AddCoins adds amt to the coins at the addr. func addCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Tags, sdk.Error) { + + if !amt.IsValid() { + return nil, nil, sdk.ErrInvalidCoins(amt.String()) + } + oldCoins := getCoins(ctx, am, addr) newCoins := oldCoins.Add(amt) @@ -260,9 +302,9 @@ func addCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt s } // SendCoins moves coins from one account to another +// Returns ErrInvalidCoins if amt is invalid. func sendCoins(ctx sdk.Context, am auth.AccountKeeper, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) { // Safety check ensuring that when sending coins the keeper must maintain the - // supply invariant. if !amt.IsValid() { return nil, sdk.ErrInvalidCoins(amt.String()) } @@ -284,7 +326,7 @@ func sendCoins(ctx sdk.Context, am auth.AccountKeeper, fromAddr sdk.AccAddress, // NOTE: Make sure to revert state changes from tx on error func inputOutputCoins(ctx sdk.Context, am auth.AccountKeeper, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error) { // Safety check ensuring that when sending coins the keeper must maintain the - // supply invariant. + // Check supply invariant and validity of Coins. if err := ValidateInputsOutputs(inputs, outputs); err != nil { return nil, err } @@ -314,6 +356,10 @@ func delegateCoins( ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins, ) (sdk.Tags, sdk.Error) { + if !amt.IsValid() { + return nil, sdk.ErrInvalidCoins(amt.String()) + } + acc := getAccount(ctx, ak, addr) if acc == nil { return nil, sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr)) @@ -344,6 +390,10 @@ func undelegateCoins( ctx sdk.Context, ak auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins, ) (sdk.Tags, sdk.Error) { + if !amt.IsValid() { + return nil, sdk.ErrInvalidCoins(amt.String()) + } + acc := getAccount(ctx, ak, addr) if acc == nil { return nil, sdk.ErrUnknownAddress(fmt.Sprintf("account %s does not exist", addr)) @@ -361,22 +411,24 @@ func undelegateCoins( ), nil } -func trackDelegation(acc auth.Account, blockTime time.Time, amount sdk.Coins) error { +// CONTRACT: assumes that amt is valid. +func trackDelegation(acc auth.Account, blockTime time.Time, amt sdk.Coins) error { vacc, ok := acc.(auth.VestingAccount) if ok { - vacc.TrackDelegation(blockTime, amount) + vacc.TrackDelegation(blockTime, amt) return nil } - return acc.SetCoins(acc.GetCoins().Sub(amount)) + return acc.SetCoins(acc.GetCoins().Sub(amt)) } -func trackUndelegation(acc auth.Account, amount sdk.Coins) error { +// CONTRACT: assumes that amt is valid. +func trackUndelegation(acc auth.Account, amt sdk.Coins) error { vacc, ok := acc.(auth.VestingAccount) if ok { - vacc.TrackUndelegation(amount) + vacc.TrackUndelegation(amt) return nil } - return acc.SetCoins(acc.GetCoins().Add(amount)) + return acc.SetCoins(acc.GetCoins().Add(amt)) } diff --git a/x/bank/msgs.go b/x/bank/msgs.go index eb047d09fc6a..8e4b27633777 100644 --- a/x/bank/msgs.go +++ b/x/bank/msgs.go @@ -35,8 +35,8 @@ func (msg MsgSend) ValidateBasic() sdk.Error { if msg.ToAddress.Empty() { return sdk.ErrInvalidAddress("missing recipient address") } - if !msg.Amount.IsAllPositive() { - return sdk.ErrInsufficientCoins("send amount must be positive") + if !msg.Amount.IsValid() { + return sdk.ErrInvalidCoins(msg.Amount.String()) } return nil } @@ -112,7 +112,7 @@ func (in Input) ValidateBasic() sdk.Error { if !in.Coins.IsValid() { return sdk.ErrInvalidCoins(in.Coins.String()) } - if !in.Coins.IsAllPositive() { + if !in.Coins.IsValid() { return sdk.ErrInvalidCoins(in.Coins.String()) } return nil @@ -140,7 +140,7 @@ func (out Output) ValidateBasic() sdk.Error { if !out.Coins.IsValid() { return sdk.ErrInvalidCoins(out.Coins.String()) } - if !out.Coins.IsAllPositive() { + if !out.Coins.IsValid() { return sdk.ErrInvalidCoins(out.Coins.String()) } return nil From 2f9a0da7e085c34db5c14f0bdf8efa13535c14f3 Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 13:00:20 -0800 Subject: [PATCH 03/50] Fix tests --- Makefile | 2 +- x/bank/msgs.go | 9 ++++++--- x/staking/keeper/delegation_test.go | 28 ++++++++++++++-------------- x/staking/keeper/keeper_test.go | 4 ++-- x/staking/keeper/validator_test.go | 4 ++-- 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/Makefile b/Makefile index 0b8c041a9dbc..f4f2eb886b2b 100644 --- a/Makefile +++ b/Makefile @@ -152,7 +152,7 @@ test_ledger: @go test -v `go list github.com/cosmos/cosmos-sdk/crypto` -tags='cgo ledger' test_unit: - @VERSION=$(VERSION) go test $(PACKAGES_NOSIMULATION) -tags='ledger test_ledger_mock' + @VERSION=$(VERSION) GOCACHE=off go test $(PACKAGES_NOSIMULATION) -tags='ledger test_ledger_mock' test_race: @VERSION=$(VERSION) go test -race $(PACKAGES_NOSIMULATION) diff --git a/x/bank/msgs.go b/x/bank/msgs.go index 8e4b27633777..c9b4f554482b 100644 --- a/x/bank/msgs.go +++ b/x/bank/msgs.go @@ -36,7 +36,10 @@ func (msg MsgSend) ValidateBasic() sdk.Error { return sdk.ErrInvalidAddress("missing recipient address") } if !msg.Amount.IsValid() { - return sdk.ErrInvalidCoins(msg.Amount.String()) + return sdk.ErrInvalidCoins("send amount is invalid: " + msg.Amount.String()) + } + if !msg.Amount.IsAllPositive() { + return sdk.ErrInsufficientCoins("send amount must be positive") } return nil } @@ -112,7 +115,7 @@ func (in Input) ValidateBasic() sdk.Error { if !in.Coins.IsValid() { return sdk.ErrInvalidCoins(in.Coins.String()) } - if !in.Coins.IsValid() { + if !in.Coins.IsAllPositive() { return sdk.ErrInvalidCoins(in.Coins.String()) } return nil @@ -140,7 +143,7 @@ func (out Output) ValidateBasic() sdk.Error { if !out.Coins.IsValid() { return sdk.ErrInvalidCoins(out.Coins.String()) } - if !out.Coins.IsValid() { + if !out.Coins.IsAllPositive() { return sdk.ErrInvalidCoins(out.Coins.String()) } return nil diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go index 1c1d8dcb143c..370296faa331 100644 --- a/x/staking/keeper/delegation_test.go +++ b/x/staking/keeper/delegation_test.go @@ -130,7 +130,7 @@ func TestDelegation(t *testing.T) { // tests Get/Set/Remove UnbondingDelegation func TestUnbondingDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) ubd := types.NewUnbondingDelegation(addrDels[0], addrVals[0], 0, time.Unix(0, 0), sdk.NewInt(5)) @@ -169,7 +169,7 @@ func TestUnbondingDelegation(t *testing.T) { } func TestUnbondDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(10) pool.NotBondedTokens = startTokens @@ -207,7 +207,7 @@ func TestUnbondDelegation(t *testing.T) { } func TestUnbondingDelegationsMaxEntries(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(10) pool.NotBondedTokens = startTokens @@ -254,7 +254,7 @@ func TestUnbondingDelegationsMaxEntries(t *testing.T) { // shift it from the bonded to unbonding state and jailed func TestUndelegateSelfDelegationBelowMinSelfDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -300,7 +300,7 @@ func TestUndelegateSelfDelegationBelowMinSelfDelegation(t *testing.T) { } func TestUndelegateFromUnbondingValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -372,7 +372,7 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) { } func TestUndelegateFromUnbondedValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -447,7 +447,7 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { } func TestUnbondingAllDelegationFromValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -507,7 +507,7 @@ func TestUnbondingAllDelegationFromValidator(t *testing.T) { // Make sure that that the retrieving the delegations doesn't affect the state func TestGetRedelegationsFromValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 0, time.Unix(0, 0), sdk.NewInt(5), @@ -531,7 +531,7 @@ func TestGetRedelegationsFromValidator(t *testing.T) { // tests Get/Set/Remove/Has UnbondingDelegation func TestRedelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 0, time.Unix(0, 0), sdk.NewInt(5), @@ -591,7 +591,7 @@ func TestRedelegation(t *testing.T) { } func TestRedelegateToSameValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens @@ -614,7 +614,7 @@ func TestRedelegateToSameValidator(t *testing.T) { } func TestRedelegationMaxEntries(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -665,7 +665,7 @@ func TestRedelegationMaxEntries(t *testing.T) { } func TestRedelegateSelfDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens @@ -716,7 +716,7 @@ func TestRedelegateSelfDelegation(t *testing.T) { } func TestRedelegateFromUnbondingValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens @@ -795,7 +795,7 @@ func TestRedelegateFromUnbondingValidator(t *testing.T) { } func TestRedelegateFromUnbondedValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens diff --git a/x/staking/keeper/keeper_test.go b/x/staking/keeper/keeper_test.go index 89ffe13d19ee..0ba9d892ef11 100644 --- a/x/staking/keeper/keeper_test.go +++ b/x/staking/keeper/keeper_test.go @@ -10,7 +10,7 @@ import ( ) func TestParams(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) expParams := types.DefaultParams() //check that the empty keeper loads the default @@ -25,7 +25,7 @@ func TestParams(t *testing.T) { } func TestPool(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) expPool := types.InitialPool() //check that the empty keeper loads the default diff --git a/x/staking/keeper/validator_test.go b/x/staking/keeper/validator_test.go index 4b07816725c4..fde49fd0f0bf 100644 --- a/x/staking/keeper/validator_test.go +++ b/x/staking/keeper/validator_test.go @@ -72,7 +72,7 @@ func TestSetValidator(t *testing.T) { } func TestUpdateValidatorByPowerIndex(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) // create a random pool @@ -115,7 +115,7 @@ func TestUpdateBondedValidatorsDecreaseCliff(t *testing.T) { maxVals := 5 // create context, keeper, and pool for tests - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) // create keeper parameters From c93803919f0b917c2078fcb22c613a30c842779a Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 13:10:24 -0800 Subject: [PATCH 04/50] Revert "Fix tests" This reverts commit 2f9a0da7e085c34db5c14f0bdf8efa13535c14f3. --- Makefile | 2 +- x/bank/msgs.go | 9 +++------ x/staking/keeper/delegation_test.go | 28 ++++++++++++++-------------- x/staking/keeper/keeper_test.go | 4 ++-- x/staking/keeper/validator_test.go | 4 ++-- 5 files changed, 22 insertions(+), 25 deletions(-) diff --git a/Makefile b/Makefile index f4f2eb886b2b..0b8c041a9dbc 100644 --- a/Makefile +++ b/Makefile @@ -152,7 +152,7 @@ test_ledger: @go test -v `go list github.com/cosmos/cosmos-sdk/crypto` -tags='cgo ledger' test_unit: - @VERSION=$(VERSION) GOCACHE=off go test $(PACKAGES_NOSIMULATION) -tags='ledger test_ledger_mock' + @VERSION=$(VERSION) go test $(PACKAGES_NOSIMULATION) -tags='ledger test_ledger_mock' test_race: @VERSION=$(VERSION) go test -race $(PACKAGES_NOSIMULATION) diff --git a/x/bank/msgs.go b/x/bank/msgs.go index c9b4f554482b..8e4b27633777 100644 --- a/x/bank/msgs.go +++ b/x/bank/msgs.go @@ -36,10 +36,7 @@ func (msg MsgSend) ValidateBasic() sdk.Error { return sdk.ErrInvalidAddress("missing recipient address") } if !msg.Amount.IsValid() { - return sdk.ErrInvalidCoins("send amount is invalid: " + msg.Amount.String()) - } - if !msg.Amount.IsAllPositive() { - return sdk.ErrInsufficientCoins("send amount must be positive") + return sdk.ErrInvalidCoins(msg.Amount.String()) } return nil } @@ -115,7 +112,7 @@ func (in Input) ValidateBasic() sdk.Error { if !in.Coins.IsValid() { return sdk.ErrInvalidCoins(in.Coins.String()) } - if !in.Coins.IsAllPositive() { + if !in.Coins.IsValid() { return sdk.ErrInvalidCoins(in.Coins.String()) } return nil @@ -143,7 +140,7 @@ func (out Output) ValidateBasic() sdk.Error { if !out.Coins.IsValid() { return sdk.ErrInvalidCoins(out.Coins.String()) } - if !out.Coins.IsAllPositive() { + if !out.Coins.IsValid() { return sdk.ErrInvalidCoins(out.Coins.String()) } return nil diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go index 370296faa331..1c1d8dcb143c 100644 --- a/x/staking/keeper/delegation_test.go +++ b/x/staking/keeper/delegation_test.go @@ -130,7 +130,7 @@ func TestDelegation(t *testing.T) { // tests Get/Set/Remove UnbondingDelegation func TestUnbondingDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) ubd := types.NewUnbondingDelegation(addrDels[0], addrVals[0], 0, time.Unix(0, 0), sdk.NewInt(5)) @@ -169,7 +169,7 @@ func TestUnbondingDelegation(t *testing.T) { } func TestUnbondDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(10) pool.NotBondedTokens = startTokens @@ -207,7 +207,7 @@ func TestUnbondDelegation(t *testing.T) { } func TestUnbondingDelegationsMaxEntries(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(10) pool.NotBondedTokens = startTokens @@ -254,7 +254,7 @@ func TestUnbondingDelegationsMaxEntries(t *testing.T) { // shift it from the bonded to unbonding state and jailed func TestUndelegateSelfDelegationBelowMinSelfDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -300,7 +300,7 @@ func TestUndelegateSelfDelegationBelowMinSelfDelegation(t *testing.T) { } func TestUndelegateFromUnbondingValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -372,7 +372,7 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) { } func TestUndelegateFromUnbondedValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -447,7 +447,7 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { } func TestUnbondingAllDelegationFromValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -507,7 +507,7 @@ func TestUnbondingAllDelegationFromValidator(t *testing.T) { // Make sure that that the retrieving the delegations doesn't affect the state func TestGetRedelegationsFromValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 0, time.Unix(0, 0), sdk.NewInt(5), @@ -531,7 +531,7 @@ func TestGetRedelegationsFromValidator(t *testing.T) { // tests Get/Set/Remove/Has UnbondingDelegation func TestRedelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) rd := types.NewRedelegation(addrDels[0], addrVals[0], addrVals[1], 0, time.Unix(0, 0), sdk.NewInt(5), @@ -591,7 +591,7 @@ func TestRedelegation(t *testing.T) { } func TestRedelegateToSameValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens @@ -614,7 +614,7 @@ func TestRedelegateToSameValidator(t *testing.T) { } func TestRedelegationMaxEntries(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens @@ -665,7 +665,7 @@ func TestRedelegationMaxEntries(t *testing.T) { } func TestRedelegateSelfDelegation(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens @@ -716,7 +716,7 @@ func TestRedelegateSelfDelegation(t *testing.T) { } func TestRedelegateFromUnbondingValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens @@ -795,7 +795,7 @@ func TestRedelegateFromUnbondingValidator(t *testing.T) { } func TestRedelegateFromUnbondedValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(30) pool.NotBondedTokens = startTokens diff --git a/x/staking/keeper/keeper_test.go b/x/staking/keeper/keeper_test.go index 0ba9d892ef11..89ffe13d19ee 100644 --- a/x/staking/keeper/keeper_test.go +++ b/x/staking/keeper/keeper_test.go @@ -10,7 +10,7 @@ import ( ) func TestParams(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) expParams := types.DefaultParams() //check that the empty keeper loads the default @@ -25,7 +25,7 @@ func TestParams(t *testing.T) { } func TestPool(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) expPool := types.InitialPool() //check that the empty keeper loads the default diff --git a/x/staking/keeper/validator_test.go b/x/staking/keeper/validator_test.go index fde49fd0f0bf..4b07816725c4 100644 --- a/x/staking/keeper/validator_test.go +++ b/x/staking/keeper/validator_test.go @@ -72,7 +72,7 @@ func TestSetValidator(t *testing.T) { } func TestUpdateValidatorByPowerIndex(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) // create a random pool @@ -115,7 +115,7 @@ func TestUpdateBondedValidatorsDecreaseCliff(t *testing.T) { maxVals := 5 // create context, keeper, and pool for tests - ctx, _, keeper := CreateTestInput(t, false, 1) + ctx, _, keeper := CreateTestInput(t, false, 0) pool := keeper.GetPool(ctx) // create keeper parameters From 066e11f190b3a0a4677d24b8faee2a9b2713caaf Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 13:28:50 -0800 Subject: [PATCH 05/50] fix tests --- x/bank/msgs.go | 9 ++++++--- x/staking/keeper/delegation_test.go | 4 ++-- x/staking/keeper/keeper_test.go | 4 ++-- x/staking/keeper/test_common.go | 9 ++++++--- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/x/bank/msgs.go b/x/bank/msgs.go index 8e4b27633777..c9b4f554482b 100644 --- a/x/bank/msgs.go +++ b/x/bank/msgs.go @@ -36,7 +36,10 @@ func (msg MsgSend) ValidateBasic() sdk.Error { return sdk.ErrInvalidAddress("missing recipient address") } if !msg.Amount.IsValid() { - return sdk.ErrInvalidCoins(msg.Amount.String()) + return sdk.ErrInvalidCoins("send amount is invalid: " + msg.Amount.String()) + } + if !msg.Amount.IsAllPositive() { + return sdk.ErrInsufficientCoins("send amount must be positive") } return nil } @@ -112,7 +115,7 @@ func (in Input) ValidateBasic() sdk.Error { if !in.Coins.IsValid() { return sdk.ErrInvalidCoins(in.Coins.String()) } - if !in.Coins.IsValid() { + if !in.Coins.IsAllPositive() { return sdk.ErrInvalidCoins(in.Coins.String()) } return nil @@ -140,7 +143,7 @@ func (out Output) ValidateBasic() sdk.Error { if !out.Coins.IsValid() { return sdk.ErrInvalidCoins(out.Coins.String()) } - if !out.Coins.IsValid() { + if !out.Coins.IsAllPositive() { return sdk.ErrInvalidCoins(out.Coins.String()) } return nil diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go index 1c1d8dcb143c..bc9d8e72bfae 100644 --- a/x/staking/keeper/delegation_test.go +++ b/x/staking/keeper/delegation_test.go @@ -207,7 +207,7 @@ func TestUnbondDelegation(t *testing.T) { } func TestUnbondingDelegationsMaxEntries(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(10) pool.NotBondedTokens = startTokens @@ -372,7 +372,7 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) { } func TestUndelegateFromUnbondedValidator(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + ctx, _, keeper := CreateTestInput(t, false, 1) pool := keeper.GetPool(ctx) startTokens := sdk.TokensFromTendermintPower(20) pool.NotBondedTokens = startTokens diff --git a/x/staking/keeper/keeper_test.go b/x/staking/keeper/keeper_test.go index 89ffe13d19ee..30feb8b284ce 100644 --- a/x/staking/keeper/keeper_test.go +++ b/x/staking/keeper/keeper_test.go @@ -30,11 +30,11 @@ func TestPool(t *testing.T) { //check that the empty keeper loads the default resPool := keeper.GetPool(ctx) - require.True(t, expPool.Equal(resPool)) + require.Equal(t, expPool, resPool) //modify a params, save, and retrieve expPool.BondedTokens = sdk.NewInt(777) keeper.SetPool(ctx, expPool) resPool = keeper.GetPool(ctx) - require.True(t, expPool.Equal(resPool)) + require.Equal(t, expPool, resPool) } diff --git a/x/staking/keeper/test_common.go b/x/staking/keeper/test_common.go index 853583acb7cc..1c722c1dbda8 100644 --- a/x/staking/keeper/test_common.go +++ b/x/staking/keeper/test_common.go @@ -128,9 +128,12 @@ func CreateTestInput(t *testing.T, isCheckTx bool, initPower int64) (sdk.Context // fill all the addresses with some coins, set the loose pool tokens simultaneously for _, addr := range Addrs { pool := keeper.GetPool(ctx) - _, _, err := ck.AddCoins(ctx, addr, sdk.Coins{ - {keeper.BondDenom(ctx), initCoins}, - }) + err := error(nil) + if !initCoins.IsZero() { + _, _, err = ck.AddCoins(ctx, addr, sdk.Coins{ + {keeper.BondDenom(ctx), initCoins}, + }) + } require.Nil(t, err) pool.NotBondedTokens = pool.NotBondedTokens.Add(initCoins) keeper.SetPool(ctx, pool) From f7c3cc60b9d50b0bcffa5684a59e7d3a6498da89 Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 13:33:42 -0800 Subject: [PATCH 06/50] Add comment on new behavior --- x/staking/keeper/test_common.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x/staking/keeper/test_common.go b/x/staking/keeper/test_common.go index 1c722c1dbda8..5fe9887bccd9 100644 --- a/x/staking/keeper/test_common.go +++ b/x/staking/keeper/test_common.go @@ -74,8 +74,9 @@ func MakeTestCodec() *codec.Codec { return cdc } -// hogpodge of all sorts of input required for testing -// init power is converted to an amount of tokens +// Hogpodge of all sorts of input required for testing. +// `initPower` is converted to an amount of tokens. +// If `initPower` is 0, no addrs get created. func CreateTestInput(t *testing.T, isCheckTx bool, initPower int64) (sdk.Context, auth.AccountKeeper, Keeper) { initCoins := sdk.TokensFromTendermintPower(initPower) From 637902d7d6682659733c0fbf41af7f30275f66b1 Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 14:15:17 -0800 Subject: [PATCH 07/50] Call AddCoins conditionally --- x/distribution/keeper/delegation.go | 8 +++++--- x/distribution/keeper/hooks.go | 10 ++++++---- x/distribution/keeper/keeper.go | 10 ++++++---- x/ibc/handler.go | 1 + 4 files changed, 18 insertions(+), 11 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index af140474cb85..d63d8cd2632a 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -107,9 +107,11 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de k.SetFeePool(ctx, feePool) // add coins to user account - withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, del.GetDelegatorAddr()) - if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil { - return err + if !coins.IsZero() { + withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, del.GetDelegatorAddr()) + if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil { + return err + } } // remove delegator starting info diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 0466e8f7ea33..a0534bf59e21 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -37,11 +37,13 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr h.k.SetOutstandingRewards(ctx, outstanding.Sub(commission)) // add to validator account - accAddr := sdk.AccAddress(valAddr) - withdrawAddr := h.k.GetDelegatorWithdrawAddr(ctx, accAddr) + if !coins.IsZero() { + accAddr := sdk.AccAddress(valAddr) + withdrawAddr := h.k.GetDelegatorWithdrawAddr(ctx, accAddr) - if _, _, err := h.k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil { - panic(err) + if _, _, err := h.k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil { + panic(err) + } } } // remove commission record diff --git a/x/distribution/keeper/keeper.go b/x/distribution/keeper/keeper.go index 4a57c9bba663..40fe13e8ad6f 100644 --- a/x/distribution/keeper/keeper.go +++ b/x/distribution/keeper/keeper.go @@ -87,11 +87,13 @@ func (k Keeper) WithdrawValidatorCommission(ctx sdk.Context, valAddr sdk.ValAddr outstanding := k.GetOutstandingRewards(ctx) k.SetOutstandingRewards(ctx, outstanding.Sub(sdk.NewDecCoins(coins))) - accAddr := sdk.AccAddress(valAddr) - withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, accAddr) + if !coins.IsZero() { + accAddr := sdk.AccAddress(valAddr) + withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, accAddr) - if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil { - return err + if _, _, err := k.bankKeeper.AddCoins(ctx, withdrawAddr, coins); err != nil { + return err + } } return nil diff --git a/x/ibc/handler.go b/x/ibc/handler.go index afc3027682de..ddbbd3d72294 100644 --- a/x/ibc/handler.go +++ b/x/ibc/handler.go @@ -44,6 +44,7 @@ func handleIBCReceiveMsg(ctx sdk.Context, ibcm Mapper, ck BankKeeper, msg IBCRec return ErrInvalidSequence(ibcm.codespace).Result() } + // XXX Check that packet.Coins is valid and positive (nonzero) _, _, err := ck.AddCoins(ctx, packet.DestAddr, packet.Coins) if err != nil { return err.Result() From 71774b82665cbbd60288da244eeb4e47a7129e6b Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 17:45:59 -0800 Subject: [PATCH 08/50] Do not panic with unknown evidence --- x/slashing/keeper.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index de3751a157c2..a042eb46613a 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -47,7 +47,16 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio consAddr := sdk.ConsAddress(addr) pubkey, err := k.getPubkey(ctx, addr) if err != nil { - panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr)) + // ignore evidence that cannot be handled. + return + // NOTE: + // We used to panic with: + // `panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))`, + // but this couples the expectations of the app to both Tendermint and + // the simulator. Both are expected to provide the full range of + // allowable but none of the disallowed evidence types. Instead of + // getting this coordination right, it is easier to relax the + // constraints and ignore evidence that cannot be handled. } // Reject evidence if the double-sign is too old From d9b5071514afdcf093c0cc2c2723ac612fdb3eec Mon Sep 17 00:00:00 2001 From: jaekwon Date: Fri, 22 Feb 2019 18:19:59 -0800 Subject: [PATCH 09/50] update comments from bez's branch --- PENDING.md | 5 +++++ docs/spec/auth/05_vesting.md | 4 ++++ x/auth/account.go | 6 +++++- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/PENDING.md b/PENDING.md index 60fafbdc1670..3e2e8367de8c 100644 --- a/PENDING.md +++ b/PENDING.md @@ -72,4 +72,9 @@ decoded automatically. * [\#3411] Include the `RequestInitChain.Time` in the block header init during `InitChain`. +* Update the vesting specification and implementation to cap deduction from +`DelegatedVesting` by at most `DelegatedVesting`. This accounts for the case where +the undelegation amount may exceed the original delegation amount due to +truncation of undelegation tokens. + ### Tendermint diff --git a/docs/spec/auth/05_vesting.md b/docs/spec/auth/05_vesting.md index 4b435f9f6cd9..727a31c20d01 100644 --- a/docs/spec/auth/05_vesting.md +++ b/docs/spec/auth/05_vesting.md @@ -276,6 +276,10 @@ func (cva ContinuousVestingAccount) TrackUndelegation(amount Coins) { with an excess `DV` amount, even after all its coins have vested. This is because undelegating free coins are prioritized. +**Note**: The undelegation (bond refund) amount may exceed the delegated +vesting (bond) amount due to the way undelegation truncates the bond refund, +which increases the validator's exchange rate (tokens/shares) slightly. + #### Keepers/Handlers ```go diff --git a/x/auth/account.go b/x/auth/account.go index 6f10fa080d0e..b4eb5ae22a3d 100644 --- a/x/auth/account.go +++ b/x/auth/account.go @@ -287,6 +287,10 @@ func (bva *BaseVestingAccount) trackDelegation(vestingCoins, amount sdk.Coins) { // by which amount the base coins need to increase. The resulting base coins are // returned. // +// NOTE: The undelegation (bond refund) amount may exceed the delegated vesting +// (bond) amount due to the way undelegation truncates the bond refund, which +// increases the validator's exchange rate (tokens/shares) slightly. +// // CONTRACT: The account's coins and undelegation coins must be sorted. func (bva *BaseVestingAccount) TrackUndelegation(amount sdk.Coins) { for _, coin := range amount { @@ -299,7 +303,7 @@ func (bva *BaseVestingAccount) TrackUndelegation(amount sdk.Coins) { // compute x and y per the specification, where: // X := min(DF, D) - // Y := min(D - X, DV) + // Y := min(DV, D - X) x := sdk.MinInt(delegatedFree, coin.Amount) y := sdk.MinInt(delegatedVesting, coin.Amount.Sub(x)) From 0c0b14261b9035393c1b2c87237e1cffcdaa2f0a Mon Sep 17 00:00:00 2001 From: jaekwon Date: Sat, 23 Feb 2019 16:23:52 -0800 Subject: [PATCH 10/50] fixing more sim issues --- cmd/gaia/app/sim_test.go | 2 +- x/gov/simulation/msgs.go | 2 +- x/mock/simulation/mock_tendermint.go | 8 ++++++++ x/slashing/keeper.go | 2 +- x/staking/simulation/msgs.go | 11 ++++++++++- 5 files changed, 21 insertions(+), 4 deletions(-) diff --git a/cmd/gaia/app/sim_test.go b/cmd/gaia/app/sim_test.go index 7e4d465fe66b..79684571ffe5 100644 --- a/cmd/gaia/app/sim_test.go +++ b/cmd/gaia/app/sim_test.go @@ -172,7 +172,7 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest Pool: staking.InitialPool(), Params: staking.Params{ UnbondingTime: time.Duration(randIntBetween(r, 60, 60*60*24*3*2)) * time.Second, - MaxValidators: uint16(r.Intn(250)), + MaxValidators: uint16(r.Intn(250) + 1), BondDenom: sdk.DefaultBondDenom, }, } diff --git a/x/gov/simulation/msgs.go b/x/gov/simulation/msgs.go index d3707d525baf..551ed6538655 100644 --- a/x/gov/simulation/msgs.go +++ b/x/gov/simulation/msgs.go @@ -183,7 +183,7 @@ func randomDeposit(r *rand.Rand) sdk.Coins { // Pick a random proposal ID func randomProposalID(r *rand.Rand, k gov.Keeper, ctx sdk.Context) (proposalID uint64, ok bool) { lastProposalID := k.GetLastProposalID(ctx) - if lastProposalID < 1 { + if lastProposalID < 1 || lastProposalID == (2<<63-1) { return 0, false } proposalID = uint64(r.Intn(1+int(lastProposalID)) - 1) diff --git a/x/mock/simulation/mock_tendermint.go b/x/mock/simulation/mock_tendermint.go index 54e38d5c7830..e241e9c67288 100644 --- a/x/mock/simulation/mock_tendermint.go +++ b/x/mock/simulation/mock_tendermint.go @@ -17,6 +17,14 @@ type mockValidator struct { livenessState int } +func (mv mockValidator) String() string { + return fmt.Sprintf("mockValidator{%s:%X power:%v state:%v}", + mv.val.PubKey.Type, + mv.val.PubKey.Data, + mv.val.Power, + mv.livenessState) +} + type mockValidators map[string]mockValidator // get mockValidators from abci validators diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index a042eb46613a..16ec2ed31520 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -163,7 +163,7 @@ func (k Keeper) handleValidatorSignature(ctx sdk.Context, addr crypto.Address, p } if missed { - logger.Info(fmt.Sprintf("Absent validator %s at height %d, %d missed, threshold %d", addr, height, signInfo.MissedBlocksCounter, k.MinSignedPerWindow(ctx))) + logger.Info(fmt.Sprintf("Absent validator %s (%v) at height %d, %d missed, threshold %d", addr, pubkey, height, signInfo.MissedBlocksCounter, k.MinSignedPerWindow(ctx))) } minHeight := signInfo.StartHeight + k.SignedBlocksWindow(ctx) diff --git a/x/staking/simulation/msgs.go b/x/staking/simulation/msgs.go index d1735482d631..8ab64de89040 100644 --- a/x/staking/simulation/msgs.go +++ b/x/staking/simulation/msgs.go @@ -82,6 +82,9 @@ func SimulateMsgEditValidator(k staking.Keeper) simulation.Operation { Details: simulation.RandStringOfLength(r, 10), } + if len(k.GetAllValidators(ctx)) == 0 { + return noOperation, nil, nil + } val := keeper.RandomValidator(r, k, ctx) address := val.GetOperator() newCommissionRate := simulation.RandomDecAmount(r, val.Commission.MaxRate) @@ -110,6 +113,9 @@ func SimulateMsgDelegate(m auth.AccountKeeper, k staking.Keeper) simulation.Oper action string, fOp []simulation.FutureOperation, err error) { denom := k.GetParams(ctx).BondDenom + if len(k.GetAllValidators(ctx)) == 0 { + return noOperation, nil, nil + } val := keeper.RandomValidator(r, k, ctx) validatorAddress := val.GetOperator() delegatorAcc := simulation.RandomAcc(r, accs) @@ -119,7 +125,7 @@ func SimulateMsgDelegate(m auth.AccountKeeper, k staking.Keeper) simulation.Oper amount = simulation.RandomAmount(r, amount) } if amount.Equal(sdk.ZeroInt()) { - return "no-operation", nil, nil + return noOperation, nil, nil } msg := staking.NewMsgDelegate( @@ -186,6 +192,9 @@ func SimulateMsgBeginRedelegate(m auth.AccountKeeper, k staking.Keeper) simulati action string, fOp []simulation.FutureOperation, err error) { denom := k.GetParams(ctx).BondDenom + if len(k.GetAllValidators(ctx)) == 0 { + return noOperation, nil, nil + } srcVal := keeper.RandomValidator(r, k, ctx) srcValidatorAddress := srcVal.GetOperator() destVal := keeper.RandomValidator(r, k, ctx) From aca59ac1f8608156a44faace31a431c68d6f4b43 Mon Sep 17 00:00:00 2001 From: jaekwon Date: Sun, 24 Feb 2019 13:25:49 -0800 Subject: [PATCH 11/50] Ignore unknown proposer --- x/distribution/keeper/allocation.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 646784204cfc..dc2bf01da705 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -35,9 +35,17 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in proposerReward := feesCollected.MulDec(proposerMultiplier) // pay proposer + remaining := feesCollected proposerValidator := k.stakingKeeper.ValidatorByConsAddr(ctx, proposer) - k.AllocateTokensToValidator(ctx, proposerValidator, proposerReward) - remaining := feesCollected.Sub(proposerReward) + if proposerValidator != nil { + k.AllocateTokensToValidator(ctx, proposerValidator, proposerReward) + remaining = feesCollected.Sub(proposerReward) + } else { + // proposer can be unknown if say, the unbonding period is 1 block, so + // e.g. a validator undelegates at block X, it's removed entirely by + // block X+1's endblock, then X+2 we need to refer to the previous + // proposer for X+1, but we've forgotten about them. + } // calculate fraction allocated to validators communityTax := k.GetCommunityTax(ctx) From 002e49f651250717409b6f615d691b458cc68c2e Mon Sep 17 00:00:00 2001 From: jaekwon Date: Sun, 24 Feb 2019 23:17:45 -0800 Subject: [PATCH 12/50] Add bias to return 0 to RandomAmount --- x/mock/simulation/rand_util.go | 43 +++++++++++++++------------------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/x/mock/simulation/rand_util.go b/x/mock/simulation/rand_util.go index 4301781cd685..1a5e4566f652 100644 --- a/x/mock/simulation/rand_util.go +++ b/x/mock/simulation/rand_util.go @@ -1,7 +1,6 @@ package simulation import ( - "fmt" "math/big" "math/rand" "time" @@ -38,37 +37,33 @@ func RandStringOfLength(r *rand.Rand, n int) string { } // Generate a random amount -// Note: The range of RandomAmount includes max, and is, in fact, biased to return max. +// Note: The range of RandomAmount includes max, and is, in fact, biased to return max as well as 0. func RandomAmount(r *rand.Rand, max sdk.Int) sdk.Int { - // return sdk.NewInt(int64(r.Intn(int(max.Int64())))) - max2 := big.NewInt(0).Mul(max.BigInt(), big.NewInt(2)) - randInt := big.NewInt(0).Rand(r, max2) - if randInt.Cmp(max.BigInt()) > 0 { + var randInt = big.NewInt(0) + switch r.Intn(10) { + case 0: + // randInt = big.NewInt(0) + case 1: randInt = max.BigInt() + default: // NOTE: there are 10 total cases. + randInt = big.NewInt(0).Rand(r, max.BigInt()) // up to max - 1 } - result := sdk.NewIntFromBigInt(randInt) - // Sanity - if result.GT(max) { - panic(fmt.Sprintf("%v > %v", result, max)) - } - return result + return sdk.NewIntFromBigInt(randInt) } // RandomDecAmount generates a random decimal amount -// Note: The range of RandomDecAmount includes max, and is, in fact, biased to return max. +// Note: The range of RandomDecAmount includes max, and is, in fact, biased to return max as well as 0. func RandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec { - // randInt := big.NewInt(0).Rand(r, max.Int) - max2 := big.NewInt(0).Mul(max.Int, big.NewInt(2)) - randInt := big.NewInt(0).Rand(r, max2) - if randInt.Cmp(max.Int) > 0 { - randInt = max.Int - } - result := sdk.NewDecFromBigIntWithPrec(randInt, sdk.Precision) - // Sanity - if result.GT(max) { - panic(fmt.Sprintf("%v > %v", result, max)) + var randInt = big.NewInt(0) + switch r.Intn(10) { + case 0: + // randInt = big.NewInt(0) + case 1: + randInt = max.Int // the underlying big int with all precision bits. + default: // NOTE: there are 10 total cases. + randInt = big.NewInt(0).Rand(r, max.Int) } - return result + return sdk.NewDecFromBigIntWithPrec(randInt, sdk.Precision) } // RandomSetGenesis wraps mock.RandomSetGenesis, but using simulation accounts From 89e84f524998d317f7a2adc1ea6e37c6f8d7be1f Mon Sep 17 00:00:00 2001 From: Hleb Albau Date: Sun, 24 Feb 2019 12:50:35 +0700 Subject: [PATCH 13/50] Negative coins amount during fees distribution --- x/distribution/keeper/allocation_test.go | 31 ++++++++++++++++++------ 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/x/distribution/keeper/allocation_test.go b/x/distribution/keeper/allocation_test.go index d2c354a1050d..74153fc4353f 100644 --- a/x/distribution/keeper/allocation_test.go +++ b/x/distribution/keeper/allocation_test.go @@ -41,31 +41,42 @@ func TestAllocateTokensToValidatorWithCommission(t *testing.T) { } func TestAllocateTokensToManyValidators(t *testing.T) { - ctx, _, k, sk, fck := CreateTestInputDefault(t, false, 1000) + communityTax := sdk.NewDec(0) + ctx, _, k, sk, fck := CreateTestInputAdvanced(t, false, 1000000, communityTax) sh := staking.NewHandler(sk) // initialize state k.SetOutstandingRewards(ctx, sdk.DecCoins{}) // create validator with 50% commission - commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) + commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(1, 1), sdk.NewDecWithPrec(1, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, - sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, commission, sdk.OneInt()) + sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(110)), staking.Description{}, commission, sdk.OneInt()) require.True(t, sh(ctx, msg).IsOK()) // create second validator with 0% commission - commission = staking.NewCommissionMsg(sdk.NewDec(0), sdk.NewDec(0), sdk.NewDec(0)) + commission = staking.NewCommissionMsg(sdk.NewDecWithPrec(1, 1), sdk.NewDecWithPrec(1, 1), sdk.NewDec(0)) msg = staking.NewMsgCreateValidator(valOpAddr2, valConsPk2, sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, commission, sdk.OneInt()) require.True(t, sh(ctx, msg).IsOK()) + // create second validator with 0% commission + commission = staking.NewCommissionMsg(sdk.NewDecWithPrec(1, 1), sdk.NewDecWithPrec(1, 1), sdk.NewDec(0)) + msg = staking.NewMsgCreateValidator(valOpAddr3, valConsPk3, + sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, commission, sdk.OneInt()) + require.True(t, sh(ctx, msg).IsOK()) + abciValA := abci.Validator{ Address: valConsPk1.Address(), - Power: 100, + Power: 11, } abciValB := abci.Validator{ Address: valConsPk2.Address(), - Power: 100, + Power: 10, + } + abciValС := abci.Validator{ + Address: valConsPk3.Address(), + Power: 10, } // assert initial state: zero outstanding rewards, zero community pool, zero commission, zero current rewards @@ -78,7 +89,7 @@ func TestAllocateTokensToManyValidators(t *testing.T) { // allocate tokens as if both had voted and second was proposer fees := sdk.Coins{ - {sdk.DefaultBondDenom, sdk.NewInt(100)}, + {sdk.DefaultBondDenom, sdk.NewInt(634195840)}, } fck.SetCollectedFees(fees) votes := []abci.VoteInfo{ @@ -90,8 +101,12 @@ func TestAllocateTokensToManyValidators(t *testing.T) { Validator: abciValB, SignedLastBlock: true, }, + { + Validator: abciValС, + SignedLastBlock: true, + }, } - k.AllocateTokens(ctx, 200, 200, valConsAddr2, votes) + k.AllocateTokens(ctx, 31, 31, valConsAddr2, votes) // 98 outstanding rewards (100 less 2 to community pool) require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(98)}}, k.GetOutstandingRewards(ctx)) From d25f793c2b557b6a5db66475b501dcc70834eae4 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 25 Feb 2019 16:03:55 +0100 Subject: [PATCH 14/50] Truncate multiplication & division; add back old testcase --- x/distribution/keeper/allocation.go | 8 +-- x/distribution/keeper/allocation_test.go | 80 ++++++++++++++++++++---- 2 files changed, 72 insertions(+), 16 deletions(-) diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 646784204cfc..3f6c423c0291 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -31,8 +31,8 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // calculate proposer reward baseProposerReward := k.GetBaseProposerReward(ctx) bonusProposerReward := k.GetBonusProposerReward(ctx) - proposerMultiplier := baseProposerReward.Add(bonusProposerReward.Mul(fractionVotes)) - proposerReward := feesCollected.MulDec(proposerMultiplier) + proposerMultiplier := baseProposerReward.Add(bonusProposerReward.MulTruncate(fractionVotes)) + proposerReward := feesCollected.MulDecTruncate(proposerMultiplier) // pay proposer proposerValidator := k.stakingKeeper.ValidatorByConsAddr(ctx, proposer) @@ -50,8 +50,8 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // TODO likely we should only reward validators who actually signed the block. // ref https://github.com/cosmos/cosmos-sdk/issues/2525#issuecomment-430838701 - powerFraction := sdk.NewDec(vote.Validator.Power).Quo(sdk.NewDec(totalPower)) - reward := feesCollected.MulDec(voteMultiplier).MulDec(powerFraction) + powerFraction := sdk.NewDec(vote.Validator.Power).QuoTruncate(sdk.NewDec(totalPower)) + reward := feesCollected.MulDecTruncate(voteMultiplier).MulDecTruncate(powerFraction) k.AllocateTokensToValidator(ctx, validator, reward) remaining = remaining.Sub(reward) } diff --git a/x/distribution/keeper/allocation_test.go b/x/distribution/keeper/allocation_test.go index 74153fc4353f..74e24e541787 100644 --- a/x/distribution/keeper/allocation_test.go +++ b/x/distribution/keeper/allocation_test.go @@ -41,6 +41,73 @@ func TestAllocateTokensToValidatorWithCommission(t *testing.T) { } func TestAllocateTokensToManyValidators(t *testing.T) { + ctx, _, k, sk, fck := CreateTestInputDefault(t, false, 1000) + sh := staking.NewHandler(sk) + + // initialize state + k.SetOutstandingRewards(ctx, sdk.DecCoins{}) + + // create validator with 50% commission + commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) + msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, + sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, commission, sdk.OneInt()) + require.True(t, sh(ctx, msg).IsOK()) + + // create second validator with 0% commission + commission = staking.NewCommissionMsg(sdk.NewDec(0), sdk.NewDec(0), sdk.NewDec(0)) + msg = staking.NewMsgCreateValidator(valOpAddr2, valConsPk2, + sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, commission, sdk.OneInt()) + require.True(t, sh(ctx, msg).IsOK()) + + abciValA := abci.Validator{ + Address: valConsPk1.Address(), + Power: 100, + } + abciValB := abci.Validator{ + Address: valConsPk2.Address(), + Power: 100, + } + + // assert initial state: zero outstanding rewards, zero community pool, zero commission, zero current rewards + require.True(t, k.GetOutstandingRewards(ctx).IsZero()) + require.True(t, k.GetFeePool(ctx).CommunityPool.IsZero()) + require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr1).IsZero()) + require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr2).IsZero()) + require.True(t, k.GetValidatorCurrentRewards(ctx, valOpAddr1).Rewards.IsZero()) + require.True(t, k.GetValidatorCurrentRewards(ctx, valOpAddr2).Rewards.IsZero()) + + // allocate tokens as if both had voted and second was proposer + fees := sdk.Coins{ + {sdk.DefaultBondDenom, sdk.NewInt(100)}, + } + fck.SetCollectedFees(fees) + votes := []abci.VoteInfo{ + { + Validator: abciValA, + SignedLastBlock: true, + }, + { + Validator: abciValB, + SignedLastBlock: true, + }, + } + k.AllocateTokens(ctx, 200, 200, valConsAddr2, votes) + + // 98 outstanding rewards (100 less 2 to community pool) + require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(98)}}, k.GetOutstandingRewards(ctx)) + // 2 community pool coins + require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(2)}}, k.GetFeePool(ctx).CommunityPool) + // 50% commission for first proposer, (0.5 * 93%) * 100 / 2 = 23.25 + require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(2325, 2)}}, k.GetValidatorAccumulatedCommission(ctx, valOpAddr1)) + // zero commission for second proposer + require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr2).IsZero()) + // just staking.proportional for first proposer less commission = (0.5 * 93%) * 100 / 2 = 23.25 + require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(2325, 2)}}, k.GetValidatorCurrentRewards(ctx, valOpAddr1).Rewards) + // proposer reward + staking.proportional for second proposer = (5 % + 0.5 * (93%)) * 100 = 51.5 + require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(515, 1)}}, k.GetValidatorCurrentRewards(ctx, valOpAddr2).Rewards) +} + +func TestAllocateTokensTruncation(t *testing.T) { communityTax := sdk.NewDec(0) ctx, _, k, sk, fck := CreateTestInputAdvanced(t, false, 1000000, communityTax) sh := staking.NewHandler(sk) @@ -108,16 +175,5 @@ func TestAllocateTokensToManyValidators(t *testing.T) { } k.AllocateTokens(ctx, 31, 31, valConsAddr2, votes) - // 98 outstanding rewards (100 less 2 to community pool) - require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(98)}}, k.GetOutstandingRewards(ctx)) - // 2 community pool coins - require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(2)}}, k.GetFeePool(ctx).CommunityPool) - // 50% commission for first proposer, (0.5 * 93%) * 100 / 2 = 23.25 - require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(2325, 2)}}, k.GetValidatorAccumulatedCommission(ctx, valOpAddr1)) - // zero commission for second proposer - require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr2).IsZero()) - // just staking.proportional for first proposer less commission = (0.5 * 93%) * 100 / 2 = 23.25 - require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(2325, 2)}}, k.GetValidatorCurrentRewards(ctx, valOpAddr1).Rewards) - // proposer reward + staking.proportional for second proposer = (5 % + 0.5 * (93%)) * 100 = 51.5 - require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(515, 1)}}, k.GetValidatorCurrentRewards(ctx, valOpAddr2).Rewards) + require.True(t, k.GetOutstandingRewards(ctx).IsValid()) } From c388d00bc8f70a5aef571e432b07e8a7806d0ca0 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 25 Feb 2019 16:04:53 +0100 Subject: [PATCH 15/50] PENDING.md --- PENDING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/PENDING.md b/PENDING.md index ad4f173c55c4..2de3f74c90ac 100644 --- a/PENDING.md +++ b/PENDING.md @@ -73,6 +73,8 @@ CLI flag. ### SDK +* \#3728 Truncate decimal multiplication & division in distribution to ensure + no more than the collected fees / inflation are distributed * \#3559 fix occasional failing due to non-determinism in lcd test TestBonding where validator is unexpectedly slashed throwing off test calculations * [\#3411] Include the `RequestInitChain.Time` in the block header init during From 02b076fac8f66fe08462d901aed7014a239e1df0 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Mon, 25 Feb 2019 16:21:29 +0100 Subject: [PATCH 16/50] Address @alexanderbez comments --- x/distribution/keeper/allocation_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x/distribution/keeper/allocation_test.go b/x/distribution/keeper/allocation_test.go index 74e24e541787..4ac5b3f656d8 100644 --- a/x/distribution/keeper/allocation_test.go +++ b/x/distribution/keeper/allocation_test.go @@ -115,19 +115,19 @@ func TestAllocateTokensTruncation(t *testing.T) { // initialize state k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission + // create validator with 10% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(1, 1), sdk.NewDecWithPrec(1, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(110)), staking.Description{}, commission, sdk.OneInt()) require.True(t, sh(ctx, msg).IsOK()) - // create second validator with 0% commission + // create second validator with 10% commission commission = staking.NewCommissionMsg(sdk.NewDecWithPrec(1, 1), sdk.NewDecWithPrec(1, 1), sdk.NewDec(0)) msg = staking.NewMsgCreateValidator(valOpAddr2, valConsPk2, sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, commission, sdk.OneInt()) require.True(t, sh(ctx, msg).IsOK()) - // create second validator with 0% commission + // create third validator with 10% commission commission = staking.NewCommissionMsg(sdk.NewDecWithPrec(1, 1), sdk.NewDecWithPrec(1, 1), sdk.NewDec(0)) msg = staking.NewMsgCreateValidator(valOpAddr3, valConsPk3, sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, commission, sdk.OneInt()) @@ -156,7 +156,7 @@ func TestAllocateTokensTruncation(t *testing.T) { // allocate tokens as if both had voted and second was proposer fees := sdk.Coins{ - {sdk.DefaultBondDenom, sdk.NewInt(634195840)}, + sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(634195840)), } fck.SetCollectedFees(fees) votes := []abci.VoteInfo{ From 8ff6302c766a69102e4eb4c6fab6c5063f86c3e4 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 26 Feb 2019 14:19:39 +0100 Subject: [PATCH 17/50] Debugging... --- x/distribution/keeper/delegation.go | 2 ++ x/mock/simulation/params.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index d63d8cd2632a..f70ebbfe7f0a 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/distribution/types" @@ -101,6 +102,7 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de coins, remainder := rewards.TruncateDecimal() outstanding := k.GetOutstandingRewards(ctx) + fmt.Printf("outstanding: %v, rewards: %v\n", outstanding, rewards) k.SetOutstandingRewards(ctx, outstanding.Sub(rewards)) feePool := k.GetFeePool(ctx) feePool.CommunityPool = feePool.CommunityPool.Add(remainder) diff --git a/x/mock/simulation/params.go b/x/mock/simulation/params.go index 8499e6c1189b..e844758881ae 100644 --- a/x/mock/simulation/params.go +++ b/x/mock/simulation/params.go @@ -12,7 +12,7 @@ const ( maxTimePerBlock int64 = 10000 // TODO Remove in favor of binary search for invariant violation - onOperation bool = false + onOperation bool = true ) // TODO explain transitional matrix usage From 4e15d3309c605894462b43b2e95f520bbb8b9343 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 26 Feb 2019 17:29:31 +0100 Subject: [PATCH 18/50] More debugging... --- x/distribution/abci_app.go | 5 +++++ x/distribution/keeper/allocation.go | 6 ++++++ x/distribution/keeper/delegation.go | 11 ++++++++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/x/distribution/abci_app.go b/x/distribution/abci_app.go index aae399062320..daa3b2a251d6 100644 --- a/x/distribution/abci_app.go +++ b/x/distribution/abci_app.go @@ -1,6 +1,7 @@ package distribution import ( + "fmt" abci "github.com/tendermint/tendermint/abci/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -23,6 +24,10 @@ func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) // ref https://github.com/cosmos/cosmos-sdk/issues/3095 if ctx.BlockHeight() > 1 { previousProposer := k.GetPreviousProposerConsAddr(ctx) + if ctx.BlockHeight() > 52 { + fmt.Printf("allocating tokens: sumPrecommitPower %v, totalPower %v, previousProposer %v, votes %v\n", + sumPrecommitPower, totalPower, previousProposer, req.LastCommitInfo.GetVotes()) + } k.AllocateTokens(ctx, sumPrecommitPower, totalPower, previousProposer, req.LastCommitInfo.GetVotes()) } diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 875af431f1d8..8aa2547b6ab2 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" abci "github.com/tendermint/tendermint/abci/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -64,6 +65,8 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in remaining = remaining.Sub(reward) } + fmt.Printf("remaining after: %v\n", remaining) + // allocate community funding feePool.CommunityPool = feePool.CommunityPool.Add(remaining) k.SetFeePool(ctx, feePool) @@ -77,6 +80,9 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // allocate tokens to a particular validator, splitting according to commission func (k Keeper) AllocateTokensToValidator(ctx sdk.Context, val sdk.Validator, tokens sdk.DecCoins) { + + fmt.Printf("allocate to validator: val %+v, tokens %v\n", val, tokens) + // split tokens between validator and delegators according to commission commission := tokens.MulDec(val.GetCommission()) shared := tokens.Sub(commission) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index f70ebbfe7f0a..1dbccb0b0358 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -67,6 +67,11 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod + if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" { + fmt.Printf("at block height %v found slash event: height %v, endingPeriod %v, rewards before: %v\n", + ctx.BlockHeight(), + height, endingPeriod, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) + } rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) // note: necessary to truncate so we don't allow withdrawing more rewards than owed stake = stake.MulTruncate(sdk.OneDec().Sub(event.Fraction)) @@ -102,7 +107,11 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de coins, remainder := rewards.TruncateDecimal() outstanding := k.GetOutstandingRewards(ctx) - fmt.Printf("outstanding: %v, rewards: %v\n", outstanding, rewards) + if len(rewards) > 0 && len(outstanding) > 0 && rewards[0].IsGTE(outstanding[0]) { + fmt.Printf("startingPeriod: %v\n", startingPeriod) + fmt.Printf("endingPeriod: %v\n", endingPeriod) + fmt.Printf("withdraw from %v to %v\n", val, del) + } k.SetOutstandingRewards(ctx, outstanding.Sub(rewards)) feePool := k.GetFeePool(ctx) feePool.CommunityPool = feePool.CommunityPool.Add(remainder) From f29bbaea3e7cbe997644f105bb8458f69a74659b Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 26 Feb 2019 18:53:39 +0100 Subject: [PATCH 19/50] Moar debugging --- x/distribution/handler.go | 5 +++++ x/distribution/keeper/allocation.go | 4 +++- x/distribution/keeper/delegation.go | 5 +++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/x/distribution/handler.go b/x/distribution/handler.go index 081baa869f96..91cde8f1188a 100644 --- a/x/distribution/handler.go +++ b/x/distribution/handler.go @@ -1,6 +1,7 @@ package distribution import ( + "fmt" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/distribution/keeper" "github.com/cosmos/cosmos-sdk/x/distribution/tags" @@ -42,6 +43,10 @@ func handleMsgModifyWithdrawAddress(ctx sdk.Context, msg types.MsgSetWithdrawAdd func handleMsgWithdrawDelegatorReward(ctx sdk.Context, msg types.MsgWithdrawDelegatorReward, k keeper.Keeper) sdk.Result { + if msg.ValidatorAddress.String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + fmt.Printf("handleMsgWithdrawDelegatorReward at height %v: %v\n", ctx.BlockHeight(), msg) + } + err := k.WithdrawDelegationRewards(ctx, msg.DelegatorAddress, msg.ValidatorAddress) if err != nil { return err.Result() diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 8aa2547b6ab2..02910696406b 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -81,7 +81,9 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // allocate tokens to a particular validator, splitting according to commission func (k Keeper) AllocateTokensToValidator(ctx sdk.Context, val sdk.Validator, tokens sdk.DecCoins) { - fmt.Printf("allocate to validator: val %+v, tokens %v\n", val, tokens) + if val.GetOperator().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + fmt.Printf("allocate to validator: val %+v, tokens %v\n", val, tokens) + } // split tokens between validator and delegators according to commission commission := tokens.MulDec(val.GetCommission()) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 1dbccb0b0358..79e09c9215e3 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -84,6 +84,10 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // calculate rewards for final period rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) + if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" { + fmt.Printf("rewards for final period (startingPeriod %v, endingPeriod %v) with stake %v: %v\n", startingPeriod, endingPeriod, stake, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) + } + return } @@ -111,6 +115,7 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de fmt.Printf("startingPeriod: %v\n", startingPeriod) fmt.Printf("endingPeriod: %v\n", endingPeriod) fmt.Printf("withdraw from %v to %v\n", val, del) + fmt.Printf("rewards: %v, outstanding: %v\n", rewards, outstanding) } k.SetOutstandingRewards(ctx, outstanding.Sub(rewards)) feePool := k.GetFeePool(ctx) From 72a850e17784c1f68b1b79ac71f27d0ad40359db Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 26 Feb 2019 19:49:03 +0100 Subject: [PATCH 20/50] ... --- x/distribution/keeper/delegation.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 79e09c9215e3..b7bc3cffacea 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -63,11 +63,14 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d startingHeight := startingInfo.Height + 1 // ... or slashes which happened in *this* block, since they would have happened after reward allocation endingHeight := uint64(ctx.BlockHeight()) - 1 + if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" && del.GetValidatorAddr().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + fmt.Printf("iterating over slashes from height %v to height %v\n", startingHeight, endingHeight) + } if endingHeight >= startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod - if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" { + if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" && del.GetValidatorAddr().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { fmt.Printf("at block height %v found slash event: height %v, endingPeriod %v, rewards before: %v\n", ctx.BlockHeight(), height, endingPeriod, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) @@ -84,8 +87,11 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // calculate rewards for final period rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" { + if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" && del.GetValidatorAddr().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + starting := k.GetValidatorHistoricalRewards(ctx, val.GetOperator(), startingPeriod) + ending := k.GetValidatorHistoricalRewards(ctx, val.GetOperator(), endingPeriod) fmt.Printf("rewards for final period (startingPeriod %v, endingPeriod %v) with stake %v: %v\n", startingPeriod, endingPeriod, stake, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) + fmt.Printf("starting: %v, ending: %v\n", starting, ending) } return From 1c57f327d10dbd34bcc014af88e07fc775ac1fc1 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 26 Feb 2019 21:43:37 +0100 Subject: [PATCH 21/50] ... --- x/distribution/handler.go | 5 ++++- x/distribution/keeper/allocation.go | 4 +++- x/distribution/keeper/delegation.go | 4 ++++ x/mock/simulation/params.go | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/x/distribution/handler.go b/x/distribution/handler.go index 91cde8f1188a..4176a496cf53 100644 --- a/x/distribution/handler.go +++ b/x/distribution/handler.go @@ -44,10 +44,13 @@ func handleMsgModifyWithdrawAddress(ctx sdk.Context, msg types.MsgSetWithdrawAdd func handleMsgWithdrawDelegatorReward(ctx sdk.Context, msg types.MsgWithdrawDelegatorReward, k keeper.Keeper) sdk.Result { if msg.ValidatorAddress.String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { - fmt.Printf("handleMsgWithdrawDelegatorReward at height %v: %v\n", ctx.BlockHeight(), msg) + fmt.Printf("handleMsgWithdrawDelegatorReward at height %v - delegator %v: %v\n", ctx.BlockHeight(), msg.DelegatorAddress.String()) } err := k.WithdrawDelegationRewards(ctx, msg.DelegatorAddress, msg.ValidatorAddress) + if msg.ValidatorAddress.String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + fmt.Printf("error: %v\n", err) + } if err != nil { return err.Result() } diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 02910696406b..3e22aeb436fe 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -40,7 +40,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in proposerValidator := k.stakingKeeper.ValidatorByConsAddr(ctx, proposer) if proposerValidator != nil { k.AllocateTokensToValidator(ctx, proposerValidator, proposerReward) - remaining = feesCollected.Sub(proposerReward) + remaining = remaining.Sub(proposerReward) } else { // proposer can be unknown if say, the unbonding period is 1 block, so // e.g. a validator undelegates at block X, it's removed entirely by @@ -73,7 +73,9 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // update outstanding rewards outstanding := k.GetOutstandingRewards(ctx) + fmt.Printf("add %v to outstanding %v\n", feesCollected.Sub(remaining), outstanding) outstanding = outstanding.Add(feesCollected.Sub(remaining)) + fmt.Printf("new outstanding %v\n", outstanding) k.SetOutstandingRewards(ctx, outstanding) } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index b7bc3cffacea..355916fe097e 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -12,6 +12,10 @@ func (k Keeper) initializeDelegation(ctx sdk.Context, val sdk.ValAddress, del sd // period has already been incremented - we want to store the period ended by this delegation action previousPeriod := k.GetValidatorCurrentRewards(ctx, val).Period - 1 + if del.String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" { + fmt.Printf("initialize delegation for period: %v\n", previousPeriod) + } + // increment reference count for the period we're going to track k.incrementReferenceCount(ctx, val, previousPeriod) diff --git a/x/mock/simulation/params.go b/x/mock/simulation/params.go index e844758881ae..8499e6c1189b 100644 --- a/x/mock/simulation/params.go +++ b/x/mock/simulation/params.go @@ -12,7 +12,7 @@ const ( maxTimePerBlock int64 = 10000 // TODO Remove in favor of binary search for invariant violation - onOperation bool = true + onOperation bool = false ) // TODO explain transitional matrix usage From ed989af3c8b1525fd63886e2a12803e657732290 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 26 Feb 2019 22:01:41 +0100 Subject: [PATCH 22/50] Outstanding per-validator --- cmd/gaia/app/invariants.go | 2 +- x/distribution/genesis.go | 6 ++-- x/distribution/keeper/alias_functions.go | 4 +-- x/distribution/keeper/allocation.go | 10 +++--- x/distribution/keeper/delegation.go | 5 +-- x/distribution/keeper/hooks.go | 4 +-- x/distribution/keeper/keeper.go | 4 +-- x/distribution/keeper/key.go | 11 +++++-- x/distribution/keeper/querier.go | 2 +- x/distribution/keeper/store.go | 8 ++--- x/distribution/keeper/validator.go | 7 ++-- x/distribution/simulation/invariants.go | 42 +++++++++++++++++------- x/distribution/types/genesis.go | 12 +++++-- x/staking/simulation/invariants.go | 5 ++- x/staking/types/expected_keepers.go | 2 +- 15 files changed, 79 insertions(+), 45 deletions(-) diff --git a/cmd/gaia/app/invariants.go b/cmd/gaia/app/invariants.go index 535cbff3711c..ecd9e39a5a1f 100644 --- a/cmd/gaia/app/invariants.go +++ b/cmd/gaia/app/invariants.go @@ -15,7 +15,7 @@ import ( func (app *GaiaApp) runtimeInvariants() []sdk.Invariant { return []sdk.Invariant{ banksim.NonnegativeBalanceInvariant(app.accountKeeper), - distrsim.NonNegativeOutstandingInvariant(app.distrKeeper), + distrsim.NonNegativeOutstandingInvariant(app.distrKeeper, app.stakingKeeper), stakingsim.SupplyInvariants(app.stakingKeeper, app.feeCollectionKeeper, app.distrKeeper, app.accountKeeper), stakingsim.NonNegativePowerInvariant(app.stakingKeeper), } diff --git a/x/distribution/genesis.go b/x/distribution/genesis.go index eecaa612306a..9e2c89caf965 100644 --- a/x/distribution/genesis.go +++ b/x/distribution/genesis.go @@ -16,7 +16,9 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, data types.GenesisState) { keeper.SetDelegatorWithdrawAddr(ctx, dwi.DelegatorAddress, dwi.WithdrawAddress) } keeper.SetPreviousProposerConsAddr(ctx, data.PreviousProposer) - keeper.SetOutstandingRewards(ctx, data.OutstandingRewards) + for _, rew := range data.OutstandingRewards { + keeper.SetOutstandingRewards(ctx, rew.ValidatorAddress, rew.OutstandingRewards) + } for _, acc := range data.ValidatorAccumulatedCommissions { keeper.SetValidatorAccumulatedCommission(ctx, acc.ValidatorAddress, acc.Accumulated) } @@ -50,7 +52,7 @@ func ExportGenesis(ctx sdk.Context, keeper Keeper) types.GenesisState { return false }) pp := keeper.GetPreviousProposerConsAddr(ctx) - outstanding := keeper.GetOutstandingRewards(ctx) + outstanding := make([]types.ValidatorOutstandingRewardsRecord, 0) acc := make([]types.ValidatorAccumulatedCommissionRecord, 0) keeper.IterateValidatorAccumulatedCommissions(ctx, func(addr sdk.ValAddress, commission types.ValidatorAccumulatedCommission) (stop bool) { diff --git a/x/distribution/keeper/alias_functions.go b/x/distribution/keeper/alias_functions.go index 575eafe5399a..d420139662f4 100644 --- a/x/distribution/keeper/alias_functions.go +++ b/x/distribution/keeper/alias_functions.go @@ -5,8 +5,8 @@ import ( ) // get outstanding rewards -func (k Keeper) GetOutstandingRewardsCoins(ctx sdk.Context) sdk.DecCoins { - return k.GetOutstandingRewards(ctx) +func (k Keeper) GetOutstandingRewardsCoins(ctx sdk.Context, val sdk.ValAddress) sdk.DecCoins { + return k.GetOutstandingRewards(ctx, val) } // get the community coins diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 3f6c423c0291..8260b372308d 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -60,11 +60,6 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in feePool.CommunityPool = feePool.CommunityPool.Add(remaining) k.SetFeePool(ctx, feePool) - // update outstanding rewards - outstanding := k.GetOutstandingRewards(ctx) - outstanding = outstanding.Add(feesCollected.Sub(remaining)) - k.SetOutstandingRewards(ctx, outstanding) - } // allocate tokens to a particular validator, splitting according to commission @@ -82,4 +77,9 @@ func (k Keeper) AllocateTokensToValidator(ctx sdk.Context, val sdk.Validator, to currentRewards := k.GetValidatorCurrentRewards(ctx, val.GetOperator()) currentRewards.Rewards = currentRewards.Rewards.Add(shared) k.SetValidatorCurrentRewards(ctx, val.GetOperator(), currentRewards) + + // update outstanding rewards + outstanding := k.GetOutstandingRewards(ctx, val.GetOperator()) + outstanding = outstanding.Add(tokens) + k.SetOutstandingRewards(ctx, val.GetOperator(), outstanding) } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index af140474cb85..ed93b61098c5 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -99,9 +99,10 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de // truncate coins, return remainder to community pool coins, remainder := rewards.TruncateDecimal() - outstanding := k.GetOutstandingRewards(ctx) - k.SetOutstandingRewards(ctx, outstanding.Sub(rewards)) + outstanding := k.GetOutstandingRewards(ctx, del.GetValidatorAddr()) + k.SetOutstandingRewards(ctx, del.GetValidatorAddr(), outstanding.Sub(rewards)) + feePool := k.GetFeePool(ctx) feePool.CommunityPool = feePool.CommunityPool.Add(remainder) k.SetFeePool(ctx, feePool) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 0466e8f7ea33..2ceb9c3be916 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -33,8 +33,8 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr h.k.SetFeePool(ctx, feePool) // update outstanding - outstanding := h.k.GetOutstandingRewards(ctx) - h.k.SetOutstandingRewards(ctx, outstanding.Sub(commission)) + outstanding := h.k.GetOutstandingRewards(ctx, valAddr) + h.k.SetOutstandingRewards(ctx, valAddr, outstanding.Sub(commission)) // add to validator account accAddr := sdk.AccAddress(valAddr) diff --git a/x/distribution/keeper/keeper.go b/x/distribution/keeper/keeper.go index 4a57c9bba663..f00f090b4636 100644 --- a/x/distribution/keeper/keeper.go +++ b/x/distribution/keeper/keeper.go @@ -84,8 +84,8 @@ func (k Keeper) WithdrawValidatorCommission(ctx sdk.Context, valAddr sdk.ValAddr k.SetValidatorAccumulatedCommission(ctx, valAddr, remainder) // update outstanding - outstanding := k.GetOutstandingRewards(ctx) - k.SetOutstandingRewards(ctx, outstanding.Sub(sdk.NewDecCoins(coins))) + outstanding := k.GetOutstandingRewards(ctx, valAddr) + k.SetOutstandingRewards(ctx, valAddr, outstanding.Sub(sdk.NewDecCoins(coins))) accAddr := sdk.AccAddress(valAddr) withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, accAddr) diff --git a/x/distribution/keeper/key.go b/x/distribution/keeper/key.go index 1b8437b4cd95..9da714d42367 100644 --- a/x/distribution/keeper/key.go +++ b/x/distribution/keeper/key.go @@ -13,9 +13,9 @@ const ( // keys var ( - FeePoolKey = []byte{0x00} // key for global distribution state - ProposerKey = []byte{0x01} // key for the proposer operator address - OutstandingRewardsKey = []byte{0x02} // key for outstanding rewards + FeePoolKey = []byte{0x00} // key for global distribution state + ProposerKey = []byte{0x01} // key for the proposer operator address + OutstandingRewardsPrefix = []byte{0x02} // key for outstanding rewards DelegatorWithdrawAddrPrefix = []byte{0x03} // key for delegator withdraw address DelegatorStartingInfoPrefix = []byte{0x04} // key for delegator starting info @@ -30,6 +30,11 @@ var ( ParamStoreKeyWithdrawAddrEnabled = []byte("withdrawaddrenabled") ) +// gets the outstanding rewards key for a validator +func GetOutstandingRewardsKey(valAddr sdk.ValAddress) []byte { + return append(OutstandingRewardsPrefix, valAddr.Bytes()...) +} + // gets an address from a delegator's withdraw info key func GetDelegatorWithdrawInfoAddress(key []byte) (delAddr sdk.AccAddress) { addr := key[1:] diff --git a/x/distribution/keeper/querier.go b/x/distribution/keeper/querier.go index 6099116fecd6..dcc052ff4d11 100644 --- a/x/distribution/keeper/querier.go +++ b/x/distribution/keeper/querier.go @@ -92,7 +92,7 @@ func queryParams(ctx sdk.Context, path []string, req abci.RequestQuery, k Keeper } func queryOutstandingRewards(ctx sdk.Context, path []string, req abci.RequestQuery, k Keeper) ([]byte, sdk.Error) { - bz, err := codec.MarshalJSONIndent(k.cdc, k.GetOutstandingRewards(ctx)) + bz, err := codec.MarshalJSONIndent(k.cdc, k.GetOutstandingRewards(ctx, sdk.ValAddress([]byte{}))) // TODO if err != nil { return nil, sdk.ErrInternal(sdk.AppendMsgToErr("could not marshal result to JSON", err.Error())) } diff --git a/x/distribution/keeper/store.go b/x/distribution/keeper/store.go index ef1f7c783c34..40f7e88ae159 100644 --- a/x/distribution/keeper/store.go +++ b/x/distribution/keeper/store.go @@ -264,18 +264,18 @@ func (k Keeper) IterateValidatorAccumulatedCommissions(ctx sdk.Context, handler } // get outstanding rewards -func (k Keeper) GetOutstandingRewards(ctx sdk.Context) (rewards types.OutstandingRewards) { +func (k Keeper) GetOutstandingRewards(ctx sdk.Context, val sdk.ValAddress) (rewards types.OutstandingRewards) { store := ctx.KVStore(k.storeKey) - b := store.Get(OutstandingRewardsKey) + b := store.Get(GetOutstandingRewardsKey(val)) k.cdc.MustUnmarshalBinaryLengthPrefixed(b, &rewards) return } // set outstanding rewards -func (k Keeper) SetOutstandingRewards(ctx sdk.Context, rewards types.OutstandingRewards) { +func (k Keeper) SetOutstandingRewards(ctx sdk.Context, val sdk.ValAddress, rewards types.OutstandingRewards) { store := ctx.KVStore(k.storeKey) b := k.cdc.MustMarshalBinaryLengthPrefixed(rewards) - store.Set(OutstandingRewardsKey, b) + store.Set(GetOutstandingRewardsKey(val), b) } // get slash event for height diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index 21ffa24a304b..b577e096ac4f 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -16,6 +16,9 @@ func (k Keeper) initializeValidator(ctx sdk.Context, val sdk.Validator) { // set accumulated commission k.SetValidatorAccumulatedCommission(ctx, val.GetOperator(), types.InitialValidatorAccumulatedCommission()) + + // set outstanding rewards + k.SetOutstandingRewards(ctx, val.GetOperator(), sdk.DecCoins{}) } // increment validator period, returning the period just ended @@ -30,11 +33,11 @@ func (k Keeper) incrementValidatorPeriod(ctx sdk.Context, val sdk.Validator) uin // can't calculate ratio for zero-token validators // ergo we instead add to the community pool feePool := k.GetFeePool(ctx) - outstanding := k.GetOutstandingRewards(ctx) + outstanding := k.GetOutstandingRewards(ctx, val.GetOperator()) feePool.CommunityPool = feePool.CommunityPool.Add(rewards.Rewards) outstanding = outstanding.Sub(rewards.Rewards) k.SetFeePool(ctx, feePool) - k.SetOutstandingRewards(ctx, outstanding) + k.SetOutstandingRewards(ctx, val.GetOperator(), outstanding) current = sdk.DecCoins{} } else { diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index f72241065149..fd0d681ea1eb 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -15,7 +15,7 @@ func AllInvariants(d distr.Keeper, stk types.StakingKeeper) sdk.Invariant { if err != nil { return err } - err = NonNegativeOutstandingInvariant(d)(ctx) + err = NonNegativeOutstandingInvariant(d, stk)(ctx) if err != nil { return err } @@ -28,13 +28,25 @@ func AllInvariants(d distr.Keeper, stk types.StakingKeeper) sdk.Invariant { } // NonNegativeOutstandingInvariant checks that outstanding unwithdrawn fees are never negative -func NonNegativeOutstandingInvariant(k distr.Keeper) sdk.Invariant { +func NonNegativeOutstandingInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant { return func(ctx sdk.Context) error { - outstanding := k.GetOutstandingRewards(ctx) + + var outstanding sdk.DecCoins + + // iterate over all validators + sk.IterateValidators(ctx, func(_ int64, val sdk.Validator) (stop bool) { + outstanding = k.GetOutstandingRewards(ctx, val.GetOperator()) + if outstanding.IsAnyNegative() { + return true + } + return false + }) if outstanding.IsAnyNegative() { return fmt.Errorf("negative outstanding coins: %v", outstanding) } + return nil + } } @@ -45,20 +57,26 @@ func CanWithdrawInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant // cache, we don't want to write changes ctx, _ = ctx.CacheContext() - // iterate over all bonded validators, withdraw commission + var remaining sdk.DecCoins + + // iterate over all validators sk.IterateValidators(ctx, func(_ int64, val sdk.Validator) (stop bool) { _ = k.WithdrawValidatorCommission(ctx, val.GetOperator()) + // ugh so slow TODO FIXME + // iterate over all current delegations, withdraw rewards + dels := sk.GetAllSDKDelegations(ctx) + for _, delegation := range dels { + if delegation.GetValidatorAddr().String() == val.GetOperator().String() { + _ = k.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr()) + } + } + remaining = k.GetOutstandingRewards(ctx, val.GetOperator()) + if len(remaining) > 0 && remaining[0].Amount.LT(sdk.ZeroDec()) { + return true + } return false }) - // iterate over all current delegations, withdraw rewards - dels := sk.GetAllSDKDelegations(ctx) - for _, delegation := range dels { - _ = k.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr()) - } - - remaining := k.GetOutstandingRewards(ctx) - if len(remaining) > 0 && remaining[0].Amount.LT(sdk.ZeroDec()) { return fmt.Errorf("negative remaining coins: %v", remaining) } diff --git a/x/distribution/types/genesis.go b/x/distribution/types/genesis.go index 80d2ff22a1c9..e87d8a33fdeb 100644 --- a/x/distribution/types/genesis.go +++ b/x/distribution/types/genesis.go @@ -13,6 +13,12 @@ type DelegatorWithdrawInfo struct { WithdrawAddress sdk.AccAddress `json:"withdraw_address"` } +// used for import/export via genesis json +type ValidatorOutstandingRewardsRecord struct { + ValidatorAddress sdk.ValAddress `json:"validator_address"` + OutstandingRewards sdk.DecCoins `json:"outstanding_rewards"` +} + // used for import / export via genesis json type ValidatorAccumulatedCommissionRecord struct { ValidatorAddress sdk.ValAddress `json:"validator_address"` @@ -55,7 +61,7 @@ type GenesisState struct { WithdrawAddrEnabled bool `json:"withdraw_addr_enabled"` DelegatorWithdrawInfos []DelegatorWithdrawInfo `json:"delegator_withdraw_infos"` PreviousProposer sdk.ConsAddress `json:"previous_proposer"` - OutstandingRewards sdk.DecCoins `json:"outstanding_rewards"` + OutstandingRewards []ValidatorOutstandingRewardsRecord `json:"outstanding_rewards"` ValidatorAccumulatedCommissions []ValidatorAccumulatedCommissionRecord `json:"validator_accumulated_commissions"` ValidatorHistoricalRewards []ValidatorHistoricalRewardsRecord `json:"validator_historical_rewards"` ValidatorCurrentRewards []ValidatorCurrentRewardsRecord `json:"validator_current_rewards"` @@ -64,7 +70,7 @@ type GenesisState struct { } func NewGenesisState(feePool FeePool, communityTax, baseProposerReward, bonusProposerReward sdk.Dec, - withdrawAddrEnabled bool, dwis []DelegatorWithdrawInfo, pp sdk.ConsAddress, r OutstandingRewards, + withdrawAddrEnabled bool, dwis []DelegatorWithdrawInfo, pp sdk.ConsAddress, r []ValidatorOutstandingRewardsRecord, acc []ValidatorAccumulatedCommissionRecord, historical []ValidatorHistoricalRewardsRecord, cur []ValidatorCurrentRewardsRecord, dels []DelegatorStartingInfoRecord, slashes []ValidatorSlashEventRecord) GenesisState { @@ -96,7 +102,7 @@ func DefaultGenesisState() GenesisState { WithdrawAddrEnabled: true, DelegatorWithdrawInfos: []DelegatorWithdrawInfo{}, PreviousProposer: nil, - OutstandingRewards: sdk.DecCoins{}, + OutstandingRewards: []ValidatorOutstandingRewardsRecord{}, ValidatorAccumulatedCommissions: []ValidatorAccumulatedCommissionRecord{}, ValidatorHistoricalRewards: []ValidatorHistoricalRewardsRecord{}, ValidatorCurrentRewards: []ValidatorCurrentRewardsRecord{}, diff --git a/x/staking/simulation/invariants.go b/x/staking/simulation/invariants.go index d0d11e8124f2..6000b11a5c8e 100644 --- a/x/staking/simulation/invariants.go +++ b/x/staking/simulation/invariants.go @@ -67,6 +67,8 @@ func SupplyInvariants(k staking.Keeper, case sdk.Unbonding, sdk.Unbonded: loose = loose.Add(validator.GetTokens().ToDec()) } + // add yet-to-be-withdrawn + loose = loose.Add(d.GetOutstandingRewardsCoins(ctx, validator.GetOperator()).AmountOf(k.BondDenom(ctx))) return false }) @@ -76,9 +78,6 @@ func SupplyInvariants(k staking.Keeper, // add community pool loose = loose.Add(d.GetFeePoolCommunityCoins(ctx).AmountOf(k.BondDenom(ctx))) - // add yet-to-be-withdrawn - loose = loose.Add(d.GetOutstandingRewardsCoins(ctx).AmountOf(k.BondDenom(ctx))) - // Not-bonded tokens should equal coin supply plus unbonding delegations // plus tokens on unbonded validators if !pool.NotBondedTokens.ToDec().Equal(loose) { diff --git a/x/staking/types/expected_keepers.go b/x/staking/types/expected_keepers.go index affa2207076e..9395d3733490 100644 --- a/x/staking/types/expected_keepers.go +++ b/x/staking/types/expected_keepers.go @@ -5,7 +5,7 @@ import sdk "github.com/cosmos/cosmos-sdk/types" // expected coin keeper type DistributionKeeper interface { GetFeePoolCommunityCoins(ctx sdk.Context) sdk.DecCoins - GetOutstandingRewardsCoins(ctx sdk.Context) sdk.DecCoins + GetOutstandingRewardsCoins(ctx sdk.Context, val sdk.ValAddress) sdk.DecCoins } // expected fee collection keeper From 4dd5f14b4ea21c338f1ec2551588d668a8346db0 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 26 Feb 2019 22:12:43 +0100 Subject: [PATCH 23/50] Naming; genesis fix, ... --- x/distribution/genesis.go | 11 +++++++++- x/distribution/keeper/alias_functions.go | 2 +- x/distribution/keeper/allocation.go | 4 ++-- x/distribution/keeper/delegation.go | 4 ++-- x/distribution/keeper/hooks.go | 4 ++-- x/distribution/keeper/keeper.go | 4 ++-- x/distribution/keeper/key.go | 21 ++++++++++++------ x/distribution/keeper/querier.go | 2 +- x/distribution/keeper/store.go | 27 ++++++++++++++++++------ x/distribution/keeper/validator.go | 6 +++--- x/distribution/simulation/invariants.go | 9 ++++---- x/distribution/types/fee_pool.go | 5 ----- x/distribution/types/validator.go | 4 ++++ 13 files changed, 68 insertions(+), 35 deletions(-) diff --git a/x/distribution/genesis.go b/x/distribution/genesis.go index 9e2c89caf965..524e2237cae0 100644 --- a/x/distribution/genesis.go +++ b/x/distribution/genesis.go @@ -17,7 +17,7 @@ func InitGenesis(ctx sdk.Context, keeper Keeper, data types.GenesisState) { } keeper.SetPreviousProposerConsAddr(ctx, data.PreviousProposer) for _, rew := range data.OutstandingRewards { - keeper.SetOutstandingRewards(ctx, rew.ValidatorAddress, rew.OutstandingRewards) + keeper.SetValidatorOutstandingRewards(ctx, rew.ValidatorAddress, rew.OutstandingRewards) } for _, acc := range data.ValidatorAccumulatedCommissions { keeper.SetValidatorAccumulatedCommission(ctx, acc.ValidatorAddress, acc.Accumulated) @@ -53,6 +53,15 @@ func ExportGenesis(ctx sdk.Context, keeper Keeper) types.GenesisState { }) pp := keeper.GetPreviousProposerConsAddr(ctx) outstanding := make([]types.ValidatorOutstandingRewardsRecord, 0) + keeper.IterateValidatorOutstandingRewards(ctx, + func(addr sdk.ValAddress, rewards types.ValidatorOutstandingRewards) (stop bool) { + outstanding = append(outstanding, types.ValidatorOutstandingRewardsRecord{ + ValidatorAddress: addr, + OutstandingRewards: rewards, + }) + return false + }, + ) acc := make([]types.ValidatorAccumulatedCommissionRecord, 0) keeper.IterateValidatorAccumulatedCommissions(ctx, func(addr sdk.ValAddress, commission types.ValidatorAccumulatedCommission) (stop bool) { diff --git a/x/distribution/keeper/alias_functions.go b/x/distribution/keeper/alias_functions.go index d420139662f4..a2afd8b0a3f6 100644 --- a/x/distribution/keeper/alias_functions.go +++ b/x/distribution/keeper/alias_functions.go @@ -6,7 +6,7 @@ import ( // get outstanding rewards func (k Keeper) GetOutstandingRewardsCoins(ctx sdk.Context, val sdk.ValAddress) sdk.DecCoins { - return k.GetOutstandingRewards(ctx, val) + return k.GetValidatorOutstandingRewards(ctx, val) } // get the community coins diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 8260b372308d..145f4d0785d0 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -79,7 +79,7 @@ func (k Keeper) AllocateTokensToValidator(ctx sdk.Context, val sdk.Validator, to k.SetValidatorCurrentRewards(ctx, val.GetOperator(), currentRewards) // update outstanding rewards - outstanding := k.GetOutstandingRewards(ctx, val.GetOperator()) + outstanding := k.GetValidatorOutstandingRewards(ctx, val.GetOperator()) outstanding = outstanding.Add(tokens) - k.SetOutstandingRewards(ctx, val.GetOperator(), outstanding) + k.SetValidatorOutstandingRewards(ctx, val.GetOperator(), outstanding) } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index ed93b61098c5..1ef9b15feae5 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -100,8 +100,8 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de // truncate coins, return remainder to community pool coins, remainder := rewards.TruncateDecimal() - outstanding := k.GetOutstandingRewards(ctx, del.GetValidatorAddr()) - k.SetOutstandingRewards(ctx, del.GetValidatorAddr(), outstanding.Sub(rewards)) + outstanding := k.GetValidatorOutstandingRewards(ctx, del.GetValidatorAddr()) + k.SetValidatorOutstandingRewards(ctx, del.GetValidatorAddr(), outstanding.Sub(rewards)) feePool := k.GetFeePool(ctx) feePool.CommunityPool = feePool.CommunityPool.Add(remainder) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 2ceb9c3be916..d1a5b92fde5f 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -33,8 +33,8 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr h.k.SetFeePool(ctx, feePool) // update outstanding - outstanding := h.k.GetOutstandingRewards(ctx, valAddr) - h.k.SetOutstandingRewards(ctx, valAddr, outstanding.Sub(commission)) + outstanding := h.k.GetValidatorOutstandingRewards(ctx, valAddr) + h.k.SetValidatorOutstandingRewards(ctx, valAddr, outstanding.Sub(commission)) // add to validator account accAddr := sdk.AccAddress(valAddr) diff --git a/x/distribution/keeper/keeper.go b/x/distribution/keeper/keeper.go index f00f090b4636..0595980fd567 100644 --- a/x/distribution/keeper/keeper.go +++ b/x/distribution/keeper/keeper.go @@ -84,8 +84,8 @@ func (k Keeper) WithdrawValidatorCommission(ctx sdk.Context, valAddr sdk.ValAddr k.SetValidatorAccumulatedCommission(ctx, valAddr, remainder) // update outstanding - outstanding := k.GetOutstandingRewards(ctx, valAddr) - k.SetOutstandingRewards(ctx, valAddr, outstanding.Sub(sdk.NewDecCoins(coins))) + outstanding := k.GetValidatorOutstandingRewards(ctx, valAddr) + k.SetValidatorOutstandingRewards(ctx, valAddr, outstanding.Sub(sdk.NewDecCoins(coins))) accAddr := sdk.AccAddress(valAddr) withdrawAddr := k.GetDelegatorWithdrawAddr(ctx, accAddr) diff --git a/x/distribution/keeper/key.go b/x/distribution/keeper/key.go index 9da714d42367..068e05b857ec 100644 --- a/x/distribution/keeper/key.go +++ b/x/distribution/keeper/key.go @@ -13,9 +13,9 @@ const ( // keys var ( - FeePoolKey = []byte{0x00} // key for global distribution state - ProposerKey = []byte{0x01} // key for the proposer operator address - OutstandingRewardsPrefix = []byte{0x02} // key for outstanding rewards + FeePoolKey = []byte{0x00} // key for global distribution state + ProposerKey = []byte{0x01} // key for the proposer operator address + ValidatorOutstandingRewardsPrefix = []byte{0x02} // key for outstanding rewards DelegatorWithdrawAddrPrefix = []byte{0x03} // key for delegator withdraw address DelegatorStartingInfoPrefix = []byte{0x04} // key for delegator starting info @@ -30,9 +30,13 @@ var ( ParamStoreKeyWithdrawAddrEnabled = []byte("withdrawaddrenabled") ) -// gets the outstanding rewards key for a validator -func GetOutstandingRewardsKey(valAddr sdk.ValAddress) []byte { - return append(OutstandingRewardsPrefix, valAddr.Bytes()...) +// gets an address from a validator's outstanding rewards key +func GetValidatorOutstandingRewardsAddress(key []byte) (valAddr sdk.ValAddress) { + addr := key[1:] + if len(addr) != sdk.AddrLen { + panic("unexpected key length") + } + return sdk.ValAddress(addr) } // gets an address from a delegator's withdraw info key @@ -107,6 +111,11 @@ func GetValidatorSlashEventAddressHeight(key []byte) (valAddr sdk.ValAddress, he return } +// gets the outstanding rewards key for a validator +func GetValidatorOutstandingRewardsKey(valAddr sdk.ValAddress) []byte { + return append(ValidatorOutstandingRewardsPrefix, valAddr.Bytes()...) +} + // gets the key for a delegator's withdraw addr func GetDelegatorWithdrawAddrKey(delAddr sdk.AccAddress) []byte { return append(DelegatorWithdrawAddrPrefix, delAddr.Bytes()...) diff --git a/x/distribution/keeper/querier.go b/x/distribution/keeper/querier.go index dcc052ff4d11..85ffc2ad0467 100644 --- a/x/distribution/keeper/querier.go +++ b/x/distribution/keeper/querier.go @@ -92,7 +92,7 @@ func queryParams(ctx sdk.Context, path []string, req abci.RequestQuery, k Keeper } func queryOutstandingRewards(ctx sdk.Context, path []string, req abci.RequestQuery, k Keeper) ([]byte, sdk.Error) { - bz, err := codec.MarshalJSONIndent(k.cdc, k.GetOutstandingRewards(ctx, sdk.ValAddress([]byte{}))) // TODO + bz, err := codec.MarshalJSONIndent(k.cdc, k.GetValidatorOutstandingRewards(ctx, sdk.ValAddress([]byte{}))) // TODO if err != nil { return nil, sdk.ErrInternal(sdk.AppendMsgToErr("could not marshal result to JSON", err.Error())) } diff --git a/x/distribution/keeper/store.go b/x/distribution/keeper/store.go index 40f7e88ae159..d9538cd672ad 100644 --- a/x/distribution/keeper/store.go +++ b/x/distribution/keeper/store.go @@ -263,19 +263,34 @@ func (k Keeper) IterateValidatorAccumulatedCommissions(ctx sdk.Context, handler } } -// get outstanding rewards -func (k Keeper) GetOutstandingRewards(ctx sdk.Context, val sdk.ValAddress) (rewards types.OutstandingRewards) { +// get validator outstanding rewards +func (k Keeper) GetValidatorOutstandingRewards(ctx sdk.Context, val sdk.ValAddress) (rewards types.ValidatorOutstandingRewards) { store := ctx.KVStore(k.storeKey) - b := store.Get(GetOutstandingRewardsKey(val)) + b := store.Get(GetValidatorOutstandingRewardsKey(val)) k.cdc.MustUnmarshalBinaryLengthPrefixed(b, &rewards) return } -// set outstanding rewards -func (k Keeper) SetOutstandingRewards(ctx sdk.Context, val sdk.ValAddress, rewards types.OutstandingRewards) { +// set validator outstanding rewards +func (k Keeper) SetValidatorOutstandingRewards(ctx sdk.Context, val sdk.ValAddress, rewards types.ValidatorOutstandingRewards) { store := ctx.KVStore(k.storeKey) b := k.cdc.MustMarshalBinaryLengthPrefixed(rewards) - store.Set(GetOutstandingRewardsKey(val), b) + store.Set(GetValidatorOutstandingRewardsKey(val), b) +} + +// iterate validator outstanding rewards +func (k Keeper) IterateValidatorOutstandingRewards(ctx sdk.Context, handler func(val sdk.ValAddress, rewards types.ValidatorOutstandingRewards) (stop bool)) { + store := ctx.KVStore(k.storeKey) + iter := sdk.KVStorePrefixIterator(store, ValidatorOutstandingRewardsPrefix) + defer iter.Close() + for ; iter.Valid(); iter.Next() { + var rewards types.ValidatorOutstandingRewards + k.cdc.MustUnmarshalBinaryLengthPrefixed(iter.Value(), &rewards) + addr := GetValidatorOutstandingRewardsAddress(iter.Key()) + if handler(addr, rewards) { + break + } + } } // get slash event for height diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index b577e096ac4f..18c6cc6e87e0 100644 --- a/x/distribution/keeper/validator.go +++ b/x/distribution/keeper/validator.go @@ -18,7 +18,7 @@ func (k Keeper) initializeValidator(ctx sdk.Context, val sdk.Validator) { k.SetValidatorAccumulatedCommission(ctx, val.GetOperator(), types.InitialValidatorAccumulatedCommission()) // set outstanding rewards - k.SetOutstandingRewards(ctx, val.GetOperator(), sdk.DecCoins{}) + k.SetValidatorOutstandingRewards(ctx, val.GetOperator(), sdk.DecCoins{}) } // increment validator period, returning the period just ended @@ -33,11 +33,11 @@ func (k Keeper) incrementValidatorPeriod(ctx sdk.Context, val sdk.Validator) uin // can't calculate ratio for zero-token validators // ergo we instead add to the community pool feePool := k.GetFeePool(ctx) - outstanding := k.GetOutstandingRewards(ctx, val.GetOperator()) + outstanding := k.GetValidatorOutstandingRewards(ctx, val.GetOperator()) feePool.CommunityPool = feePool.CommunityPool.Add(rewards.Rewards) outstanding = outstanding.Sub(rewards.Rewards) k.SetFeePool(ctx, feePool) - k.SetOutstandingRewards(ctx, val.GetOperator(), outstanding) + k.SetValidatorOutstandingRewards(ctx, val.GetOperator(), outstanding) current = sdk.DecCoins{} } else { diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index fd0d681ea1eb..02c721ef906e 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -33,14 +33,14 @@ func NonNegativeOutstandingInvariant(k distr.Keeper, sk types.StakingKeeper) sdk var outstanding sdk.DecCoins - // iterate over all validators - sk.IterateValidators(ctx, func(_ int64, val sdk.Validator) (stop bool) { - outstanding = k.GetOutstandingRewards(ctx, val.GetOperator()) + k.IterateValidatorOutstandingRewards(ctx, func(_ sdk.ValAddress, rewards types.ValidatorOutstandingRewards) (stop bool) { + outstanding = rewards if outstanding.IsAnyNegative() { return true } return false }) + if outstanding.IsAnyNegative() { return fmt.Errorf("negative outstanding coins: %v", outstanding) } @@ -70,8 +70,9 @@ func CanWithdrawInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant _ = k.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr()) } } - remaining = k.GetOutstandingRewards(ctx, val.GetOperator()) + remaining = k.GetValidatorOutstandingRewards(ctx, val.GetOperator()) if len(remaining) > 0 && remaining[0].Amount.LT(sdk.ZeroDec()) { + panic("OH NO") return true } return false diff --git a/x/distribution/types/fee_pool.go b/x/distribution/types/fee_pool.go index bb34ab47cd4a..caf9905e1d8c 100644 --- a/x/distribution/types/fee_pool.go +++ b/x/distribution/types/fee_pool.go @@ -27,8 +27,3 @@ func (f FeePool) ValidateGenesis() error { return nil } - -// outstanding (un-withdrawn) rewards for everyone -// excludes the community pool -// inexpensive to track, allows simple sanity checks -type OutstandingRewards = sdk.DecCoins diff --git a/x/distribution/types/validator.go b/x/distribution/types/validator.go index 0bde7fc073ee..a7fc0dd2a9fb 100644 --- a/x/distribution/types/validator.go +++ b/x/distribution/types/validator.go @@ -91,3 +91,7 @@ func (vs ValidatorSlashEvents) String() string { } return strings.TrimSpace(out) } + +// outstanding (un-withdrawn) rewards for a validator +// inexpensive to track, allows simple sanity checks +type ValidatorOutstandingRewards = sdk.DecCoins From f4e2c661e38922c60988dcf575d8b916dee2809d Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 16:46:43 +0100 Subject: [PATCH 24/50] .. --- x/distribution/keeper/allocation.go | 2 +- x/distribution/keeper/delegation.go | 6 +++--- x/distribution/simulation/invariants.go | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 15c92744a971..47779d27fd46 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -76,7 +76,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // allocate tokens to a particular validator, splitting according to commission func (k Keeper) AllocateTokensToValidator(ctx sdk.Context, val sdk.Validator, tokens sdk.DecCoins) { - if val.GetOperator().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + if val.GetOperator().String() == "cosmosvaloper1qu379fd7lzvl9pfwclw2984n99dfqdgxjypypy" { fmt.Printf("allocate to validator: val %+v, tokens %v\n", val, tokens) } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 6433c3da3015..16783ef5f8fa 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -67,14 +67,14 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d startingHeight := startingInfo.Height + 1 // ... or slashes which happened in *this* block, since they would have happened after reward allocation endingHeight := uint64(ctx.BlockHeight()) - 1 - if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" && del.GetValidatorAddr().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + if del.GetValidatorAddr().String() == "cosmosvaloper1qu379fd7lzvl9pfwclw2984n99dfqdgxjypypy" { fmt.Printf("iterating over slashes from height %v to height %v\n", startingHeight, endingHeight) } if endingHeight >= startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod - if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" && del.GetValidatorAddr().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + if del.GetValidatorAddr().String() == "cosmosvaloper1qu379fd7lzvl9pfwclw2984n99dfqdgxjypypy" { fmt.Printf("at block height %v found slash event: height %v, endingPeriod %v, rewards before: %v\n", ctx.BlockHeight(), height, endingPeriod, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) @@ -91,7 +91,7 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // calculate rewards for final period rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - if del.GetDelegatorAddr().String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" && del.GetValidatorAddr().String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { + if del.GetValidatorAddr().String() == "cosmosvaloper1qu379fd7lzvl9pfwclw2984n99dfqdgxjypypy" { starting := k.GetValidatorHistoricalRewards(ctx, val.GetOperator(), startingPeriod) ending := k.GetValidatorHistoricalRewards(ctx, val.GetOperator(), endingPeriod) fmt.Printf("rewards for final period (startingPeriod %v, endingPeriod %v) with stake %v: %v\n", startingPeriod, endingPeriod, stake, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index 02c721ef906e..1047fdd78aa6 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -61,6 +61,7 @@ func CanWithdrawInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant // iterate over all validators sk.IterateValidators(ctx, func(_ int64, val sdk.Validator) (stop bool) { + fmt.Printf("withdrawing all from validator %s\n", val.GetOperator()) _ = k.WithdrawValidatorCommission(ctx, val.GetOperator()) // ugh so slow TODO FIXME // iterate over all current delegations, withdraw rewards From f312638c005b448a0a4d12f6228f2385d35e25a3 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 17:08:28 +0100 Subject: [PATCH 25/50] .. --- x/distribution/simulation/invariants.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index 1047fdd78aa6..ee2dddc43fa3 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -61,7 +61,6 @@ func CanWithdrawInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant // iterate over all validators sk.IterateValidators(ctx, func(_ int64, val sdk.Validator) (stop bool) { - fmt.Printf("withdrawing all from validator %s\n", val.GetOperator()) _ = k.WithdrawValidatorCommission(ctx, val.GetOperator()) // ugh so slow TODO FIXME // iterate over all current delegations, withdraw rewards @@ -73,7 +72,6 @@ func CanWithdrawInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant } remaining = k.GetValidatorOutstandingRewards(ctx, val.GetOperator()) if len(remaining) > 0 && remaining[0].Amount.LT(sdk.ZeroDec()) { - panic("OH NO") return true } return false From 79809ee9bada8576b2aacac975771ee9dae864e2 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 22:22:46 +0100 Subject: [PATCH 26/50] Cleanup, separate out invariant changes --- PENDING.md | 6 +--- x/distribution/abci_app.go | 5 --- x/distribution/handler.go | 8 ----- x/distribution/keeper/delegation.go | 26 ---------------- x/distribution/keeper/querier.go | 41 +++++++++++++++++-------- x/distribution/simulation/invariants.go | 2 +- 6 files changed, 31 insertions(+), 57 deletions(-) diff --git a/PENDING.md b/PENDING.md index 6c1690855c74..7a3a666b30be 100644 --- a/PENDING.md +++ b/PENDING.md @@ -61,6 +61,7 @@ CLI flag. ### SDK +* \#3750 Track outstanding rewards per-validator instead of globally * \#3753 Remove no-longer-used governance penalty parameter * \#3679 Consistent operators across Coins, DecCoins, Int, Dec replaced: Minus->Sub Plus->Add Div->Quo @@ -104,9 +105,4 @@ truncation of undelegation tokens. unbonding period was just 1 block and proposer was already deleted. * [\#3726] Cap(clip) reward to remaining coins in AllocateTokens. -* Update the vesting specification and implementation to cap deduction from -`DelegatedVesting` by at most `DelegatedVesting`. This accounts for the case where -the undelegation amount may exceed the original delegation amount due to -truncation of undelegation tokens. - ### Tendermint diff --git a/x/distribution/abci_app.go b/x/distribution/abci_app.go index daa3b2a251d6..aae399062320 100644 --- a/x/distribution/abci_app.go +++ b/x/distribution/abci_app.go @@ -1,7 +1,6 @@ package distribution import ( - "fmt" abci "github.com/tendermint/tendermint/abci/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -24,10 +23,6 @@ func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) // ref https://github.com/cosmos/cosmos-sdk/issues/3095 if ctx.BlockHeight() > 1 { previousProposer := k.GetPreviousProposerConsAddr(ctx) - if ctx.BlockHeight() > 52 { - fmt.Printf("allocating tokens: sumPrecommitPower %v, totalPower %v, previousProposer %v, votes %v\n", - sumPrecommitPower, totalPower, previousProposer, req.LastCommitInfo.GetVotes()) - } k.AllocateTokens(ctx, sumPrecommitPower, totalPower, previousProposer, req.LastCommitInfo.GetVotes()) } diff --git a/x/distribution/handler.go b/x/distribution/handler.go index 4176a496cf53..081baa869f96 100644 --- a/x/distribution/handler.go +++ b/x/distribution/handler.go @@ -1,7 +1,6 @@ package distribution import ( - "fmt" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/distribution/keeper" "github.com/cosmos/cosmos-sdk/x/distribution/tags" @@ -43,14 +42,7 @@ func handleMsgModifyWithdrawAddress(ctx sdk.Context, msg types.MsgSetWithdrawAdd func handleMsgWithdrawDelegatorReward(ctx sdk.Context, msg types.MsgWithdrawDelegatorReward, k keeper.Keeper) sdk.Result { - if msg.ValidatorAddress.String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { - fmt.Printf("handleMsgWithdrawDelegatorReward at height %v - delegator %v: %v\n", ctx.BlockHeight(), msg.DelegatorAddress.String()) - } - err := k.WithdrawDelegationRewards(ctx, msg.DelegatorAddress, msg.ValidatorAddress) - if msg.ValidatorAddress.String() == "cosmosvaloper1l67uvpuauv6wd90rvuln8ywp3trwfcc6csx0hn" { - fmt.Printf("error: %v\n", err) - } if err != nil { return err.Result() } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 16783ef5f8fa..e9ce231cd0d9 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -1,7 +1,6 @@ package keeper import ( - "fmt" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/distribution/types" @@ -12,10 +11,6 @@ func (k Keeper) initializeDelegation(ctx sdk.Context, val sdk.ValAddress, del sd // period has already been incremented - we want to store the period ended by this delegation action previousPeriod := k.GetValidatorCurrentRewards(ctx, val).Period - 1 - if del.String() == "cosmos1l67uvpuauv6wd90rvuln8ywp3trwfcc6ayj6mq" { - fmt.Printf("initialize delegation for period: %v\n", previousPeriod) - } - // increment reference count for the period we're going to track k.incrementReferenceCount(ctx, val, previousPeriod) @@ -67,18 +62,10 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d startingHeight := startingInfo.Height + 1 // ... or slashes which happened in *this* block, since they would have happened after reward allocation endingHeight := uint64(ctx.BlockHeight()) - 1 - if del.GetValidatorAddr().String() == "cosmosvaloper1qu379fd7lzvl9pfwclw2984n99dfqdgxjypypy" { - fmt.Printf("iterating over slashes from height %v to height %v\n", startingHeight, endingHeight) - } if endingHeight >= startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod - if del.GetValidatorAddr().String() == "cosmosvaloper1qu379fd7lzvl9pfwclw2984n99dfqdgxjypypy" { - fmt.Printf("at block height %v found slash event: height %v, endingPeriod %v, rewards before: %v\n", - ctx.BlockHeight(), - height, endingPeriod, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - } rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) // note: necessary to truncate so we don't allow withdrawing more rewards than owed stake = stake.MulTruncate(sdk.OneDec().Sub(event.Fraction)) @@ -91,13 +78,6 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // calculate rewards for final period rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - if del.GetValidatorAddr().String() == "cosmosvaloper1qu379fd7lzvl9pfwclw2984n99dfqdgxjypypy" { - starting := k.GetValidatorHistoricalRewards(ctx, val.GetOperator(), startingPeriod) - ending := k.GetValidatorHistoricalRewards(ctx, val.GetOperator(), endingPeriod) - fmt.Printf("rewards for final period (startingPeriod %v, endingPeriod %v) with stake %v: %v\n", startingPeriod, endingPeriod, stake, k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - fmt.Printf("starting: %v, ending: %v\n", starting, ending) - } - return } @@ -121,12 +101,6 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de coins, remainder := rewards.TruncateDecimal() outstanding := k.GetValidatorOutstandingRewards(ctx, del.GetValidatorAddr()) - if len(rewards) > 0 && len(outstanding) > 0 && rewards[0].IsGTE(outstanding[0]) { - fmt.Printf("startingPeriod: %v\n", startingPeriod) - fmt.Printf("endingPeriod: %v\n", endingPeriod) - fmt.Printf("withdraw from %v to %v\n", val, del) - fmt.Printf("rewards: %v, outstanding: %v\n", rewards, outstanding) - } k.SetValidatorOutstandingRewards(ctx, del.GetValidatorAddr(), outstanding.Sub(rewards)) feePool := k.GetFeePool(ctx) feePool.CommunityPool = feePool.CommunityPool.Add(remainder) diff --git a/x/distribution/keeper/querier.go b/x/distribution/keeper/querier.go index 85ffc2ad0467..348c8b9a0e3f 100644 --- a/x/distribution/keeper/querier.go +++ b/x/distribution/keeper/querier.go @@ -12,14 +12,14 @@ import ( // nolint const ( - QueryParams = "params" - QueryOutstandingRewards = "outstanding_rewards" - QueryValidatorCommission = "validator_commission" - QueryValidatorSlashes = "validator_slashes" - QueryDelegationRewards = "delegation_rewards" - QueryDelegatorTotalRewards = "delegator_total_rewards" - QueryDelegatorValidators = "delegator_validators" - QueryWithdrawAddr = "withdraw_addr" + QueryParams = "params" + QueryValidatorOutstandingRewards = "validator_outstanding_rewards" + QueryValidatorCommission = "validator_commission" + QueryValidatorSlashes = "validator_slashes" + QueryDelegationRewards = "delegation_rewards" + QueryDelegatorTotalRewards = "delegator_total_rewards" + QueryDelegatorValidators = "delegator_validators" + QueryWithdrawAddr = "withdraw_addr" ParamCommunityTax = "community_tax" ParamBaseProposerReward = "base_proposer_reward" @@ -33,8 +33,8 @@ func NewQuerier(k Keeper) sdk.Querier { case QueryParams: return queryParams(ctx, path[1:], req, k) - case QueryOutstandingRewards: - return queryOutstandingRewards(ctx, path[1:], req, k) + case QueryValidatorOutstandingRewards: + return queryValidatorOutstandingRewards(ctx, path[1:], req, k) case QueryValidatorCommission: return queryValidatorCommission(ctx, path[1:], req, k) @@ -91,8 +91,25 @@ func queryParams(ctx sdk.Context, path []string, req abci.RequestQuery, k Keeper } } -func queryOutstandingRewards(ctx sdk.Context, path []string, req abci.RequestQuery, k Keeper) ([]byte, sdk.Error) { - bz, err := codec.MarshalJSONIndent(k.cdc, k.GetValidatorOutstandingRewards(ctx, sdk.ValAddress([]byte{}))) // TODO +// params for query 'custom/distr/validator_outstanding_rewards' +type QueryValidatorOutstandingRewardsParams struct { + ValidatorAddress sdk.ValAddress `json:"validator_address"` +} + +// creates a new instance of QueryValidatorOutstandingRewardsParams +func NewQueryValidatorOutstandingRewardsParams(validatorAddr sdk.ValAddress) QueryValidatorOutstandingRewardsParams { + return QueryValidatorOutstandingRewardsParams{ + ValidatorAddress: validatorAddr, + } +} + +func queryValidatorOutstandingRewards(ctx sdk.Context, path []string, req abci.RequestQuery, k Keeper) ([]byte, sdk.Error) { + var params QueryValidatorOutstandingRewardsParams + err := k.cdc.UnmarshalJSON(req.Data, ¶ms) + if err != nil { + return nil, sdk.ErrUnknownRequest(sdk.AppendMsgToErr("incorrectly formatted request data", err.Error())) + } + bz, err := codec.MarshalJSONIndent(k.cdc, k.GetValidatorOutstandingRewards(ctx, params.ValidatorAddress)) if err != nil { return nil, sdk.ErrInternal(sdk.AppendMsgToErr("could not marshal result to JSON", err.Error())) } diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index ee2dddc43fa3..33ca4415101d 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -62,7 +62,7 @@ func CanWithdrawInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant // iterate over all validators sk.IterateValidators(ctx, func(_ int64, val sdk.Validator) (stop bool) { _ = k.WithdrawValidatorCommission(ctx, val.GetOperator()) - // ugh so slow TODO FIXME + // TODO fetch delegations just for the validator, requires sdk.ValidatorSet change // iterate over all current delegations, withdraw rewards dels := sk.GetAllSDKDelegations(ctx) for _, delegation := range dels { From e434724fbaf06a2f3b43e3701065ab2229b291c7 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 22:24:20 +0100 Subject: [PATCH 27/50] Fixes --- x/distribution/client/cli/query.go | 12 ++++++------ x/distribution/client/module_client.go | 2 +- x/slashing/keeper.go | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/x/distribution/client/cli/query.go b/x/distribution/client/cli/query.go index 4adcaa13098f..06fbc4661a1c 100644 --- a/x/distribution/client/cli/query.go +++ b/x/distribution/client/cli/query.go @@ -32,22 +32,22 @@ func GetCmdQueryParams(queryRoute string, cdc *codec.Codec) *cobra.Command { } } -// GetCmdQueryOutstandingRewards implements the query outstanding rewards command. -func GetCmdQueryOutstandingRewards(queryRoute string, cdc *codec.Codec) *cobra.Command { +// GetCmdQueryValidatorOutstandingRewards implements the query validator outstanding rewards command. +func GetCmdQueryValidatorOutstandingRewards(queryRoute string, cdc *codec.Codec) *cobra.Command { return &cobra.Command{ - Use: "outstanding-rewards", + Use: "validator-outstanding-rewards", Args: cobra.NoArgs, - Short: "Query distribution outstanding (un-withdrawn) rewards", + Short: "Query distribution outstanding (un-withdrawn) rewards for a validator", RunE: func(cmd *cobra.Command, args []string) error { cliCtx := context.NewCLIContext().WithCodec(cdc) - route := fmt.Sprintf("custom/%s/outstanding_rewards", queryRoute) + route := fmt.Sprintf("custom/%s/validator_outstanding_rewards", queryRoute) res, err := cliCtx.QueryWithData(route, []byte{}) if err != nil { return err } - var outstandingRewards types.OutstandingRewards + var outstandingRewards types.ValidatorOutstandingRewards cdc.MustUnmarshalJSON(res, &outstandingRewards) return cliCtx.PrintOutput(outstandingRewards) }, diff --git a/x/distribution/client/module_client.go b/x/distribution/client/module_client.go index b9dcb1c82bac..0acb62a72003 100644 --- a/x/distribution/client/module_client.go +++ b/x/distribution/client/module_client.go @@ -27,7 +27,7 @@ func (mc ModuleClient) GetQueryCmd() *cobra.Command { distQueryCmd.AddCommand(client.GetCommands( distCmds.GetCmdQueryParams(mc.storeKey, mc.cdc), - distCmds.GetCmdQueryOutstandingRewards(mc.storeKey, mc.cdc), + distCmds.GetCmdQueryValidatorOutstandingRewards(mc.storeKey, mc.cdc), distCmds.GetCmdQueryValidatorCommission(mc.storeKey, mc.cdc), distCmds.GetCmdQueryValidatorSlashes(mc.storeKey, mc.cdc), distCmds.GetCmdQueryDelegatorRewards(mc.storeKey, mc.cdc), diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 3ad81be06a3c..8b8360492b6c 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -47,7 +47,6 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio consAddr := sdk.ConsAddress(addr) pubkey, err := k.getPubkey(ctx, addr) if err != nil { - return // Ignore evidence that cannot be handled. // NOTE: // We used to panic with: @@ -57,6 +56,7 @@ func (k Keeper) handleDoubleSign(ctx sdk.Context, addr crypto.Address, infractio // allowable but none of the disallowed evidence types. Instead of // getting this coordination right, it is easier to relax the // constraints and ignore evidence that cannot be handled. + return } // Reject evidence if the double-sign is too old From 9e1a4e4fc40f82f13bb8c3b2b3aa309887e8e98d Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 22:33:06 +0100 Subject: [PATCH 28/50] Testcase fixes --- x/distribution/keeper/allocation_test.go | 23 ++++++++---------- x/distribution/keeper/delegation_test.go | 31 ++---------------------- x/distribution/keeper/keeper_test.go | 6 ++--- x/distribution/keeper/querier_test.go | 13 +++++----- 4 files changed, 21 insertions(+), 52 deletions(-) diff --git a/x/distribution/keeper/allocation_test.go b/x/distribution/keeper/allocation_test.go index 4ac5b3f656d8..8bbab7aef20a 100644 --- a/x/distribution/keeper/allocation_test.go +++ b/x/distribution/keeper/allocation_test.go @@ -14,9 +14,6 @@ func TestAllocateTokensToValidatorWithCommission(t *testing.T) { ctx, _, k, sk, _ := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, @@ -44,9 +41,6 @@ func TestAllocateTokensToManyValidators(t *testing.T) { ctx, _, k, sk, fck := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, @@ -69,7 +63,8 @@ func TestAllocateTokensToManyValidators(t *testing.T) { } // assert initial state: zero outstanding rewards, zero community pool, zero commission, zero current rewards - require.True(t, k.GetOutstandingRewards(ctx).IsZero()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr1).IsZero()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr2).IsZero()) require.True(t, k.GetFeePool(ctx).CommunityPool.IsZero()) require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr1).IsZero()) require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr2).IsZero()) @@ -94,7 +89,8 @@ func TestAllocateTokensToManyValidators(t *testing.T) { k.AllocateTokens(ctx, 200, 200, valConsAddr2, votes) // 98 outstanding rewards (100 less 2 to community pool) - require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(98)}}, k.GetOutstandingRewards(ctx)) + require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(465, 1)}}, k.GetValidatorOutstandingRewards(ctx, valOpAddr1)) + require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDecWithPrec(515, 1)}}, k.GetValidatorOutstandingRewards(ctx, valOpAddr2)) // 2 community pool coins require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(2)}}, k.GetFeePool(ctx).CommunityPool) // 50% commission for first proposer, (0.5 * 93%) * 100 / 2 = 23.25 @@ -112,9 +108,6 @@ func TestAllocateTokensTruncation(t *testing.T) { ctx, _, k, sk, fck := CreateTestInputAdvanced(t, false, 1000000, communityTax) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 10% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(1, 1), sdk.NewDecWithPrec(1, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, @@ -147,7 +140,9 @@ func TestAllocateTokensTruncation(t *testing.T) { } // assert initial state: zero outstanding rewards, zero community pool, zero commission, zero current rewards - require.True(t, k.GetOutstandingRewards(ctx).IsZero()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr1).IsZero()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr2).IsZero()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr3).IsZero()) require.True(t, k.GetFeePool(ctx).CommunityPool.IsZero()) require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr1).IsZero()) require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr2).IsZero()) @@ -175,5 +170,7 @@ func TestAllocateTokensTruncation(t *testing.T) { } k.AllocateTokens(ctx, 31, 31, valConsAddr2, votes) - require.True(t, k.GetOutstandingRewards(ctx).IsValid()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr1).IsValid()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr2).IsValid()) + require.True(t, k.GetValidatorOutstandingRewards(ctx, valOpAddr3).IsValid()) } diff --git a/x/distribution/keeper/delegation_test.go b/x/distribution/keeper/delegation_test.go index d62e4b8e1936..681158465007 100644 --- a/x/distribution/keeper/delegation_test.go +++ b/x/distribution/keeper/delegation_test.go @@ -13,9 +13,6 @@ func TestCalculateRewardsBasic(t *testing.T) { ctx, _, k, sk, _ := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, @@ -66,9 +63,6 @@ func TestCalculateRewardsAfterSlash(t *testing.T) { ctx, _, k, sk, _ := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) valPower := int64(100) @@ -129,9 +123,6 @@ func TestCalculateRewardsAfterManySlashes(t *testing.T) { ctx, _, k, sk, _ := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission power := int64(100) valTokens := sdk.TokensFromTendermintPower(power) @@ -203,9 +194,6 @@ func TestCalculateRewardsMultiDelegator(t *testing.T) { ctx, _, k, sk, _ := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, @@ -263,9 +251,6 @@ func TestWithdrawDelegationRewardsBasic(t *testing.T) { ctx, ak, k, sk, _ := CreateTestInputDefault(t, false, balancePower) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission power := int64(100) valTokens := sdk.TokensFromTendermintPower(power) @@ -294,7 +279,6 @@ func TestWithdrawDelegationRewardsBasic(t *testing.T) { initial := sdk.TokensFromTendermintPower(10) tokens := sdk.DecCoins{sdk.NewDecCoin(sdk.DefaultBondDenom, initial)} - k.SetOutstandingRewards(ctx, tokens) k.AllocateTokensToValidator(ctx, val, tokens) // historical count should be 2 (initial + latest for delegation) @@ -328,9 +312,6 @@ func TestCalculateRewardsAfterManySlashesInSameBlock(t *testing.T) { ctx, _, k, sk, _ := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission power := int64(100) valTokens := sdk.TokensFromTendermintPower(power) @@ -395,9 +376,6 @@ func TestCalculateRewardsMultiDelegatorMultiSlash(t *testing.T) { ctx, _, k, sk, _ := CreateTestInputDefault(t, false, 1000) sh := staking.NewHandler(sk) - // initialize state - k.SetOutstandingRewards(ctx, sdk.DecCoins{}) - // create validator with 50% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) power := int64(100) @@ -471,9 +449,6 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) { totalRewards := sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, sdk.NewDec(initial*2))} tokens := sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, sdk.NewDec(initial))} - // initialize state - k.SetOutstandingRewards(ctx, totalRewards) - // create validator with 50% commission commission := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, @@ -540,8 +515,7 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) { // commission should be zero require.True(t, k.GetValidatorAccumulatedCommission(ctx, valOpAddr1).IsZero()) - totalRewards = k.GetOutstandingRewards(ctx).Add(tokens) - k.SetOutstandingRewards(ctx, totalRewards) + totalRewards = totalRewards.Add(tokens) // allocate some more rewards k.AllocateTokensToValidator(ctx, val, tokens) @@ -567,8 +541,7 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) { // commission should be half initial require.Equal(t, sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(initial / 2)}}, k.GetValidatorAccumulatedCommission(ctx, valOpAddr1)) - totalRewards = k.GetOutstandingRewards(ctx).Add(tokens) - k.SetOutstandingRewards(ctx, totalRewards) + totalRewards = k.GetValidatorOutstandingRewards(ctx, valOpAddr1).Add(tokens) // allocate some more rewards k.AllocateTokensToValidator(ctx, val, tokens) diff --git a/x/distribution/keeper/keeper_test.go b/x/distribution/keeper/keeper_test.go index fc6f7cc0a1cc..59471a13287a 100644 --- a/x/distribution/keeper/keeper_test.go +++ b/x/distribution/keeper/keeper_test.go @@ -30,9 +30,6 @@ func TestWithdrawValidatorCommission(t *testing.T) { sdk.NewDecCoinFromDec("stake", sdk.NewDec(3).Quo(sdk.NewDec(2))), } - // set zero outstanding rewards - keeper.SetOutstandingRewards(ctx, valCommission) - // check initial balance balance := ak.GetAccount(ctx, sdk.AccAddress(valOpAddr3)).GetCoins() expTokens := sdk.TokensFromTendermintPower(1000) @@ -40,6 +37,9 @@ func TestWithdrawValidatorCommission(t *testing.T) { sdk.NewCoin("stake", sdk.TokensFromTendermintPower(1000)), }, balance) + // set outstanding rewards + keeper.SetValidatorOutstandingRewards(ctx, valOpAddr3, valCommission) + // set commission keeper.SetValidatorAccumulatedCommission(ctx, valOpAddr3, valCommission) diff --git a/x/distribution/keeper/querier_test.go b/x/distribution/keeper/querier_test.go index 874b85034a5b..8c547e717740 100644 --- a/x/distribution/keeper/querier_test.go +++ b/x/distribution/keeper/querier_test.go @@ -57,13 +57,13 @@ func getQueriedParams(t *testing.T, ctx sdk.Context, cdc *codec.Codec, querier s return } -func getQueriedOutstandingRewards(t *testing.T, ctx sdk.Context, cdc *codec.Codec, querier sdk.Querier) (outstandingRewards sdk.DecCoins) { +func getQueriedValidatorOutstandingRewards(t *testing.T, ctx sdk.Context, cdc *codec.Codec, querier sdk.Querier, validatorAddr sdk.ValAddress) (outstandingRewards sdk.DecCoins) { query := abci.RequestQuery{ - Path: strings.Join([]string{custom, types.QuerierRoute, QueryOutstandingRewards}, "/"), - Data: []byte{}, + Path: strings.Join([]string{custom, types.QuerierRoute, QueryValidatorOutstandingRewards}, "/"), + Data: cdc.MustMarshalJSON(NewQueryValidatorOutstandingRewardsParams(validatorAddr)), } - bz, err := querier(ctx, []string{QueryOutstandingRewards}, query) + bz, err := querier(ctx, []string{QueryValidatorOutstandingRewards}, query) require.Nil(t, err) require.Nil(t, cdc.UnmarshalJSON(bz, &outstandingRewards)) @@ -131,8 +131,8 @@ func TestQueries(t *testing.T) { // test outstanding rewards query outstandingRewards := sdk.DecCoins{{"mytoken", sdk.NewDec(3)}, {"myothertoken", sdk.NewDecWithPrec(3, 7)}} - keeper.SetOutstandingRewards(ctx, outstandingRewards) - retOutstandingRewards := getQueriedOutstandingRewards(t, ctx, cdc, querier) + keeper.SetValidatorOutstandingRewards(ctx, valOpAddr1, outstandingRewards) + retOutstandingRewards := getQueriedValidatorOutstandingRewards(t, ctx, cdc, querier, valOpAddr1) require.Equal(t, outstandingRewards, retOutstandingRewards) // test validator commission query @@ -155,7 +155,6 @@ func TestQueries(t *testing.T) { // test delegation rewards query sh := staking.NewHandler(sk) - keeper.SetOutstandingRewards(ctx, sdk.DecCoins{}) comm := staking.NewCommissionMsg(sdk.NewDecWithPrec(5, 1), sdk.NewDecWithPrec(5, 1), sdk.NewDec(0)) msg := staking.NewMsgCreateValidator(valOpAddr1, valConsPk1, sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(100)), staking.Description{}, comm, sdk.OneInt()) From bf9c168a779a86006cbb2b885885621008c4047b Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 22:50:47 +0100 Subject: [PATCH 29/50] Fixup outstanding-rewards REST --- client/lcd/lcd_test.go | 4 ++-- x/distribution/alias.go | 17 +++++++++-------- x/distribution/client/rest/query.go | 19 +++++++++++++------ 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/client/lcd/lcd_test.go b/client/lcd/lcd_test.go index ccb5895dad92..140c4657d533 100644 --- a/client/lcd/lcd_test.go +++ b/client/lcd/lcd_test.go @@ -945,7 +945,7 @@ func TestDistributionFlow(t *testing.T) { operAddr := sdk.AccAddress(valAddr) var rewards sdk.DecCoins - res, body := Request(t, port, "GET", fmt.Sprintf("/distribution/outstanding_rewards"), nil) + res, body := Request(t, port, "GET", fmt.Sprintf("/distribution/validators/%s/outstanding_rewards", valAddr), nil) require.Equal(t, http.StatusOK, res.StatusCode, body) require.NoError(t, cdc.UnmarshalJSON([]byte(body), &rewards)) @@ -967,7 +967,7 @@ func TestDistributionFlow(t *testing.T) { require.Equal(t, uint32(0), resultTx.Code) // Query outstanding rewards changed - res, body = Request(t, port, "GET", fmt.Sprintf("/distribution/outstanding_rewards"), nil) + res, body = Request(t, port, "GET", fmt.Sprintf("/distribution/validators/%s/outstanding_rewards", valAddr), nil) require.Equal(t, http.StatusOK, res.StatusCode, body) require.NoError(t, cdc.UnmarshalJSON([]byte(body), &rewards)) diff --git a/x/distribution/alias.go b/x/distribution/alias.go index 85b93fc1745b..34ad7dd57604 100644 --- a/x/distribution/alias.go +++ b/x/distribution/alias.go @@ -50,14 +50,15 @@ var ( NewMsgWithdrawDelegatorReward = types.NewMsgWithdrawDelegatorReward NewMsgWithdrawValidatorCommission = types.NewMsgWithdrawValidatorCommission - NewKeeper = keeper.NewKeeper - NewQuerier = keeper.NewQuerier - NewQueryValidatorCommissionParams = keeper.NewQueryValidatorCommissionParams - NewQueryValidatorSlashesParams = keeper.NewQueryValidatorSlashesParams - NewQueryDelegationRewardsParams = keeper.NewQueryDelegationRewardsParams - NewQueryDelegatorParams = keeper.NewQueryDelegatorParams - NewQueryDelegatorWithdrawAddrParams = keeper.NewQueryDelegatorWithdrawAddrParams - DefaultParamspace = keeper.DefaultParamspace + NewKeeper = keeper.NewKeeper + NewQuerier = keeper.NewQuerier + NewQueryValidatorOutstandingRewardsParams = keeper.NewQueryValidatorOutstandingRewardsParams + NewQueryValidatorCommissionParams = keeper.NewQueryValidatorCommissionParams + NewQueryValidatorSlashesParams = keeper.NewQueryValidatorSlashesParams + NewQueryDelegationRewardsParams = keeper.NewQueryDelegationRewardsParams + NewQueryDelegatorParams = keeper.NewQueryDelegatorParams + NewQueryDelegatorWithdrawAddrParams = keeper.NewQueryDelegatorWithdrawAddrParams + DefaultParamspace = keeper.DefaultParamspace RegisterCodec = types.RegisterCodec DefaultGenesisState = types.DefaultGenesisState diff --git a/x/distribution/client/rest/query.go b/x/distribution/client/rest/query.go index a8a4c98658fc..a8a6e13263e4 100644 --- a/x/distribution/client/rest/query.go +++ b/x/distribution/client/rest/query.go @@ -49,17 +49,18 @@ func registerQueryRoutes(cliCtx context.CLIContext, r *mux.Router, validatorRewardsHandlerFn(cliCtx, cdc, queryRoute), ).Methods("GET") + // Outstanding rewards of a single validator + r.HandleFunc( + "/distribution/validators/{validatorAddr}/outstanding_rewards", + outstandingRewardsHandlerFn(cliCtx, cdc, queryRoute), + ).Methods("GET") + // Get the current distribution parameter values r.HandleFunc( "/distribution/parameters", paramsHandlerFn(cliCtx, cdc, queryRoute), ).Methods("GET") - // Get the current distribution pool - r.HandleFunc( - "/distribution/outstanding_rewards", - outstandingRewardsHandlerFn(cliCtx, cdc, queryRoute), - ).Methods("GET") } // HTTP request handler to query the total rewards balance from all delegations @@ -211,7 +212,13 @@ func outstandingRewardsHandlerFn(cliCtx context.CLIContext, cdc *codec.Codec, queryRoute string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - res, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/outstanding_rewards", queryRoute), []byte{}) + validatorAddr, ok := checkValidatorAddressVar(w, r) + if !ok { + return + } + + bin := cdc.MustMarshalJSON(distribution.NewQueryValidatorOutstandingRewardsParams(validatorAddr)) + res, err := cliCtx.QueryWithData(fmt.Sprintf("custom/%s/validator_outstanding_rewards", queryRoute), bin) if err != nil { rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) return From db5c7993fe4bf7d9b7f78109b84599ef3e3ae057 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 22:56:47 +0100 Subject: [PATCH 30/50] Naming agreement; swagger.yaml update --- client/lcd/swagger-ui/swagger.yaml | 42 ++++++++++++++---------- x/distribution/keeper/alias_functions.go | 2 +- x/staking/simulation/invariants.go | 2 +- x/staking/types/expected_keepers.go | 2 +- 4 files changed, 27 insertions(+), 21 deletions(-) diff --git a/client/lcd/swagger-ui/swagger.yaml b/client/lcd/swagger-ui/swagger.yaml index 803da4aedf3e..0c6b78da8c53 100644 --- a/client/lcd/swagger-ui/swagger.yaml +++ b/client/lcd/swagger-ui/swagger.yaml @@ -1558,6 +1558,28 @@ paths: description: Invalid validator address 500: description: Internal Server Error + /distribution/validators/{validatorAddr}/outstanding_rewards: + parameters: + - in: path + name: validatorAddr + description: Bech32 OperatorAddress of validator + required: true + type: string + get: + summary: Fee distribution outstanding rewards of a single validator + tags: + - ICS24 + produces: + - application/json + responses: + 200: + description: OK + schema: + type: array + items: + $ref: "#/definitions/Coin" + 500: + description: Internal Server Error /distribution/validators/{validatorAddr}/rewards: parameters: - in: path @@ -1566,8 +1588,8 @@ paths: required: true type: string get: - summary: Commission and self-delegation rewards of a single a validator - description: Query the commission and self-delegation rewards of a validator. + summary: Commission and self-delegation rewards of a single validator + description: Query the commission and self-delegation rewards of validator. tags: - ICS24 produces: @@ -1630,22 +1652,6 @@ paths: type: string 500: description: Internal Server Error - /distribution/outstanding_rewards: - get: - summary: Fee distribution outstanding rewards - tags: - - ICS24 - produces: - - application/json - responses: - 200: - description: OK - schema: - type: array - items: - $ref: "#/definitions/Coin" - 500: - description: Internal Server Error definitions: CheckTxResult: type: object diff --git a/x/distribution/keeper/alias_functions.go b/x/distribution/keeper/alias_functions.go index a2afd8b0a3f6..6081cf348a0d 100644 --- a/x/distribution/keeper/alias_functions.go +++ b/x/distribution/keeper/alias_functions.go @@ -5,7 +5,7 @@ import ( ) // get outstanding rewards -func (k Keeper) GetOutstandingRewardsCoins(ctx sdk.Context, val sdk.ValAddress) sdk.DecCoins { +func (k Keeper) GetValidatorOutstandingRewardsCoins(ctx sdk.Context, val sdk.ValAddress) sdk.DecCoins { return k.GetValidatorOutstandingRewards(ctx, val) } diff --git a/x/staking/simulation/invariants.go b/x/staking/simulation/invariants.go index 6000b11a5c8e..db6249719f3f 100644 --- a/x/staking/simulation/invariants.go +++ b/x/staking/simulation/invariants.go @@ -68,7 +68,7 @@ func SupplyInvariants(k staking.Keeper, loose = loose.Add(validator.GetTokens().ToDec()) } // add yet-to-be-withdrawn - loose = loose.Add(d.GetOutstandingRewardsCoins(ctx, validator.GetOperator()).AmountOf(k.BondDenom(ctx))) + loose = loose.Add(d.GetValidatorOutstandingRewardsCoins(ctx, validator.GetOperator()).AmountOf(k.BondDenom(ctx))) return false }) diff --git a/x/staking/types/expected_keepers.go b/x/staking/types/expected_keepers.go index 9395d3733490..54c9bad62287 100644 --- a/x/staking/types/expected_keepers.go +++ b/x/staking/types/expected_keepers.go @@ -5,7 +5,7 @@ import sdk "github.com/cosmos/cosmos-sdk/types" // expected coin keeper type DistributionKeeper interface { GetFeePoolCommunityCoins(ctx sdk.Context) sdk.DecCoins - GetOutstandingRewardsCoins(ctx sdk.Context, val sdk.ValAddress) sdk.DecCoins + GetValidatorOutstandingRewardsCoins(ctx sdk.Context, val sdk.ValAddress) sdk.DecCoins } // expected fee collection keeper From a9a72d151d5f214f5e0e868aaef7359ee302c5ac Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 27 Feb 2019 23:04:24 +0100 Subject: [PATCH 31/50] Remove unused param --- cmd/gaia/app/invariants.go | 2 +- x/distribution/simulation/invariants.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/gaia/app/invariants.go b/cmd/gaia/app/invariants.go index ecd9e39a5a1f..535cbff3711c 100644 --- a/cmd/gaia/app/invariants.go +++ b/cmd/gaia/app/invariants.go @@ -15,7 +15,7 @@ import ( func (app *GaiaApp) runtimeInvariants() []sdk.Invariant { return []sdk.Invariant{ banksim.NonnegativeBalanceInvariant(app.accountKeeper), - distrsim.NonNegativeOutstandingInvariant(app.distrKeeper, app.stakingKeeper), + distrsim.NonNegativeOutstandingInvariant(app.distrKeeper), stakingsim.SupplyInvariants(app.stakingKeeper, app.feeCollectionKeeper, app.distrKeeper, app.accountKeeper), stakingsim.NonNegativePowerInvariant(app.stakingKeeper), } diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index 33ca4415101d..a53065b6e885 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -15,7 +15,7 @@ func AllInvariants(d distr.Keeper, stk types.StakingKeeper) sdk.Invariant { if err != nil { return err } - err = NonNegativeOutstandingInvariant(d, stk)(ctx) + err = NonNegativeOutstandingInvariant(d)(ctx) if err != nil { return err } @@ -28,7 +28,7 @@ func AllInvariants(d distr.Keeper, stk types.StakingKeeper) sdk.Invariant { } // NonNegativeOutstandingInvariant checks that outstanding unwithdrawn fees are never negative -func NonNegativeOutstandingInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant { +func NonNegativeOutstandingInvariant(k distr.Keeper) sdk.Invariant { return func(ctx sdk.Context) error { var outstanding sdk.DecCoins From bc26e22ced8f72037b9ffc36ee49d6673db946be Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 18:34:40 +0100 Subject: [PATCH 32/50] Fix hook --- x/distribution/keeper/hooks.go | 9 +++++---- x/distribution/keeper/store.go | 5 +++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 44a1f5f9c887..6a860cc0107d 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -22,6 +22,7 @@ func (h Hooks) AfterValidatorCreated(ctx sdk.Context, valAddr sdk.ValAddress) { func (h Hooks) BeforeValidatorModified(ctx sdk.Context, valAddr sdk.ValAddress) { } func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) { + // force-withdraw commission commission := h.k.GetValidatorAccumulatedCommission(ctx, valAddr) if !commission.IsZero() { @@ -32,10 +33,6 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr feePool.CommunityPool = feePool.CommunityPool.Add(remainder) h.k.SetFeePool(ctx, feePool) - // update outstanding - outstanding := h.k.GetValidatorOutstandingRewards(ctx, valAddr) - h.k.SetValidatorOutstandingRewards(ctx, valAddr, outstanding.Sub(commission)) - // add to validator account if !coins.IsZero() { accAddr := sdk.AccAddress(valAddr) @@ -46,6 +43,10 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr } } } + + // delete outstanding + h.k.DeleteValidatorOutstandingRewards(ctx, valAddr) + // remove commission record h.k.DeleteValidatorAccumulatedCommission(ctx, valAddr) diff --git a/x/distribution/keeper/store.go b/x/distribution/keeper/store.go index d9538cd672ad..04c8add4f61e 100644 --- a/x/distribution/keeper/store.go +++ b/x/distribution/keeper/store.go @@ -278,6 +278,11 @@ func (k Keeper) SetValidatorOutstandingRewards(ctx sdk.Context, val sdk.ValAddre store.Set(GetValidatorOutstandingRewardsKey(val), b) } +func (k Keeper) DeleteValidatorOutstandingRewards(ctx sdk.Context, val sdk.ValAddress) { + store := ctx.KVStore(k.storeKey) + store.Delete(GetValidatorOutstandingRewardsKey(val)) +} + // iterate validator outstanding rewards func (k Keeper) IterateValidatorOutstandingRewards(ctx sdk.Context, handler func(val sdk.ValAddress, rewards types.ValidatorOutstandingRewards) (stop bool)) { store := ctx.KVStore(k.storeKey) From 713e7b66404d1479b0bba948f37d7c8dad724623 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 18:42:25 +0100 Subject: [PATCH 33/50] Fix linter --- x/distribution/keeper/store.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/distribution/keeper/store.go b/x/distribution/keeper/store.go index 04c8add4f61e..1326c89d4548 100644 --- a/x/distribution/keeper/store.go +++ b/x/distribution/keeper/store.go @@ -278,6 +278,7 @@ func (k Keeper) SetValidatorOutstandingRewards(ctx sdk.Context, val sdk.ValAddre store.Set(GetValidatorOutstandingRewardsKey(val), b) } +// delete validator outstanding rewards func (k Keeper) DeleteValidatorOutstandingRewards(ctx sdk.Context, val sdk.ValAddress) { store := ctx.KVStore(k.storeKey) store.Delete(GetValidatorOutstandingRewardsKey(val)) From 38bb5e15ea8084603301cebe0398cf295c4c0f52 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 18:58:39 +0100 Subject: [PATCH 34/50] Add outstanding to community pool --- x/distribution/keeper/hooks.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 6a860cc0107d..0c52b205974c 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -44,6 +44,12 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr } } + // add outstanding to community pool + outstanding := h.k.GetValidatorOutstandingRewards(ctx, valAddr) + feePool := h.k.GetFeePool(ctx) + feePool.CommunityPool = feePool.CommunityPool.Add(outstanding) + h.k.SetFeePool(ctx, feePool) + // delete outstanding h.k.DeleteValidatorOutstandingRewards(ctx, valAddr) From 93f0556b261d5a03bd95835e1db604b56c925ffd Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 19:21:35 +0100 Subject: [PATCH 35/50] ... --- x/distribution/keeper/hooks.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index 0c52b205974c..b66c772cb8d5 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -23,6 +23,9 @@ func (h Hooks) BeforeValidatorModified(ctx sdk.Context, valAddr sdk.ValAddress) } func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr sdk.ValAddress) { + // fetch outstanding + outstanding := h.k.GetValidatorOutstandingRewards(ctx, valAddr) + // force-withdraw commission commission := h.k.GetValidatorAccumulatedCommission(ctx, valAddr) if !commission.IsZero() { @@ -35,6 +38,8 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr // add to validator account if !coins.IsZero() { + outstanding = outstanding.Sub(sdk.NewDecCoins(coins)) + accAddr := sdk.AccAddress(valAddr) withdrawAddr := h.k.GetDelegatorWithdrawAddr(ctx, accAddr) @@ -45,7 +50,6 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr } // add outstanding to community pool - outstanding := h.k.GetValidatorOutstandingRewards(ctx, valAddr) feePool := h.k.GetFeePool(ctx) feePool.CommunityPool = feePool.CommunityPool.Add(outstanding) h.k.SetFeePool(ctx, feePool) From ea06e12109221d4de4e3810874af83598e4c90bc Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 19:22:54 +0100 Subject: [PATCH 36/50] ... --- x/distribution/keeper/hooks.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/x/distribution/keeper/hooks.go b/x/distribution/keeper/hooks.go index b66c772cb8d5..1ce7a1fd63dc 100644 --- a/x/distribution/keeper/hooks.go +++ b/x/distribution/keeper/hooks.go @@ -29,6 +29,10 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr // force-withdraw commission commission := h.k.GetValidatorAccumulatedCommission(ctx, valAddr) if !commission.IsZero() { + // subtract from outstanding + outstanding = outstanding.Sub(commission) + + // split into integral & remainder coins, remainder := commission.TruncateDecimal() // remainder to community pool @@ -38,7 +42,6 @@ func (h Hooks) AfterValidatorRemoved(ctx sdk.Context, _ sdk.ConsAddress, valAddr // add to validator account if !coins.IsZero() { - outstanding = outstanding.Sub(sdk.NewDecCoins(coins)) accAddr := sdk.AccAddress(valAddr) withdrawAddr := h.k.GetDelegatorWithdrawAddr(ctx, accAddr) From 7d02f1b2a6f4d7cd0c473cdd7da2c0382b05b123 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 20:20:34 +0100 Subject: [PATCH 37/50] Correctly calculate effective slash fraction --- x/staking/keeper/slash.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index dbc3b84eadfb..a7e5fd114f9a 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -57,17 +57,6 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh // call the before-modification hook k.BeforeValidatorModified(ctx, operatorAddress) - // we need to calculate the *effective* slash fraction for distribution - if validator.Tokens.GT(sdk.ZeroInt()) { - effectiveFraction := slashAmountDec.Quo(validator.Tokens.ToDec()) - // possible if power has changed - if effectiveFraction.GT(sdk.OneDec()) { - effectiveFraction = sdk.OneDec() - } - // call the before-slashed hook - k.BeforeValidatorSlashed(ctx, operatorAddress, effectiveFraction) - } - // Track remaining slash amount for the validator // This will decrease when we slash unbondings and // redelegations, as that stake has since unbonded @@ -115,6 +104,17 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh tokensToBurn := sdk.MinInt(remainingSlashAmount, validator.Tokens) tokensToBurn = sdk.MaxInt(tokensToBurn, sdk.ZeroInt()) // defensive. + // we need to calculate the *effective* slash fraction for distribution + if validator.Tokens.GT(sdk.ZeroInt()) { + effectiveFraction := tokensToBurn.ToDec().Quo(validator.Tokens.ToDec()) + // possible if power has changed + if effectiveFraction.GT(sdk.OneDec()) { + effectiveFraction = sdk.OneDec() + } + // call the before-slashed hook + k.BeforeValidatorSlashed(ctx, operatorAddress, effectiveFraction) + } + // Deduct from validator's bonded tokens and update the validator. // The deducted tokens are returned to pool.NotBondedTokens. // TODO: Move the token accounting outside of `RemoveValidatorTokens` so it is less confusing From 132b30fc5b7715ceb08b20d36e6eccca6772795d Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 20:40:43 +0100 Subject: [PATCH 38/50] Maximum of the slash fractions --- x/distribution/keeper/delegation.go | 2 ++ x/staking/keeper/slash.go | 3 +++ 2 files changed, 5 insertions(+) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index e9ce231cd0d9..9028eb53d455 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/distribution/types" @@ -65,6 +66,7 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d if endingHeight >= startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { + fmt.Printf("hit slash event: %v\n", event) endingPeriod := event.ValidatorPeriod rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) // note: necessary to truncate so we don't allow withdrawing more rewards than owed diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index a7e5fd114f9a..8ee42c5eeea9 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -111,6 +111,9 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh if effectiveFraction.GT(sdk.OneDec()) { effectiveFraction = sdk.OneDec() } + if effectiveFraction.LT(slashFactor) { + effectiveFraction = slashFactor + } // call the before-slashed hook k.BeforeValidatorSlashed(ctx, operatorAddress, effectiveFraction) } From 280d9f4db394b7a97e5a351eb4323021460359ab Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 20:58:49 +0100 Subject: [PATCH 39/50] Sanity check --- x/distribution/keeper/delegation.go | 5 ++++- x/staking/keeper/slash.go | 3 --- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 9028eb53d455..ed6ab47cb238 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -66,7 +66,6 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d if endingHeight >= startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { - fmt.Printf("hit slash event: %v\n", event) endingPeriod := event.ValidatorPeriod rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) // note: necessary to truncate so we don't allow withdrawing more rewards than owed @@ -77,6 +76,10 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d ) } + if stake.GT(del.GetShares().Mul(val.GetDelegatorShareExRate())) { + panic(fmt.Sprintf("calculated final stake greater than current stake: %s, %s", stake, del.GetShares().Mul(val.GetDelegatorShareExRate()))) + } + // calculate rewards for final period rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index 8ee42c5eeea9..a7e5fd114f9a 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -111,9 +111,6 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh if effectiveFraction.GT(sdk.OneDec()) { effectiveFraction = sdk.OneDec() } - if effectiveFraction.LT(slashFactor) { - effectiveFraction = slashFactor - } // call the before-slashed hook k.BeforeValidatorSlashed(ctx, operatorAddress, effectiveFraction) } From 0bc856e318135ae994112ff0276603834873cb98 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 21:14:21 +0100 Subject: [PATCH 40/50] Sanity check; on the right track --- types/decimal.go | 36 +++++++++++++++++++++++++++++ types/staking.go | 27 +++++++++++----------- x/distribution/keeper/delegation.go | 21 +++++++++++------ x/staking/keeper/slash.go | 2 +- x/staking/types/validator.go | 13 +++++++++++ 5 files changed, 78 insertions(+), 21 deletions(-) diff --git a/types/decimal.go b/types/decimal.go index 5f59a0f54857..963588a2ed1f 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -291,6 +291,20 @@ func (d Dec) QuoTruncate(d2 Dec) Dec { return Dec{chopped} } +func (d Dec) QuoRoundUp(d2 Dec) Dec { + // multiply precision twice + mul := new(big.Int).Mul(d.Int, precisionReuse) + mul.Mul(mul, precisionReuse) + + quo := new(big.Int).Quo(mul, d2.Int) + chopped := chopPrecisionAndRoundUp(quo) + + if chopped.BitLen() > 255+DecimalPrecisionBits { + panic("Int overflow") + } + return Dec{chopped} +} + // quotient func (d Dec) QuoInt(i Int) Dec { mul := new(big.Int).Quo(d.Int, i.i) @@ -412,6 +426,28 @@ func chopPrecisionAndRound(d *big.Int) *big.Int { } } +func chopPrecisionAndRoundUp(d *big.Int) *big.Int { + + // remove the negative and add it back when returning + if d.Sign() == -1 { + // make d positive, compute chopped value, and then un-mutate d + d = d.Neg(d) + d = chopPrecisionAndRound(d) + d = d.Neg(d) + return d + } + + // get the truncated quotient and remainder + quo, rem := d, big.NewInt(0) + quo, rem = quo.QuoRem(d, precisionReuse, rem) + + if rem.Sign() == 0 { // remainder is zero + return quo + } + + return quo.Add(quo, oneInt) +} + func chopPrecisionAndRoundNonMutative(d *big.Int) *big.Int { tmp := new(big.Int).Set(d) return chopPrecisionAndRound(tmp) diff --git a/types/staking.go b/types/staking.go index 51961f39c531..2e7dbd0eb87c 100644 --- a/types/staking.go +++ b/types/staking.go @@ -61,19 +61,20 @@ func (b BondStatus) Equal(b2 BondStatus) bool { // validator for a delegated proof of stake system type Validator interface { - GetJailed() bool // whether the validator is jailed - GetMoniker() string // moniker of the validator - GetStatus() BondStatus // status of the validator - GetOperator() ValAddress // operator address to receive/return validators coins - GetConsPubKey() crypto.PubKey // validation consensus pubkey - GetConsAddr() ConsAddress // validation consensus address - GetTokens() Int // validation tokens - GetBondedTokens() Int // validator bonded tokens - GetTendermintPower() int64 // validation power in tendermint - GetCommission() Dec // validator commission rate - GetMinSelfDelegation() Int // validator minimum self delegation - GetDelegatorShares() Dec // total outstanding delegator shares - GetDelegatorShareExRate() Dec // tokens per delegator share exchange rate + GetJailed() bool // whether the validator is jailed + GetMoniker() string // moniker of the validator + GetStatus() BondStatus // status of the validator + GetOperator() ValAddress // operator address to receive/return validators coins + GetConsPubKey() crypto.PubKey // validation consensus pubkey + GetConsAddr() ConsAddress // validation consensus address + GetTokens() Int // validation tokens + GetBondedTokens() Int // validator bonded tokens + GetTendermintPower() int64 // validation power in tendermint + GetCommission() Dec // validator commission rate + GetMinSelfDelegation() Int // validator minimum self delegation + GetDelegatorShares() Dec // total outstanding delegator shares + GetDelegatorShareExRate() Dec // tokens per delegator share exchange rate + GetDelegatorShareExRateTruncated() Dec // tokens per delegator share exchange rate } // validator which fulfills abci validator interface for use in Tendermint diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index ed6ab47cb238..c0cab908e5c3 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -21,7 +21,7 @@ func (k Keeper) initializeDelegation(ctx sdk.Context, val sdk.ValAddress, del sd // calculate delegation stake in tokens // we don't store directly, so multiply delegation shares * (tokens per share) // note: necessary to truncate so we don't allow withdrawing more rewards than owed - stake := delegation.GetShares().MulTruncate(validator.GetDelegatorShareExRate()) + stake := delegation.GetShares().MulTruncate(validator.GetDelegatorShareExRateTruncated()) k.SetDelegatorStartingInfo(ctx, val, del, types.NewDelegatorStartingInfo(previousPeriod, stake, uint64(ctx.BlockHeight()))) } @@ -54,30 +54,37 @@ func (k Keeper) calculateDelegationRewardsBetween(ctx sdk.Context, val sdk.Valid func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, del sdk.Delegation, endingPeriod uint64) (rewards sdk.DecCoins) { // fetch starting info for delegation startingInfo := k.GetDelegatorStartingInfo(ctx, del.GetValidatorAddr(), del.GetDelegatorAddr()) + + if startingInfo.Height == uint64(ctx.BlockHeight()) { + // started this height, no rewards yet + return + } + startingPeriod := startingInfo.PreviousPeriod stake := startingInfo.Stake // iterate through slashes and withdraw with calculated staking for sub-intervals // these offsets are dependent on *when* slashes happen - namely, in BeginBlock, after rewards are allocated... - // ... so we don't reduce stake for slashes which happened in the *first* block, because the delegation wouldn't have existed + // slashes which happened in the first block would have been before this delegation existed startingHeight := startingInfo.Height + 1 - // ... or slashes which happened in *this* block, since they would have happened after reward allocation - endingHeight := uint64(ctx.BlockHeight()) - 1 + // slashes this block happened after reward allocation, but we have to account for them for the stake sanity check + endingHeight := uint64(ctx.BlockHeight()) if endingHeight >= startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod - rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - // note: necessary to truncate so we don't allow withdrawing more rewards than owed stake = stake.MulTruncate(sdk.OneDec().Sub(event.Fraction)) + rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) startingPeriod = endingPeriod + // note: necessary to truncate so we don't allow withdrawing more rewards than owed + fmt.Printf("recalculated stake for delegator %s after slash of %v\n", del.GetDelegatorAddr(), event.Fraction) return false }, ) } if stake.GT(del.GetShares().Mul(val.GetDelegatorShareExRate())) { - panic(fmt.Sprintf("calculated final stake greater than current stake: %s, %s", stake, del.GetShares().Mul(val.GetDelegatorShareExRate()))) + panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake: %s, %s", del.GetDelegatorAddr(), stake, del.GetShares().Mul(val.GetDelegatorShareExRate()))) } // calculate rewards for final period diff --git a/x/staking/keeper/slash.go b/x/staking/keeper/slash.go index a7e5fd114f9a..dbd19a9410e3 100644 --- a/x/staking/keeper/slash.go +++ b/x/staking/keeper/slash.go @@ -106,7 +106,7 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh // we need to calculate the *effective* slash fraction for distribution if validator.Tokens.GT(sdk.ZeroInt()) { - effectiveFraction := tokensToBurn.ToDec().Quo(validator.Tokens.ToDec()) + effectiveFraction := tokensToBurn.ToDec().QuoRoundUp(validator.Tokens.ToDec()) // possible if power has changed if effectiveFraction.GT(sdk.OneDec()) { effectiveFraction = sdk.OneDec() diff --git a/x/staking/types/validator.go b/x/staking/types/validator.go index ebf51426d938..ac427e645b86 100644 --- a/x/staking/types/validator.go +++ b/x/staking/types/validator.go @@ -407,6 +407,16 @@ func (v Validator) DelegatorShareExRate() sdk.Dec { return v.Tokens.ToDec().Quo(v.DelegatorShares) } +// DelegatorShareExRateTruncated gets the exchange rate of tokens over delegator shares, truncated. +// UNITS: tokens/delegator-shares +func (v Validator) DelegatorShareExRateTruncated() sdk.Dec { + if v.DelegatorShares.IsZero() { + // the first delegation to a validator sets the exchange rate to one + return sdk.OneDec() + } + return v.Tokens.ToDec().QuoTruncate(v.DelegatorShares) +} + // get the bonded tokens which the validator holds func (v Validator) BondedTokens() sdk.Int { if v.Status == sdk.Bonded { @@ -446,3 +456,6 @@ func (v Validator) GetCommission() sdk.Dec { return v.Commission.Rate func (v Validator) GetMinSelfDelegation() sdk.Int { return v.MinSelfDelegation } func (v Validator) GetDelegatorShares() sdk.Dec { return v.DelegatorShares } func (v Validator) GetDelegatorShareExRate() sdk.Dec { return v.DelegatorShareExRate() } +func (v Validator) GetDelegatorShareExRateTruncated() sdk.Dec { + return v.DelegatorShareExRateTruncated() +} From 77959f535815deb4f348aeecf7e9c09cb6080111 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 22:08:13 +0100 Subject: [PATCH 41/50] Fix it --- types/decimal.go | 3 ++- x/distribution/keeper/delegation.go | 16 +++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/types/decimal.go b/types/decimal.go index 963588a2ed1f..6567caf03c4a 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -432,7 +432,8 @@ func chopPrecisionAndRoundUp(d *big.Int) *big.Int { if d.Sign() == -1 { // make d positive, compute chopped value, and then un-mutate d d = d.Neg(d) - d = chopPrecisionAndRound(d) + // truncate since d is negative... + d = chopPrecisionAndTruncate(d) d = d.Neg(d) return d } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index c0cab908e5c3..499cb5088c4f 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -65,19 +65,21 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // iterate through slashes and withdraw with calculated staking for sub-intervals // these offsets are dependent on *when* slashes happen - namely, in BeginBlock, after rewards are allocated... - // slashes which happened in the first block would have been before this delegation existed - startingHeight := startingInfo.Height + 1 + // slashes which happened in the first block would have been before this delegation existed, + // UNLESS they were slashes of a redelegation to this validator which was itself slashed earlier in the same BeginBlock + startingHeight := startingInfo.Height // slashes this block happened after reward allocation, but we have to account for them for the stake sanity check endingHeight := uint64(ctx.BlockHeight()) if endingHeight >= startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod - stake = stake.MulTruncate(sdk.OneDec().Sub(event.Fraction)) - rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - startingPeriod = endingPeriod - // note: necessary to truncate so we don't allow withdrawing more rewards than owed - fmt.Printf("recalculated stake for delegator %s after slash of %v\n", del.GetDelegatorAddr(), event.Fraction) + if endingPeriod > startingPeriod { + // note: necessary to truncate so we don't allow withdrawing more rewards than owed + stake = stake.MulTruncate(sdk.OneDec().Sub(event.Fraction)) + rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) + startingPeriod = endingPeriod + } return false }, ) From 66f44f9fb6607677d29d069ca9750540c8a12e20 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 22:13:02 +0100 Subject: [PATCH 42/50] Fix linter --- types/decimal.go | 1 + 1 file changed, 1 insertion(+) diff --git a/types/decimal.go b/types/decimal.go index 6567caf03c4a..bec27468c59b 100644 --- a/types/decimal.go +++ b/types/decimal.go @@ -291,6 +291,7 @@ func (d Dec) QuoTruncate(d2 Dec) Dec { return Dec{chopped} } +// quotient, round up func (d Dec) QuoRoundUp(d2 Dec) Dec { // multiply precision twice mul := new(big.Int).Mul(d.Int, precisionReuse) From 1d47060f8b53c2517345780ce755569b4b2adcdf Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Thu, 28 Feb 2019 17:22:02 -0500 Subject: [PATCH 43/50] exapand decimal tests --- types/decimal_test.go | 61 +++++++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/types/decimal_test.go b/types/decimal_test.go index e0009236f50e..55aa31db9b58 100644 --- a/types/decimal_test.go +++ b/types/decimal_test.go @@ -156,44 +156,61 @@ func TestDecsEqual(t *testing.T) { func TestArithmetic(t *testing.T) { tests := []struct { - d1, d2 Dec - expMul, expQuo, expAdd, expSub Dec + d1, d2 Dec + expMul, expMulTruncate Dec + expQuo, expQuoRoundUp, expQuoTruncate Dec + expAdd, expSub Dec }{ - // d1 d2 MUL DIV ADD SUB - {NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0)}, - {NewDec(1), NewDec(0), NewDec(0), NewDec(0), NewDec(1), NewDec(1)}, - {NewDec(0), NewDec(1), NewDec(0), NewDec(0), NewDec(1), NewDec(-1)}, - {NewDec(0), NewDec(-1), NewDec(0), NewDec(0), NewDec(-1), NewDec(1)}, - {NewDec(-1), NewDec(0), NewDec(0), NewDec(0), NewDec(-1), NewDec(-1)}, - - {NewDec(1), NewDec(1), NewDec(1), NewDec(1), NewDec(2), NewDec(0)}, - {NewDec(-1), NewDec(-1), NewDec(1), NewDec(1), NewDec(-2), NewDec(0)}, - {NewDec(1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(0), NewDec(2)}, - {NewDec(-1), NewDec(1), NewDec(-1), NewDec(-1), NewDec(0), NewDec(-2)}, - - {NewDec(3), NewDec(7), NewDec(21), NewDecWithPrec(428571428571428571, 18), NewDec(10), NewDec(-4)}, - {NewDec(2), NewDec(4), NewDec(8), NewDecWithPrec(5, 1), NewDec(6), NewDec(-2)}, - {NewDec(100), NewDec(100), NewDec(10000), NewDec(1), NewDec(200), NewDec(0)}, - - {NewDecWithPrec(15, 1), NewDecWithPrec(15, 1), NewDecWithPrec(225, 2), - NewDec(1), NewDec(3), NewDec(0)}, - {NewDecWithPrec(3333, 4), NewDecWithPrec(333, 4), NewDecWithPrec(1109889, 8), - MustNewDecFromStr("10.009009009009009009"), NewDecWithPrec(3666, 4), NewDecWithPrec(3, 1)}, + // d1 d2 MUL MulTruncate QUO QUORoundUp QUOTrunctate ADD SUB + {NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0)}, + {NewDec(1), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(1), NewDec(1)}, + {NewDec(0), NewDec(1), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(1), NewDec(-1)}, + {NewDec(0), NewDec(-1), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(-1), NewDec(1)}, + {NewDec(-1), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(0), NewDec(-1), NewDec(-1)}, + + {NewDec(1), NewDec(1), NewDec(1), NewDec(1), NewDec(1), NewDec(1), NewDec(1), NewDec(2), NewDec(0)}, + {NewDec(-1), NewDec(-1), NewDec(1), NewDec(1), NewDec(1), NewDec(1), NewDec(1), NewDec(-2), NewDec(0)}, + {NewDec(1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(0), NewDec(2)}, + {NewDec(-1), NewDec(1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(-1), NewDec(0), NewDec(-2)}, + + {NewDec(3), NewDec(7), NewDec(21), NewDec(21), + NewDecWithPrec(428571428571428571, 18), NewDecWithPrec(428571428571428572, 18), NewDecWithPrec(428571428571428571, 18), + NewDec(10), NewDec(-4)}, + {NewDec(2), NewDec(4), NewDec(8), NewDec(8), NewDecWithPrec(5, 1), NewDecWithPrec(5, 1), NewDecWithPrec(5, 1), + NewDec(6), NewDec(-2)}, + + {NewDec(100), NewDec(100), NewDec(10000), NewDec(10000), NewDec(1), NewDec(1), NewDec(1), NewDec(200), NewDec(0)}, + + {NewDecWithPrec(15, 1), NewDecWithPrec(15, 1), NewDecWithPrec(225, 2), NewDecWithPrec(225, 2), + NewDec(1), NewDec(1), NewDec(1), NewDec(3), NewDec(0)}, + {NewDecWithPrec(3333, 4), NewDecWithPrec(333, 4), NewDecWithPrec(1109889, 8), NewDecWithPrec(1109889, 8), + MustNewDecFromStr("10.009009009009009009"), MustNewDecFromStr("10.009009009009009010"), MustNewDecFromStr("10.009009009009009009"), + NewDecWithPrec(3666, 4), NewDecWithPrec(3, 1)}, } for tcIndex, tc := range tests { resAdd := tc.d1.Add(tc.d2) resSub := tc.d1.Sub(tc.d2) resMul := tc.d1.Mul(tc.d2) + resMulTruncate := tc.d1.MulTruncate(tc.d2) require.True(t, tc.expAdd.Equal(resAdd), "exp %v, res %v, tc %d", tc.expAdd, resAdd, tcIndex) require.True(t, tc.expSub.Equal(resSub), "exp %v, res %v, tc %d", tc.expSub, resSub, tcIndex) require.True(t, tc.expMul.Equal(resMul), "exp %v, res %v, tc %d", tc.expMul, resMul, tcIndex) + require.True(t, tc.expMulTruncate.Equal(resMulTruncate), "exp %v, res %v, tc %d", tc.expMulTruncate, resMulTruncate, tcIndex) if tc.d2.IsZero() { // panic for divide by zero require.Panics(t, func() { tc.d1.Quo(tc.d2) }) } else { resQuo := tc.d1.Quo(tc.d2) require.True(t, tc.expQuo.Equal(resQuo), "exp %v, res %v, tc %d", tc.expQuo.String(), resQuo.String(), tcIndex) + + resQuoRoundUp := tc.d1.QuoRoundUp(tc.d2) + require.True(t, tc.expQuoRoundUp.Equal(resQuoRoundUp), "exp %v, res %v, tc %d", + tc.expQuoRoundUp.String(), resQuoRoundUp.String(), tcIndex) + + resQuoTruncate := tc.d1.QuoTruncate(tc.d2) + require.True(t, tc.expQuoTruncate.Equal(resQuoTruncate), "exp %v, res %v, tc %d", + tc.expQuoTruncate.String(), resQuoTruncate.String(), tcIndex) } } } From 9360b0804302353ce49ef7e190736fdc57b2b7f7 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 28 Feb 2019 23:59:29 +0100 Subject: [PATCH 44/50] Testcase fixes --- x/distribution/keeper/delegation_test.go | 39 ++++++++++++++++++++++++ x/distribution/keeper/querier_test.go | 1 + 2 files changed, 40 insertions(+) diff --git a/x/distribution/keeper/delegation_test.go b/x/distribution/keeper/delegation_test.go index 681158465007..d6eb848e7d9a 100644 --- a/x/distribution/keeper/delegation_test.go +++ b/x/distribution/keeper/delegation_test.go @@ -22,6 +22,9 @@ func TestCalculateRewardsBasic(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) del := sk.Delegation(ctx, sdk.AccAddress(valOpAddr1), valOpAddr1) @@ -75,6 +78,9 @@ func TestCalculateRewardsAfterSlash(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) del := sk.Delegation(ctx, sdk.AccAddress(valOpAddr1), valOpAddr1) @@ -134,6 +140,9 @@ func TestCalculateRewardsAfterManySlashes(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) del := sk.Delegation(ctx, sdk.AccAddress(valOpAddr1), valOpAddr1) @@ -203,6 +212,9 @@ func TestCalculateRewardsMultiDelegator(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) del1 := sk.Delegation(ctx, sdk.AccAddress(valOpAddr1), valOpAddr1) @@ -223,6 +235,9 @@ func TestCalculateRewardsMultiDelegator(t *testing.T) { // end block staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // allocate some more rewards k.AllocateTokensToValidator(ctx, val, tokens) @@ -272,6 +287,9 @@ func TestWithdrawDelegationRewardsBasic(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) @@ -323,6 +341,9 @@ func TestCalculateRewardsAfterManySlashesInSameBlock(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) del := sk.Delegation(ctx, sdk.AccAddress(valOpAddr1), valOpAddr1) @@ -387,6 +408,9 @@ func TestCalculateRewardsMultiDelegatorMultiSlash(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) del1 := sk.Delegation(ctx, sdk.AccAddress(valOpAddr1), valOpAddr1) @@ -411,6 +435,9 @@ func TestCalculateRewardsMultiDelegatorMultiSlash(t *testing.T) { // end block staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // allocate some more rewards k.AllocateTokensToValidator(ctx, val, tokens) @@ -458,6 +485,9 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) { // end block to bond validator staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // fetch validator and delegation val := sk.Validator(ctx, valOpAddr1) del1 := sk.Delegation(ctx, sdk.AccAddress(valOpAddr1), valOpAddr1) @@ -482,6 +512,9 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) { // end block staking.EndBlocker(ctx, sk) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // allocate some more rewards k.AllocateTokensToValidator(ctx, val, tokens) @@ -517,6 +550,9 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) { totalRewards = totalRewards.Add(tokens) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // allocate some more rewards k.AllocateTokensToValidator(ctx, val, tokens) @@ -543,6 +579,9 @@ func TestCalculateRewardsMultiDelegatorMultWithdraw(t *testing.T) { totalRewards = k.GetValidatorOutstandingRewards(ctx, valOpAddr1).Add(tokens) + // next block + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + // allocate some more rewards k.AllocateTokensToValidator(ctx, val, tokens) diff --git a/x/distribution/keeper/querier_test.go b/x/distribution/keeper/querier_test.go index 8c547e717740..9593f1bff472 100644 --- a/x/distribution/keeper/querier_test.go +++ b/x/distribution/keeper/querier_test.go @@ -164,6 +164,7 @@ func TestQueries(t *testing.T) { rewards := getQueriedDelegationRewards(t, ctx, cdc, querier, sdk.AccAddress(valOpAddr1), valOpAddr1) require.True(t, rewards.IsZero()) initial := int64(10) + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) tokens := sdk.DecCoins{{sdk.DefaultBondDenom, sdk.NewDec(initial)}} keeper.AllocateTokensToValidator(ctx, val, tokens) rewards = getQueriedDelegationRewards(t, ctx, cdc, querier, sdk.AccAddress(valOpAddr1), valOpAddr1) From 96bf7e9bde99dac07700f5277730abd9dcd80594 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 1 Mar 2019 00:02:26 +0100 Subject: [PATCH 45/50] Fix stake calculation ordering --- x/distribution/keeper/delegation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 499cb5088c4f..e7d1659097ba 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -75,9 +75,9 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod if endingPeriod > startingPeriod { + rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) // note: necessary to truncate so we don't allow withdrawing more rewards than owed stake = stake.MulTruncate(sdk.OneDec().Sub(event.Fraction)) - rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) startingPeriod = endingPeriod } return false From 37311cae8ed37e82fff118041f3bc7b27fe8faf5 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 1 Mar 2019 00:11:34 +0100 Subject: [PATCH 46/50] Address @rigelrozanski comments --- PENDING.md | 7 ++++++- x/distribution/client/cli/query.go | 2 +- x/distribution/simulation/invariants.go | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/PENDING.md b/PENDING.md index 7a3a666b30be..f6b339bb6c81 100644 --- a/PENDING.md +++ b/PENDING.md @@ -61,7 +61,12 @@ CLI flag. ### SDK -* \#3750 Track outstanding rewards per-validator instead of globally +* \#3750 Track outstanding rewards per-validator instead of globally, + and fix the main simulation issue, which was that slashes of + re-delegations to a validator were not correctly accounted for + in fee distribution when the redelegation in question had itself + been slashed (from a fault committed by a different validator) + in the same BeginBlock * \#3753 Remove no-longer-used governance penalty parameter * \#3679 Consistent operators across Coins, DecCoins, Int, Dec replaced: Minus->Sub Plus->Add Div->Quo diff --git a/x/distribution/client/cli/query.go b/x/distribution/client/cli/query.go index 06fbc4661a1c..4ba5b03b1608 100644 --- a/x/distribution/client/cli/query.go +++ b/x/distribution/client/cli/query.go @@ -37,7 +37,7 @@ func GetCmdQueryValidatorOutstandingRewards(queryRoute string, cdc *codec.Codec) return &cobra.Command{ Use: "validator-outstanding-rewards", Args: cobra.NoArgs, - Short: "Query distribution outstanding (un-withdrawn) rewards for a validator", + Short: "Query distribution outstanding (un-withdrawn) rewards for a validator and all their delegations", RunE: func(cmd *cobra.Command, args []string) error { cliCtx := context.NewCLIContext().WithCodec(cdc) diff --git a/x/distribution/simulation/invariants.go b/x/distribution/simulation/invariants.go index a53065b6e885..fa5f585c98d0 100644 --- a/x/distribution/simulation/invariants.go +++ b/x/distribution/simulation/invariants.go @@ -67,7 +67,10 @@ func CanWithdrawInvariant(k distr.Keeper, sk types.StakingKeeper) sdk.Invariant dels := sk.GetAllSDKDelegations(ctx) for _, delegation := range dels { if delegation.GetValidatorAddr().String() == val.GetOperator().String() { - _ = k.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr()) + err := k.WithdrawDelegationRewards(ctx, delegation.GetDelegatorAddr(), delegation.GetValidatorAddr()) + if err != nil { + panic(err) + } } } remaining = k.GetValidatorOutstandingRewards(ctx, val.GetOperator()) From 4c1fd17d8a26de975f648959bba80ea3a73e0047 Mon Sep 17 00:00:00 2001 From: rigelrozanski Date: Thu, 28 Feb 2019 18:45:22 -0500 Subject: [PATCH 47/50] comment --- x/distribution/keeper/delegation.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index e7d1659097ba..cc9aaf829135 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -2,6 +2,7 @@ package keeper import ( "fmt" + sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/distribution/types" @@ -85,8 +86,11 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d ) } + // here we cannot use Equals because stake is truncated when multiplied by slash fractions + // we could only use equals if we had arbitrary-precision rationals if stake.GT(del.GetShares().Mul(val.GetDelegatorShareExRate())) { - panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake: %s, %s", del.GetDelegatorAddr(), stake, del.GetShares().Mul(val.GetDelegatorShareExRate()))) + panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake: %s, %s", + del.GetDelegatorAddr(), stake, del.GetShares().Mul(val.GetDelegatorShareExRate()))) } // calculate rewards for final period From 86763f8c8676f5b870f6f3dab4ab7daa8066ccdb Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Fri, 1 Mar 2019 14:58:21 +0100 Subject: [PATCH 48/50] Address @jaekwon comments --- x/distribution/keeper/delegation.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index e7d1659097ba..93be403b2476 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -66,11 +66,12 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // iterate through slashes and withdraw with calculated staking for sub-intervals // these offsets are dependent on *when* slashes happen - namely, in BeginBlock, after rewards are allocated... // slashes which happened in the first block would have been before this delegation existed, - // UNLESS they were slashes of a redelegation to this validator which was itself slashed earlier in the same BeginBlock + // UNLESS they were slashes of a redelegation to this validator which was itself slashed + // (from a fault committed by the redelegation source validator) earlier in the same BeginBlock startingHeight := startingInfo.Height - // slashes this block happened after reward allocation, but we have to account for them for the stake sanity check + // slashes this block happened after reward allocation, but we have to account for them for the stake sanity check below endingHeight := uint64(ctx.BlockHeight()) - if endingHeight >= startingHeight { + if endingHeight > startingHeight { k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight, func(height uint64, event types.ValidatorSlashEvent) (stop bool) { endingPeriod := event.ValidatorPeriod @@ -85,6 +86,7 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d ) } + // a stake sanity check - recalculated final stake should be less than or equal to current stake if stake.GT(del.GetShares().Mul(val.GetDelegatorShareExRate())) { panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake: %s, %s", del.GetDelegatorAddr(), stake, del.GetShares().Mul(val.GetDelegatorShareExRate()))) } From afdac451cb95529fda134c5af61654413515d33f Mon Sep 17 00:00:00 2001 From: frog power 4000 Date: Wed, 6 Mar 2019 06:19:20 -0500 Subject: [PATCH 49/50] Merge PR #3788: F1 mechanism rounding fix --- PENDING.md | 2 + client/lcd/lcd_test.go | 2 +- cmd/gaia/app/export.go | 7 +++ types/dec_coin.go | 9 ++-- types/dec_coin_test.go | 6 +-- types/staking.go | 28 ++++++------ x/distribution/keeper/allocation.go | 8 +++- x/distribution/keeper/delegation.go | 23 +++++++--- x/slashing/handler.go | 2 +- x/staking/handler_test.go | 5 -- x/staking/keeper/delegation.go | 6 ++- x/staking/types/validator.go | 71 ++++++++++++++--------------- x/staking/types/validator_test.go | 53 +++++++++++++++------ 13 files changed, 134 insertions(+), 88 deletions(-) diff --git a/PENDING.md b/PENDING.md index f6b339bb6c81..cb1301e10a82 100644 --- a/PENDING.md +++ b/PENDING.md @@ -23,6 +23,8 @@ * [\#3669] Ensure consistency in message naming, codec registration, and JSON tags. +* #3788 Change order of operations for greater accuracy when calculating delegation share token value +* #3788 DecCoins.Cap -> DecCoins.Intersect ### Tendermint diff --git a/client/lcd/lcd_test.go b/client/lcd/lcd_test.go index 140c4657d533..88845a731569 100644 --- a/client/lcd/lcd_test.go +++ b/client/lcd/lcd_test.go @@ -553,7 +553,7 @@ func TestBonding(t *testing.T) { // hence we utilize the exchange rate in the following test validator2 := getValidator(t, port, operAddrs[1]) - delTokensAfterRedelegation := delegatorDels[0].GetShares().Mul(validator2.DelegatorShareExRate()) + delTokensAfterRedelegation := validator2.ShareTokens(delegatorDels[0].GetShares()) require.Equal(t, rdTokens.ToDec(), delTokensAfterRedelegation) redelegation := getRedelegations(t, port, addr, operAddrs[0], operAddrs[1]) diff --git a/cmd/gaia/app/export.go b/cmd/gaia/app/export.go index 978bcf92c538..3bd439396262 100644 --- a/cmd/gaia/app/export.go +++ b/cmd/gaia/app/export.go @@ -104,6 +104,13 @@ func (app *GaiaApp) prepForZeroHeightGenesis(ctx sdk.Context, jailWhiteList []st // reinitialize all validators app.stakingKeeper.IterateValidators(ctx, func(_ int64, val sdk.Validator) (stop bool) { + + // donate any unwithdrawn outstanding reward fraction tokens to the community pool + scraps := app.distrKeeper.GetValidatorOutstandingRewards(ctx, val.GetOperator()) + feePool := app.distrKeeper.GetFeePool(ctx) + feePool.CommunityPool = feePool.CommunityPool.Add(scraps) + app.distrKeeper.SetFeePool(ctx, feePool) + app.distrKeeper.Hooks().AfterValidatorCreated(ctx, val.GetOperator()) return false }) diff --git a/types/dec_coin.go b/types/dec_coin.go index fe0dc9da7488..7b5a4c45a928 100644 --- a/types/dec_coin.go +++ b/types/dec_coin.go @@ -278,9 +278,12 @@ func (coins DecCoins) SafeSub(coinsB DecCoins) (DecCoins, bool) { return diff, diff.IsAnyNegative() } -// Trims any denom amount from coin which exceeds that of coinB, -// such that (coin.Cap(coinB)).IsLTE(coinB). -func (coins DecCoins) Cap(coinsB DecCoins) DecCoins { +// Intersect will return a new set of coins which contains the minimum DecCoin +// for common denoms found in both `coins` and `coinsB`. For denoms not common +// to both `coins` and `coinsB` the minimum is considered to be 0, thus they +// are not added to the final set.In other words, trim any denom amount from +// coin which exceeds that of coinB, such that (coin.Intersect(coinB)).IsLTE(coinB). +func (coins DecCoins) Intersect(coinsB DecCoins) DecCoins { res := make([]DecCoin, len(coins)) for i, coin := range coins { minCoin := DecCoin{ diff --git a/types/dec_coin_test.go b/types/dec_coin_test.go index d5ddec2da5da..b7b6f4d0c28e 100644 --- a/types/dec_coin_test.go +++ b/types/dec_coin_test.go @@ -225,7 +225,7 @@ func TestDecCoinsString(t *testing.T) { } } -func TestDecCoinsCap(t *testing.T) { +func TestDecCoinsIntersect(t *testing.T) { testCases := []struct { input1 string input2 string @@ -252,7 +252,7 @@ func TestDecCoinsCap(t *testing.T) { exr, err := ParseDecCoins(tc.expectedResult) require.NoError(t, err, "unexpected parse error in %v", i) - require.True(t, in1.Cap(in2).IsEqual(exr), "in1.cap(in2) != exr in %v", i) - // require.Equal(t, tc.expectedResult, in1.Cap(in2).String(), "in1.cap(in2) != exr in %v", i) + require.True(t, in1.Intersect(in2).IsEqual(exr), "in1.cap(in2) != exr in %v", i) + // require.Equal(t, tc.expectedResult, in1.Intersect(in2).String(), "in1.cap(in2) != exr in %v", i) } } diff --git a/types/staking.go b/types/staking.go index 2e7dbd0eb87c..428979a18658 100644 --- a/types/staking.go +++ b/types/staking.go @@ -61,20 +61,20 @@ func (b BondStatus) Equal(b2 BondStatus) bool { // validator for a delegated proof of stake system type Validator interface { - GetJailed() bool // whether the validator is jailed - GetMoniker() string // moniker of the validator - GetStatus() BondStatus // status of the validator - GetOperator() ValAddress // operator address to receive/return validators coins - GetConsPubKey() crypto.PubKey // validation consensus pubkey - GetConsAddr() ConsAddress // validation consensus address - GetTokens() Int // validation tokens - GetBondedTokens() Int // validator bonded tokens - GetTendermintPower() int64 // validation power in tendermint - GetCommission() Dec // validator commission rate - GetMinSelfDelegation() Int // validator minimum self delegation - GetDelegatorShares() Dec // total outstanding delegator shares - GetDelegatorShareExRate() Dec // tokens per delegator share exchange rate - GetDelegatorShareExRateTruncated() Dec // tokens per delegator share exchange rate + GetJailed() bool // whether the validator is jailed + GetMoniker() string // moniker of the validator + GetStatus() BondStatus // status of the validator + GetOperator() ValAddress // operator address to receive/return validators coins + GetConsPubKey() crypto.PubKey // validation consensus pubkey + GetConsAddr() ConsAddress // validation consensus address + GetTokens() Int // validation tokens + GetBondedTokens() Int // validator bonded tokens + GetTendermintPower() int64 // validation power in tendermint + GetCommission() Dec // validator commission rate + GetMinSelfDelegation() Int // validator minimum self delegation + GetDelegatorShares() Dec // total outstanding delegator shares + ShareTokens(Dec) Dec // token worth of provided delegator shares + ShareTokensTruncated(Dec) Dec // token worth of provided delegator shares, truncated } // validator which fulfills abci validator interface for use in Tendermint diff --git a/x/distribution/keeper/allocation.go b/x/distribution/keeper/allocation.go index 355e5fc81b6b..50dcd2e93d79 100644 --- a/x/distribution/keeper/allocation.go +++ b/x/distribution/keeper/allocation.go @@ -2,6 +2,7 @@ package keeper import ( "fmt" + abci "github.com/tendermint/tendermint/abci/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -48,7 +49,10 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // block X+1's endblock, then X+2 we need to refer to the previous // proposer for X+1, but we've forgotten about them. logger.Error(fmt.Sprintf( - "WARNING: Attempt to allocate proposer rewards to unknown proposer %s. This should happen only if the proposer unbonded completely within a single block, which generally should not happen except in exceptional circumstances (or fuzz testing). We recommend you investigate immediately.", + "WARNING: Attempt to allocate proposer rewards to unknown proposer %s. "+ + "This should happen only if the proposer unbonded completely within a single block, "+ + "which generally should not happen except in exceptional circumstances (or fuzz testing). "+ + "We recommend you investigate immediately.", proposer.String())) } @@ -65,7 +69,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in // ref https://github.com/cosmos/cosmos-sdk/issues/2525#issuecomment-430838701 powerFraction := sdk.NewDec(vote.Validator.Power).QuoTruncate(sdk.NewDec(totalPower)) reward := feesCollected.MulDecTruncate(voteMultiplier).MulDecTruncate(powerFraction) - reward = reward.Cap(remaining) + reward = reward.Intersect(remaining) k.AllocateTokensToValidator(ctx, validator, reward) remaining = remaining.Sub(reward) } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index 5dbfd1fe6188..00e7669bbd90 100644 --- a/x/distribution/keeper/delegation.go +++ b/x/distribution/keeper/delegation.go @@ -22,7 +22,7 @@ func (k Keeper) initializeDelegation(ctx sdk.Context, val sdk.ValAddress, del sd // calculate delegation stake in tokens // we don't store directly, so multiply delegation shares * (tokens per share) // note: necessary to truncate so we don't allow withdrawing more rewards than owed - stake := delegation.GetShares().MulTruncate(validator.GetDelegatorShareExRateTruncated()) + stake := validator.ShareTokensTruncated(delegation.GetShares()) k.SetDelegatorStartingInfo(ctx, val, del, types.NewDelegatorStartingInfo(previousPeriod, stake, uint64(ctx.BlockHeight()))) } @@ -90,15 +90,16 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d // a stake sanity check - recalculated final stake should be less than or equal to current stake // here we cannot use Equals because stake is truncated when multiplied by slash fractions // we could only use equals if we had arbitrary-precision rationals - if stake.GT(del.GetShares().Mul(val.GetDelegatorShareExRate())) { + currentStake := val.ShareTokens(del.GetShares()) + if stake.GT(currentStake) { panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake: %s, %s", - del.GetDelegatorAddr(), stake, del.GetShares().Mul(val.GetDelegatorShareExRate()))) + del.GetDelegatorAddr(), stake, currentStake)) } // calculate rewards for final period rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake)) - return + return rewards } func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, del sdk.Delegation) sdk.Error { @@ -110,7 +111,18 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de // end current period and calculate rewards endingPeriod := k.incrementValidatorPeriod(ctx, val) - rewards := k.calculateDelegationRewards(ctx, val, del, endingPeriod) + rewardsRaw := k.calculateDelegationRewards(ctx, val, del, endingPeriod) + outstanding := k.GetValidatorOutstandingRewards(ctx, del.GetValidatorAddr()) + + // defensive edge case may happen on the very final digits + // of the decCoins due to operation order of the distribution mechanism. + rewards := rewardsRaw.Intersect(outstanding) + if !rewards.IsEqual(rewardsRaw) { + logger := ctx.Logger().With("module", "x/distr") + logger.Info(fmt.Sprintf("missing rewards rounding error, delegator %v"+ + "withdrawing rewards from validator %v, should have received %v, got %v", + val.GetOperator(), del.GetDelegatorAddr(), rewardsRaw, rewards)) + } // decrement reference count of starting period startingInfo := k.GetDelegatorStartingInfo(ctx, del.GetValidatorAddr(), del.GetDelegatorAddr()) @@ -120,7 +132,6 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de // truncate coins, return remainder to community pool coins, remainder := rewards.TruncateDecimal() - outstanding := k.GetValidatorOutstandingRewards(ctx, del.GetValidatorAddr()) k.SetValidatorOutstandingRewards(ctx, del.GetValidatorAddr(), outstanding.Sub(rewards)) feePool := k.GetFeePool(ctx) feePool.CommunityPool = feePool.CommunityPool.Add(remainder) diff --git a/x/slashing/handler.go b/x/slashing/handler.go index d9ae9f2d8127..fcc141a86c03 100644 --- a/x/slashing/handler.go +++ b/x/slashing/handler.go @@ -31,7 +31,7 @@ func handleMsgUnjail(ctx sdk.Context, msg MsgUnjail, k Keeper) sdk.Result { return ErrMissingSelfDelegation(k.codespace).Result() } - if validator.GetDelegatorShareExRate().Mul(selfDel.GetShares()).TruncateInt().LT(validator.GetMinSelfDelegation()) { + if validator.ShareTokens(selfDel.GetShares()).TruncateInt().LT(validator.GetMinSelfDelegation()) { return ErrSelfDelegationTooLowToUnjail(k.codespace).Result() } diff --git a/x/staking/handler_test.go b/x/staking/handler_test.go index 348d07935ba4..874f96dc006e 100644 --- a/x/staking/handler_test.go +++ b/x/staking/handler_test.go @@ -333,8 +333,6 @@ func TestIncrementsMsgDelegate(t *testing.T) { require.Equal(t, bondAmount, bond.Shares.RoundInt()) pool := keeper.GetPool(ctx) - exRate := validator.DelegatorShareExRate() - require.True(t, exRate.Equal(sdk.OneDec()), "expected exRate 1 got %v", exRate) require.Equal(t, bondAmount, pool.BondedTokens) // just send the same msgbond multiple times @@ -352,9 +350,6 @@ func TestIncrementsMsgDelegate(t *testing.T) { bond, found := keeper.GetDelegation(ctx, delegatorAddr, validatorAddr) require.True(t, found) - exRate := validator.DelegatorShareExRate() - require.True(t, exRate.Equal(sdk.OneDec()), "expected exRate 1 got %v, i = %v", exRate, i) - expBond := bondAmount.MulRaw(i + 1) expDelegatorShares := bondAmount.MulRaw(i + 2) // (1 self delegation) expDelegatorAcc := initBond.Sub(expBond) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index a15b78d358ff..86adf577cb8a 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -450,7 +450,7 @@ func (k Keeper) Delegate(ctx sdk.Context, delAddr sdk.AccAddress, bondAmt sdk.In // In some situations, the exchange rate becomes invalid, e.g. if // Validator loses all tokens due to slashing. In this case, // make all future delegations invalid. - if validator.DelegatorShareExRate().IsZero() { + if validator.InvalidExRate() { return sdk.ZeroDec(), types.ErrDelegatorShareExRateInvalid(k.Codespace()) } @@ -517,7 +517,9 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA // if the delegation is the operator of the validator and undelegating will decrease the validator's self delegation below their minimum // trigger a jail validator - if isValidatorOperator && !validator.Jailed && validator.DelegatorShareExRate().Mul(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) { + if isValidatorOperator && !validator.Jailed && + validator.ShareTokens(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) { + k.jailValidator(ctx, validator) validator = k.mustGetValidator(ctx, validator.OperatorAddress) } diff --git a/x/staking/types/validator.go b/x/staking/types/validator.go index ac427e645b86..730d43073e29 100644 --- a/x/staking/types/validator.go +++ b/x/staking/types/validator.go @@ -348,10 +348,13 @@ func (v Validator) SetInitialCommission(commission Commission) (Validator, sdk.E // CONTRACT: Tokens are assumed to have come from not-bonded pool. func (v Validator) AddTokensFromDel(pool Pool, amount sdk.Int) (Validator, Pool, sdk.Dec) { - // bondedShare/delegatedShare - exRate := v.DelegatorShareExRate() - if exRate.IsZero() { - panic("zero exRate should not happen") + // calculate the shares to issue + var issuedShares sdk.Dec + if v.DelegatorShares.IsZero() { + // the first delegation to a validator sets the exchange rate to one + issuedShares = amount.ToDec() + } else { + issuedShares = v.DelegatorShares.MulInt(amount).QuoInt(v.Tokens) } if v.Status == sdk.Bonded { @@ -359,7 +362,6 @@ func (v Validator) AddTokensFromDel(pool Pool, amount sdk.Int) (Validator, Pool, } v.Tokens = v.Tokens.Add(amount) - issuedShares := amount.ToDec().Quo(exRate) v.DelegatorShares = v.DelegatorShares.Add(issuedShares) return v, pool, issuedShares @@ -382,7 +384,7 @@ func (v Validator) RemoveDelShares(pool Pool, delShares sdk.Dec) (Validator, Poo // leave excess tokens in the validator // however fully use all the delegator shares - issuedTokens = v.DelegatorShareExRate().Mul(delShares).TruncateInt() + issuedTokens = v.ShareTokens(delShares).TruncateInt() v.Tokens = v.Tokens.Sub(issuedTokens) if v.Tokens.IsNegative() { panic("attempting to remove more tokens than available in validator") @@ -397,24 +399,21 @@ func (v Validator) RemoveDelShares(pool Pool, delShares sdk.Dec) (Validator, Poo return v, pool, issuedTokens } -// DelegatorShareExRate gets the exchange rate of tokens over delegator shares. -// UNITS: tokens/delegator-shares -func (v Validator) DelegatorShareExRate() sdk.Dec { - if v.DelegatorShares.IsZero() { - // the first delegation to a validator sets the exchange rate to one - return sdk.OneDec() - } - return v.Tokens.ToDec().Quo(v.DelegatorShares) +// In some situations, the exchange rate becomes invalid, e.g. if +// Validator loses all tokens due to slashing. In this case, +// make all future delegations invalid. +func (v Validator) InvalidExRate() bool { + return v.Tokens.IsZero() && v.DelegatorShares.IsPositive() } -// DelegatorShareExRateTruncated gets the exchange rate of tokens over delegator shares, truncated. -// UNITS: tokens/delegator-shares -func (v Validator) DelegatorShareExRateTruncated() sdk.Dec { - if v.DelegatorShares.IsZero() { - // the first delegation to a validator sets the exchange rate to one - return sdk.OneDec() - } - return v.Tokens.ToDec().QuoTruncate(v.DelegatorShares) +// calculate the token worth of provided shares +func (v Validator) ShareTokens(shares sdk.Dec) sdk.Dec { + return (shares.MulInt(v.Tokens)).Quo(v.DelegatorShares) +} + +// calculate the token worth of provided shares, truncated +func (v Validator) ShareTokensTruncated(shares sdk.Dec) sdk.Dec { + return (shares.MulInt(v.Tokens)).QuoTruncate(v.DelegatorShares) } // get the bonded tokens which the validator holds @@ -443,19 +442,15 @@ func (v Validator) PotentialTendermintPower() int64 { var _ sdk.Validator = Validator{} // nolint - for sdk.Validator -func (v Validator) GetJailed() bool { return v.Jailed } -func (v Validator) GetMoniker() string { return v.Description.Moniker } -func (v Validator) GetStatus() sdk.BondStatus { return v.Status } -func (v Validator) GetOperator() sdk.ValAddress { return v.OperatorAddress } -func (v Validator) GetConsPubKey() crypto.PubKey { return v.ConsPubKey } -func (v Validator) GetConsAddr() sdk.ConsAddress { return sdk.ConsAddress(v.ConsPubKey.Address()) } -func (v Validator) GetTokens() sdk.Int { return v.Tokens } -func (v Validator) GetBondedTokens() sdk.Int { return v.BondedTokens() } -func (v Validator) GetTendermintPower() int64 { return v.TendermintPower() } -func (v Validator) GetCommission() sdk.Dec { return v.Commission.Rate } -func (v Validator) GetMinSelfDelegation() sdk.Int { return v.MinSelfDelegation } -func (v Validator) GetDelegatorShares() sdk.Dec { return v.DelegatorShares } -func (v Validator) GetDelegatorShareExRate() sdk.Dec { return v.DelegatorShareExRate() } -func (v Validator) GetDelegatorShareExRateTruncated() sdk.Dec { - return v.DelegatorShareExRateTruncated() -} +func (v Validator) GetJailed() bool { return v.Jailed } +func (v Validator) GetMoniker() string { return v.Description.Moniker } +func (v Validator) GetStatus() sdk.BondStatus { return v.Status } +func (v Validator) GetOperator() sdk.ValAddress { return v.OperatorAddress } +func (v Validator) GetConsPubKey() crypto.PubKey { return v.ConsPubKey } +func (v Validator) GetConsAddr() sdk.ConsAddress { return sdk.ConsAddress(v.ConsPubKey.Address()) } +func (v Validator) GetTokens() sdk.Int { return v.Tokens } +func (v Validator) GetBondedTokens() sdk.Int { return v.BondedTokens() } +func (v Validator) GetTendermintPower() int64 { return v.TendermintPower() } +func (v Validator) GetCommission() sdk.Dec { return v.Commission.Rate } +func (v Validator) GetMinSelfDelegation() sdk.Int { return v.MinSelfDelegation } +func (v Validator) GetDelegatorShares() sdk.Dec { return v.DelegatorShares } diff --git a/x/staking/types/validator_test.go b/x/staking/types/validator_test.go index 5cb1992068de..b8b1fb5c7f57 100644 --- a/x/staking/types/validator_test.go +++ b/x/staking/types/validator_test.go @@ -1,7 +1,6 @@ package types import ( - "fmt" "testing" "github.com/cosmos/cosmos-sdk/codec" @@ -70,6 +69,21 @@ func TestABCIValidatorUpdateZero(t *testing.T) { require.Equal(t, int64(0), abciVal.Power) } +func TestShareTokens(t *testing.T) { + validator := Validator{ + OperatorAddress: addr1, + ConsPubKey: pk1, + Status: sdk.Bonded, + Tokens: sdk.NewInt(100), + DelegatorShares: sdk.NewDec(100), + } + assert.True(sdk.DecEq(t, sdk.NewDec(50), validator.ShareTokens(sdk.NewDec(50)))) + + validator.Tokens = sdk.NewInt(50) + assert.True(sdk.DecEq(t, sdk.NewDec(25), validator.ShareTokens(sdk.NewDec(50)))) + assert.True(sdk.DecEq(t, sdk.NewDec(5), validator.ShareTokens(sdk.NewDec(10)))) +} + func TestRemoveTokens(t *testing.T) { validator := Validator{ @@ -112,10 +126,9 @@ func TestAddTokensValidatorBonded(t *testing.T) { validator, pool = validator.UpdateStatus(pool, sdk.Bonded) validator, pool, delShares := validator.AddTokensFromDel(pool, sdk.NewInt(10)) - require.Equal(t, sdk.OneDec(), validator.DelegatorShareExRate()) - assert.True(sdk.DecEq(t, sdk.NewDec(10), delShares)) assert.True(sdk.IntEq(t, sdk.NewInt(10), validator.BondedTokens())) + assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.DelegatorShares)) } func TestAddTokensValidatorUnbonding(t *testing.T) { @@ -125,11 +138,10 @@ func TestAddTokensValidatorUnbonding(t *testing.T) { validator, pool = validator.UpdateStatus(pool, sdk.Unbonding) validator, pool, delShares := validator.AddTokensFromDel(pool, sdk.NewInt(10)) - require.Equal(t, sdk.OneDec(), validator.DelegatorShareExRate()) - assert.True(sdk.DecEq(t, sdk.NewDec(10), delShares)) assert.Equal(t, sdk.Unbonding, validator.Status) assert.True(sdk.IntEq(t, sdk.NewInt(10), validator.Tokens)) + assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.DelegatorShares)) } func TestAddTokensValidatorUnbonded(t *testing.T) { @@ -139,11 +151,10 @@ func TestAddTokensValidatorUnbonded(t *testing.T) { validator, pool = validator.UpdateStatus(pool, sdk.Unbonded) validator, pool, delShares := validator.AddTokensFromDel(pool, sdk.NewInt(10)) - require.Equal(t, sdk.OneDec(), validator.DelegatorShareExRate()) - assert.True(sdk.DecEq(t, sdk.NewDec(10), delShares)) assert.Equal(t, sdk.Unbonded, validator.Status) assert.True(sdk.IntEq(t, sdk.NewInt(10), validator.Tokens)) + assert.True(sdk.DecEq(t, sdk.NewDec(10), validator.DelegatorShares)) } // TODO refactor to make simpler like the AddToken tests above @@ -158,7 +169,6 @@ func TestRemoveDelShares(t *testing.T) { poolA := InitialPool() poolA.NotBondedTokens = sdk.NewInt(10) poolA.BondedTokens = valA.BondedTokens() - require.Equal(t, valA.DelegatorShareExRate(), sdk.OneDec()) // Remove delegator shares valB, poolB, coinsB := valA.RemoveDelShares(poolA, sdk.NewDec(10)) @@ -197,6 +207,26 @@ func TestRemoveDelShares(t *testing.T) { pool.NotBondedTokens.Add(pool.BondedTokens))) } +func TestAddTokensFromDel(t *testing.T) { + val := NewValidator(addr1, pk1, Description{}) + pool := InitialPool() + pool.NotBondedTokens = sdk.NewInt(10) + + val, pool, shares := val.AddTokensFromDel(pool, sdk.NewInt(6)) + require.True(sdk.DecEq(t, sdk.NewDec(6), shares)) + require.True(sdk.DecEq(t, sdk.NewDec(6), val.DelegatorShares)) + require.True(sdk.IntEq(t, sdk.NewInt(6), val.Tokens)) + require.True(sdk.IntEq(t, sdk.NewInt(0), pool.BondedTokens)) + require.True(sdk.IntEq(t, sdk.NewInt(10), pool.NotBondedTokens)) + + val, pool, shares = val.AddTokensFromDel(pool, sdk.NewInt(3)) + require.True(sdk.DecEq(t, sdk.NewDec(3), shares)) + require.True(sdk.DecEq(t, sdk.NewDec(9), val.DelegatorShares)) + require.True(sdk.IntEq(t, sdk.NewInt(9), val.Tokens)) + require.True(sdk.IntEq(t, sdk.NewInt(0), pool.BondedTokens)) + require.True(sdk.IntEq(t, sdk.NewInt(10), pool.NotBondedTokens)) +} + func TestUpdateStatus(t *testing.T) { pool := InitialPool() pool.NotBondedTokens = sdk.NewInt(100) @@ -236,13 +266,10 @@ func TestPossibleOverflow(t *testing.T) { BondedTokens: poolTokens, } tokens := int64(71) - msg := fmt.Sprintf("validator %#v", validator) newValidator, _, _ := validator.AddTokensFromDel(pool, sdk.NewInt(tokens)) - msg = fmt.Sprintf("Added %d tokens to %s", tokens, msg) - require.False(t, newValidator.DelegatorShareExRate().LT(sdk.ZeroDec()), - "Applying operation \"%s\" resulted in negative DelegatorShareExRate(): %v", - msg, newValidator.DelegatorShareExRate()) + require.False(t, newValidator.DelegatorShares.IsNegative()) + require.False(t, newValidator.Tokens.IsNegative()) } func TestValidatorMarshalUnmarshalJSON(t *testing.T) { From 121737361ddb8343d551c5722016dffc68f703da Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Wed, 6 Mar 2019 12:24:39 +0100 Subject: [PATCH 50/50] Update PENDING.md --- PENDING.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/PENDING.md b/PENDING.md index b48f136d8085..7ae8ed1b45f6 100644 --- a/PENDING.md +++ b/PENDING.md @@ -26,6 +26,13 @@ ### SDK +* \#3750 Track outstanding rewards per-validator instead of globally, + and fix the main simulation issue, which was that slashes of + re-delegations to a validator were not correctly accounted for + in fee distribution when the redelegation in question had itself + been slashed (from a fault committed by a different validator) + in the same BeginBlock. Outstanding rewards are now available + on a per-validator basis in REST. * [\#3669] Ensure consistency in message naming, codec registration, and JSON tags. * #3788 Change order of operations for greater accuracy when calculating delegation share token value @@ -80,12 +87,6 @@ CLI flag. ### SDK -* \#3750 Track outstanding rewards per-validator instead of globally, - and fix the main simulation issue, which was that slashes of - re-delegations to a validator were not correctly accounted for - in fee distribution when the redelegation in question had itself - been slashed (from a fault committed by a different validator) - in the same BeginBlock * \#3753 Remove no-longer-used governance penalty parameter * \#3679 Consistent operators across Coins, DecCoins, Int, Dec replaced: Minus->Sub Plus->Add Div->Quo