fix: remove the ordering on CompoundCycles by mraszyk · Pull Request #11529 · dfinity/ic

mraszyk · 2026-09-10T10:46:14Z

Summary

CompoundCycles pairs a real amount, which moves the canister's cycles balance,
with a nominal one, which feeds the consumed cycles metrics. It derived Ord,
which orders lexicographically, i.e. by the real part first, so a comparison ignored
the nominal parts whenever the real parts differed.

There is no meaningful order on a pair of independently accounted amounts, and that one
is actively wrong for what the cycles accounting in ic-cycles-account-manager compares:
an amount recorded when a call is performed against one derived when its response is
executed. Under the free cost schedule the real part of an amount made free is zero
however large its nominal part, so the amount made free always compares as the smaller
one.

The derive is removed, so such a comparison is now a compile error, and the type
documents why it has no ordering. This also corrects a claim in the same doc comment:
the generics enforce the same CyclesUseCase, not the same cost schedule, which is
not part of the type at all.

The four `min` calls

site	before	after
`settle_prepayment_for_unexecuted_response`	`p - base_fee.min(p)`	`p - base_fee`
`refund_for_response_transmission`	`p - cost.min(p)`	`p - cost`
`refund_unused_execution_cycles`	`refund.min(p)`	`refund.component_wise_min(p)`
`CanisterManager::load_canister_snapshot`	`p - cost.min(p)`	`p - cost.component_wise_min(p)`

Subtraction saturates in both parts, so x - y already equals
x - x.component_wise_min(y): capping a cost before subtracting it from that same
prepayment was redundant. Where a cap is wanted, component_wise_min replaces the
ordering with the minimum of the real parts paired with the minimum of the nominal
parts. load_canister_snapshot keeps its redundant cap so that it stays identical to
the refund_unused_execution_cycles refund it recomputes.

Motivation

Hardening rather than a fix. Under a single cost schedule the two parts of an amount
agree on every ordering previously consulted, so the lexicographic tie-break was never
reached: no cycles balance and no consumed cycles metric changes. Removing the derive
keeps it out of reach as the accounting changes.

Concretely, were the two amounts ever to carry different cost schedules, the removed
ordering would produce:

settle_prepayment_for_unexecuted_response: for a prepayment made under the free
cost schedule and settled under the normal one, the ordering picks the whole
prepayment as the charge. The consumed cycles counter then reports the nominal
prepayment for the entire instruction limit — for a callback that never ran — instead
of the fixed per-message execution fee.
refund_for_response_transmission: same shape, same direction. Nothing is refunded
and the whole nominal prepayment is reported as consumed.
refund_unused_execution_cycles: the cap is decided by the real parts, so it need
not bound the nominal one at all. For a prepayment made under the normal cost
schedule and refunded under the free one, it selects the refund and leaves that
refund's nominal part uncapped: exceeding the prepayment there trips the debug
assertion in refund_cycles, and saturates both metrics in a release build. The
other way round it selects the whole prepayment, refunding all of it and reporting
the response as having consumed nothing.

None of the three moves a cycles balance: wherever the two orderings disagree, one of
the amounts carries the free cost schedule and hence a zero real part, so only the
nominal parts, i.e. the consumed cycles metrics, can differ.

One comparison of this kind is left after this PR, in
adjust_prepayment_for_response_execution, which weighs the prepayment for a response
against the requirement derived when that response is executed. It is likewise sound
only while both carry the same cost schedule. #11431 removes it.

Note

#11431 builds on this and currently contains these hunks too. It will be rebased onto
this PR, so review this one on its own.

🤖 Generated with Claude Code

`CompoundCycles` derived `Ord`, which orders lexicographically, i.e. by the real part first, and hence answers "which of the two amounts is larger?" from the real part alone whenever the real parts differ. That is the wrong tie-break under the free cost schedule, where the real part of a use case made free is zero no matter how large the nominal part is. The two parts of an amount are accounted for independently and there is no meaningful order on the pair, so the derive is removed and the type documents why it has none. All four uses of the resulting `min` turn out not to need an ordering at all: - `settle_prepayment_for_unexecuted_response`, `refund_for_response_transmission` and the net charge recorded in `load_canister_snapshot` clamped a cost to a prepayment only to subtract it from that prepayment right after. Subtracting two `CompoundCycles` saturates part by part, so `x - x.min(y)` is `x - y` and the clamp was redundant. The one in `load_canister_snapshot` mirrors the refund that `refund_unused_execution_cycles` derives from the identical formula, so both sides keep agreeing. - `refund_unused_execution_cycles` performs the one genuine clamp, now expressed as `x - (x - y)`, which is the part-wise minimum of `x` and `y` because both subtractions saturate. No cycle amount and no metric changes: under a single cost schedule the real and the nominal part of an amount agree on every ordering that was previously consulted, so the lexicographic tie-break was never reached. Six test assertions comparing accumulated execution costs now compare their nominal parts, as `assert_gt!` no longer applies to a `CompoundCycles`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The new public documentation inaccurately describes both the old free-schedule ordering and the removed lexicographic min behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Removes misleading ordering from CompoundCycles and replaces affected comparisons with part-wise saturating arithmetic.

