fix: remove the ordering on CompoundCycles (#11529)

# 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](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: mraszyk

rs/cycles_account_manager/src/cycles_account_manager.rs

@@ -555,12 +555,13 @@ impl CyclesAccountManager {
         }
         let num_instructions_to_refund =
             std::cmp::min(num_instructions, num_instructions_initially_charged);
+        // Never refund more than was prepaid, in either the real or the nominal part.
         let cycles_to_refund = self
             .scale_cost(
                 self.convert_instructions_to_cycles(num_instructions_to_refund, execution_mode),
                 subnet_cycles_config,
             )
-            .min(prepaid_execution_cycles);
+            .component_wise_min(prepaid_execution_cycles);
         system_state.refund_cycles(prepaid_execution_cycles, cycles_to_refund);
     }
 
@@ -938,7 +939,8 @@ impl CyclesAccountManager {
     ///
     /// Note that the prepayment is never topped up for such a response: the additional
     /// cycles would be refunded right away and, unlike this refund, the withdrawal
-    /// could fail.
+    /// could fail. The subtraction below saturates in both the real and the nominal
+    /// part, so the canister is charged at most what it prepaid in each of them.
     pub fn settle_prepayment_for_unexecuted_response(
         &self,
         system_state: &mut SystemState,
@@ -952,12 +954,12 @@ impl CyclesAccountManager {
             subnet_cycles_config,
             execution_mode,
         );
-        // The prepayment covers the fixed per-message execution fee, but clamp the
-        // charge to it so that no more than the prepayment is ever charged.
-        let charge = base_fee.min(prepayment_for_response_execution);
+        // The prepayment covers the fixed per-message execution fee. The subtraction
+        // saturates in both the real and the nominal part, so no more than the
+        // prepayment is ever charged.
         system_state.refund_cycles(
             prepayment_for_response_execution,
-            prepayment_for_response_execution - charge,
+            prepayment_for_response_execution - base_fee,
         );
     }
 
@@ -1000,8 +1002,9 @@ impl CyclesAccountManager {
             self.config.xnet_byte_transmission_fee * transmitted_bytes,
             subnet_cycles_config,
         );
-        prepayment_for_response_transmission
-            - transmission_cost.min(prepayment_for_response_transmission)
+        // The subtraction saturates in both the real and the nominal part, so a
+        // transmission cost exceeding the prepayment leaves nothing to refund.
+        prepayment_for_response_transmission - transmission_cost
     }
 
     ////////////////////////////////////////////////////////////////////////////

rs/execution_environment/src/canister_manager.rs

@@ -2400,15 +2400,17 @@ impl CanisterManager {
                 &self.log,
             );
             // Record the net cycle charge (prepay minus refund) so it survives
-            // the canister state rollback if a subsequent step fails.
+            // the canister state rollback if a subsequent step fails. This recomputes
+            // what `refund_unused_execution_cycles` above refunded, cap included, so
+            // that the two agree by construction.
             let cycles_to_refund = self
                 .cycles_account_manager
                 .variable_execution_cost(
                     instructions_to_refund,
                     subnet_cycles_config,
                     wasm_execution_mode,
                 )
