R4R: Allow the undelegation of more coins than were delegated; More validity checks.

PENDING.md

@@ -96,6 +96,12 @@ CLI flag.
   where validator is unexpectedly slashed throwing off test calculations
 * [\#3411] Include the `RequestInitChain.Time` in the block header init during
 `InitChain`.
+* [\#3717] 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.
+* [\#3717] Ignore unknown proposers in allocating rewards for proposers, in case
+  unbonding period was just 1 block and proposer was already deleted.
 * [\#3726] Cap(clip) reward to remaining coins in AllocateTokens.
 
 ### Tendermint

cmd/gaia/app/sim_test.go

@@ -171,7 +171,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,
 		},
 	}

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`
@@ -274,6 +276,11 @@ 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 can increase the validator's exchange rate (tokens/shares) slightly if the
+undelegated tokens are non-integral.
+
 #### Keepers/Handlers
 
 ```go

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

x/auth/account.go

@@ -287,6 +287,11 @@ 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 can increase the validator's exchange rate (tokens/shares) slightly if
+// the undelegated tokens are non-integral.
+//
 // CONTRACT: The account's coins and undelegation coins must be sorted.
 func (bva *BaseVestingAccount) TrackUndelegation(amount sdk.Coins) {
 	for _, coin := range amount {
@@ -295,12 +300,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(DV, D - X)
 		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)

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))
 }

x/bank/msgs.go

@@ -35,6 +35,9 @@ func (msg MsgSend) ValidateBasic() sdk.Error {
 	if msg.ToAddress.Empty() {
 		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")
 	}

x/distribution/keeper/allocation.go

@@ -1,13 +1,16 @@
 package keeper
 
 import (
+	"fmt"
+
 	abci "github.com/tendermint/tendermint/abci/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
 // allocate fees handles distribution of the collected fees
 func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower int64, proposer sdk.ConsAddress, votes []abci.VoteInfo) {
+	logger := ctx.Logger().With("module", "x/distribution")
 
 	// fetch collected fees & fee pool
 	feesCollectedInt := k.feeCollectionKeeper.GetCollectedFees(ctx)
@@ -35,9 +38,20 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in
 	proposerReward := feesCollected.MulDecTruncate(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.
+		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.",
+			proposer.String()))
+	}
 
 	// calculate fraction allocated to validators
 	communityTax := k.GetCommunityTax(ctx)

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

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

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

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)

x/ibc/handler.go

@@ -44,6 +44,7 @@ func handleIBCReceiveMsg(ctx sdk.Context, ibcm Mapper, ck BankKeeper, msg MsgIBC
 		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()