Changes:

Removes Ord and PartialOrd.
Rewrites four refund/clamping operations.
Updates execution-cost assertions to compare nominal cycles.

File summaries

File	Description
`rs/types/cycles/src/compound_cycles.rs`	Removes ordering and documents alternatives.
`rs/cycles_account_manager/src/cycles_account_manager.rs`	Reworks refund calculations.
`rs/execution_environment/src/canister_manager.rs`	Updates snapshot execution accounting.
`rs/execution_environment/src/execution/response/tests.rs`	Compares nominal execution costs.
`rs/execution_environment/tests/hypervisor.rs`	Compares nominal execution costs.

Review details

Suppressed comments (1)

rs/types/cycles/src/compound_cycles.rs:91

This identity is not true for the removed lexicographic min. For example, with x = (real: 5, nominal: 20) and y = (real: 10, nominal: 1), x - x.min(y) was zero while x - y is (0, 19). Since CompoundCycles no longer has min, describe the direct subtraction as a part-wise clamp rather than equating it with the old operation.

/// Compare `real()` or `nominal()` explicitly instead. Note that subtraction
/// saturates part by part, which covers the two idioms that would otherwise want an
/// ordering: `x - x.min(y)` is simply `x - y`, and the part-wise minimum of `x` and
/// `y` is `x - (x - y)`.

Files reviewed: 5/5 changed files
Comments generated: 1
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Three corrections to the comments added by the previous commit, all of them imprecise rather than wrong about the code: - The note on `CompoundCycles` blamed the free cost schedule for the lexicographic order being the wrong one. That is backwards: when both amounts carry the free cost schedule, the real part of a use case made free is zero on both sides, so the comparison falls through to the nominal parts and gets it right, just as it does under the normal cost schedule, where the two parts of an amount coincide. The order is misleading precisely when the two amounts carry different cost schedules, which is the case this crate hits: one amount is recorded when a call is performed and the other derived when its response is executed. - The same note claimed `x - x.min(y)` is `x - y`. That holds for a part-wise minimum, but not for the lexicographic one that was removed: with `x = (real 5, nominal 20)` and `y = (real 10, nominal 1)`, the latter gives `x.min(y) == x` and hence `(0, 0)`, whereas `x - y` is `(0, 19)`. The note now states the two idioms directly instead of relating them to an operation that no longer exists. - The cap in `refund_unused_execution_cycles` is not merely defense in depth against an inconsistent caller: `scale_cost` scales both parts of an amount by the subnet size, so a subnet that grew between the prepayment and the refund makes the refund exceed the prepayment. That is the one input of the four that can differ in practice today. The counterpart comment in `load_canister_snapshot` also claimed to recompute "exactly the refund" that function derived, which is the refund before its cap, and asserted that no cap is needed there without giving the identity that makes the two agree. Comments only, no functional change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The accounting changes are equivalent, with only two non-blocking documentation clarifications identified.

Review details

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

Two clarifications from review, comments only: - The note on the cap in `refund_unused_execution_cycles` listed the instruction count alongside the Wasm execution mode, the cost schedule and the subnet size as inputs "the prepayment was made with as well", and then allowed all of them to have changed since. That conflates two different things: the refund deliberately covers fewer instructions than the prepayment was made for, whereas the other three are meant to be the same and are the ones that can differ. - The counterpart note in `load_canister_snapshot` justified dropping the cap with `prepaid - refund == prepaid - refund.min(prepaid)`, which reads as the removed lexicographic `min`, for which it does not hold. The cap that `refund_unused_execution_cycles` applies is part-wise, and the identity holds in each part unconditionally, so the note now says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The part-wise cap needs distinguishing regression coverage, and the new documentation contains conflicting or overstated claims.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

rs/types/cycles/src/compound_cycles.rs:93

