From 7f32121fbabd5f999d567d21cb42d664af0d3d0f Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Thu, 3 Sep 2026 14:57:12 +0000 Subject: [PATCH] chore: Testing out AI instructions for shorter comments I asked Claude to rewrite the comments for commit e120aa1 given its current local rules, pointing out that I had modified `~/.claude/CLAUDE.md`. This is the result. For reference, here are the instructions (first section has been there for a while now, the rest I just added) if you want to iterate on them (tweak the rules, then pick a commit you don't like and ask Claude to redo the comments): ```md Try very, very hard to be as concise as you can when writing documents or comments; and when chatting. Unless it's really necessary to refer to some other concept / comment / document section, prefer not to. Definitely do not explain the same concept repeatedly, in multiple locations. Explain it once and simply mention it when actually necessary (i.e. when the statement you are making is likely to be hard to understand without this extra context). - **Why, Not What:** Never comment *what* the code is doing (e.g., avoid `// Reset foo`). Comment only *why* it is being done, or the non-obvious architectural context. - **Micro-Comments Only:** Keep inline comments to one or two hyper-focused lines. No multi-paragraph prose blocks. - **High Signal-to-Noise:** If the code is clear, write zero comments. With very few exceptions, there should be more code than documentation. Else, the code is likely too opaque and needs refactoring. - **Context:** Comments must make sense to a future reader in the context of all of the surrounding code (and codebase), NOT in the context of the changes being made now. Code represents the current state; let git handle history. - **Hierarchy of Detail:** Where context is needed (e.g. "the resources returned here were reserved when...") provide it at the highest appropriate level (e.g. describe the resource reservation model once in the module or crate doc comment, instead of selective bits across function or inline comments). Expect future readers to acquire the full context by reading the documentation of the full hierarchy. ``` --- .../src/cycles_account_manager.rs | 69 ++++++----------- .../src/execution/response.rs | 75 +++++-------------- 2 files changed, 42 insertions(+), 102 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..bb1e334e1dd8 100644 --- a/rs/cycles_account_manager/src/cycles_account_manager.rs +++ b/rs/cycles_account_manager/src/cycles_account_manager.rs @@ -849,30 +849,19 @@ impl CyclesAccountManager { ) } - /// Adjusts the cycles prepaid for the execution of a response so that they match - /// exactly the cycles required for executing the response in the given Wasm - /// execution mode. + /// Adjusts the cycles prepaid for a response execution to those required in the + /// given Wasm execution mode and returns them, withdrawing any shortfall from + /// the canister or refunding it any excess. /// - /// The cycles for a response execution are prepaid when the corresponding call - /// is performed, i.e., using the instruction costs of the Wasm execution mode - /// of the calling canister at that time. The canister might have been upgraded - /// to a different Wasm execution mode before the response arrives: + /// The cycles were prepaid when the corresponding call was performed, at the + /// instruction costs of the Wasm execution mode the canister had back then; an + /// upgrade since may have changed that mode. Adjusting makes the canister pay + /// for the instructions it executes at the costs of the mode it executes them + /// in. /// - /// - if the prepayment falls short of the requirement (the canister was upgraded - /// to a more expensive Wasm execution mode), then the missing cycles are - /// withdrawn from the canister's balance. No freezing threshold is applied: - /// the canister already committed to executing the response when it performed - /// the corresponding call; - /// - if the prepayment exceeds the requirement (the canister was upgraded to a - /// cheaper Wasm execution mode), then the excess is refunded immediately. - /// - /// Matching the prepayment to the requirement lets the canister pay exactly for - /// the instructions it executed, at the instruction costs of the Wasm execution - /// mode it executed them in. - /// - /// 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. + /// The withdrawal ignores the freezing threshold, as the canister committed to + /// executing the response when it performed the call. It leaves the canister + /// untouched and fails if the balance does not cover the shortfall. pub fn adjust_prepayment_for_response_execution( &self, system_state: &mut SystemState, @@ -884,7 +873,6 @@ impl CyclesAccountManager { let required = self.prepayment_for_response_execution(subnet_cycles_config, execution_mode); let prepaid = prepayment_for_response_execution; if prepaid.real() < required.real() { - // No freezing threshold is applied, i.e., the threshold is zero. self.consume_with_threshold_impl( system_state, required - prepaid, @@ -901,13 +889,8 @@ impl CyclesAccountManager { )) } - /// 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. + /// Refunds the part of the cycles prepaid for a response execution that exceeds + /// the cycles required in the given Wasm execution mode and returns the rest. fn refund_excess_prepayment_for_response_execution( &self, system_state: &mut SystemState, @@ -919,26 +902,20 @@ impl CyclesAccountManager { if prepayment_for_response_execution.real() <= required.real() { return prepayment_for_response_execution; } - // The excess part of the prepayment is refunded in full and hence it does not - // contribute to the consumed cycles of the canister. + // Refunded in full, so the excess never counts as consumed by the canister. let excess = prepayment_for_response_execution - required; system_state.refund_cycles(excess, excess); required } - /// Settles the cycles prepaid for the execution of a response whose callback is - /// not executed at all: the canister is charged the fixed per-message execution - /// fee and the rest of the prepayment is refunded to it. - /// - /// Since no instructions are executed, the fixed per-message execution fee is all - /// that is due, no matter which Wasm execution mode the cycles were prepaid for - /// and which one the canister has now. In particular, the canister keeps the rest - /// of its prepayment even if the prepayment falls short of the cycles that - /// executing the response in its current Wasm execution mode would require. + /// Settles the cycles prepaid for a response whose callback is not executed at + /// all, charging the fixed per-message execution fee and refunding the rest. /// - /// 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. + /// With no instructions executed, that fee is all that is due, whichever Wasm + /// execution mode the cycles were prepaid for and whichever one the canister + /// has now. The prepayment is deliberately not adjusted first: topping it up + /// would refund the additional cycles right away and, unlike this refund, the + /// withdrawal could fail. pub fn settle_prepayment_for_unexecuted_response( &self, system_state: &mut SystemState, @@ -946,14 +923,12 @@ impl CyclesAccountManager { subnet_cycles_config: CyclesAccountManagerSubnetConfig, execution_mode: WasmExecutionMode, ) { - // Executing no instructions costs the fixed per-message execution fee only. let base_fee = self.execution_cost( NumInstructions::from(0), 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. + // A prepayment made in a cheaper mode may fall short of this mode's fee. let charge = base_fee.min(prepayment_for_response_execution); system_state.refund_cycles( prepayment_for_response_execution, diff --git a/rs/execution_environment/src/execution/response.rs b/rs/execution_environment/src/execution/response.rs index 665a6831f2f9..b3339cfeea21 100644 --- a/rs/execution_environment/src/execution/response.rs +++ b/rs/execution_environment/src/execution/response.rs @@ -125,15 +125,9 @@ struct ResponseHelper { prepayment_for_response_transmission: CompoundCycles, prepayment_for_call_transmission: CompoundCycles, refund_for_response_transmission: CompoundCycles, - /// Cycles prepaid for the execution of this response. - /// - /// Initially the prepayment recorded in the callback when the corresponding call - /// was performed, i.e., the cycles required for executing the response in the - /// Wasm execution mode the canister had at that time. Before the callback is - /// executed, `adjust_prepayment_for_response_execution()` replaces this with the - /// cycles required for executing the response in the canister's current Wasm - /// execution mode; for responses whose callback is not executed at all, - /// `early_finish()` settles the prepayment in full instead. + /// Cycles prepaid for the execution of this response, taken from the callback + /// and adjusted to the canister's current Wasm execution mode before the + /// callback runs. prepayment_for_response_execution: CompoundCycles, initial_cycles_balance: Cycles, response_sender: CanisterId, @@ -255,11 +249,8 @@ impl ResponseHelper { ); } - /// Returns the Wasm execution mode of the canister executing this response. - /// - /// This is the mode of the code that actually runs the callback, which is not - /// necessarily the mode the canister had when it performed the corresponding - /// call: the canister might have been upgraded in the meantime. + /// Returns the Wasm execution mode of the code that runs the callback, which is + /// not necessarily the one the canister had when it performed the call. fn wasm_execution_mode(&self) -> WasmExecutionMode { self.canister .execution_state @@ -267,17 +258,9 @@ impl ResponseHelper { .map_or(WasmExecutionMode::Wasm32, |state| state.wasm_execution_mode) } - /// Adjusts the cycles prepaid for the execution of this response to the cycles - /// required for executing it in the canister's current Wasm execution mode. - /// - /// The cycles were prepaid when the corresponding call was performed, i.e., - /// using the instruction costs of the Wasm execution mode of the canister at - /// that time. If the canister has been upgraded since then, then the missing - /// cycles are withdrawn from the canister's balance (without applying the - /// freezing threshold) or the excess cycles are refunded to it. - /// - /// Returns an error if the canister's balance does not cover the additional - /// prepayment, in which case the canister state is left unchanged. + /// Adjusts `prepayment_for_response_execution` to the canister's current Wasm + /// execution mode, see + /// `CyclesAccountManager::adjust_prepayment_for_response_execution()`. fn adjust_prepayment_for_response_execution( &mut self, original: &OriginalContext, @@ -456,21 +439,11 @@ impl ResponseHelper { return Err((helper, err)); } - // Replay the adjustment of the prepayment for the response execution: the - // initial steps are replayed on the clean canister state, which does not - // contain the changes of the ongoing DTS execution. - // - // The required prepayment is the same as in `execute_response()`: the Wasm - // module, and hence its Wasm execution mode, cannot change while a DTS - // execution of this canister is in progress because installing code is not - // executed for a canister that has a paused or an aborted execution (see - // `can_execute_subnet_msg()`), i.e., it is deferred to a later round. Should - // the paused execution be aborted nevertheless, e.g., before a checkpoint, - // then the response execution starts over in `execute_response()`, where the - // prepayment is adjusted afresh for whatever module is installed by then. - // - // Together with the check above that the cycles balance of the clean canister - // has not decreased, this replay is therefore expected to succeed. + // The adjustment is not persisted in the callback, so it must be replayed on + // the clean canister state. It is expected to succeed: the balance has not + // decreased (checked above) and the Wasm module cannot change while a DTS + // execution is in progress, as installing code is deferred for a canister + // with a paused or an aborted execution (see `can_execute_subnet_msg()`). if let Err(err) = helper.adjust_prepayment_for_response_execution(original, round) { let msg = format!("Failed to prepay for resuming a response call: {err}"); let err = HypervisorError::WasmEngineError(FailedToApplySystemChanges(msg)); @@ -735,15 +708,9 @@ impl ResponseHelper { /// Completes execution of the response and cleanup callbacks without /// consuming any instructions and without producing any heap delta. /// - /// No Wasm code is executed at all, hence the fixed per-message execution fee is - /// all that is due: it is charged here and the rest of the cycles prepaid for the - /// response execution is refunded, independently of the Wasm execution mode the - /// cycles were prepaid for and of the one the canister has now. In particular, - /// the canister keeps the rest of its prepayment even if the prepayment falls - /// short of what its current Wasm execution mode requires. - /// - /// Note that the prepayment is never topped up here: the additional cycles would - /// be refunded right away and, unlike this refund, the withdrawal could fail. + /// The prepayment for the response execution is settled here instead of being + /// adjusted, see + /// `CyclesAccountManager::settle_prepayment_for_unexecuted_response()`. fn early_finish( mut self, result: Result, HypervisorError>, @@ -760,8 +727,7 @@ impl ResponseHelper { original.subnet_cycles_config, execution_mode, ); - // The prepayment has been settled in full, hence there is nothing left for - // `finish()` to settle. + // Settled in full, so `finish()` has nothing left to refund. self.prepayment_for_response_execution = CompoundCycles::new(Cycles::zero(), original.subnet_cycles_config.cost_schedule); @@ -1150,10 +1116,9 @@ pub fn execute_response( } }; - // The cycles prepaid for this response execution when the corresponding call was - // performed might not match the cost of executing it in the canister's current - // Wasm execution mode. If the canister cannot pay a shortfall, then the response - // is rejected without executing the callback. + // An upgrade since the call was performed may have made the response execution + // more expensive; if the canister cannot pay the difference, the response is + // rejected without executing the callback. if let Err(err) = helper.adjust_prepayment_for_response_execution(&original, &round) { info!( round.log,