-                .min(prepaid_execution_cycles);
+                .component_wise_min(prepaid_execution_cycles);
             consumed_cycles.add(
                 prepaid_execution_cycles - cycles_to_refund,
                 instructions_for_execution,

rs/execution_environment/src/execution/response/tests.rs

@@ -457,7 +457,10 @@ fn cycles_correct_if_response_fails() {
     let execution_cost_before = test.canister_execution_cost(a_id);
     test.execute_message(a_id);
     let execution_cost_after = test.canister_execution_cost(a_id);
-    assert_gt!(execution_cost_after, execution_cost_before);
+    assert_gt!(
+        execution_cost_after.nominal(),
+        execution_cost_before.nominal()
+    );
     assert_eq!(
         test.canister_state(a_id).system_state.balance(),
         initial_cycles
@@ -507,7 +510,10 @@ fn cycles_correct_if_cleanup_fails() {
     let execution_cost_before = test.canister_execution_cost(a_id);
     test.execute_message(a_id);
     let execution_cost_after = test.canister_execution_cost(a_id);
-    assert_gt!(execution_cost_after, execution_cost_before);
+    assert_gt!(
+        execution_cost_after.nominal(),
+        execution_cost_before.nominal()
+    );
     assert_eq!(
         test.canister_state(a_id).system_state.balance(),
         initial_cycles
@@ -1188,7 +1194,10 @@ fn response_fail_scenario(test: &mut ExecutionTest) -> (CanisterId, MessageId) {
     let execution_cost_before = test.canister_execution_cost(a_id);
     test.execute_message(a_id);
     let execution_cost_after = test.canister_execution_cost(a_id);
-    assert_gt!(execution_cost_after, execution_cost_before);
+    assert_gt!(
+        execution_cost_after.nominal(),
+        execution_cost_before.nominal()
+    );
 
     let ingress_status = test.ingress_status(&ingress_id);
     let result = check_ingress_status(ingress_status).unwrap_err();
@@ -1237,7 +1246,10 @@ fn cleanup_fail_scenario(test: &mut ExecutionTest) -> (CanisterId, MessageId) {
     let execution_cost_before = test.canister_execution_cost(a_id);
     test.execute_message(a_id);
     let execution_cost_after = test.canister_execution_cost(a_id);
-    assert_gt!(execution_cost_after, execution_cost_before);
+    assert_gt!(
+        execution_cost_after.nominal(),
+        execution_cost_before.nominal()
+    );
 
     let ingress_status = test.ingress_status(&ingress_id);
     let result = check_ingress_status(ingress_status).unwrap_err();
@@ -2025,7 +2037,10 @@ fn reserve_instructions_for_cleanup_callback_scenario(
     let execution_cost_before = test.canister_execution_cost(a_id);
     test.execute_message(a_id);
     let execution_cost_after = test.canister_execution_cost(a_id);
-    assert_gt!(execution_cost_after, execution_cost_before);
+    assert_gt!(
+        execution_cost_after.nominal(),
+        execution_cost_before.nominal()
+    );
 
     // Assert that the response failed with exceeding instructions limit.
     let ingress_status = test.ingress_status(&ingress_id);

rs/execution_environment/tests/hypervisor.rs

@@ -7228,7 +7228,10 @@ fn cycles_correct_if_update_fails() {
     let execution_cost_before = test.canister_execution_cost(b_id);
     test.execute_message(b_id);
     let execution_cost_after = test.canister_execution_cost(b_id);
-    assert_gt!(execution_cost_after, execution_cost_before);
+    assert_gt!(
+        execution_cost_after.nominal(),
+        execution_cost_before.nominal()
+    );
     assert_eq!(
         test.canister_state(b_id).system_state.balance(),
         initial_cycles - test.canister_execution_cost(b_id).real()

rs/types/cycles/src/compound_cycles.rs

@@ -55,7 +55,10 @@ use std::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign};
 ///
 /// Extra type-safety is added via use of generics and phantom data to enforce
 /// that arithmetic operations can only be performed on amounts that were
-/// created for the same `CyclesUseCase` and `CanisterCyclesCostSchedule`.
+/// created for the same `CyclesUseCase`. The `CanisterCyclesCostSchedule` is not
+/// part of the type: `new` folds it into the real part and does not retain it, so
+/// nothing stops two amounts created under different cost schedules from being
+/// combined (see the note on ordering below).
 ///
 /// E.g. the following code would not compile:
 ///
@@ -74,7 +77,29 @@ use std::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign};
 /// let total = cc_instructions + cc_memory;
 /// assert_eq!(total.real(), Cycles::new(30));
 /// ```
-#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug, Serialize, Deserialize)]
+///
+/// # No ordering
+///
+/// `CompoundCycles` deliberately implements neither `Ord` nor `PartialOrd`: its two
+/// parts are accounted for independently and there is no meaningful order on the
+/// pair. A derived impl would order lexicographically, i.e. by the real part first,
+/// and hence decide a comparison on the real parts alone whenever those differ, no
+/// matter how the nominal parts compare.
+///
+/// Two amounts carrying the same cost schedule are safe to compare that way: under
+/// the normal cost schedule the two parts of an amount coincide, and under 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. Such an order is misleading
+/// precisely when the two amounts carry *different* cost schedules, e.g. because one
+/// was recorded when a call was performed and the other derived when its response is
+/// executed: the one made free has a zero real part and compares as the smaller
+/// amount however large its nominal part is.
+///
+/// Compare `real()` or `nominal()` explicitly instead, or use `component_wise_min`
+/// to bound both parts at once. Note also that subtraction saturates in both the
+/// real and the nominal part, so capping an amount before subtracting it is
+/// redundant: `x - y` already equals `x - x.component_wise_min(y)`.
+#[derive(Copy, Clone, Eq, PartialEq, Debug, Serialize, Deserialize)]
 pub struct CompoundCycles<T: CyclesUseCaseKind> {
     real: Cycles,
     nominal: NominalCycles,
@@ -130,6 +155,22 @@ impl<T: CyclesUseCaseKind> CompoundCycles<T> {
         self.real.is_zero() && self.nominal.is_zero()
     }
 
+    /// Returns the component-wise minimum of this amount and `other`, i.e. the
+    /// minimum of their real parts paired with the minimum of their nominal parts.
+    ///
+    /// The two components are minimized separately because there is no ordering on
+    /// the pair (see the note on this type). They coincide under the normal cost
+    /// schedule; under the free cost schedule the real part of a use case made free
+    /// is zero, so minimizing by the real parts alone would leave the nominal part
+    /// of the result unbounded.
+    pub fn component_wise_min(self, other: Self) -> Self {
+        Self {
+            real: self.real.min(other.real),
+            nominal: self.nominal.min(other.nominal),
+            _cycles_use_case_marker: self._cycles_use_case_marker,
+        }
+    }
+
     /// Returns this amount reduced by the part of `real()` that could not be
     /// charged, e.g. because the balance it was to be subtracted from did not
     /// cover it. Such an amount is never removed from any balance, so it must
@@ -243,3 +284,49 @@ impl<T: CyclesUseCaseKind> TryFrom<PbCompoundCycles> for CompoundCycles<T> {
         })
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::cycles_use_case::Instructions;
+    use crate::nominal_cycles::testing::NominalCyclesTesting;
+
+    /// An `Instructions` amount has coincident parts under the normal cost schedule,
+    /// whereas its real part is zero under the free cost schedule. The two amounts
+    /// below therefore order one way in their real parts and the other way in their
+    /// nominal parts, which is exactly the case a lexicographic ordering of the pair
+    /// would decide on the real parts alone.
+    #[test]
+    fn arithmetic_is_component_wise() {
+        let x =
+            CompoundCycles::<Instructions>::new(Cycles::new(5), CanisterCyclesCostSchedule::Normal);
+        let y =
+            CompoundCycles::<Instructions>::new(Cycles::new(10), CanisterCyclesCostSchedule::Free);
+        assert_eq!(
+            (x.real(), x.nominal()),
+            (Cycles::new(5), NominalCycles::new(5))
+        );
+        assert_eq!(
+            (y.real(), y.nominal()),
+            (Cycles::zero(), NominalCycles::new(10))
+        );
+
+        // Subtracting `y` from `x` without going below zero in either part.
+        let difference = x - y;
+        assert_eq!(
+            (difference.real(), difference.nominal()),
+            (Cycles::new(5), NominalCycles::zero())
+        );
+
+        // The component-wise minimum takes each part from a different amount.
+        let minimum = x.component_wise_min(y);
+        assert_eq!(
+            (minimum.real(), minimum.nominal()),
+            (Cycles::zero(), NominalCycles::new(5))
+        );
+        assert_eq!(y.component_wise_min(x), minimum);
+
+        // Capping before subtracting is redundant.
+        assert_eq!(x - x.component_wise_min(y), difference);
+    }
+}