fix(engine)!: charge finalization fees against the state actually persisted - #2417
fix(engine)!: charge finalization fees against the state actually persisted#2417sdbondi wants to merge 3 commits into
Conversation
5fc5991 to
f8818e9
Compare
1d43477 to
388b02d
Compare
f8818e9 to
0ae4812
Compare
388b02d to
4baadaa
Compare
Update: closing the fee-intent commit holeCharging categories earlier turned out not to be a workable defence — see the discussion on #2418. Each charge moved before This PR now decides at the point where the answer is finally known. Reproduced against a local swarm before the fix: claiming testnet funds with Whatever rejected the main intent stays the reason reported; the shortfall only decides that not even the fee intent survives it. Test fallout, all of it the intended semantic change — underfunded fee intents that used to commit now reject:
|
5f4d96e to
54d0e33
Compare
e091f8f to
a544897
Compare
…sisted Every finalization charge was computed against the live working state, but a transaction that cannot pay for its main intent commits only its fee intent — and that diff comes from the fee checkpoint. `Storage`, `SubstateCreate` and `TemplatePublish` were therefore priced over substates that are discarded, and `ExhaustBurn` inherited the inflation since it is taken over the running total. A transaction rejected for fees paid ~4x the storage its receipt and substates actually occupy. Charging only at the end cannot fix this on its own: the charge has to be in place before the paid-in-full check, or nothing gates a transaction that commits state it never paid for. The charge decides the outcome, the outcome picks the state, and the state determines the charge. Break the cycle by charging twice. `on_before_finalize` runs against the live state and gates the commit/reject decision — a transaction is admitted on the cost of the state it asked to commit, which is the right question to gate on. A new `on_before_persist` hook then runs against the state that was chosen, replacing those charges with the cost of what is really written. To make that work the finalization charges are now assigned rather than accumulated (`FeeBreakdown::set`), so a second pass over a different state replaces the first instead of doubling it. `ExhaustBurn` is zeroed before its total is taken so it never compounds across passes. `StateTracker::finalize` is split into `select_finalized_state` and `finalize` so the runtime can drive its modules in between; the two near-identical commit and fee-intent-commit tails collapse into one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… state `on_before_persist` and `with_working_state_mut` handed `WorkingState` — a `pub(crate)` type — through public API, so rustc warned that both were reachable at a visibility their parameter is not (`private_interfaces`, `private_bounds`). Introduce `ChargeableState`: the view a module gets when charging for a transaction's state. It exposes what a charge computed from the state needs to read — the substates to persist, how many are new, the receipt's footprint, the fee state to record against — and nothing that would let a module alter the state it is pricing. `WorkingState` goes back to being private to the runtime module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A fee-intent commit persists real state — the substates the fee instructions touched, plus a receipt carrying every event they emitted — and it was committed unconditionally. The only gate before it, `checkpoint_fee_intent`, weighs payments against the charges accrued by then, and every charge proportional to state size lands after it. So a fee intent could write state, cover only its execution, and have that state committed with the rest of the bill unpaid. It is why a 1000 uT claim commits against 2548 uT of charges. Charging earlier does not fix this: each category moved before the gate leaves the others behind, and the receipt is not a substate during execution at all, so it can never be metered that way. Instead, decide at the point where the answer is finally known. `on_before_persist` has just recomputed the charges over this exact state, so test them against the payments there: if the payment cannot cover committing, nothing commits. That is complete by construction — it tests the final total, so no charge can be added later and escape it. Whatever rejected the main intent stays the reason reported; the shortfall only decides that not even the fee intent survives it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
54d0e33 to
99b9eff
Compare
a544897 to
8ae5af6
Compare
Problem
Every finalization charge is computed against the live working state. But a transaction that cannot pay for its main intent commits only its fee intent, and that diff comes from the fee checkpoint — a snapshot taken when the fee instructions ended.
So
Storage,SubstateCreateandTemplatePublishwere priced over substates that are then thrown away, andExhaustBurninherited the inflation since it is taken over the running total. This predates #2416; pricing the receipt just made it visible, because the receipt bound scales with the discarded state's substate count and events.Measured on
fail_partial_paid_fees: 1019 µT of storage charged against a receipt and substate set that occupy 444 µT.Why the obvious fix doesn't work
Charging only at the end cannot fix this on its own. The charge has to be in place before the paid-in-full check in
select_finalized_state, or nothing gates a transaction that commits state it never paid for. But the charge decides the outcome, the outcome picks the state, and the state determines the charge — a cycle.Charge twice
on_before_finalizeruns against the live state, as before. Its only job is to gate the commit/reject decision. Pricing what would be persisted is the right question to gate on: to commit that state you must be able to pay for it.on_before_persist(new hook) runs once the state has been chosen and before its fees are settled, replacing those charges with the cost of what is really written.On a commit the second pass sees the same state and changes nothing. On a fee-intent commit it sees only what the fee intent touched.
For the replacement to be sound the finalization charges are now assigned rather than accumulated —
FeeBreakdown::set— so a second pass over a different state replaces the first instead of doubling it.ExhaustBurnis zeroed before its total is taken, so it never compounds across passes.Structure
StateTracker::finalizeis split intoselect_finalized_state(chooses the working state or the fee checkpoint, and carries the live fee state onto it) andfinalize(settles fees, builds the diff and receipt), so the runtime can drive its modules in between. The two near-identical commit and fee-intent-commit tails collapse into one.WorkingStatebecomespub(crate)and gainscount_newly_created_substates, so the whole computation can run against a detached state.Consequence worth reviewing
A transaction rejected for insufficient fees now gets refunded the storage it was provisionally charged for state that was never written. Execution charges (
WasmExecution,NativeExecution,RuntimeCall, …) are untouched — the work was done and is still paid for — so this is not a spam vector, but it is a real change in what a failed transaction costs.Testing
New regression test
a_fee_intent_commit_is_not_charged_for_the_state_it_abandonsruns the same transaction at two fee levels and asserts the rejected run's storage charge is less than half the committed run's. Withon_before_persiststubbed out it fails at 1769 vs 1772 — the two are within three bytes, which is exactly the bug.tari_engine,engine_types,transaction,ootle_sdk_core,storageandconsensus_testsall pass with no other test changes.Follow-up
Storage is still charged in one tally at finalization, so a transaction that cannot afford its state only finds out after doing all the work — and
checkpoint_fee_intent's paid-in-full check still sees no storage charge at all, since none has been computed by then. Charging incrementally during execution would close both. It composes cleanly with this PR: with the second pass authoritative, the running charge only has to be an over-estimate.