diff --git a/CHANGELOG.md b/CHANGELOG.md index 706ac4e96a3d..ff1081e7c4bb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/types/staking.go b/types/staking.go index fadf57f91355..79832ac00ab0 100644 --- a/types/staking.go +++ b/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 } diff --git a/x/distribution/keeper/delegation.go b/x/distribution/keeper/delegation.go index f6d9bdc280a3..677c38342c0f 100644 --- a/x/distribution/keeper/delegation.go +++ b/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 - // 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 - // (from a fault committed by the redelegation source validator) earlier in the same BeginBlock + // Iterate through slashes and withdraw with calculated staking for + // distribution periods. These period 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 (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 - // 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 + // A total 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 (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", - del.GetDelegatorAddr(), stake, currentStake)) + // account for rounding errors due to stake being multiplied by slash fractions + 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 } diff --git a/x/distribution/keeper/validator.go b/x/distribution/keeper/validator.go index 18c6cc6e87e0..93c20f47932a 100644 --- a/x/distribution/keeper/validator.go +++ b/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") } diff --git a/x/staking/types/validator.go b/x/staking/types/validator.go index e7132ebff3af..c048de744c0d 100644 --- a/x/staking/types/validator.go +++ b/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) {