fix: adjust the response execution prepayment on its nominal part - #11431
Open
mraszyk wants to merge 2 commits into
Open
fix: adjust the response execution prepayment on its nominal part#11431mraszyk wants to merge 2 commits into
mraszyk wants to merge 2 commits into
Conversation
The cycles prepaid for a response execution are adjusted to the canister's current Wasm execution mode before the callback is executed, so that a canister upgraded from Wasm32 to Wasm64 (or vice versa) across a call is charged exactly as if it had performed the call in the Wasm execution mode in which the response is executed. That adjustment compared the prepayment to the requirement on their real parts, which made it a no-op under the free cost schedule: the real part of an `Instructions` amount is zero under that schedule, so both comparisons saw 0 against 0 and left the nominal part at the amount prepaid for the Wasm execution mode the canister had when it performed the call. The refund of the cycles for the unused instructions is computed in the canister's current Wasm execution mode and clamped to the prepayment, so the consumed cycles metrics were then misreported in both directions. With a 1e9 instruction limit and 1e6 instructions executed by the callback: - Wasm32 at the call, Wasm64 at the response: the refund exceeds the prepayment and is clamped to it, so the whole response execution, including the fixed per-message execution fee, disappears from the metrics: 0 is reported instead of 7_000_000; - Wasm64 at the call, Wasm32 at the response: 1_006_000_000 is reported instead of 6_000_000, an over-report by the difference of the two prepayments for the full instruction limit, i.e. one that scales with the instruction limit rather than with the instructions actually executed. Both comparisons are now performed on the nominal parts. Under the normal cost schedule the real and the nominal parts coincide, hence no cycle amount and no metric changes there. Under the free cost schedule the real part of the missing prepayment is zero, so topping it up cannot fail and only the metrics move. Both functions document that comparing a single part is sound only because the prepayment recorded in the callback and the requirement derived at response time carry the same cost schedule, which holds as long as the cost schedule of a subnet cannot change once the subnet exists. `settle_prepayment_for_unexecuted_response` is left untouched: the fixed per-message execution fee does not depend on the Wasm execution mode and the real parts it compares are equal, so it already charges and reports that fee in both cost schedules and in all four combinations of Wasm execution modes. The added test pins this down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The accounting fix is focused, documented, and comprehensively tested with no unresolved issues.
Pull request overview
Fixes response-execution cycle metrics when Wasm execution modes change under the free cost schedule.
Changes:
- Compares nominal rather than real prepaid cycles.
- Adds matrix-based regression tests for adjustments, refunds, and unexecuted responses.
File summaries
| File | Description |
|---|---|
rs/cycles_account_manager/tests/cycles_account_manager.rs |
Adds comprehensive cost-schedule and Wasm-mode regression coverage. |
rs/cycles_account_manager/src/cycles_account_manager.rs |
Corrects prepayment adjustment comparisons and documents assumptions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The note on the assumption that the cost schedule of a subnet never changes attributed the zero refund to `refund_unused_execution_cycles` clamping the refund to the prepayment. That clamp is in fact inactive in this scenario: the refund is derived with the cost schedule in effect when the response is executed, and `CompoundCycles::new` gives an `Instructions` amount a zero real part under the free cost schedule, so the refund already has a zero real part before it is clamped. The clamp then compares a real part of zero against the non-zero real part of the prepayment and returns the refund unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 1329ba8. Security Overview
Detected Code Changes
|
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.
Summary
The cycles prepaid for a response execution are adjusted to the canister's current
Wasm execution mode before the callback is executed (#11414), so that a canister
upgraded from Wasm32 to Wasm64 (or vice versa) across a call is charged exactly as
if it had performed the call in the Wasm execution mode in which the response is
executed.
That adjustment compared the prepayment to the requirement on their real parts,
which made it a no-op under the free cost schedule: the real part of an
Instructionsamount is zero under that schedule, so both comparisons saw0against
0and left the nominal part at the amount prepaid for the Wasm executionmode the canister had when it performed the call.
refund_unused_execution_cyclesthen computes the refund in the canister's currentWasm execution mode and clamps it to the prepayment, so the cycles reported into the
consumed cycles metrics were wrong in both directions.
Impact
With a 1e9 instruction limit and 1e6 instructions executed by the callback, on a
subnet using the free cost schedule:
whole response execution, including the fixed per-message execution fee,
disappears from the metrics.
full instruction limit, i.e. one that scales with the instruction limit rather
than with the instructions actually executed, dwarfing the true value.
Only the metrics are affected: no cycles balance changes, since the real parts are
zero under the free cost schedule to begin with.
Details
Both comparisons are now performed on the nominal parts:
this is equivalent to what was there before: no cycle amount and no metric changes,
and in particular no change for any subnet on that schedule;
so
consume_with_threshold_implcannot fail when topping the prepayment up, i.e.the out-of-cycles failure path introduced by fix: Adjust the response execution prepayment to the Wasm execution mode #11414 is not reachable there, and
only the metrics move.
settle_prepayment_for_unexecuted_response, which settles a response whose callbackis not executed at all, is left untouched: the fixed per-message execution fee does
not depend on the Wasm execution mode and the real parts it compares are equal, so it
already charges and reports that fee correctly in both cost schedules and in all four
combinations of Wasm execution modes. The added test pins this down.
Assumption
Comparing a single part, be it the real or the nominal one, is sound only because the
prepayment recorded in the callback and the requirement derived when the response is
executed carry the same cost schedule. That holds as long as the cost schedule of
a subnet cannot change once the subnet exists: a subnet is assigned its cost schedule
when it is created (
do_create_subnet) and keeps it (a subnet split inherits it andsplitting a rental subnet is rejected outright), and
UpdateSubnetPayloadexposes nofield to change it.
Were the cost schedule of a live subnet allowed to change, this would break: the real
and the nominal parts would no longer agree on how the prepayment and the requirement
compare, and settling the prepayment would need to account for both parts separately.
In particular, a canister whose subnet switched from the normal to the free cost
schedule across a call would take the branch returning the prepayment unchanged, i.e.
keep the prepayment it made under the normal cost schedule, real part and all.
refund_unused_execution_cycleswould then derive the refund with the cost schedulein effect when the response is executed, where the real part of an
Instructionsamount is zero, so it would return nothing at all to the balance and the canister
would forfeit that whole real prepayment instead of getting it back. Both functions
document this assumption.
Testing
Three tests in
rs/cycles_account_manager/tests/cycles_account_manager.rs, eachcovering the full matrix of 2 cost schedules × 2 Wasm execution modes at the call ×
2 Wasm execution modes at the response:
adjust_prepayment_for_response_execution_matches_wasm_execution_mode: theadjusted prepayment matches the requirement in both its real and its nominal part,
the balance moved by the real difference, and the consumed cycles metrics report
the adjusted prepayment;
response_execution_consumed_cycles_match_wasm_execution_mode: over the wholeprepay → adjust → refund sequence, both the cycles charged and the consumed cycles
metrics match the cost of executing the response in the Wasm execution mode in
which it was executed;
settle_prepayment_for_unexecuted_response_charges_only_the_base_fee: a responsewhose callback is not executed costs, and reports, the fixed per-message execution
fee only.
Reverting just the two comparisons makes the first two tests fail with exactly the
values from the table above; the third passes either way and is regression coverage
for the path that was already correct.