Skip to content

fix(enterprise/poa): avoid panic when allocated fees exceed the balance by dust - #26814

Open
kriss39 wants to merge 1 commit into
cosmos:mainfrom
kriss39:fix/poa-fee-rounding-overshoot
Open

kriss39 wants to merge 1 commit into
cosmos:mainfrom
kriss39:fix/poa-fee-rounding-overshoot

Conversation

@kriss39

@kriss39 kriss39 commented Sep 13, 2026

Copy link
Copy Markdown

Description

calculateValidatorPendingFees rounds each validator's share of the unallocated fees (QuoDec rounds half-even at 18 decimals), so the sum of the shares can exceed the amount that was split by a few units of dust. Six equal-power validators splitting 1 unit each get 0.166666666666666667, and totalAllocatedFees becomes 1.000000000000000002 while the module balance is 1.

getUnallocatedFees then computes balance - totalAllocated with DecCoins.Sub, which panics on a negative result (negative coin amount). The !unallocated.IsAllPositive() guard right after it is unreachable. Every call site of checkpointAllValidators panics until new fees land in the module account:

  • WithdrawValidatorFees (MsgWithdrawValidatorFees)
  • validator power updates (UpdateValidator), AddValidator, RotateConsPubKey
  • the WithdrawableFees query

On a chain with zero-fee txs, or with fees in a denom other than the one that overshot, the state does not recover by itself.

Changes

  • Subtract with SafeSub and keep only the denoms that still have a positive remainder, so an overshoot in one denom neither panics nor blocks allocation of new fees in other denoms.
  • Regression test TestGetUnallocatedFeesRoundingOvershoot (panics on main). It also checks the query and withdraw paths and that new fees in another denom are still allocated.

The rounding itself is left unchanged (switching to QuoDecTruncate would change existing allocations by 1e-18 and the expectations in TestProportionalDistribution); the dust-level overshoot is harmless once it can no longer panic. Happy to switch to truncation instead if you prefer a hard "allocated ≤ balance" invariant.

enterprise/poa tests pass.

@greptile-apps

greptile-apps Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@kriss39
kriss39 force-pushed the fix/poa-fee-rounding-overshoot branch from 229a13f to 1b9a839 Compare September 13, 2026 20:13
@kriss39 kriss39 closed this Sep 14, 2026
@kriss39
kriss39 force-pushed the fix/poa-fee-rounding-overshoot branch from 1b9a839 to 8f265eb Compare September 14, 2026 08:47
…ce by dust

calculateValidatorPendingFees rounds each validator's share, so the sum of
the shares can exceed the unallocated amount by a few units of dust (six
equal-power validators splitting 1 unit allocate 1.000000000000000002).
getUnallocatedFees then computed balance - totalAllocated with
DecCoins.Sub, which panics on a negative result, so every subsequent
checkpoint (fee withdrawals, validator power updates, key rotations and the
WithdrawableFees query) panicked until new fees landed in the module
account. The IsAllPositive guard after the subtraction was unreachable.

Subtract with SafeSub and keep only the denoms with a positive remainder.
@kriss39 kriss39 reopened this Sep 14, 2026
@kriss39

kriss39 commented Sep 14, 2026

Copy link
Copy Markdown
Author

Replaced the commit with a signed one (396760f, same content) so it shows as Verified; the force-push briefly auto-closed the PR, reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant