fix(engine)!: cap the fee intent's compute at a flat credit - #2441
Conversation
5f4ce84 to
b747131
Compare
`DEVNET` is initialized by a call to `devnet`, and an associated const whose initializer is a `const fn` call is not evaluated by a runtime use of it — so `devnet`'s body escaped the compile-time check the other three networks get by being const items their constructors return. A rate above the ceiling in `devnet` compiled clean while the same edit to `MAINNET` failed the build. A `const _` item of the type forces the evaluation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMgCpP2Ef2xh3FRqpMku7E
A payment raised the compute allowance inside the fee intent, but the fee checkpoint — the only thing that makes a fee collectable — is taken after the last fee instruction. An abort before that leaves no checkpoint to fall back to, so the transaction settles as a rejection that collects nothing. Fee-intent compute was therefore payment-funded but not payment-bound: a larger `max_fee` raised the ceiling on work that was never paid for, repeatably, with the same funds. That made the main intent pointless. The same work costs nothing when it fails in the fee intent and costs a fee-intent commit when it fails in the main instructions, for an allowance that was no smaller. The fee intent now runs on a flat `FREE_COMPUTE_GRACE_POINTS`, whatever it has paid — the credit is sized to source a fee and that is all it authorizes. Payments raise the allowance only past the checkpoint, where a failure still commits the fee intent and collects. Unpaid work per transaction is bounded by a constant again rather than by the fee the sender declares. `StateTracker::compute_allowance` replaces `wasm_point_allowance` and reports what authorizes the allowance alongside the points, so exceeding it in the fee intent is reported as the credit being exhausted — paying more will not help, and the message says to move the work to the main instructions — rather than as underpayment. BREAKING CHANGE: compute in the fee intent is capped at `FREE_COMPUTE_GRACE_POINTS` regardless of the fee paid, so a transaction that ran more than that in its fee instructions is now rejected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMgCpP2Ef2xh3FRqpMku7E
b747131 to
57d2ab3
Compare
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMgCpP2Ef2xh3FRqpMku7E
sdbondi
left a comment
There was a problem hiding this comment.
Verdict
The core change is right and strictly a tightening — compute_allowance in the fee intent can only ever return less than before, so no new attack surface. The diagnosis (fee-intent abort ⇒ no checkpoint ⇒ collects nothing ⇒ payment-funded compute there is free) holds, and the fix is at the right seam. The tests discriminate properly: assert_fee_intent_credit_exceeded matches "compute credit", which neither InsufficientFeesForCompute nor InsufficientFeesForNativeExecution contains.
Findings
1. Dry runs skip the credit, so fee estimation can't surface the new rejection. compute_allowance returns None on is_dry_run() before reaching the fee-intent branch. That guard was justified when the bound depended on a payment the estimator hadn't decided yet — but the fee-intent credit is now flat and payment-independent, so enforcing it during a dry run costs nothing in estimate accuracy and is the only way a wallet learns before submission. As it stands a dry run succeeds and the real transaction comes back with "move this work to the main instructions" and no remediation. Consider returning Some(ComputeAllowance { points: FREE_COMPUTE_GRACE_POINTS, funding: FeeIntentCredit }) even in dry runs, and moving the is_dry_run check into the payment branch.
2. The ? in the payment branch is now dead, and fails open. points_funded_by returns None iff per_point_cost == 0, which !rate.prices_execution() already returned on. If anything later adds a second None case, the ? silently makes the allowance unbounded rather than zero. .unwrap_or(0) (or expect) is the fail-closed form.
3. credit_points is hardcoded in the trap path. wasm/process.rs reports limits::FREE_COMPUTE_GRACE_POINTS rather than allowance.points — equal today only because the fee-intent branch constructs it from that constant. Carrying the value through also removes the expect("BUG: allowance_remaining is Some"):
let allowance_remaining = allowance
.map(|a| (a, a.points.saturating_sub(consumed.saturating_add(native_consumed))));
let points_before = match allowance_remaining {
Some((_, remaining)) => per_call_cap.min(budget_remaining).min(remaining),
None => per_call_cap.min(budget_remaining),
};
let binding = allowance_remaining
.filter(|(_, r)| *r < budget_remaining && *r <= per_call_cap)
.map(|(a, _)| a);4. Stale comments. In wasm/process.rs, above the allowance block: "Cap further to the compute the fees paid so far can cover (plus the free-compute grace)" — no longer true in the fee intent. Same for the "measured against the charges standing when it is asked" paragraph in compute_allowance's doc, which now only describes the payment branch but reads as covering the whole function.
5. The native pre-charge branch has no paying-path test. paying_first_does_not_raise_the_fee_intents_allowance pins the WASM trap; both native tests are non-paying, so nothing pins that a payment doesn't lift the native allowance in the fee intent — the RuntimeError::FeeIntentComputeExceeded arm. A paying variant of unpaid_native_verification_traps_before_the_crypto_runs is cheap.
On "whether anything sits in that gap"
Stealth shapes do. STEALTH_LIMITS permits one fee-intent transfer with up to 16 outputs and 1000 inputs — ~140M points (2.1M + 16×6M + 1000×42k). unpaid_native_verification_traps_before_the_crypto_runs already uses an 8-output statement at ≈50M. Anything from ~5 outputs (2.1M + 5×6M = 32.1M) or ~710 inputs upward is now unconditionally rejected in the fee intent, where previously paying first made it work.
In-tree that's covered: the wallet SDK's merged_statement_fits_credit already sizes merged statements at 80% of the credit, and its doc comment already describes the credit as the entire allowance funding the fee intent until pay_fee settles — i.e. it was written against the new model. So this only bites clients building fee-intent transfers directly. If that's acceptable, worth tightening the fee-intent stealth caps (alongside max_fee_intent_transfers: 1) so those shapes fail at the structural check with a clear message rather than as a compute trap.
Minor
- The
const _doc says "nothing else reaches it at compile time", but the same commit makesFrom<Network>referenceSelf::DEVNETviamap_or. Given the verification that a runtime reference alone doesn't force evaluation the comment is accurate, but it reads as contradicting the code two lines below — worth a clause. Theconst _only pins the default committee size, which is fine since nothing indevnet's body depends on the argument. - This is a consensus-visible execution rule change with no epoch gate, unlike the seam
exhaust_burn_rate(&self, _epoch)sets up. Presumably fine pre-mainnet, but worth being deliberate about.
Review follow-ups on tari-project#2441. `compute_allowance` returned no bound at all on a dry run, so estimation never surfaced the credit and a wallet learned of it only from the rejected submission. The credit is flat and payment-independent, so a dry run can be metered against it at no cost in accuracy; the dry-run guard now sits in the payment branch, which is the one with no payment to derive a bound from. The payment branch's `?` could only fire on the case `prices_execution` already returned on, and would have widened the allowance to unbounded rather than narrowing it to zero if a second case were ever added. It is now `unwrap_or(0)`. The trap path carries the binding allowance through rather than re-reading the constant and asserting the option is populated, so the reported credit is the one that bound the call. Adds a paying variant of the native pre-charge test: nothing pinned that a payment fails to lift the native allowance inside the fee intent. Also corrects the comments describing the old payment-plus-credit rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMgCpP2Ef2xh3FRqpMku7E
sdbondi
left a comment
There was a problem hiding this comment.
Re-reviewed at 7aa5a68. All five points are addressed, and correctly:
- The dry-run guard moved into the payment branch, which is the right split — the credit is the same figure either way, and past the checkpoint there genuinely is no payment to derive a bound from.
unwrap_or(0)is the fail-closed form, and the comment above it says why.binding_allowancecarries the allowance through, socredit_pointsis now the figure that actually bound the call and theexpectis gone.paying_first_does_not_raise_the_fee_intents_native_allowancepins the native arm with a payment that funds the statement four times over, and reuses the corrupted proof so it still proves the pre-charge fired first.
Three follow-ups, all on the dry-run change:
1. The dry-run enforcement is itself untested. It's the behavioural change in this commit — a dry run of an above-credit fee intent now aborts where it previously ran unbounded — and nothing asserts it. A dry run of fee_intent_cannot_exceed_grace_compute's transaction, asserting the credit message rather than a required_fees() figure, would pin it. Without that, the guard can drift back to the old placement and every test still passes.
2. charge_native_execution's doc is now stale. It still reads: "No allowance applies (dry runs, unpriced WASM execution) ⇒ the charge only accumulates, so dry-run fee estimates stay accurate." Dry runs are no longer in that set inside the fee intent — the pre-charge can now reject one. The parenthetical wants narrowing to unpriced WASM execution, plus the past-checkpoint dry-run case.
3. fees.mdx "Estimating what to submit" says the engine "meters it exactly as it would for real, but never aborts for insufficient payment." Still literally true — the credit is not an insufficient-payment abort — but it now reads as a promise that a dry run always yields a required_fees(). This is the user-facing payoff of the change and worth a sentence: the fee intent's credit binds a dry run as it binds a real run, so a fee intent above it fails estimation rather than surfacing at submission.
Nit: the reworded comment in process.rs lost its subject — "The allowance when it — not the per-transaction hard cap — is what bounds this call."
On the consensus comment: "a const whose initializer is a const fn call is only evaluated by a const context" is an observed rustc behaviour rather than a documented guarantee, so it's worth reading as the reason the const _ is there rather than as a rule to rely on elsewhere. The const _ holds either way, which is the point.
Review follow-ups on tari-project#2441. The dry-run enforcement was the behavioural change of the previous commit and nothing asserted it, so the guard could drift back to its old placement with every test still green. `TemplateTest::set_dry_run` gives the harness the indexer's estimation path, and the new case fails when the guard moves back. Narrows `charge_native_execution`'s doc, which still listed dry runs among the cases where no allowance applies, and says in the fee reference that estimation enforces the credit — the reason the change is worth anything to a sender. The `const _` comment no longer reads as a rule about when rustc evaluates a const: what rustc was observed to do is why the item is there, and the item holds regardless. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMgCpP2Ef2xh3FRqpMku7E
sdbondi
left a comment
There was a problem hiding this comment.
Re-reviewed at f7cbcb9. All three follow-ups landed, and the dry-run test does pin the guard: move it back to the combined is_dry_run() || !prices_execution() early return and the 64M call runs unbounded to success, so execute_expect_failure fails. The const _ comment now separates the observation from the reason the item is there, which is the right split.
Three things in this commit:
1. enable_fees lost its doc comment to set_dry_run. The new method was inserted between the existing doc and the function it documented:
/// Enables fee charging for subsequent transaction executions.
/// By default, fees are disabled in tests.
/// Executes subsequent transactions as a dry run: fees are metered but not settled, as the
/// indexer's fee estimation does.
pub fn set_dry_run(&mut self, dry_run: bool) -> &mut Self { … }
pub fn enable_fees(&mut self) -> &mut Self { … }set_dry_run now carries all four lines and enable_fees is undocumented.
2. Stray unrelated change. crates/wallet/ootle-rs/examples/stealth_transfer.rs swaps the hardcoded recipient address. Nothing in this PR touches that path — looks like a local scratch edit that got swept into the commit.
3. The new docs paragraph breaks the antecedent below it. It sits between "Read FinalizeResult::required_fees() … and use it as the max_fee" and "That figure is total_fees_required + FEE_ESTIMATE_ALLOWANCE". "That figure" now trails a paragraph ending in "restructure the transaction" rather than the required_fees() sentence it refers to. Moving it below the FEE_ESTIMATE_ALLOWANCE bullets — it is a caveat on estimation as a whole, not on the figure — keeps both readings intact.
Nothing outstanding on the substance. CI: everything green so far, test (1-3/3), clippy and integration still pending.
…ng it A dry run priced whatever shape the caller's guessed fee produced, and that fee is an input to the shape: stealth input selection targets `amount + max_fee`, so the fee decides which UTXOs are spent and whether any change is left over. A change output is another stealth output to verify — `PER_OUTPUT` is 6,000,000 points — so the estimate could describe a materially cheaper transaction than the one built from it. Observed on a swarm: estimating at the frontend's `max_fee` of 1 selected inputs covering the transfer exactly, leaving no change and a single output, and reported 9354. Submitting at 9354 selected a larger pair, left change, and needed 15951 — rejected as underpaid, with nothing collected. The dry-run path now rebuilds at each figure it reports until a build at that figure pays for itself, then answers with the run that named it. The caller therefore reads a fee that some build has been held to. The settling test is against what the shape was charged, not against `required_fees`: the estimate allowance on top of it is the caller's margin for the metering drift a wider `max_fee` causes, and testing against it would treat a fee that pays in full as insufficient and spend another round chasing it. Also from review on tari-project#2441: restores `enable_fees`'s doc comment, which the `set_dry_run` insertion took, and moves the dry-run credit paragraph in the fee reference below the allowance bullets so "That figure" refers to `required_fees()` again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMgCpP2Ef2xh3FRqpMku7E
…ng it (#2443) ## The bug A dry run prices whatever shape the caller's guessed fee produces — and that fee is an input to the shape. Stealth input selection targets `amount + max_fee`, so the fee decides which UTXOs are spent and whether any change is left over, and a change output is another stealth output to verify at `PER_OUTPUT` = 6,000,000 points. The estimate could therefore describe a materially cheaper transaction than the one built from it. Reproduced on a local swarm, on the transaction that prompted this: | | inputs selected | stealth outputs | `NativeExecution` | required | |---|---|---|---|---| | dry run at the frontend's `max_fee=1` | worth 8,000,001 | 1 (change = 0) | 8,184 | **9,354** | | dry run at 9,354 | worth 8,986,059 | 2 (change 976,705) | 14,184 | 15,951 | | the submission built from the estimate | worth 8,986,059 | 2 | 14,184 | **15,951** | At `max_fee=1` the selected inputs covered the transfer exactly, so there was no change and the statement carried a single output. At the real fee they no longer cover it, a larger pair is chosen, change appears, and the transaction is rejected as underpaid — collecting nothing, since a fee intent that fails leaves no checkpoint to fall back to. The whole 6,597 shortfall is one stealth output: 6,000 for the output, 300 of exhaust burn on it, and ~297 of storage and substate creation. ## The fix The dry-run path in `handle_stealth_transfer` rebuilds at each figure it reports until a build at that figure pays for itself, then answers with the run that named it. The caller therefore reads a fee that some build has been held to, rather than one derived from a shape that is about to change. The settling test is against **what the shape was charged**, not against `required_fees`. The estimate allowance `required_fees` carries on top is the caller's margin for the metering drift a wider `max_fee` causes; testing against it treats a fee that already pays in full as insufficient and spends another round chasing the padding. On the swarm that difference alone took the loop from 4 rounds to 2. The reported figure still includes the allowance. Bounded at 5 rounds, with a warning and the highest figure reached if it does not settle — an estimate below the cost cannot be submitted at all, so the higher figure is the safer answer. `StealthTransferParams` gains `Clone` so a round can rebuild. Each round's lock is released as before, which also discards that round's unconfirmed change outputs. ## Verified on a swarm Wallet daemon rebuilt and restarted on this branch, same account and destination as the failing transaction: | transfer | estimate | rounds | submission | |---|---|---|---| | 8,000,000 | 15,978 | 4 (before the `charged` refinement) | **Accepted**, paid 15,978 | | 500,000 | 15,909 | 2 | **Accepted**, paid 15,909 | Both settle with an overcharge equal to the estimate allowance (25 and 23 µT), which is expected and is not refundable on a stealth-revealed fee — the reason this iterates rather than padding the estimate. ## Also, from review on #2441 - `enable_fees` gets its doc comment back; inserting `set_dry_run` above it had taken it. - The dry-run credit paragraph in `reference/fees.mdx` moves below the `FEE_ESTIMATE_ALLOWANCE` bullets, so "That figure" refers to `required_fees()` again. - Adds a paragraph there on why an estimate only describes the shape it was metered on, which is the bug class this PR fixes. ## Test plan - [x] `cargo check` clean on `tari_ootle_walletd` and `tari_ootle_wallet_sdk`, including tests - [x] Docs site builds - [x] End-to-end on a local swarm: estimate → submit → committed, twice (table above) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Also carries #2439's outstanding review fix
ConsensusConstants::DEVNETis initialized by a call todevnet, and an associated const whoseinitializer is a
const fncall is not evaluated by a runtime use of it — sodevnet's bodyescaped the compile-time ceiling check the other three networks get by being const items their
constructors return. Verified: a
20_000bps rate indevnet's body compiled clean while the sameedit to
MAINNETfailed withE0080.Note the fix suggested in review — falling back to
Self::DEVNETinFrom<Network>— does notwork; I applied it and the bad rate still compiled. A
const _item of the type is what forces theevaluation.
The hole
A payment raised the compute allowance inside the fee intent
(
StateTracker::wasm_point_allowance:points_funded_by(unspent) + FREE_COMPUTE_GRACE_POINTS).But the fee checkpoint — the only thing that makes a fee collectable — is taken in
checkpoint_fee_intent()after the last fee instruction. An abort before that leaves nocheckpoint to fall back to, so the transaction settles as a rejection that collects nothing.
Fee-intent compute was therefore payment-funded but not payment-bound: declaring a larger
max_feeraised the ceiling on work that was never paid for, repeatably, with the same funds.That made the main intent pointless. The same work costs nothing when it fails in the fee intent
and costs a fee-intent commit when it fails in the main instructions — for an allowance that was no
smaller. There was no reason to use the main intent at all.
The change
The fee intent now runs on a flat
FREE_COMPUTE_GRACE_POINTS, whatever it has paid. The creditis sized to source a fee and that is all it authorizes. Payments raise the allowance only past the
checkpoint, where a failure still commits the fee intent and collects. Work that needs more than the
credit has to go in the main instructions.
Unpaid work per transaction is bounded by a constant again, rather than by the fee the sender
declares.
StateTracker::compute_allowancereplaceswasm_point_allowanceand reports what authorizes theallowance (
ComputeFunding::{FeeIntentCredit, Payment}) alongside the points, so exceeding it inthe fee intent is reported as the credit being exhausted — paying more will not help, and the
message says to move the work to the main instructions — rather than as underpayment. Two new
error variants carry that:
WasmExecutionError::FeeIntentComputeExceededon the trap path andRuntimeError::FeeIntentComputeExceededon the native pre-charge.The "WASM not priced ⇒ no bound" case is preserved via
WasmMeteringRate::prices_execution(), sofee-disabled tests keep only the per-transaction hard cap.
The credit stays at 32M
It was already sized at ~3x the most expensive legitimate fee-sourcing flow. What changes is that
the headroom is now a ceiling rather than slack, so a real flow sitting closer to it than
grace_covers_legitimate_fee_sourcing_flowsandcomplex_fee_paymentcover would now be rejected.Worth a second opinion on whether anything sits in that gap.
Breaking change
Compute in the fee intent is capped at
FREE_COMPUTE_GRACE_POINTSregardless of the fee paid, so atransaction that ran more than that in its fee instructions is now rejected.
Test plan
paying_first_does_not_raise_the_fee_intents_allowance: pays first, thenruns an above-credit call inside the fee intent. Verified it fails under the old behaviour
— with the payment-funded fee-intent branch restored it reports "Transaction succeeded but it
was expected to fail".
fee_intent_cannot_exceed_grace_computeand the twonative_compute_budgetfee-intent casesnow assert the credit message, matched loosely so the constant can move.
cargo check --workspace --teststari_engine --test {fees, compute_fee_budget, native_compute_budget, metering_budget, complex_fee_payment, limits}tari_template_builtin --test liquidity_poolwhich is now a hard cap rather than a floor.