Merge PR #4094: Account for Rounding Errors in Distribution Calculations

CHANGELOG.md

@@ -145,6 +145,8 @@
 when the validator or delegation do not exist.
 * [\#4050](https://github.com/cosmos/cosmos-sdk/issues/4050) Fix DecCoins APIs
 where rounding or truncation could result in zero decimal coins.
+* [\#4088](https://github.com/cosmos/cosmos-sdk/issues/4088) Fix `calculateDelegationRewards`
+by accounting for rounding errors when multiplying stake by slashing fractions.
 
 ## 0.33.2
 

types/staking.go

@@ -75,6 +75,7 @@ type Validator interface {
 	GetDelegatorShares() Dec                        // total outstanding delegator shares
 	TokensFromShares(Dec) Dec                       // token worth of provided delegator shares
 	TokensFromSharesTruncated(Dec) Dec              // token worth of provided delegator shares, truncated
+	TokensFromSharesRoundUp(Dec) Dec                // token worth of provided delegator shares, rounded up
 	SharesFromTokens(amt Int) (Dec, Error)          // shares worth of delegator's bond
 	SharesFromTokensTruncated(amt Int) (Dec, Error) // truncated shares worth of delegator's bond
 }

x/distribution/keeper/delegation.go

@@ -64,13 +64,16 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d
 	startingPeriod := startingInfo.PreviousPeriod
 	stake := startingInfo.Stake
 
-	// iterate through slashes and withdraw with calculated staking for sub-intervals
+	// Iterate through slashes and withdraw with calculated staking for
-	// these offsets are dependent on *when* slashes happen - namely, in BeginBlock, after rewards are allocated...
+	// distribution periods. These period offsets are dependent on *when* slashes
-	// slashes which happened in the first block would have been before this delegation existed,
+	// happen - namely, in BeginBlock, after rewards are allocated...
-	// UNLESS they were slashes of a redelegation to this validator which was itself slashed
+	// Slashes which happened in the first block would have been before this
-	// (from a fault committed by the redelegation source validator) earlier in the same BeginBlock
+	// delegation existed, 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 below
+	// 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 {
 		k.IterateValidatorSlashEventsBetween(ctx, del.GetValidatorAddr(), startingHeight, endingHeight,
@@ -78,7 +81,9 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d
 				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
+
+					// Note: It is necessary to truncate so we don't allow withdrawing
+					// more rewards than owed.
 					stake = stake.MulTruncate(sdk.OneDec().Sub(event.Fraction))
 					startingPeriod = endingPeriod
 				}
@@ -87,18 +92,26 @@ 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
+	// A total stake sanity check; Recalculated final stake should be less than or
-	// here we cannot use Equals because stake is truncated when multiplied by slash fractions
+	// equal to current stake here. We cannot use Equals because stake is truncated
-	// we could only use equals if we had arbitrary-precision rationals
+	// when multiplied by slash fractions (see above). We could only use equals if
+	// we had arbitrary-precision rationals.
 	currentStake := val.TokensFromShares(del.GetShares())
 	if stake.GT(currentStake) {
-		panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake: %s, %s",
+		// account for rounding errors due to stake being multiplied by slash fractions
-			del.GetDelegatorAddr(), stake, currentStake))
+		currentStakeRoundUp := val.TokensFromSharesRoundUp(del.GetShares())
+		if stake.Equal(currentStakeRoundUp) {
+			stake = currentStake
+		} else {
+			panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake"+
+				"\n\tfinal stake:\t%s"+
+				"\n\tcurrent stake:\t%s",
+				del.GetDelegatorAddr(), stake, currentStake))
+		}
 	}
 
 	// calculate rewards for final period
 	rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake))
-
 	return rewards
 }
 

x/distribution/keeper/validator.go

@@ -106,7 +106,10 @@ func (k Keeper) updateValidatorSlashFraction(ctx sdk.Context, valAddr sdk.ValAdd
 	}
 	currentMultiplicand := sdk.OneDec().Sub(currentFraction)
 	newMultiplicand := sdk.OneDec().Sub(fraction)
-	updatedFraction := sdk.OneDec().Sub(currentMultiplicand.Mul(newMultiplicand))
+
+	// using MulTruncate here conservatively increases the slashing amount
+	updatedFraction := sdk.OneDec().Sub(currentMultiplicand.MulTruncate(newMultiplicand))
+
 	if updatedFraction.LT(sdk.ZeroDec()) {
 		panic("negative slash fraction")
 	}

x/staking/types/validator.go

@@ -421,6 +421,12 @@ func (v Validator) TokensFromSharesTruncated(shares sdk.Dec) sdk.Dec {
 	return (shares.MulInt(v.Tokens)).QuoTruncate(v.DelegatorShares)
 }
 
+// TokensFromSharesRoundUp returns the token worth of provided shares, rounded
+// up.
+func (v Validator) TokensFromSharesRoundUp(shares sdk.Dec) sdk.Dec {
+	return (shares.MulInt(v.Tokens)).QuoRoundUp(v.DelegatorShares)
+}
+
 // SharesFromTokens returns the shares of a delegation given a bond amount. It
 // returns an error if the validator has no tokens.
 func (v Validator) SharesFromTokens(amt sdk.Int) (sdk.Dec, sdk.Error) {