From 8d80a3ddb83328ebebe14f01e66a1446c0323e93 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 10:45:43 +0000 Subject: [PATCH 01/14] refactor: remove the ordering on CompoundCycles `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) --- .../src/cycles_account_manager.rs | 33 +++++++++++-------- .../src/canister_manager.rs | 16 ++++----- .../src/execution/response/tests.rs | 25 +++++++++++--- rs/execution_environment/tests/hypervisor.rs | 5 ++- rs/types/cycles/src/compound_cycles.rs | 17 +++++++++- 5 files changed, 68 insertions(+), 28 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 48f939ea7bfa..3e4ab8c24704 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -555,12 +555,18 @@ impl CyclesAccountManager { } let num_instructions_to_refund = std::cmp::min(num_instructions, num_instructions_initially_charged); - 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); + let cycles_to_refund = self.scale_cost( + self.convert_instructions_to_cycles(num_instructions_to_refund, execution_mode), + subnet_cycles_config, + ); + // Never refund more than was prepaid, part by part: `x - (x - y)` is the + // part-wise minimum of `x` and `y` because both subtractions saturate. The + // prepayment covers the instructions it was made for, and hence any refund + // derived from them, so this is defense in depth against a caller whose + // prepayment and refund disagree on the instruction count, the Wasm + // execution mode or the cost schedule. + let cycles_to_refund = + prepaid_execution_cycles - (prepaid_execution_cycles - cycles_to_refund); system_state.refund_cycles(prepaid_execution_cycles, cycles_to_refund); } @@ -938,7 +944,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 part by part, so the canister is + /// charged at most what it prepaid in each part. pub fn settle_prepayment_for_unexecuted_response( &self, system_state: &mut SystemState, @@ -952,12 +959,11 @@ 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 part by 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 +1006,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 part by part, so a transmission cost exceeding + // the prepayment simply leaves nothing to refund. + prepayment_for_response_transmission - transmission_cost } //////////////////////////////////////////////////////////////////////////// diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index cddf9c951a94..113183e9b9d0 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -2401,14 +2401,14 @@ impl CanisterManager { ); // Record the net cycle charge (prepay minus refund) so it survives // the canister state rollback if a subsequent step fails. - let cycles_to_refund = self - .cycles_account_manager - .variable_execution_cost( - instructions_to_refund, - subnet_cycles_config, - wasm_execution_mode, - ) - .min(prepaid_execution_cycles); + // This is exactly the refund that `refund_unused_execution_cycles` above + // derived; the subtraction below saturates part by part, so its clamp to + // the prepayment needs no counterpart here. + let cycles_to_refund = self.cycles_account_manager.variable_execution_cost( + instructions_to_refund, + subnet_cycles_config, + wasm_execution_mode, + ); consumed_cycles.add( prepaid_execution_cycles - cycles_to_refund, instructions_for_execution, diff --git a/rs/execution_environment/src/execution/response/tests.rs b/rs/execution_environment/src/execution/response/tests.rs index 39fef208362b..b72263e880a9 100644 --- a/rs/execution_environment/src/execution/response/tests.rs +++ b/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); diff --git a/rs/execution_environment/tests/hypervisor.rs b/rs/execution_environment/tests/hypervisor.rs index 2a23ed3e5891..5718536db93d 100644 --- a/rs/execution_environment/tests/hypervisor.rs +++ b/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() diff --git a/rs/types/cycles/src/compound_cycles.rs b/rs/types/cycles/src/compound_cycles.rs index 2682f758f9ff..6254bf13a752 100644 --- a/rs/types/cycles/src/compound_cycles.rs +++ b/rs/types/cycles/src/compound_cycles.rs @@ -74,7 +74,22 @@ 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 answer "which of the two amounts is larger?" from the real part alone +/// whenever the real parts differ. That is exactly 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. +/// +/// 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)`. +#[derive(Copy, Clone, Eq, PartialEq, Debug, Serialize, Deserialize)] pub struct CompoundCycles { real: Cycles, nominal: NominalCycles, From af08bb1aa7fc3518493babbd7b559ddb6d3db293 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 10:56:57 +0000 Subject: [PATCH 02/14] docs: describe when the removed ordering was actually misleading 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) --- .../src/cycles_account_manager.rs | 12 ++++++----- .../src/canister_manager.rs | 9 +++++--- rs/types/cycles/src/compound_cycles.rs | 21 ++++++++++++------- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 3e4ab8c24704..6671130fdfbd 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -560,11 +560,13 @@ impl CyclesAccountManager { subnet_cycles_config, ); // Never refund more than was prepaid, part by part: `x - (x - y)` is the - // part-wise minimum of `x` and `y` because both subtractions saturate. The - // prepayment covers the instructions it was made for, and hence any refund - // derived from them, so this is defense in depth against a caller whose - // prepayment and refund disagree on the instruction count, the Wasm - // execution mode or the cost schedule. + // part-wise minimum of `x` and `y` because both subtractions saturate. + // + // The refund is derived from the instruction count, the Wasm execution mode, + // the cost schedule and the subnet size passed to this function, all of which + // the prepayment was made with as well. Should any of them have changed since + // then, e.g. should the subnet have grown between the prepayment and this + // refund, the refund can exceed the prepayment and is capped at it. let cycles_to_refund = prepaid_execution_cycles - (prepaid_execution_cycles - cycles_to_refund); system_state.refund_cycles(prepaid_execution_cycles, cycles_to_refund); diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 113183e9b9d0..ba38f5dd1c5d 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -2401,9 +2401,12 @@ impl CanisterManager { ); // Record the net cycle charge (prepay minus refund) so it survives // the canister state rollback if a subsequent step fails. - // This is exactly the refund that `refund_unused_execution_cycles` above - // derived; the subtraction below saturates part by part, so its clamp to - // the prepayment needs no counterpart here. + // This recomputes the refund that `refund_unused_execution_cycles` above + // derived from the identical formula, before the cap it applies to the + // prepayment. Capping it here as well would make no difference: the + // subtraction below saturates part by part, so `prepaid - refund` already + // equals `prepaid - refund.min(prepaid)` and hence the net charge matches + // what that function actually refunded. let cycles_to_refund = self.cycles_account_manager.variable_execution_cost( instructions_to_refund, subnet_cycles_config, diff --git a/rs/types/cycles/src/compound_cycles.rs b/rs/types/cycles/src/compound_cycles.rs index 6254bf13a752..f6e10eb4b95b 100644 --- a/rs/types/cycles/src/compound_cycles.rs +++ b/rs/types/cycles/src/compound_cycles.rs @@ -80,15 +80,22 @@ use std::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign}; /// `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 answer "which of the two amounts is larger?" from the real part alone -/// whenever the real parts differ. That is exactly 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. +/// 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. 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)`. +/// saturates part by part, which covers the two idioms that would otherwise reach +/// for an ordering: subtracting `y` from `x` without going below zero is `x - y`, +/// and the part-wise minimum of `x` and `y` is `x - (x - y)`. #[derive(Copy, Clone, Eq, PartialEq, Debug, Serialize, Deserialize)] pub struct CompoundCycles { real: Cycles, From 214e437fa5256c7b1f5c83a0e815fe36c239d2cd Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 11:14:51 +0000 Subject: [PATCH 03/14] docs: separate the prepayment-time from the refund-time inputs 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) --- .../src/cycles_account_manager.rs | 11 ++++++----- rs/execution_environment/src/canister_manager.rs | 10 +++++----- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 6671130fdfbd..8736f163b243 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -562,11 +562,12 @@ impl CyclesAccountManager { // Never refund more than was prepaid, part by part: `x - (x - y)` is the // part-wise minimum of `x` and `y` because both subtractions saturate. // - // The refund is derived from the instruction count, the Wasm execution mode, - // the cost schedule and the subnet size passed to this function, all of which - // the prepayment was made with as well. Should any of them have changed since - // then, e.g. should the subnet have grown between the prepayment and this - // refund, the refund can exceed the prepayment and is capped at it. + // The refund covers at most the instructions the prepayment was made for, but + // it is priced with the Wasm execution mode, the cost schedule and the subnet + // size passed to this function, i.e. with the ones in effect now rather than + // the ones the prepayment was priced with. `scale_cost` scales both parts of + // an amount by the subnet size, so a subnet that grew in between makes the + // refund exceed the prepayment, and the cap bounds it. let cycles_to_refund = prepaid_execution_cycles - (prepaid_execution_cycles - cycles_to_refund); system_state.refund_cycles(prepaid_execution_cycles, cycles_to_refund); diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index ba38f5dd1c5d..1756fd615e16 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -2402,11 +2402,11 @@ impl CanisterManager { // Record the net cycle charge (prepay minus refund) so it survives // the canister state rollback if a subsequent step fails. // This recomputes the refund that `refund_unused_execution_cycles` above - // derived from the identical formula, before the cap it applies to the - // prepayment. Capping it here as well would make no difference: the - // subtraction below saturates part by part, so `prepaid - refund` already - // equals `prepaid - refund.min(prepaid)` and hence the net charge matches - // what that function actually refunded. + // derived from the identical formula, before the part-wise cap to the + // prepayment that it applies. Applying that cap here as well would make no + // difference: both subtractions saturate part by part, so in each part + // `prepaid - refund` equals `prepaid - min(prepaid, refund)`, and hence + // the net charge recorded here matches what that function refunded. let cycles_to_refund = self.cycles_account_manager.variable_execution_cost( instructions_to_refund, subnet_cycles_config, From ef2af1da08a48dcb506a5d4a60bb7dde26137489 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 11:24:35 +0000 Subject: [PATCH 04/14] test: pin the part-wise identities and correct what the type enforces 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) --- .../src/cycles_account_manager.rs | 11 +++-- rs/types/cycles/src/compound_cycles.rs | 48 ++++++++++++++++++- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 8736f163b243..905aff543ea1 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -564,10 +564,13 @@ impl CyclesAccountManager { // // The refund covers at most the instructions the prepayment was made for, but // it is priced with the Wasm execution mode, the cost schedule and the subnet - // size passed to this function, i.e. with the ones in effect now rather than - // the ones the prepayment was priced with. `scale_cost` scales both parts of - // an amount by the subnet size, so a subnet that grew in between makes the - // refund exceed the prepayment, and the cap bounds it. + // size passed to this function, which need not be the ones the prepayment was + // priced with. For an update call or an install these coincide, as the caller + // passes the same `subnet_cycles_config` it prepaid with. They can differ for + // a response execution, whose prepayment was made in an earlier round, when + // the corresponding call was performed: `scale_cost` scales both parts of an + // amount by the subnet size, so a subnet that grew in between can price the + // refund above the prepayment, and the cap bounds it. let cycles_to_refund = prepaid_execution_cycles - (prepaid_execution_cycles - cycles_to_refund); system_state.refund_cycles(prepaid_execution_cycles, cycles_to_refund); diff --git a/rs/types/cycles/src/compound_cycles.rs b/rs/types/cycles/src/compound_cycles.rs index f6e10eb4b95b..b6c9d04577ec 100644 --- a/rs/types/cycles/src/compound_cycles.rs +++ b/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: /// @@ -265,3 +268,46 @@ impl TryFrom for CompoundCycles { }) } } + +#[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. Both identities documented on + /// `CompoundCycles` hold for it. + #[test] + fn saturating_subtraction_is_part_wise() { + let x = + CompoundCycles::::new(Cycles::new(5), CanisterCyclesCostSchedule::Normal); + let y = + CompoundCycles::::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, part by part. + let difference = x - y; + assert_eq!( + (difference.real(), difference.nominal()), + (Cycles::new(5), NominalCycles::zero()) + ); + + // The part-wise minimum of `x` and `y`. + let minimum = x - (x - y); + assert_eq!( + (minimum.real(), minimum.nominal()), + (Cycles::zero(), NominalCycles::new(5)) + ); + } +} From d0639d30fb3d9dc46f2f1cd535a09be0c692d667 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 11:46:31 +0000 Subject: [PATCH 05/14] refactor: name the component-wise minimum instead of open-coding it 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) --- .../src/cycles_account_manager.rs | 17 ++++----- .../src/canister_manager.rs | 7 ++-- rs/types/cycles/src/compound_cycles.rs | 37 ++++++++++++++----- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 905aff543ea1..98229b7aff26 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -555,12 +555,7 @@ impl CyclesAccountManager { } let num_instructions_to_refund = std::cmp::min(num_instructions, num_instructions_initially_charged); - let cycles_to_refund = self.scale_cost( - self.convert_instructions_to_cycles(num_instructions_to_refund, execution_mode), - subnet_cycles_config, - ); - // Never refund more than was prepaid, part by part: `x - (x - y)` is the - // part-wise minimum of `x` and `y` because both subtractions saturate. + // Never refund more than was prepaid, in either component. // // The refund covers at most the instructions the prepayment was made for, but // it is priced with the Wasm execution mode, the cost schedule and the subnet @@ -570,9 +565,13 @@ impl CyclesAccountManager { // a response execution, whose prepayment was made in an earlier round, when // the corresponding call was performed: `scale_cost` scales both parts of an // amount by the subnet size, so a subnet that grew in between can price the - // refund above the prepayment, and the cap bounds it. - let cycles_to_refund = - prepaid_execution_cycles - (prepaid_execution_cycles - cycles_to_refund); + // refund above the prepayment, and this cap bounds it. + let cycles_to_refund = self + .scale_cost( + self.convert_instructions_to_cycles(num_instructions_to_refund, execution_mode), + subnet_cycles_config, + ) + .component_wise_min(prepaid_execution_cycles); system_state.refund_cycles(prepaid_execution_cycles, cycles_to_refund); } diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 1756fd615e16..b6550d9d5b75 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -2402,10 +2402,9 @@ impl CanisterManager { // Record the net cycle charge (prepay minus refund) so it survives // the canister state rollback if a subsequent step fails. // This recomputes the refund that `refund_unused_execution_cycles` above - // derived from the identical formula, before the part-wise cap to the - // prepayment that it applies. Applying that cap here as well would make no - // difference: both subtractions saturate part by part, so in each part - // `prepaid - refund` equals `prepaid - min(prepaid, refund)`, and hence + // derived from the identical formula, before the `component_wise_min` cap + // to the prepayment that it applies. Applying that cap here as well would + // make no difference, as the subtraction below saturates part by part, so // the net charge recorded here matches what that function refunded. let cycles_to_refund = self.cycles_account_manager.variable_execution_cost( instructions_to_refund, diff --git a/rs/types/cycles/src/compound_cycles.rs b/rs/types/cycles/src/compound_cycles.rs index b6c9d04577ec..8164364ba2b7 100644 --- a/rs/types/cycles/src/compound_cycles.rs +++ b/rs/types/cycles/src/compound_cycles.rs @@ -95,10 +95,10 @@ use std::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign}; /// 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. Note that subtraction -/// saturates part by part, which covers the two idioms that would otherwise reach -/// for an ordering: subtracting `y` from `x` without going below zero is `x - y`, -/// and the part-wise minimum of `x` and `y` is `x - (x - y)`. +/// Compare `real()` or `nominal()` explicitly instead, or use `component_wise_min` +/// to bound both parts at once. Note also that subtraction saturates part by 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 { real: Cycles, @@ -155,6 +155,22 @@ impl CompoundCycles { 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 @@ -279,10 +295,9 @@ mod tests { /// 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. Both identities documented on - /// `CompoundCycles` hold for it. + /// would decide on the real parts alone. #[test] - fn saturating_subtraction_is_part_wise() { + fn arithmetic_is_component_wise() { let x = CompoundCycles::::new(Cycles::new(5), CanisterCyclesCostSchedule::Normal); let y = @@ -303,11 +318,15 @@ mod tests { (Cycles::new(5), NominalCycles::zero()) ); - // The part-wise minimum of `x` and `y`. - let minimum = x - (x - y); + // 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); } } From 8ea99c1ebf610558f4d6286ecf41a0f9a94b7465 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 13:03:30 +0000 Subject: [PATCH 06/14] refactor: cap the mirrored refund component-wise too `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) --- .../src/cycles_account_manager.rs | 25 +++++++------------ .../src/canister_manager.rs | 22 ++++++++-------- rs/types/cycles/src/compound_cycles.rs | 8 +++--- 3 files changed, 24 insertions(+), 31 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 98229b7aff26..cdece9c4fd37 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -555,17 +555,9 @@ impl CyclesAccountManager { } let num_instructions_to_refund = std::cmp::min(num_instructions, num_instructions_initially_charged); - // Never refund more than was prepaid, in either component. - // - // The refund covers at most the instructions the prepayment was made for, but - // it is priced with the Wasm execution mode, the cost schedule and the subnet - // size passed to this function, which need not be the ones the prepayment was - // priced with. For an update call or an install these coincide, as the caller - // passes the same `subnet_cycles_config` it prepaid with. They can differ for - // a response execution, whose prepayment was made in an earlier round, when - // the corresponding call was performed: `scale_cost` scales both parts of an - // amount by the subnet size, so a subnet that grew in between can price the - // refund above the prepayment, and this cap bounds it. + // Never refund more than was prepaid, in either the real or the nominal part: + // a prepayment made in an earlier round can have been priced with a smaller + // subnet size than the refund is. let cycles_to_refund = self .scale_cost( self.convert_instructions_to_cycles(num_instructions_to_refund, execution_mode), @@ -949,8 +941,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. The subtraction below saturates part by part, so the canister is - /// charged at most what it prepaid in each part. + /// 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, @@ -965,7 +957,8 @@ impl CyclesAccountManager { execution_mode, ); // The prepayment covers the fixed per-message execution fee. The subtraction - // saturates part by part, so no more than the prepayment is ever charged. + // 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 - base_fee, @@ -1011,8 +1004,8 @@ impl CyclesAccountManager { self.config.xnet_byte_transmission_fee * transmitted_bytes, subnet_cycles_config, ); - // The subtraction saturates part by part, so a transmission cost exceeding - // the prepayment simply leaves nothing to refund. + // 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 } diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index b6550d9d5b75..1e6b4b0c07ba 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -2400,17 +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. - // This recomputes the refund that `refund_unused_execution_cycles` above - // derived from the identical formula, before the `component_wise_min` cap - // to the prepayment that it applies. Applying that cap here as well would - // make no difference, as the subtraction below saturates part by part, so - // the net charge recorded here matches what that function refunded. - let cycles_to_refund = self.cycles_account_manager.variable_execution_cost( - instructions_to_refund, - subnet_cycles_config, - wasm_execution_mode, - ); + // 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, + ) + .component_wise_min(prepaid_execution_cycles); consumed_cycles.add( prepaid_execution_cycles - cycles_to_refund, instructions_for_execution, diff --git a/rs/types/cycles/src/compound_cycles.rs b/rs/types/cycles/src/compound_cycles.rs index 8164364ba2b7..74c5cf3cc784 100644 --- a/rs/types/cycles/src/compound_cycles.rs +++ b/rs/types/cycles/src/compound_cycles.rs @@ -96,9 +96,9 @@ use std::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign}; /// 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 part by part, -/// so capping an amount before subtracting it is redundant: `x - y` already equals -/// `x - x.component_wise_min(y)`. +/// 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 { real: Cycles, @@ -311,7 +311,7 @@ mod tests { (Cycles::zero(), NominalCycles::new(10)) ); - // Subtracting `y` from `x` without going below zero, part by part. + // Subtracting `y` from `x` without going below zero in either part. let difference = x - y; assert_eq!( (difference.real(), difference.nominal()), From 0945cf13c97c1656b14dd78ed00931dd2d443e07 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 13:20:46 +0000 Subject: [PATCH 07/14] docs: state the cap without enumerating why it can bind 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) --- rs/cycles_account_manager/src/cycles_account_manager.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index cdece9c4fd37..73e8af4835c8 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -555,9 +555,7 @@ 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: - // a prepayment made in an earlier round can have been priced with a smaller - // subnet size than the refund is. + // 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), From 9e47993b6c2057bb96914456b0fe694ed04d9fd1 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 3 Sep 2026 10:10:16 +0000 Subject: [PATCH 08/14] fix: adjust the response execution prepayment on its nominal part 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) --- .../src/cycles_account_manager.rs | 39 ++- .../tests/cycles_account_manager.rs | 241 +++++++++++++++++- 2 files changed, 275 insertions(+), 5 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 73e8af4835c8..0ff80f492d32 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -871,6 +871,38 @@ impl CyclesAccountManager { /// the instructions it executed, at the instruction costs of the Wasm execution /// mode it executed them in. /// + /// The prepayment is compared to the requirement on its nominal part rather than + /// on its real part, so that the adjustment applies under the free cost schedule + /// too: the real part of an `Instructions` amount is zero under that schedule, so + /// a comparison of real parts would see no difference between the Wasm execution + /// modes and leave the nominal part unadjusted, misreporting the consumed cycles + /// metrics. Under the free cost schedule nothing is withdrawn from or refunded to + /// the balance, hence only those metrics are adjusted; under the normal cost + /// schedule the real and the nominal parts coincide, so this comparison is + /// equivalent to comparing the real parts. + /// + /// # Assumption: the cost schedule of a subnet never changes + /// + /// Comparing a single part, be it the real or the nominal one, is only sound + /// because the prepayment and the requirement are stamped with the same cost + /// schedule: the prepayment recorded in the callback carries the cost schedule in + /// effect when the call was performed, whereas the requirement is derived from the + /// cost schedule in effect when the response is executed. A subnet is assigned its + /// cost schedule when it is created (`do_create_subnet`) and keeps it (a subnet + /// split inherits it and splitting a rental subnet is rejected outright), and + /// `UpdateSubnetPayload` exposes no field to change it, so the two schedules always + /// coincide today. + /// + /// 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 hit the branch below that + /// returns the prepayment unchanged, and `refund_unused_execution_cycles` would + /// then clamp the refund to the free-schedule amount of zero real cycles, so the + /// canister would forfeit the whole real prepayment it made under the normal cost + /// schedule instead of getting it back. + /// /// Returns the prepayment matching the cycles required for executing the response /// in the given Wasm execution mode, or a `CanisterOutOfCyclesError` if the /// canister's balance does not cover the additional prepayment. @@ -884,7 +916,7 @@ impl CyclesAccountManager { ) -> Result, CanisterOutOfCyclesError> { let required = self.prepayment_for_response_execution(subnet_cycles_config, execution_mode); let prepaid = prepayment_for_response_execution; - if prepaid.real() < required.real() { + if prepaid.nominal() < required.nominal() { // No freezing threshold is applied, i.e., the threshold is zero. self.consume_with_threshold_impl( system_state, @@ -909,6 +941,9 @@ impl CyclesAccountManager { /// Unlike `adjust_prepayment_for_response_execution`, which uses this function to /// handle an excessive prepayment, this never withdraws cycles from the canister's /// balance and hence it cannot fail. + /// + /// The prepayment is compared to the requirement on its nominal part for the same + /// reason as in `adjust_prepayment_for_response_execution`. fn refund_excess_prepayment_for_response_execution( &self, system_state: &mut SystemState, @@ -917,7 +952,7 @@ impl CyclesAccountManager { execution_mode: WasmExecutionMode, ) -> CompoundCycles { let required = self.prepayment_for_response_execution(subnet_cycles_config, execution_mode); - if prepayment_for_response_execution.real() <= required.real() { + if prepayment_for_response_execution.nominal() <= required.nominal() { return prepayment_for_response_execution; } // The excess part of the prepayment is refunded in full and hence it does not diff --git a/rs/cycles_account_manager/tests/cycles_account_manager.rs b/rs/cycles_account_manager/tests/cycles_account_manager.rs index 5c7e3379798d..c70cb3b84107 100644 --- a/rs/cycles_account_manager/tests/cycles_account_manager.rs +++ b/rs/cycles_account_manager/tests/cycles_account_manager.rs @@ -1,7 +1,8 @@ use ic_base_types::NumSeconds; use ic_config::subnet_config::{CyclesAccountManagerConfig, DEFAULT_REFERENCE_SUBNET_SIZE}; use ic_cycles_account_manager::{ - CyclesAccountManagerSubnetConfig, IngressInductionCost, ResourceSaturation, + CyclesAccountManager, CyclesAccountManagerSubnetConfig, IngressInductionCost, + ResourceSaturation, }; use ic_interfaces::execution_environment::{CanisterOutOfCyclesError, MessageMemoryUsage}; use ic_limits::SMALL_APP_SUBNET_MAX_SIZE; @@ -28,8 +29,8 @@ use ic_types::{ time::{CoarseTime, UNIX_EPOCH}, }; use ic_types_cycles::{ - CanisterCyclesCostSchedule, CompoundCycles, Cycles, Instructions, Memory, NominalCycles, - NominalCyclesTesting, Uninstall, + CanisterCyclesCostSchedule, CompoundCycles, Cycles, CyclesUseCase, Instructions, Memory, + NominalCycles, NominalCyclesTesting, Uninstall, }; use prometheus::IntCounter; use std::{convert::TryFrom, time::Duration}; @@ -383,6 +384,240 @@ fn verify_no_cycles_charged_for_message_execution_on_free_schedule() { assert_eq!(system_state.balance(), initial_balance); } +/// The instruction limit used by the response execution tests below. The +/// prepayment for a response execution covers exactly this many instructions. +const RESPONSE_EXECUTION_INSTRUCTION_LIMIT: NumInstructions = NumInstructions::new(1_000_000_000); + +/// Every combination of a cost schedule, the Wasm execution mode a canister had +/// when it performed a call (and hence prepaid for the response execution) and +/// the Wasm execution mode it has when the response arrives. The latter two +/// differ whenever the canister is upgraded across the call. +fn cost_schedules_and_wasm_execution_modes() -> Vec<( + CanisterCyclesCostSchedule, + WasmExecutionMode, + WasmExecutionMode, +)> { + let mut combinations = vec![]; + for cost_schedule in [ + CanisterCyclesCostSchedule::Normal, + CanisterCyclesCostSchedule::Free, + ] { + for mode_at_call in [WasmExecutionMode::Wasm32, WasmExecutionMode::Wasm64] { + for mode_at_response in [WasmExecutionMode::Wasm32, WasmExecutionMode::Wasm64] { + combinations.push((cost_schedule, mode_at_call, mode_at_response)); + } + } + } + combinations +} + +fn cycles_account_manager_and_subnet_cycles_config( + cost_schedule: CanisterCyclesCostSchedule, +) -> (CyclesAccountManager, CyclesAccountManagerSubnetConfig) { + let cycles_account_manager = CyclesAccountManagerBuilder::new() + .with_subnet_type(SubnetType::Application) + .with_max_num_instructions(RESPONSE_EXECUTION_INSTRUCTION_LIMIT) + .build(); + let subnet_cycles_config = CyclesAccountManagerSubnetConfig::new( + SMALL_APP_SUBNET_MAX_SIZE, + cost_schedule, + DEFAULT_REFERENCE_SUBNET_SIZE, + ); + (cycles_account_manager, subnet_cycles_config) +} + +fn consumed_cycles_for_instructions(system_state: &SystemState) -> (NominalCycles, NominalCycles) { + let gauge = system_state + .canister_metrics() + .consumed_cycles_by_use_cases() + .get(&CyclesUseCase::Instructions) + .copied() + .unwrap_or_else(NominalCycles::zero); + let counter = system_state + .canister_metrics() + .consumed_cycles_by_use_cases_monotonic() + .get(&CyclesUseCase::Instructions) + .copied() + .unwrap_or_else(NominalCycles::zero); + (gauge, counter) +} + +/// Adjusting the prepayment for a response execution must match the prepayment +/// required in the canister's current Wasm execution mode in *both* its real and +/// its nominal part. +/// +/// The nominal part matters under the free cost schedule, where the real part of +/// an `Instructions` amount is zero: a comparison of real parts would find +/// nothing to adjust there and leave the nominal part at the amount prepaid for +/// the Wasm execution mode the canister had when it performed the call. +#[test] +fn adjust_prepayment_for_response_execution_matches_wasm_execution_mode() { + for (cost_schedule, mode_at_call, mode_at_response) in cost_schedules_and_wasm_execution_modes() + { + let (cycles_account_manager, subnet_cycles_config) = + cycles_account_manager_and_subnet_cycles_config(cost_schedule); + let mut system_state = SystemStateBuilder::new().build(); + + // When the call was performed, the canister prepaid for executing the + // response in the Wasm execution mode it had at that time. + let prepaid = cycles_account_manager + .prepayment_for_response_execution(subnet_cycles_config, mode_at_call); + system_state.consume_cycles(prepaid); + let balance_before = system_state.balance(); + + // Now that the response has arrived, the prepayment is adjusted to the + // Wasm execution mode the canister has by now. + let required = cycles_account_manager + .prepayment_for_response_execution(subnet_cycles_config, mode_at_response); + let adjusted = cycles_account_manager + .adjust_prepayment_for_response_execution( + &mut system_state, + prepaid, + subnet_cycles_config, + mode_at_response, + false, + ) + .unwrap(); + + assert_eq!( + adjusted, required, + "unexpected prepayment for {cost_schedule:?} and \ + {mode_at_call:?} at call, {mode_at_response:?} at response" + ); + // The canister has paid the adjusted prepayment out of its balance: the + // missing cycles were withdrawn or the excess ones were refunded. + assert_eq!( + balance_before + prepaid.real(), + system_state.balance() + required.real(), + "unexpected balance for {cost_schedule:?} and \ + {mode_at_call:?} at call, {mode_at_response:?} at response" + ); + // The consumed cycles metrics report the adjusted prepayment as well. The + // counter is only updated once the prepayment is refunded, i.e. not yet. + let (gauge, counter) = consumed_cycles_for_instructions(&system_state); + assert_eq!( + (gauge, counter), + (required.nominal(), NominalCycles::zero()), + "unexpected consumed cycles for {cost_schedule:?} and \ + {mode_at_call:?} at call, {mode_at_response:?} at response" + ); + } +} + +/// A canister that is upgraded to a different Wasm execution mode across a call +/// pays for its response execution, and has that execution reported in the +/// consumed cycles metrics, exactly as if it had performed the call in the Wasm +/// execution mode in which the response is executed. +#[test] +fn response_execution_consumed_cycles_match_wasm_execution_mode() { + const EXECUTED_INSTRUCTIONS: NumInstructions = NumInstructions::new(1_000_000); + + for (cost_schedule, mode_at_call, mode_at_response) in cost_schedules_and_wasm_execution_modes() + { + let (cycles_account_manager, subnet_cycles_config) = + cycles_account_manager_and_subnet_cycles_config(cost_schedule); + let mut system_state = SystemStateBuilder::new().build(); + let balance_before = system_state.balance(); + + // Prepay for the response execution when the call is performed. + let prepaid = cycles_account_manager + .prepayment_for_response_execution(subnet_cycles_config, mode_at_call); + system_state.consume_cycles(prepaid); + + // Adjust the prepayment when the response arrives. + let adjusted = cycles_account_manager + .adjust_prepayment_for_response_execution( + &mut system_state, + prepaid, + subnet_cycles_config, + mode_at_response, + false, + ) + .unwrap(); + + // Refund the cycles for the instructions the callback did not execute. + let no_op_counter: IntCounter = IntCounter::new("no_op", "no_op").unwrap(); + cycles_account_manager.refund_unused_execution_cycles( + &mut system_state, + RESPONSE_EXECUTION_INSTRUCTION_LIMIT - EXECUTED_INSTRUCTIONS, + RESPONSE_EXECUTION_INSTRUCTION_LIMIT, + adjusted, + &no_op_counter, + subnet_cycles_config, + mode_at_response, + &no_op_logger(), + ); + + // The canister is charged, and reported to have consumed, the fixed + // per-message execution fee plus the cost of the instructions it executed + // in the Wasm execution mode it executed them in. + let expected = cycles_account_manager.execution_cost( + EXECUTED_INSTRUCTIONS, + subnet_cycles_config, + mode_at_response, + ); + assert_eq!( + system_state.balance() + expected.real(), + balance_before, + "unexpected balance for {cost_schedule:?} and \ + {mode_at_call:?} at call, {mode_at_response:?} at response" + ); + let (gauge, counter) = consumed_cycles_for_instructions(&system_state); + assert_eq!( + (gauge, counter), + (expected.nominal(), expected.nominal()), + "unexpected consumed cycles for {cost_schedule:?} and \ + {mode_at_call:?} at call, {mode_at_response:?} at response" + ); + } +} + +/// A response whose callback is not executed at all costs the fixed per-message +/// execution fee only, no matter which Wasm execution mode the cycles were +/// prepaid for and which one the canister has when the response arrives. +#[test] +fn settle_prepayment_for_unexecuted_response_charges_only_the_base_fee() { + for (cost_schedule, mode_at_call, mode_at_response) in cost_schedules_and_wasm_execution_modes() + { + let (cycles_account_manager, subnet_cycles_config) = + cycles_account_manager_and_subnet_cycles_config(cost_schedule); + let mut system_state = SystemStateBuilder::new().build(); + let balance_before = system_state.balance(); + + let prepaid = cycles_account_manager + .prepayment_for_response_execution(subnet_cycles_config, mode_at_call); + system_state.consume_cycles(prepaid); + + cycles_account_manager.settle_prepayment_for_unexecuted_response( + &mut system_state, + prepaid, + subnet_cycles_config, + mode_at_response, + ); + + // No instructions were executed, hence only the fixed per-message + // execution fee is due; the rest of the prepayment is refunded. + let base_fee = cycles_account_manager.execution_cost( + NumInstructions::from(0), + subnet_cycles_config, + mode_at_response, + ); + assert_eq!( + system_state.balance() + base_fee.real(), + balance_before, + "unexpected balance for {cost_schedule:?} and \ + {mode_at_call:?} at call, {mode_at_response:?} at response" + ); + let (gauge, counter) = consumed_cycles_for_instructions(&system_state); + assert_eq!( + (gauge, counter), + (base_fee.nominal(), base_fee.nominal()), + "unexpected consumed cycles for {cost_schedule:?} and \ + {mode_at_call:?} at call, {mode_at_response:?} at response" + ); + } +} + #[test] fn ingress_induction_cost_valid_subnet_message() { for cost_schedule in [ From 20b720006df0c41454c5bf6da528459c238a3ade Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 3 Sep 2026 12:06:49 +0000 Subject: [PATCH 09/14] docs: correct why a cost schedule flip forfeits the real prepayment 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) --- .../src/cycles_account_manager.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 0ff80f492d32..a335f7551648 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -898,10 +898,12 @@ impl CyclesAccountManager { /// 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 hit the branch below that - /// returns the prepayment unchanged, and `refund_unused_execution_cycles` would - /// then clamp the refund to the free-schedule amount of zero real cycles, so the - /// canister would forfeit the whole real prepayment it made under the normal cost - /// schedule instead of getting it back. + /// returns the prepayment unchanged, i.e. keep the prepayment it made under the + /// normal cost schedule, real part and all. `refund_unused_execution_cycles` would + /// then derive the refund under the free cost schedule, where the real part of an + /// `Instructions` amount 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. /// /// Returns the prepayment matching the cycles required for executing the response /// in the given Wasm execution mode, or a `CanisterOutOfCyclesError` if the From 2ca09e32d867948d0a509233c05bc54fef950593 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Thu, 10 Sep 2026 10:00:19 +0000 Subject: [PATCH 10/14] refactor: drop the assumption that a subnet's cost schedule never changes The cycles accounting for a response execution compares a prepayment recorded when the call was performed against a requirement derived when the response is executed. Those two amounts carry the cost schedule in effect at their respective times, so the comparison in `adjust_prepayment_for_response_execution` was only sound because a subnet keeps the cost schedule it was created with. That comparison is now gone: the function withdraws `required - prepaid` and refunds `prepaid - required` unconditionally. Both subtractions saturate part by part, so each of the real and the nominal part is topped up or refunded on its own and for either part at most one of the two is non-zero. The withdrawal stays first so that a failing withdrawal leaves the canister state unchanged, which both callers rely on. `refund_excess_prepayment_for_response_execution` existed only to hold the second branch and is removed. No cycle amount and no metric changes under a fixed cost schedule: with the two schedules equal, the real and the nominal parts agree on the ordering that was previously consulted, and the withdrawal that is now also attempted for a prepayment that already suffices requests a zero real amount, which is always available against a zero threshold and is recorded nowhere. The response execution tests range over the cost schedule at the call and at the response independently, i.e. over 16 combinations instead of 8. They pin down that a canister ends up with exactly the prepayment that the cost schedule in effect at response time requires: it gets its whole real prepayment back if its subnet switched to the free cost schedule across the call, and it pays the real requirement if the subnet switched the other way round. A response that is not executed at all is still charged at most what was prepaid, part by part, as its prepayment is never topped up. A new test pins down that a failing adjustment leaves the balance and both consumed cycles metrics untouched, in a setting where the prepayment falls short of the requirement in the real part while exceeding it in the nominal one, so that a refund performed before the failing withdrawal would show up in those metrics. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/cycles_account_manager.rs | 98 ++----- .../tests/cycles_account_manager.rs | 267 ++++++++++++------ 2 files changed, 210 insertions(+), 155 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index a335f7551648..98dfeef50bad 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -871,39 +871,27 @@ impl CyclesAccountManager { /// the instructions it executed, at the instruction costs of the Wasm execution /// mode it executed them in. /// - /// The prepayment is compared to the requirement on its nominal part rather than - /// on its real part, so that the adjustment applies under the free cost schedule - /// too: the real part of an `Instructions` amount is zero under that schedule, so - /// a comparison of real parts would see no difference between the Wasm execution - /// modes and leave the nominal part unadjusted, misreporting the consumed cycles - /// metrics. Under the free cost schedule nothing is withdrawn from or refunded to - /// the balance, hence only those metrics are adjusted; under the normal cost - /// schedule the real and the nominal parts coincide, so this comparison is - /// equivalent to comparing the real parts. + /// Both directions are applied unconditionally instead of picking one of them by + /// comparing the prepayment against the requirement: subtracting two + /// `CompoundCycles` saturates part by part, so `required - prepaid` is the + /// shortfall and `prepaid - required` the excess of the real and of the nominal + /// part on its own, and for either part at most one of the two is non-zero. /// - /// # Assumption: the cost schedule of a subnet never changes + /// This means that the adjustment does not rely on the prepayment and the + /// requirement carrying the same cost schedule: the prepayment recorded in the + /// callback carries the cost schedule in effect when the call was performed, + /// whereas the requirement is derived from the cost schedule in effect when the + /// response is executed. The two always coincide today, since a subnet keeps the + /// cost schedule it was created with, but were the cost schedule of a live + /// subnet allowed to change, then a canister whose subnet switched from the + /// normal to the free cost schedule across a call would get back here the whole + /// real prepayment it made under the normal cost schedule, and one whose subnet + /// switched the other way round would pay the real requirement here; either way + /// it ends up with exactly the prepayment that the cost schedule in effect at + /// response time requires. /// - /// Comparing a single part, be it the real or the nominal one, is only sound - /// because the prepayment and the requirement are stamped with the same cost - /// schedule: the prepayment recorded in the callback carries the cost schedule in - /// effect when the call was performed, whereas the requirement is derived from the - /// cost schedule in effect when the response is executed. A subnet is assigned its - /// cost schedule when it is created (`do_create_subnet`) and keeps it (a subnet - /// split inherits it and splitting a rental subnet is rejected outright), and - /// `UpdateSubnetPayload` exposes no field to change it, so the two schedules always - /// coincide today. - /// - /// 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 hit the branch below that - /// returns the prepayment unchanged, i.e. keep the prepayment it made under the - /// normal cost schedule, real part and all. `refund_unused_execution_cycles` would - /// then derive the refund under the free cost schedule, where the real part of an - /// `Instructions` amount 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. + /// The withdrawal is performed before the refund so that a failing withdrawal + /// leaves the canister state unchanged, as the callers expect. /// /// Returns the prepayment matching the cycles required for executing the response /// in the given Wasm execution mode, or a `CanisterOutOfCyclesError` if the @@ -918,50 +906,18 @@ impl CyclesAccountManager { ) -> Result, CanisterOutOfCyclesError> { let required = self.prepayment_for_response_execution(subnet_cycles_config, execution_mode); let prepaid = prepayment_for_response_execution; - if prepaid.nominal() < required.nominal() { - // No freezing threshold is applied, i.e., the threshold is zero. - self.consume_with_threshold_impl( - system_state, - required - prepaid, - Cycles::zero(), - reveal_top_up, - )?; - return Ok(required); - } - Ok(self.refund_excess_prepayment_for_response_execution( + // No freezing threshold is applied, i.e., the threshold is zero. + self.consume_with_threshold_impl( system_state, - prepaid, - subnet_cycles_config, - execution_mode, - )) - } - - /// Refunds the part of the cycles prepaid for the execution of a response that - /// exceeds the cycles required for executing the response in the given Wasm - /// execution mode and returns the remaining prepayment. - /// - /// Unlike `adjust_prepayment_for_response_execution`, which uses this function to - /// handle an excessive prepayment, this never withdraws cycles from the canister's - /// balance and hence it cannot fail. - /// - /// The prepayment is compared to the requirement on its nominal part for the same - /// reason as in `adjust_prepayment_for_response_execution`. - fn refund_excess_prepayment_for_response_execution( - &self, - system_state: &mut SystemState, - prepayment_for_response_execution: CompoundCycles, - subnet_cycles_config: CyclesAccountManagerSubnetConfig, - execution_mode: WasmExecutionMode, - ) -> CompoundCycles { - let required = self.prepayment_for_response_execution(subnet_cycles_config, execution_mode); - if prepayment_for_response_execution.nominal() <= required.nominal() { - return prepayment_for_response_execution; - } + required - prepaid, + Cycles::zero(), + reveal_top_up, + )?; // The excess part of the prepayment is refunded in full and hence it does not // contribute to the consumed cycles of the canister. - let excess = prepayment_for_response_execution - required; + let excess = prepaid - required; system_state.refund_cycles(excess, excess); - required + Ok(required) } /// Settles the cycles prepaid for the execution of a response whose callback is diff --git a/rs/cycles_account_manager/tests/cycles_account_manager.rs b/rs/cycles_account_manager/tests/cycles_account_manager.rs index c70cb3b84107..b0dfa3967c64 100644 --- a/rs/cycles_account_manager/tests/cycles_account_manager.rs +++ b/rs/cycles_account_manager/tests/cycles_account_manager.rs @@ -388,42 +388,60 @@ fn verify_no_cycles_charged_for_message_execution_on_free_schedule() { /// prepayment for a response execution covers exactly this many instructions. const RESPONSE_EXECUTION_INSTRUCTION_LIMIT: NumInstructions = NumInstructions::new(1_000_000_000); -/// Every combination of a cost schedule, the Wasm execution mode a canister had -/// when it performed a call (and hence prepaid for the response execution) and -/// the Wasm execution mode it has when the response arrives. The latter two -/// differ whenever the canister is upgraded across the call. -fn cost_schedules_and_wasm_execution_modes() -> Vec<( - CanisterCyclesCostSchedule, - WasmExecutionMode, - WasmExecutionMode, -)> { - let mut combinations = vec![]; +/// A cost schedule and a Wasm execution mode, as in effect at one point in time. +#[derive(Copy, Clone, Debug)] +struct ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule, + wasm_execution_mode: WasmExecutionMode, +} + +/// Every combination of the cost schedule and the Wasm execution mode a canister +/// had when it performed a call (and hence prepaid for the response execution) +/// with the ones in effect when the response arrives. +/// +/// The Wasm execution modes differ whenever the canister is upgraded across the +/// call. The cost schedules cannot differ today, since the cost schedule of a +/// subnet is fixed when the subnet is created, but the accounting must not depend +/// on that: it settles the prepayment recorded in the callback, which carries the +/// cost schedule in effect at the call, against a requirement derived from the +/// cost schedule in effect at the response. +fn response_execution_settings() -> Vec<(ResponseExecutionSetting, ResponseExecutionSetting)> { + let mut settings = vec![]; for cost_schedule in [ CanisterCyclesCostSchedule::Normal, CanisterCyclesCostSchedule::Free, ] { - for mode_at_call in [WasmExecutionMode::Wasm32, WasmExecutionMode::Wasm64] { - for mode_at_response in [WasmExecutionMode::Wasm32, WasmExecutionMode::Wasm64] { - combinations.push((cost_schedule, mode_at_call, mode_at_response)); - } + for wasm_execution_mode in [WasmExecutionMode::Wasm32, WasmExecutionMode::Wasm64] { + settings.push(ResponseExecutionSetting { + cost_schedule, + wasm_execution_mode, + }); + } + } + let mut combinations = vec![]; + for at_call in settings.iter().copied() { + for at_response in settings.iter().copied() { + combinations.push((at_call, at_response)); } } combinations } -fn cycles_account_manager_and_subnet_cycles_config( - cost_schedule: CanisterCyclesCostSchedule, -) -> (CyclesAccountManager, CyclesAccountManagerSubnetConfig) { - let cycles_account_manager = CyclesAccountManagerBuilder::new() +fn cycles_account_manager() -> CyclesAccountManager { + CyclesAccountManagerBuilder::new() .with_subnet_type(SubnetType::Application) .with_max_num_instructions(RESPONSE_EXECUTION_INSTRUCTION_LIMIT) - .build(); - let subnet_cycles_config = CyclesAccountManagerSubnetConfig::new( + .build() +} + +fn subnet_cycles_config( + cost_schedule: CanisterCyclesCostSchedule, +) -> CyclesAccountManagerSubnetConfig { + CyclesAccountManagerSubnetConfig::new( SMALL_APP_SUBNET_MAX_SIZE, cost_schedule, DEFAULT_REFERENCE_SUBNET_SIZE, - ); - (cycles_account_manager, subnet_cycles_config) + ) } fn consumed_cycles_for_instructions(system_state: &SystemState) -> (NominalCycles, NominalCycles) { @@ -443,54 +461,45 @@ fn consumed_cycles_for_instructions(system_state: &SystemState) -> (NominalCycle } /// Adjusting the prepayment for a response execution must match the prepayment -/// required in the canister's current Wasm execution mode in *both* its real and -/// its nominal part. -/// -/// The nominal part matters under the free cost schedule, where the real part of -/// an `Instructions` amount is zero: a comparison of real parts would find -/// nothing to adjust there and leave the nominal part at the amount prepaid for -/// the Wasm execution mode the canister had when it performed the call. +/// required at response time in *both* its real and its nominal part. #[test] -fn adjust_prepayment_for_response_execution_matches_wasm_execution_mode() { - for (cost_schedule, mode_at_call, mode_at_response) in cost_schedules_and_wasm_execution_modes() - { - let (cycles_account_manager, subnet_cycles_config) = - cycles_account_manager_and_subnet_cycles_config(cost_schedule); +fn adjust_prepayment_for_response_execution_matches_response_execution_setting() { + for (at_call, at_response) in response_execution_settings() { + let cycles_account_manager = cycles_account_manager(); + let config_at_call = subnet_cycles_config(at_call.cost_schedule); + let config_at_response = subnet_cycles_config(at_response.cost_schedule); + let context = format!("{at_call:?} at call, {at_response:?} at response"); let mut system_state = SystemStateBuilder::new().build(); // When the call was performed, the canister prepaid for executing the - // response in the Wasm execution mode it had at that time. + // response under the cost schedule and in the Wasm execution mode in + // effect at that time. let prepaid = cycles_account_manager - .prepayment_for_response_execution(subnet_cycles_config, mode_at_call); + .prepayment_for_response_execution(config_at_call, at_call.wasm_execution_mode); system_state.consume_cycles(prepaid); let balance_before = system_state.balance(); // Now that the response has arrived, the prepayment is adjusted to the - // Wasm execution mode the canister has by now. + // cost schedule and the Wasm execution mode in effect by now. let required = cycles_account_manager - .prepayment_for_response_execution(subnet_cycles_config, mode_at_response); + .prepayment_for_response_execution(config_at_response, at_response.wasm_execution_mode); let adjusted = cycles_account_manager .adjust_prepayment_for_response_execution( &mut system_state, prepaid, - subnet_cycles_config, - mode_at_response, + config_at_response, + at_response.wasm_execution_mode, false, ) .unwrap(); - assert_eq!( - adjusted, required, - "unexpected prepayment for {cost_schedule:?} and \ - {mode_at_call:?} at call, {mode_at_response:?} at response" - ); + assert_eq!(adjusted, required, "unexpected prepayment for {context}"); // The canister has paid the adjusted prepayment out of its balance: the // missing cycles were withdrawn or the excess ones were refunded. assert_eq!( balance_before + prepaid.real(), system_state.balance() + required.real(), - "unexpected balance for {cost_schedule:?} and \ - {mode_at_call:?} at call, {mode_at_response:?} at response" + "unexpected balance for {context}" ); // The consumed cycles metrics report the adjusted prepayment as well. The // counter is only updated once the prepayment is refunded, i.e. not yet. @@ -498,30 +507,118 @@ fn adjust_prepayment_for_response_execution_matches_wasm_execution_mode() { assert_eq!( (gauge, counter), (required.nominal(), NominalCycles::zero()), - "unexpected consumed cycles for {cost_schedule:?} and \ - {mode_at_call:?} at call, {mode_at_response:?} at response" + "unexpected consumed cycles for {context}" ); } } -/// A canister that is upgraded to a different Wasm execution mode across a call +/// If the canister's balance does not cover the cycles missing from the +/// prepayment, then the adjustment fails and leaves the canister state unchanged. +/// Its callers rely on that: one replays the adjustment on a clean canister state +/// once a paused DTS execution is resumed, the other settles the unchanged +/// prepayment for a response that is not executed at all. +/// +/// In the second setting below the prepayment falls short of the requirement in +/// the real part while exceeding it in the nominal one, so that the excess to be +/// refunded is not zero. That is what pins down the order of the two: were the +/// refund performed before the failing withdrawal, it would lower the consumed +/// cycles gauge by that excess. +#[test] +fn adjust_prepayment_for_response_execution_leaves_state_unchanged_on_failure() { + const SETTINGS: [(ResponseExecutionSetting, ResponseExecutionSetting); 2] = [ + // Prepaid in the cheaper Wasm execution mode, so that the requirement in + // the more expensive one exceeds the prepayment in both parts. + ( + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Normal, + wasm_execution_mode: WasmExecutionMode::Wasm32, + }, + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Normal, + wasm_execution_mode: WasmExecutionMode::Wasm64, + }, + ), + // Prepaid under the free cost schedule, so that the requirement exceeds the + // prepayment in the real part; prepaid in the more expensive Wasm execution + // mode, so that the prepayment exceeds the requirement in the nominal part. + ( + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Free, + wasm_execution_mode: WasmExecutionMode::Wasm64, + }, + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Normal, + wasm_execution_mode: WasmExecutionMode::Wasm32, + }, + ), + ]; + + for (at_call, at_response) in SETTINGS { + let cycles_account_manager = cycles_account_manager(); + let config_at_call = subnet_cycles_config(at_call.cost_schedule); + let config_at_response = subnet_cycles_config(at_response.cost_schedule); + let context = format!("{at_call:?} at call, {at_response:?} at response"); + + let prepaid = cycles_account_manager + .prepayment_for_response_execution(config_at_call, at_call.wasm_execution_mode); + let required = cycles_account_manager + .prepayment_for_response_execution(config_at_response, at_response.wasm_execution_mode); + let missing = required.real() - prepaid.real(); + assert!(missing > Cycles::zero(), "nothing missing for {context}"); + + // Once the canister has paid the prepayment, its balance covers all but one + // cycle of the cycles missing from it. + let balance = missing - Cycles::new(1); + let mut system_state = SystemStateBuilder::new() + .initial_cycles(balance + prepaid.real()) + .build(); + system_state.consume_cycles(prepaid); + assert_eq!(system_state.balance(), balance); + let consumed_before = consumed_cycles_for_instructions(&system_state); + + let err = cycles_account_manager + .adjust_prepayment_for_response_execution( + &mut system_state, + prepaid, + config_at_response, + at_response.wasm_execution_mode, + false, + ) + .unwrap_err(); + + assert_eq!(err.requested, missing, "unexpected shortfall for {context}"); + assert_eq!( + system_state.balance(), + balance, + "unexpected balance for {context}" + ); + assert_eq!( + consumed_cycles_for_instructions(&system_state), + consumed_before, + "unexpected consumed cycles for {context}" + ); + } +} + +/// A canister whose cost schedule or Wasm execution mode changed across a call /// pays for its response execution, and has that execution reported in the -/// consumed cycles metrics, exactly as if it had performed the call in the Wasm -/// execution mode in which the response is executed. +/// consumed cycles metrics, exactly as if it had performed the call under the cost +/// schedule and in the Wasm execution mode in which the response is executed. #[test] -fn response_execution_consumed_cycles_match_wasm_execution_mode() { +fn response_execution_consumed_cycles_match_response_execution_setting() { const EXECUTED_INSTRUCTIONS: NumInstructions = NumInstructions::new(1_000_000); - for (cost_schedule, mode_at_call, mode_at_response) in cost_schedules_and_wasm_execution_modes() - { - let (cycles_account_manager, subnet_cycles_config) = - cycles_account_manager_and_subnet_cycles_config(cost_schedule); + for (at_call, at_response) in response_execution_settings() { + let cycles_account_manager = cycles_account_manager(); + let config_at_call = subnet_cycles_config(at_call.cost_schedule); + let config_at_response = subnet_cycles_config(at_response.cost_schedule); + let context = format!("{at_call:?} at call, {at_response:?} at response"); let mut system_state = SystemStateBuilder::new().build(); let balance_before = system_state.balance(); // Prepay for the response execution when the call is performed. let prepaid = cycles_account_manager - .prepayment_for_response_execution(subnet_cycles_config, mode_at_call); + .prepayment_for_response_execution(config_at_call, at_call.wasm_execution_mode); system_state.consume_cycles(prepaid); // Adjust the prepayment when the response arrives. @@ -529,8 +626,8 @@ fn response_execution_consumed_cycles_match_wasm_execution_mode() { .adjust_prepayment_for_response_execution( &mut system_state, prepaid, - subnet_cycles_config, - mode_at_response, + config_at_response, + at_response.wasm_execution_mode, false, ) .unwrap(); @@ -543,8 +640,8 @@ fn response_execution_consumed_cycles_match_wasm_execution_mode() { RESPONSE_EXECUTION_INSTRUCTION_LIMIT, adjusted, &no_op_counter, - subnet_cycles_config, - mode_at_response, + config_at_response, + at_response.wasm_execution_mode, &no_op_logger(), ); @@ -553,67 +650,69 @@ fn response_execution_consumed_cycles_match_wasm_execution_mode() { // in the Wasm execution mode it executed them in. let expected = cycles_account_manager.execution_cost( EXECUTED_INSTRUCTIONS, - subnet_cycles_config, - mode_at_response, + config_at_response, + at_response.wasm_execution_mode, ); assert_eq!( system_state.balance() + expected.real(), balance_before, - "unexpected balance for {cost_schedule:?} and \ - {mode_at_call:?} at call, {mode_at_response:?} at response" + "unexpected balance for {context}" ); let (gauge, counter) = consumed_cycles_for_instructions(&system_state); assert_eq!( (gauge, counter), (expected.nominal(), expected.nominal()), - "unexpected consumed cycles for {cost_schedule:?} and \ - {mode_at_call:?} at call, {mode_at_response:?} at response" + "unexpected consumed cycles for {context}" ); } } /// A response whose callback is not executed at all costs the fixed per-message -/// execution fee only, no matter which Wasm execution mode the cycles were -/// prepaid for and which one the canister has when the response arrives. +/// execution fee only, no matter what the cost schedule and the Wasm execution +/// mode were when the cycles were prepaid and what they are when the response +/// arrives. +/// +/// The prepayment is never topped up here, so the canister is charged at most what +/// it prepaid: a canister whose subnet switched from the free to the normal cost +/// schedule across the call prepaid nothing real and hence pays nothing real. #[test] fn settle_prepayment_for_unexecuted_response_charges_only_the_base_fee() { - for (cost_schedule, mode_at_call, mode_at_response) in cost_schedules_and_wasm_execution_modes() - { - let (cycles_account_manager, subnet_cycles_config) = - cycles_account_manager_and_subnet_cycles_config(cost_schedule); + for (at_call, at_response) in response_execution_settings() { + let cycles_account_manager = cycles_account_manager(); + let config_at_call = subnet_cycles_config(at_call.cost_schedule); + let config_at_response = subnet_cycles_config(at_response.cost_schedule); + let context = format!("{at_call:?} at call, {at_response:?} at response"); let mut system_state = SystemStateBuilder::new().build(); let balance_before = system_state.balance(); let prepaid = cycles_account_manager - .prepayment_for_response_execution(subnet_cycles_config, mode_at_call); + .prepayment_for_response_execution(config_at_call, at_call.wasm_execution_mode); system_state.consume_cycles(prepaid); cycles_account_manager.settle_prepayment_for_unexecuted_response( &mut system_state, prepaid, - subnet_cycles_config, - mode_at_response, + config_at_response, + at_response.wasm_execution_mode, ); // No instructions were executed, hence only the fixed per-message // execution fee is due; the rest of the prepayment is refunded. let base_fee = cycles_account_manager.execution_cost( NumInstructions::from(0), - subnet_cycles_config, - mode_at_response, + config_at_response, + at_response.wasm_execution_mode, ); assert_eq!( - system_state.balance() + base_fee.real(), + system_state.balance() + base_fee.real().min(prepaid.real()), balance_before, - "unexpected balance for {cost_schedule:?} and \ - {mode_at_call:?} at call, {mode_at_response:?} at response" + "unexpected balance for {context}" ); let (gauge, counter) = consumed_cycles_for_instructions(&system_state); assert_eq!( (gauge, counter), (base_fee.nominal(), base_fee.nominal()), - "unexpected consumed cycles for {cost_schedule:?} and \ - {mode_at_call:?} at call, {mode_at_response:?} at response" + "unexpected consumed cycles for {context}" ); } } From 648e84ca214eea9a87b9319ad2df00f763345bbb Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Fri, 11 Sep 2026 13:58:42 +0000 Subject: [PATCH 11/14] test: pin that an adjustment without a real shortfall cannot fail `adjust_prepayment_for_response_execution` withdraws `required - prepaid` unconditionally, i.e. nothing at all when the requirement does not exceed the prepayment in the real part. Pin that down on a canister that spent its whole balance on the prepayment: any withdrawal attempted there would fail. One of the two settings has the nominal part of the prepayment fall short of the requirement while its real part is in excess, i.e. the nominal part is topped up while the real one is refunded. `adjust_prepayment_for_response_execution_matches_response_execution_setting` covers both settings as well, but it leaves the canister cycles to spare, so a withdrawal attempted here would succeed there and go unnoticed. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/cycles_account_manager.rs | 107 +++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/rs/cycles_account_manager/tests/cycles_account_manager.rs b/rs/cycles_account_manager/tests/cycles_account_manager.rs index b0dfa3967c64..a9828815c64d 100644 --- a/rs/cycles_account_manager/tests/cycles_account_manager.rs +++ b/rs/cycles_account_manager/tests/cycles_account_manager.rs @@ -33,7 +33,7 @@ use ic_types_cycles::{ NominalCycles, NominalCyclesTesting, Uninstall, }; use prometheus::IntCounter; -use std::{convert::TryFrom, time::Duration}; +use std::{cmp::Ordering, convert::TryFrom, time::Duration}; const WASM_EXECUTION_MODE: WasmExecutionMode = WasmExecutionMode::Wasm32; @@ -600,6 +600,111 @@ fn adjust_prepayment_for_response_execution_leaves_state_unchanged_on_failure() } } +/// The mirror image of the test above: a requirement that does not exceed the +/// prepayment in the real part withdraws nothing, so the adjustment cannot fail, +/// not even for a canister that spent its whole balance on the prepayment. The +/// adjustment relies on that, since it withdraws `required - prepaid` +/// unconditionally, i.e. an amount whose real part saturates at zero here. +/// +/// `adjust_prepayment_for_response_execution_matches_response_execution_setting` +/// covers the two settings below as well, but it leaves the canister cycles to +/// spare, so a withdrawal attempted here would succeed there and go unnoticed. +/// What this test pins down is that none is attempted in the first place. +/// +/// Both settings have the requirement below the prepayment in the real part, but +/// they differ in the nominal one: the second one has the nominal part topped up +/// while nothing real is withdrawn. +#[test] +fn adjust_prepayment_for_response_execution_cannot_fail_without_a_real_shortfall() { + // The third component is the direction of the nominal part of the prepayment + // relative to the nominal part of the requirement. + const SETTINGS: [(ResponseExecutionSetting, ResponseExecutionSetting, Ordering); 2] = [ + // Prepaid in the more expensive Wasm execution mode, so that the prepayment + // exceeds the requirement in both parts, by the same amount. + ( + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Normal, + wasm_execution_mode: WasmExecutionMode::Wasm64, + }, + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Normal, + wasm_execution_mode: WasmExecutionMode::Wasm32, + }, + Ordering::Greater, + ), + // Prepaid under the normal cost schedule while the response is executed + // under the free one, so that the whole real prepayment is in excess; and + // prepaid in the cheaper Wasm execution mode, so that the nominal part of + // the prepayment falls short of the requirement and is topped up, all while + // nothing real is withdrawn. + ( + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Normal, + wasm_execution_mode: WasmExecutionMode::Wasm32, + }, + ResponseExecutionSetting { + cost_schedule: CanisterCyclesCostSchedule::Free, + wasm_execution_mode: WasmExecutionMode::Wasm64, + }, + Ordering::Less, + ), + ]; + + for (at_call, at_response, nominal_direction) in SETTINGS { + let cycles_account_manager = cycles_account_manager(); + let config_at_call = subnet_cycles_config(at_call.cost_schedule); + let config_at_response = subnet_cycles_config(at_response.cost_schedule); + let context = format!("{at_call:?} at call, {at_response:?} at response"); + + let prepaid = cycles_account_manager + .prepayment_for_response_execution(config_at_call, at_call.wasm_execution_mode); + let required = cycles_account_manager + .prepayment_for_response_execution(config_at_response, at_response.wasm_execution_mode); + assert!( + required.real() < prepaid.real(), + "nothing in excess in the real part for {context}" + ); + assert_eq!( + prepaid.nominal().cmp(&required.nominal()), + nominal_direction, + "unexpected direction of the nominal part for {context}" + ); + + // The canister spent its whole balance on the prepayment, i.e. any + // withdrawal at all would fail below. + let mut system_state = SystemStateBuilder::new() + .initial_cycles(prepaid.real()) + .build(); + system_state.consume_cycles(prepaid); + assert_eq!(system_state.balance(), Cycles::zero()); + + let adjusted = cycles_account_manager + .adjust_prepayment_for_response_execution( + &mut system_state, + prepaid, + config_at_response, + at_response.wasm_execution_mode, + false, + ) + .unwrap_or_else(|err| panic!("adjustment failed for {context}: {err}")); + + assert_eq!(adjusted, required, "unexpected prepayment for {context}"); + // The excess is refunded to the balance in the real part and taken off the + // consumed cycles gauge in the nominal one. + assert_eq!( + system_state.balance(), + prepaid.real() - required.real(), + "unexpected balance for {context}" + ); + let (gauge, counter) = consumed_cycles_for_instructions(&system_state); + assert_eq!( + (gauge, counter), + (required.nominal(), NominalCycles::zero()), + "unexpected consumed cycles for {context}" + ); + } +} + /// A canister whose cost schedule or Wasm execution mode changed across a call /// pays for its response execution, and has that execution reported in the /// consumed cycles metrics, exactly as if it had performed the call under the cost From 6e97c2a2aee4d29f56ea0275311e797e2f7e1bd6 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Fri, 11 Sep 2026 14:18:22 +0000 Subject: [PATCH 12/14] test: merge the two response execution matrix tests `response_execution_consumed_cycles_match_response_execution_setting` ran the very sequence of `adjust_prepayment_for_response_execution_matches_response_execution_setting` and then refunded the cycles for the instructions the callback did not execute, so the latter's assertions are a checkpoint of the former. Merge the two and assert at both points at which the cycles the canister has paid are determined, naming the checkpoint in each assert message. Comparing against the balance the canister had before it prepaid also states the property more directly than relating the balances before and after the adjustment did: the canister has paid exactly the adjusted prepayment. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/cycles_account_manager.rs | 139 +++++++----------- 1 file changed, 57 insertions(+), 82 deletions(-) diff --git a/rs/cycles_account_manager/tests/cycles_account_manager.rs b/rs/cycles_account_manager/tests/cycles_account_manager.rs index a9828815c64d..2b7a037d8297 100644 --- a/rs/cycles_account_manager/tests/cycles_account_manager.rs +++ b/rs/cycles_account_manager/tests/cycles_account_manager.rs @@ -460,16 +460,28 @@ fn consumed_cycles_for_instructions(system_state: &SystemState) -> (NominalCycle (gauge, counter) } -/// Adjusting the prepayment for a response execution must match the prepayment -/// required at response time in *both* its real and its nominal part. +/// A canister whose cost schedule or Wasm execution mode changed across a call +/// pays for its response execution, and has that execution reported in the +/// consumed cycles metrics, exactly as if it had performed the call under the cost +/// schedule and in the Wasm execution mode in which the response is executed. +/// +/// Checked at the two points of the prepay, adjust and refund sequence at which +/// the cycles the canister has paid are determined: right after the adjustment, +/// where it must have paid the prepayment required at response time in both its +/// real and its nominal part, and after the cycles for the instructions the +/// callback did not execute are refunded, where it must have paid for the +/// instructions it did execute. #[test] -fn adjust_prepayment_for_response_execution_matches_response_execution_setting() { +fn response_execution_cycles_match_response_execution_setting() { + const EXECUTED_INSTRUCTIONS: NumInstructions = NumInstructions::new(1_000_000); + for (at_call, at_response) in response_execution_settings() { let cycles_account_manager = cycles_account_manager(); let config_at_call = subnet_cycles_config(at_call.cost_schedule); let config_at_response = subnet_cycles_config(at_response.cost_schedule); let context = format!("{at_call:?} at call, {at_response:?} at response"); let mut system_state = SystemStateBuilder::new().build(); + let initial_balance = system_state.balance(); // When the call was performed, the canister prepaid for executing the // response under the cost schedule and in the Wasm execution mode in @@ -477,7 +489,6 @@ fn adjust_prepayment_for_response_execution_matches_response_execution_setting() let prepaid = cycles_account_manager .prepayment_for_response_execution(config_at_call, at_call.wasm_execution_mode); system_state.consume_cycles(prepaid); - let balance_before = system_state.balance(); // Now that the response has arrived, the prepayment is adjusted to the // cost schedule and the Wasm execution mode in effect by now. @@ -494,20 +505,51 @@ fn adjust_prepayment_for_response_execution_matches_response_execution_setting() .unwrap(); assert_eq!(adjusted, required, "unexpected prepayment for {context}"); - // The canister has paid the adjusted prepayment out of its balance: the - // missing cycles were withdrawn or the excess ones were refunded. + // Whichever prepayment the canister made, it has now paid the adjusted one: + // the missing cycles were withdrawn or the excess ones were refunded. assert_eq!( - balance_before + prepaid.real(), system_state.balance() + required.real(), - "unexpected balance for {context}" + initial_balance, + "unexpected balance after the adjustment for {context}" ); // The consumed cycles metrics report the adjusted prepayment as well. The // counter is only updated once the prepayment is refunded, i.e. not yet. - let (gauge, counter) = consumed_cycles_for_instructions(&system_state); assert_eq!( - (gauge, counter), + consumed_cycles_for_instructions(&system_state), (required.nominal(), NominalCycles::zero()), - "unexpected consumed cycles for {context}" + "unexpected consumed cycles after the adjustment for {context}" + ); + + // Refund the cycles for the instructions the callback did not execute. + let no_op_counter: IntCounter = IntCounter::new("no_op", "no_op").unwrap(); + cycles_account_manager.refund_unused_execution_cycles( + &mut system_state, + RESPONSE_EXECUTION_INSTRUCTION_LIMIT - EXECUTED_INSTRUCTIONS, + RESPONSE_EXECUTION_INSTRUCTION_LIMIT, + adjusted, + &no_op_counter, + config_at_response, + at_response.wasm_execution_mode, + &no_op_logger(), + ); + + // The canister is charged, and reported to have consumed, the fixed + // per-message execution fee plus the cost of the instructions it executed + // in the Wasm execution mode it executed them in. + let expected = cycles_account_manager.execution_cost( + EXECUTED_INSTRUCTIONS, + config_at_response, + at_response.wasm_execution_mode, + ); + assert_eq!( + system_state.balance() + expected.real(), + initial_balance, + "unexpected balance after the refund for {context}" + ); + assert_eq!( + consumed_cycles_for_instructions(&system_state), + (expected.nominal(), expected.nominal()), + "unexpected consumed cycles after the refund for {context}" ); } } @@ -606,10 +648,10 @@ fn adjust_prepayment_for_response_execution_leaves_state_unchanged_on_failure() /// adjustment relies on that, since it withdraws `required - prepaid` /// unconditionally, i.e. an amount whose real part saturates at zero here. /// -/// `adjust_prepayment_for_response_execution_matches_response_execution_setting` -/// covers the two settings below as well, but it leaves the canister cycles to -/// spare, so a withdrawal attempted here would succeed there and go unnoticed. -/// What this test pins down is that none is attempted in the first place. +/// `response_execution_cycles_match_response_execution_setting` covers the two +/// settings below as well, but it leaves the canister cycles to spare, so a +/// withdrawal attempted here would succeed there and go unnoticed. What this test +/// pins down is that none is attempted in the first place. /// /// Both settings have the requirement below the prepayment in the real part, but /// they differ in the nominal one: the second one has the nominal part topped up @@ -705,73 +747,6 @@ fn adjust_prepayment_for_response_execution_cannot_fail_without_a_real_shortfall } } -/// A canister whose cost schedule or Wasm execution mode changed across a call -/// pays for its response execution, and has that execution reported in the -/// consumed cycles metrics, exactly as if it had performed the call under the cost -/// schedule and in the Wasm execution mode in which the response is executed. -#[test] -fn response_execution_consumed_cycles_match_response_execution_setting() { - const EXECUTED_INSTRUCTIONS: NumInstructions = NumInstructions::new(1_000_000); - - for (at_call, at_response) in response_execution_settings() { - let cycles_account_manager = cycles_account_manager(); - let config_at_call = subnet_cycles_config(at_call.cost_schedule); - let config_at_response = subnet_cycles_config(at_response.cost_schedule); - let context = format!("{at_call:?} at call, {at_response:?} at response"); - let mut system_state = SystemStateBuilder::new().build(); - let balance_before = system_state.balance(); - - // Prepay for the response execution when the call is performed. - let prepaid = cycles_account_manager - .prepayment_for_response_execution(config_at_call, at_call.wasm_execution_mode); - system_state.consume_cycles(prepaid); - - // Adjust the prepayment when the response arrives. - let adjusted = cycles_account_manager - .adjust_prepayment_for_response_execution( - &mut system_state, - prepaid, - config_at_response, - at_response.wasm_execution_mode, - false, - ) - .unwrap(); - - // Refund the cycles for the instructions the callback did not execute. - let no_op_counter: IntCounter = IntCounter::new("no_op", "no_op").unwrap(); - cycles_account_manager.refund_unused_execution_cycles( - &mut system_state, - RESPONSE_EXECUTION_INSTRUCTION_LIMIT - EXECUTED_INSTRUCTIONS, - RESPONSE_EXECUTION_INSTRUCTION_LIMIT, - adjusted, - &no_op_counter, - config_at_response, - at_response.wasm_execution_mode, - &no_op_logger(), - ); - - // The canister is charged, and reported to have consumed, the fixed - // per-message execution fee plus the cost of the instructions it executed - // in the Wasm execution mode it executed them in. - let expected = cycles_account_manager.execution_cost( - EXECUTED_INSTRUCTIONS, - config_at_response, - at_response.wasm_execution_mode, - ); - assert_eq!( - system_state.balance() + expected.real(), - balance_before, - "unexpected balance for {context}" - ); - let (gauge, counter) = consumed_cycles_for_instructions(&system_state); - assert_eq!( - (gauge, counter), - (expected.nominal(), expected.nominal()), - "unexpected consumed cycles for {context}" - ); - } -} - /// A response whose callback is not executed at all costs the fixed per-message /// execution fee only, no matter what the cost schedule and the Wasm execution /// mode were when the cycles were prepaid and what they are when the response From 0b6144598f54edc89863b24504be6ce9960a1974 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Fri, 11 Sep 2026 16:16:39 +0000 Subject: [PATCH 13/14] docs: say how the callers rely on a failed adjustment changing nothing Both call sites of the adjustment go on to settle the *unadjusted* prepayment when it fails, since the `ResponseHelper` method wrapping it records the adjusted prepayment only on success. Name them and say what would go wrong, rather than just stating that they rely on the property. Also state what makes the opposite direction unable to fail: the withdrawal is of `required - prepaid`, whose real part saturates at zero when the requirement does not exceed the prepayment there. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/cycles_account_manager.rs | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/rs/cycles_account_manager/tests/cycles_account_manager.rs b/rs/cycles_account_manager/tests/cycles_account_manager.rs index 2b7a037d8297..7f2823c38b46 100644 --- a/rs/cycles_account_manager/tests/cycles_account_manager.rs +++ b/rs/cycles_account_manager/tests/cycles_account_manager.rs @@ -556,9 +556,18 @@ fn response_execution_cycles_match_response_execution_setting() { /// If the canister's balance does not cover the cycles missing from the /// prepayment, then the adjustment fails and leaves the canister state unchanged. -/// Its callers rely on that: one replays the adjustment on a clean canister state -/// once a paused DTS execution is resumed, the other settles the unchanged -/// prepayment for a response that is not executed at all. +/// +/// Both of its call sites in `rs/execution_environment/src/execution/response.rs` +/// rely on that, since both go on to settle the *unadjusted* prepayment: the +/// `ResponseHelper` method wrapping the adjustment records the adjusted prepayment +/// only on success, so what a failure leaves to be settled is the prepayment +/// recorded in the callback. `execute_response` rejects the response without +/// executing the callback, settling that prepayment in +/// `settle_prepayment_for_unexecuted_response`; `ResponseHelper::resume` turns the +/// failure of replaying the adjustment on the clean canister state into a Wasm +/// execution error, which ends up settling it in `refund_unused_execution_cycles`. +/// Either way, an excess that a failed adjustment had already refunded would be +/// refunded a second time. /// /// In the second setting below the prepayment falls short of the requirement in /// the real part while exceeding it in the nominal one, so that the excess to be @@ -644,9 +653,13 @@ fn adjust_prepayment_for_response_execution_leaves_state_unchanged_on_failure() /// The mirror image of the test above: a requirement that does not exceed the /// prepayment in the real part withdraws nothing, so the adjustment cannot fail, -/// not even for a canister that spent its whole balance on the prepayment. The -/// adjustment relies on that, since it withdraws `required - prepaid` -/// unconditionally, i.e. an amount whose real part saturates at zero here. +/// not even for a canister that spent its whole balance on the prepayment. +/// +/// The adjustment withdraws `required - prepaid` without comparing the two first. +/// Subtracting two `CompoundCycles` saturates part by part, so in the settings +/// below, where the requirement is below the prepayment in the real part, that +/// difference has a zero real part; and withdrawing zero cycles against a zero +/// freezing threshold cannot fail, whatever the balance is. /// /// `response_execution_cycles_match_response_execution_setting` covers the two /// settings below as well, but it leaves the canister cycles to spare, so a From da050af2c6034168535e710129ae37a6ea9e2995 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Fri, 11 Sep 2026 16:22:13 +0000 Subject: [PATCH 14/14] docs: state leaving the state unchanged on failure as a requirement The function's doc comment presented it as a consequence of performing the withdrawal before the refund, and did not mention it at all where the error is documented. State the requirement first and the order as the means, in both the function's and the test's doc comment, without spelling out what the callers do with it. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/cycles_account_manager.rs | 8 +++++--- .../tests/cycles_account_manager.rs | 16 +++------------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/rs/cycles_account_manager/src/cycles_account_manager.rs b/rs/cycles_account_manager/src/cycles_account_manager.rs index 98dfeef50bad..dddf06faed59 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -890,12 +890,14 @@ impl CyclesAccountManager { /// it ends up with exactly the prepayment that the cost schedule in effect at /// response time requires. /// - /// The withdrawal is performed before the refund so that a failing withdrawal - /// leaves the canister state unchanged, as the callers expect. + /// A failing adjustment must leave the canister state unchanged: neither the + /// balance nor the consumed cycles metrics may move. The withdrawal is therefore + /// performed before the refund, which cannot fail. /// /// Returns the prepayment matching the cycles required for executing the response /// in the given Wasm execution mode, or a `CanisterOutOfCyclesError` if the - /// canister's balance does not cover the additional prepayment. + /// canister's balance does not cover the additional prepayment, in which case + /// the canister state is left unchanged. pub fn adjust_prepayment_for_response_execution( &self, system_state: &mut SystemState, diff --git a/rs/cycles_account_manager/tests/cycles_account_manager.rs b/rs/cycles_account_manager/tests/cycles_account_manager.rs index 7f2823c38b46..1b6a10d7446b 100644 --- a/rs/cycles_account_manager/tests/cycles_account_manager.rs +++ b/rs/cycles_account_manager/tests/cycles_account_manager.rs @@ -555,19 +555,9 @@ fn response_execution_cycles_match_response_execution_setting() { } /// If the canister's balance does not cover the cycles missing from the -/// prepayment, then the adjustment fails and leaves the canister state unchanged. -/// -/// Both of its call sites in `rs/execution_environment/src/execution/response.rs` -/// rely on that, since both go on to settle the *unadjusted* prepayment: the -/// `ResponseHelper` method wrapping the adjustment records the adjusted prepayment -/// only on success, so what a failure leaves to be settled is the prepayment -/// recorded in the callback. `execute_response` rejects the response without -/// executing the callback, settling that prepayment in -/// `settle_prepayment_for_unexecuted_response`; `ResponseHelper::resume` turns the -/// failure of replaying the adjustment on the clean canister state into a Wasm -/// execution error, which ends up settling it in `refund_unused_execution_cycles`. -/// Either way, an excess that a failed adjustment had already refunded would be -/// refunded a second time. +/// prepayment, then the adjustment fails and leaves the canister state unchanged: +/// neither the balance nor the consumed cycles metrics move. That is a requirement +/// of `adjust_prepayment_for_response_execution` which its callers rely on. /// /// In the second setting below the prepayment falls short of the requirement in /// the real part while exceeding it in the nominal one, so that the excess to be