fix: reserve the withheld-fee escrow and fix fee-pool distribution edge cases - #70
Open
giunatale wants to merge 1 commit into
Open
Conversation
giunatale
requested review from
clockworkgr,
julienrbrt and
tbruyelle
as code owners
July 30, 2026 16:05
Contributor
Author
- reserve outstanding withheld-fee escrow (balance minus outstandingWithheld) in DistributeConsumerFees, WithdrawShares, and ComputeClaim so ordinary distribution or withdrawal can no longer spend the funds backing a pending downtime challenge; a proven-innocent validator's withheld share is then paid back in full. WithdrawShares caps the tokens (not the shares) so the no-orphan-balance invariant still holds. Sweep runs only post-stop, where the escrow is already dead, so it releases the full balance (documented in place). - flag debt instead of granting a free epoch when a positive fees_per_epoch is smaller than the bonded-validator count (the even share truncates to zero); a genuinely zero fee still owes nothing and is not flagged. - apply the downtime fee exclusion to the infraction epoch, not the epoch the evidence arrived in: mark epoch downtime only when the window falls in the current, not-yet-distributed epoch. - reject a non-positive photon conversion rate in ResolveDowntimeSlashTokens (previously only a zero rate), since a negative rate would yield a negative slash. - guard PayWithheldFees so an expired record -- one backing a downtime slash that has already matured and executed -- is cleared but never refunded, even if a later same-consumer challenge lands before the record is swept. - stop requiring the withheld-fee record in the downtime-slash e2e: with the exclusion now gated on the infraction epoch, the suite's 5-block epochs make the already-distributed path (clawback via the slash) the common timing, so the e2e observes the record when present and asserts the slash execution unconditionally; withholding mechanics stay covered by unit tests. - quote the reserved escrow in the fee-claim queries: ComputeClaim, WithdrawShares and both claim queries now share one feePoolDraw computation, so ConsumerFeePoolClaim and ConsumerFeePoolClaims can no longer quote a depositor more than a withdrawal pays during an open challenge window. The queries used to recompute shares * balance / total inline with no escrow subtraction while ComputeClaim -- the version that applies the reservation -- had no caller. - credit the community pool with a fee-pool balance that no shares account for, instead of panicking on it. The send restriction deliberately exempts the distribution module so community-pool spends can reach a pool, so a spend addressed straight at a pool address lands funds with nothing minted against them; MintShares, the sweep and genesis import now book that balance to the distribution module account -- the same accounting a gov-signed MsgFundConsumerFeePool would have produced -- and log it at error level. The sweep runs from BeginBlock on consumer deletion, where the old panic would have halted the provider; store failures there still panic, so a deletion is never silently aborted. The invariant keeps reporting the state, since shares do not cover the balance until something touches that pool.
giunatale
force-pushed
the
giunatale/fix/fee-pool-correctness
branch
from
July 31, 2026 18:41
7bd0bf4 to
d5eb65b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The consumer fee pool doubles as the escrow for downtime-accusation refunds,
but nothing actually reserved the escrowed amounts — plus three smaller
distribution edges.
Quotes and payouts now come from one definition
The fee-claim queries recomputed a claim inline as
shares * balance / total,without reserving the outstanding escrow, while the helper that did apply the
cap had no production caller — so a depositor querying during an open challenge
window was quoted more than a withdrawal would pay. Capping the queries is not
quite enough either: an escrow-limited withdrawal burns whole shares and
re-derives tokens from them, so a naive
min(claim, available)can stillover-quote by a truncation unit. Both the withdrawal and both queries now go
through a single
feePoolDrawdefinition, so a quote and a payout cannotdisagree at all.
An unaccounted pool balance can no longer halt a block
The send restriction deliberately exempts the distribution module account
(governance funding routes through it), which means a community-pool spend
addressed straight at a consumer fee-pool address is waved through and creates a
balance no shares account for — exactly the state the restriction exists to
prevent. That state then panicked in three places, one of them reachable from
BeginBlock through consumer deletion, so a governance transaction could arm a
later chain halt; a third site refused to import such a balance, so a state
export taken afterwards could not be restarted. All three now absorb the balance
deterministically, crediting the distribution module account — the same
accounting the proper funding path would have produced, so the funds return to
the community pool on sweep — and log it. Genuine store or codec failures still
panic, so a deletion can never be silently aborted. The consistency invariant
still reports the state, with its godoc explaining that it is reachable and
self-healing.
Escrow reservation
When downtime evidence is accepted, the accused validator's fee share is
withheld and recorded; a successful challenge retro-pays it from the pool. But
ordinary distribution and funder withdrawal operated against the full pool
balance, so the money backing a pending challenge could be spent out from under
it and a proven-innocent validator's refund could bounce.
DistributeConsumerFeesandWithdrawSharesnow operate againstbalance - outstandingWithheld, withdrawal caps tokens (not shares) so theshare-accounting invariant holds, and the post-stop sweep deliberately does not
reserve (a stopped consumer's pending slashes are already cancelled, so its
escrow is dead and the full balance is released).
Refunds never pay a matured accusation
PayWithheldFeesrefunds every record for the consumer. A record whosechallenge window has already elapsed backs a slash that has since matured and
executed; it is now cleared without payment, so a later same-consumer challenge
can never refund an already-slashed validator.
Distribution edges
a zero share and skipped both charging and the debt flag — a free epoch. It
now flags debt; a genuinely zero fee still owes nothing.
not the epoch the evidence arrived in: excluding on arrival could dock a
later epoch the validator was genuinely owed. When the window's epoch has
already paid out, only the (separately priced) pending slash applies.
ResolveDowntimeSlashTokensrejects a non-positive photon conversion rate(previously only zero); a negative rate would have produced a negative slash.
Testing
Unit coverage for every path above, including the full
exclude -> distribute -> challenge -> refund-paid-in-full sequence, the
truncation debt flag, epoch-targeted exclusion, and the expired-record refund
guard. Build, lint, and the module suite are green.