This new section says values can carry different cost schedules, but the same type documentation still claims at lines 56–59 that the generic type safety enforces an identical CanisterCyclesCostSchedule. The schedule is not a type parameter or stored field, so those statements now directly contradict each other; please update the earlier paragraph to say that only the use-case kind is enforced.

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

Three points from review: - The type-safety paragraph claimed that the generics and phantom data enforce that arithmetic is performed on amounts created for the same `CyclesUseCase` *and* `CanisterCyclesCostSchedule`. Only the former is a type parameter: `new` folds the cost schedule into the real part and does not retain it, so nothing stops amounts created under different cost schedules from being combined. That is precisely the case the note on ordering is about, so the two statements contradicted each other. - A test pins both documented identities on two amounts whose parts are oppositely ordered, which a lexicographic ordering of the pair would decide on the real parts alone: an `Instructions` amount of 5 under the normal cost schedule, `(5, 5)`, and one of 10 under the free cost schedule, `(0, 10)`. Their difference is `(5, 0)` and their part-wise minimum, `x - (x - y)`, is `(0, 5)`. - The note on the cap in `refund_unused_execution_cycles` said its pricing inputs are the ones in effect now. That is not so for an update call or an install, where the caller passes the same `subnet_cycles_config` it prepaid with; a response execution is the case whose prepayment was made in an earlier round and can therefore have been priced with a different subnet size. Subnet growth can also only price the refund above the prepayment, not necessarily do so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The replacements preserve part-wise accounting semantics and all ordering-dependent usages were addressed.

Review details

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

The one genuine cap, in `refund_unused_execution_cycles`, was written as `x - (x - y)`, which is the component-wise minimum of `x` and `y` because both subtractions saturate. It is now a named `CompoundCycles::component_wise_min`. The identity is correct but needed explaining wherever it appeared, and it appeared twice: at the cap itself and in the `load_canister_snapshot` comment arguing that no cap is needed there. Both explanations were misread during review, once as the removed lexicographic `min`. A named operation states the intent at the call site, and the two comments now refer to it rather than re-deriving it. The type note keeps the remaining identity, which is what makes the other three sites need no cap at all: `x - y` already equals `x - x.component_wise_min(y)`, since subtraction saturates part by part. The test pins that alongside the new operation, on the same two amounts whose components are oppositely ordered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The component-wise accounting is correct and no unresolved issues were identified.

Review details

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

`load_canister_snapshot` recomputes the refund that `refund_unused_execution_cycles` derives from the identical formula, in order to record the net charge where it survives a canister state rollback. It relied on the cap being redundant once the result is subtracted from the prepayment. It now applies the same `component_wise_min` cap, so the two agree by construction rather than by an identity the reader has to verify, and stay in agreement if that cap ever changes. The comment on the cap in `refund_unused_execution_cycles` loses its walkthrough of the four pricing inputs, keeping only the reason the cap is not vacuous, now that `component_wise_min` names what it does. "Part by part" is spelled out as the real and the nominal part throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The replacement operations preserve intended accounting while preventing ambiguous comparisons.

Review details

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

The reason given for the cap in `refund_unused_execution_cycles` was one of several ways the prepayment and the refund can be priced differently, so naming it invited being read as the only one. The cap holds regardless. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The refactor consistently replaces ambiguous ordering without changing same-schedule accounting behavior.

Review details

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

zeropath-ai · 2026-09-10T16:02:07Z

✅ No security or compliance issues detected. Reviewed everything up to 0945cf1.

Security Overview

🔎 Scanned files: 5 changed file(s)
🔗 Scan Link: https://zeropath.com/app/repositories/f5e9d2bb-29a4-4b64-902a-391d65fd8b2d?scanId=1c0592e2-87f6-4687-8ac1-76cd13323d21&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► rs/cycles_account_manager/src/cycles_account_manager.rs Implement more precise refunds using component_wise_min and document saturation behavior ► rs/execution_environment/src/canister_manager.rs Record net cycle charge with component_wise_min alignment to prepaid amounts ► rs/execution_environment/src/execution/response/tests.rs Assert cost increases use nominal() when responses fail or cleanup fails, ensuring correctness across cost representations ► rs/types/cycles/src/compound_cycles.rs Enhance CompoundCycles with component_wise_min and add detailed documentation and tests

mraszyk requested a balanced review from Copilot September 10, 2026 10:46

github-actions Bot added the refactor label Sep 10, 2026

Copilot started reviewing on behalf of mraszyk September 10, 2026 10:46 View session