diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 13dcf1d5b587..010350565a06 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -1,5 +1,5 @@ use crate::as_round_instructions; -use crate::canister_settings::{CanisterSettings, ValidatedCanisterSettings}; +use crate::canister_settings::CanisterSettings; use crate::execution::common::{ validate_controller, validate_controller_or_subnet_admin, validate_snapshot_visibility, validate_subnet_admin, @@ -14,14 +14,13 @@ use crate::execution_environment_metrics::ExecutionEnvironmentMetrics; use crate::hypervisor::Hypervisor; use crate::types::{IngressResponse, Response}; use crate::util::{GOVERNANCE_CANISTER_ID, MIGRATION_CANISTER_ID}; -use ic_base_types::NumSeconds; use ic_config::embedders::Config as EmbeddersConfig; use ic_config::flag_status::FlagStatus; use ic_cycles_account_manager::{CyclesAccountManager, ResourceSaturation}; use ic_embedders::wasm_utils::decoding::decode_wasm; use ic_embedders::wasmtime_embedder::system_api::{ExecutionParameters, InstructionLimits}; use ic_error_types::{ErrorCode, RejectCode, UserError}; -use ic_interfaces::execution_environment::{MessageMemoryUsage, SubnetAvailableMemory}; +use ic_interfaces::execution_environment::SubnetAvailableMemory; use ic_limits::LOG_CANISTER_OPERATION_CYCLES_THRESHOLD; use ic_logger::{ReplicaLogger, error, fatal, info}; use ic_management_canister_types_private::{ @@ -58,9 +57,8 @@ use ic_types::messages::{ StopCanisterCallId, }; use ic_types::{ - CanisterId, CanisterTimer, ComputeAllocation, DEFAULT_AGGREGATE_LOG_MEMORY_LIMIT, - MAX_AGGREGATE_LOG_MEMORY_LIMIT, MemoryAllocation, NumBytes, NumInstructions, PrincipalId, - SnapshotId, Time, + CanisterId, CanisterTimer, DEFAULT_AGGREGATE_LOG_MEMORY_LIMIT, MAX_AGGREGATE_LOG_MEMORY_LIMIT, + NumBytes, NumInstructions, PrincipalId, SnapshotId, Time, }; use ic_types_cycles::{ CanisterCreation, CanisterCyclesCostSchedule, CompoundCycles, Cycles, CyclesUseCase, @@ -316,7 +314,7 @@ impl CanisterManager { Ok(()) } - /// Validates the new canisters settings: + /// Validates the new canister settings and applies them to the canister: /// - memory allocation: /// - it cannot be lower than the current canister memory usage. /// - there must be enough available subnet capacity for the change. @@ -335,96 +333,51 @@ impl CanisterManager { /// - must be at least the specified minimum. /// - must not exceed the specified maximum. /// - /// Keep this function in sync with `do_update_settings()`. + /// If `metrics` is `Some`, a `log_memory_limit` resize that does real + /// work (see `LogMemoryStore::would_resize`) is timed and recorded into + /// `canister_log_resize_duration`. Pass `None` to skip observation on + /// paths where the resize isn't the user-facing operation (e.g. first + /// allocation during canister creation). #[allow(clippy::too_many_arguments)] - fn validate_canister_settings( + fn validate_and_update_canister_settings( &self, settings: CanisterSettings, - canister_memory_usage: NumBytes, - canister_message_memory_usage: MessageMemoryUsage, - canister_memory_allocation: MemoryAllocation, - subnet_available_memory: &SubnetAvailableMemory, + canister: &mut CanisterState, + subnet_available_memory: &mut SubnetAvailableMemory, subnet_memory_saturation: &ResourceSaturation, - canister_compute_allocation: ComputeAllocation, subnet_compute_allocation_usage: u64, - canister_freezing_threshold: NumSeconds, - canister_cycles_balance: Cycles, subnet_size: usize, cost_schedule: CanisterCyclesCostSchedule, - canister_reserved_balance: Cycles, - canister_reserved_balance_limit: Option, - canister_log_bytes_used: NumBytes, log_resize_needed: bool, - ) -> Result { - self.validate_environment_variables(&settings)?; - - let old_memory_bytes = canister_memory_allocation.allocated_bytes(canister_memory_usage); - let new_memory_bytes = settings - .memory_allocation - .unwrap_or(canister_memory_allocation) - .allocated_bytes(canister_memory_usage); - - // If the available memory in the subnet is negative, then we must cap - // it at zero such that the new memory allocation can change between - // zero and the old memory allocation. Note that capping at zero also - // makes conversion from `i64` to `u64` valid. - let subnet_available_memory = subnet_available_memory.get_execution_memory().max(0) as u64; - let subnet_available_memory = - subnet_available_memory.saturating_add(old_memory_bytes.get()); - if new_memory_bytes.get() > subnet_available_memory { - return Err(CanisterManagerError::SubnetMemoryCapacityOverSubscribed { - requested: new_memory_bytes, - available: NumBytes::from(subnet_available_memory), - }); - } - - if let Some(new_compute_allocation) = settings.compute_allocation { - // The saturating `u64` subtractions ensure that the available compute - // capacity of the subnet never goes below zero. This means that even if - // compute capacity is oversubscribed, the new compute allocation can - // change between zero and the old compute allocation. - let available_compute_allocation = self - .config - .compute_capacity - .saturating_sub(subnet_compute_allocation_usage) - // Minus 1 below guarantees there is always at least 1% of free compute - // if the subnet was not already oversubscribed. - .saturating_sub(1) - .saturating_add(canister_compute_allocation.as_percent()); - if new_compute_allocation.as_percent() > available_compute_allocation { - return Err(CanisterManagerError::SubnetComputeCapacityOverSubscribed { - requested: new_compute_allocation, - available: available_compute_allocation, - }); - } - } - - let controllers = settings.controllers(); - if let Some(controllers) = &controllers - && controllers.len() > self.config.max_controllers - { - return Err(CanisterManagerError::InvalidSettings { - message: format!( - "Invalid settings: 'controllers' length exceeds maximum size allowed of {}.", - self.config.max_controllers - ), - }); - } - + metrics: Option<&ExecutionEnvironmentMetrics>, + ) -> Result<(), CanisterManagerError> { + // Resolve current canister state and effective new settings. The + // freeze-threshold cycles requirement (`threshold`) depends jointly on + // memory allocation, compute allocation, and freezing threshold, so we + // compute it once here and reuse it across all checks below. Storage + // cycle reservation (which would change the reserved balance and + // invalidate `threshold`) is deferred to the end of this function. + let canister_memory_usage = canister.memory_usage(); + let canister_message_memory_usage = canister.message_memory_usage(); + let canister_memory_allocation = canister.memory_allocation(); + let canister_compute_allocation = canister.compute_allocation(); + let canister_freezing_threshold = canister.system_state.freeze_threshold; + let canister_cycles_balance = canister.system_state.balance(); + let canister_reserved_balance = canister.system_state.reserved_balance(); + let canister_reserved_balance_limit = canister.system_state.reserved_balance_limit(); + let canister_log_bytes_used = + NumBytes::new(canister.system_state.log_memory_store.bytes_used() as u64); let new_memory_allocation = settings .memory_allocation .unwrap_or(canister_memory_allocation); - let new_compute_allocation = settings .compute_allocation() .unwrap_or(canister_compute_allocation); - - let freezing_threshold = settings + let new_freezing_threshold = settings .freezing_threshold .unwrap_or(canister_freezing_threshold); - let threshold = self.cycles_account_manager.freeze_threshold_cycles( - freezing_threshold, + new_freezing_threshold, new_memory_allocation, canister_memory_usage, canister_message_memory_usage, @@ -434,31 +387,86 @@ impl CanisterManager { canister_reserved_balance, ); - if canister_cycles_balance < threshold { - if new_compute_allocation > canister_compute_allocation { - // Note that the error is produced only if allocation increases. - // This is to allow increasing of the freezing threshold to make the - // canister frozen. - return Err( - CanisterManagerError::InsufficientCyclesInComputeAllocation { - compute_allocation: new_compute_allocation, - available: canister_cycles_balance, - threshold, - }, - ); - } - if new_memory_allocation > canister_memory_allocation { - // Note that the error is produced only if allocation increases. - // This is to allow increasing of the freezing threshold to make the - // canister frozen. - return Err(CanisterManagerError::InsufficientCyclesInMemoryAllocation { - memory_allocation: new_memory_allocation, - available: canister_cycles_balance, - threshold, - }); + // Environment variables: validate and apply. + self.validate_environment_variables(&settings)?; + if let Some(environment_variables) = settings.environment_variables() { + canister.system_state.environment_variables = environment_variables.clone(); + } + + // Controllers: validate count and apply. + let controllers = settings.controllers(); + if let Some(controllers) = &controllers + && controllers.len() > self.config.max_controllers + { + return Err(CanisterManagerError::InvalidSettings { + message: format!( + "Invalid settings: 'controllers' length exceeds maximum size allowed of {}.", + self.config.max_controllers + ), + }); + } + if let Some(controllers) = controllers { + canister.system_state.controllers.clear(); + for principal in controllers { + canister.system_state.controllers.insert(principal); } } + // Reserved cycles limit: validate and apply before the deferred + // reserve_cycles call at the end so that it respects the new limit. + let reserved_balance_limit = settings + .reserved_cycles_limit() + .or(canister_reserved_balance_limit); + if let Some(limit) = reserved_balance_limit + && canister_reserved_balance > limit + { + return Err(CanisterManagerError::ReservedCyclesLimitIsTooLow { + cycles: canister_reserved_balance, + limit, + }); + } + if let Some(limit) = settings.reserved_cycles_limit() { + canister.system_state.set_reserved_balance_limit(limit); + } + + // Memory allocation: validate subnet capacity and cycle balance, and apply. + let old_memory_bytes = canister_memory_allocation.allocated_bytes(canister_memory_usage); + let new_memory_bytes = new_memory_allocation.allocated_bytes(canister_memory_usage); + if new_memory_bytes >= old_memory_bytes { + let available = NumBytes::from( + (subnet_available_memory.get_execution_memory().max(0) as u64) + .saturating_add(old_memory_bytes.get()), + ); + subnet_available_memory + .try_decrement( + new_memory_bytes - old_memory_bytes, + NumBytes::from(0), + NumBytes::from(0), + ) + .map_err( + |_| CanisterManagerError::SubnetMemoryCapacityOverSubscribed { + requested: new_memory_bytes, + available, + }, + )?; + } else { + subnet_available_memory.increment( + old_memory_bytes - new_memory_bytes, + NumBytes::from(0), + NumBytes::from(0), + ); + } + // Note that the error is produced only if allocation increases. + // This is to allow increasing of the freezing threshold to make the + // canister frozen. + if canister_cycles_balance < threshold && new_memory_allocation > canister_memory_allocation + { + return Err(CanisterManagerError::InsufficientCyclesInMemoryAllocation { + memory_allocation: new_memory_allocation, + available: canister_cycles_balance, + threshold, + }); + } let allocated_bytes = new_memory_bytes.saturating_sub(&old_memory_bytes); let reservation_cycles = self .cycles_account_manager @@ -469,39 +477,55 @@ impl CanisterManager { cost_schedule, ) .real(); - let reserved_balance_limit = settings - .reserved_cycles_limit() - .or(canister_reserved_balance_limit); + if let Some(memory_allocation) = settings.memory_allocation() { + canister.system_state.memory_allocation = memory_allocation; + } - if let Some(limit) = reserved_balance_limit { - if canister_reserved_balance > limit { - return Err(CanisterManagerError::ReservedCyclesLimitIsTooLow { - cycles: canister_reserved_balance, - limit, + // Compute allocation: validate subnet capacity and cycle balance, and apply. + if let Some(new_compute_allocation) = settings.compute_allocation { + // The saturating `u64` subtractions ensure that the available compute + // capacity of the subnet never goes below zero. This means that even if + // compute capacity is oversubscribed, the new compute allocation can + // change between zero and the old compute allocation. + let available_compute_allocation = self + .config + .compute_capacity + .saturating_sub(subnet_compute_allocation_usage) + // Minus 1 below guarantees there is always at least 1% of free compute + // if the subnet was not already oversubscribed. + .saturating_sub(1) + .saturating_add(canister_compute_allocation.as_percent()); + if new_compute_allocation.as_percent() > available_compute_allocation { + return Err(CanisterManagerError::SubnetComputeCapacityOverSubscribed { + requested: new_compute_allocation, + available: available_compute_allocation, }); - } else if canister_reserved_balance + reservation_cycles > limit { - return Err( - CanisterManagerError::ReservedCyclesLimitExceededInMemoryAllocation { - memory_allocation: new_memory_allocation, - requested: canister_reserved_balance + reservation_cycles, - limit, - }, - ); } } + // Note that the error is produced only if allocation increases. + // This is to allow increasing of the freezing threshold to make the + // canister frozen. + if canister_cycles_balance < threshold + && new_compute_allocation > canister_compute_allocation + { + return Err( + CanisterManagerError::InsufficientCyclesInComputeAllocation { + compute_allocation: new_compute_allocation, + available: canister_cycles_balance, + threshold, + }, + ); + } + if let Some(compute_allocation) = settings.compute_allocation() { + canister.system_state.compute_allocation = compute_allocation; + } - // Note that this check does not include the freezing threshold to be - // consistent with the `reserve_cycles()` function, which moves - // cycles between the main and reserved balances without checking - // the freezing threshold. - if canister_cycles_balance < reservation_cycles { - return Err(CanisterManagerError::InsufficientCyclesInMemoryAllocation { - memory_allocation: new_memory_allocation, - available: canister_cycles_balance, - threshold: reservation_cycles, - }); + // Freezing threshold: apply. + if let Some(freezing_threshold) = settings.freezing_threshold() { + canister.system_state.freeze_threshold = freezing_threshold; } + // Log memory limit: validate size, charge resize cost, and apply. let log_memory_limit = if let Some(requested_limit) = settings.log_memory_limit() { // User explicitly sets log_memory_limit: validate the limit // and check the canister can afford the resize cost. @@ -520,124 +544,33 @@ impl CanisterManager { if log_resize_needed { let log_resize_instructions = NumInstructions::new(canister_log_bytes_used.get() * LOG_RESIZE_COST_PER_BYTE); - let log_resize_cycles = self - .cycles_account_manager - .management_canister_cost(log_resize_instructions, subnet_size, cost_schedule) - .real(); - if canister_cycles_balance < reservation_cycles + threshold + log_resize_cycles { - return Err(CanisterManagerError::LogResizeNotEnoughCycles { - available: canister_cycles_balance, - threshold: reservation_cycles + threshold, - requested: log_resize_cycles, - }); - } + let log_resize_cycles = self.cycles_account_manager.management_canister_cost( + log_resize_instructions, + subnet_size, + cost_schedule, + ); + self.cycles_account_manager + .consume_with_threshold( + &mut canister.system_state, + log_resize_cycles, + threshold, + true, // The caller is always a controller. + ) + .map_err(|err| CanisterManagerError::LogResizeNotEnoughCycles { + available: err.available, + threshold: err.threshold, + requested: err.requested, + })?; } Some(requested_limit) } else { // User did not set log_memory_limit: default so that new canisters - // get a log memory store initialized via do_update_settings. + // get a log memory store initialized. // For existing canisters this is a no-op (resize early-returns // when capacity is unchanged). No cycles are charged. Some(NumBytes::new(DEFAULT_AGGREGATE_LOG_MEMORY_LIMIT as u64)) }; - - Ok(ValidatedCanisterSettings::new( - settings.controllers(), - settings.compute_allocation(), - settings.memory_allocation(), - settings.wasm_memory_threshold(), - settings.freezing_threshold(), - settings.reserved_cycles_limit(), - reservation_cycles, - settings.log_visibility().cloned(), - settings.snapshot_visibility().cloned(), - log_memory_limit, - settings.wasm_memory_limit(), - settings.environment_variables().cloned(), - )) - } - - fn validate_settings_for_canister_creation( - &self, - settings: CanisterSettings, - subnet_compute_allocation_usage: u64, - subnet_available_memory: &SubnetAvailableMemory, - subnet_memory_saturation: &ResourceSaturation, - canister_cycles_balance: Cycles, - subnet_size: usize, - cost_schedule: CanisterCyclesCostSchedule, - ) -> Result { - self.validate_canister_settings( - settings, - NumBytes::new(0), - MessageMemoryUsage::ZERO, - MemoryAllocation::default(), - subnet_available_memory, - subnet_memory_saturation, - ComputeAllocation::zero(), - subnet_compute_allocation_usage, - self.config.default_freeze_threshold, - canister_cycles_balance, - subnet_size, - cost_schedule, - Cycles::zero(), - None, - NumBytes::new(0), - true, // New canister: resize always needed. - ) - } - - /// Applies the requested settings on the canister. - /// Note: Called only after validating the settings. - /// Keep this function in sync with `validate_canister_settings()`. - /// - /// If `metrics` is `Some`, a `log_memory_limit` resize that does real - /// work (see `LogMemoryStore::would_resize`) is timed and recorded into - /// `canister_log_resize_duration`. Pass `None` to skip observation on - /// paths where the resize isn't the user-facing operation (e.g. first - /// allocation during canister creation). - fn do_update_settings( - &self, - settings: &ValidatedCanisterSettings, - canister: &mut CanisterState, - metrics: Option<&ExecutionEnvironmentMetrics>, - ) { - // Note: At this point, the settings are validated. - if let Some(controllers) = settings.controllers() { - canister.system_state.controllers.clear(); - for principal in controllers { - canister.system_state.controllers.insert(principal); - } - } - if let Some(compute_allocation) = settings.compute_allocation() { - canister.system_state.compute_allocation = compute_allocation; - } - if let Some(memory_allocation) = settings.memory_allocation() { - canister.system_state.memory_allocation = memory_allocation; - } - if let Some(wasm_memory_threshold) = settings.wasm_memory_threshold() { - canister.system_state.wasm_memory_threshold = wasm_memory_threshold; - } - if let Some(limit) = settings.reserved_cycles_limit() { - canister.system_state.set_reserved_balance_limit(limit); - } - canister - .system_state - .reserve_cycles(settings.reservation_cycles()) - .expect( - "Reserving cycles should succeed because \ - the canister settings have been validated.", - ); - if let Some(freezing_threshold) = settings.freezing_threshold() { - canister.system_state.freeze_threshold = freezing_threshold; - } - if let Some(log_visibility) = settings.log_visibility() { - canister.system_state.log_visibility = log_visibility.clone(); - } - if let Some(snapshot_visibility) = settings.snapshot_visibility() { - canister.system_state.snapshot_visibility = snapshot_visibility.clone(); - } - if let Some(log_memory_limit) = settings.log_memory_limit() { + if let Some(log_memory_limit) = log_memory_limit { let limit = log_memory_limit.get() as usize; let log_memory_store = &mut canister.system_state.log_memory_store; { @@ -647,12 +580,51 @@ impl CanisterManager { log_memory_store.resize(limit, self.fd_factory.clone()); } } + + // Log visibility: apply. + if let Some(log_visibility) = settings.log_visibility() { + canister.system_state.log_visibility = log_visibility.clone(); + } + + // Snapshot visibility: apply. + if let Some(snapshot_visibility) = settings.snapshot_visibility() { + canister.system_state.snapshot_visibility = snapshot_visibility.clone(); + } + + // Wasm memory threshold: apply. + if let Some(wasm_memory_threshold) = settings.wasm_memory_threshold() { + canister.system_state.wasm_memory_threshold = wasm_memory_threshold; + } + + // Wasm memory limit: apply. if let Some(wasm_memory_limit) = settings.wasm_memory_limit() { canister.system_state.wasm_memory_limit = Some(wasm_memory_limit); } - if let Some(environment_variables) = settings.environment_variables() { - canister.system_state.environment_variables = environment_variables.clone(); - } + + // Memory allocation: reserve storage cycles now that all checks have + // passed, so that `threshold` above remained valid throughout. + canister + .system_state + .reserve_cycles(reservation_cycles) + .map_err(|err| match err { + ReservationError::InsufficientCycles { + requested, + available, + } => CanisterManagerError::InsufficientCyclesInMemoryAllocation { + memory_allocation: new_memory_allocation, + available, + threshold: requested, + }, + ReservationError::ReservedLimitExceed { requested, limit } => { + CanisterManagerError::ReservedCyclesLimitExceededInMemoryAllocation { + memory_allocation: new_memory_allocation, + requested, + limit, + } + } + })?; + + Ok(()) } /// Tries to apply the requested settings on the canister identified by @@ -683,32 +655,23 @@ impl CanisterManager { .would_resize(limit.get() as usize) }) .unwrap_or(false); - let validated_settings = self.validate_canister_settings( + let controllers_changed = settings.controllers().is_some(); + + let old_compute_allocation = canister.compute_allocation().as_percent(); + let old_log_bytes_used = canister.system_state.log_memory_store.bytes_used() as u64; + + self.validate_and_update_canister_settings( settings, - canister.memory_usage(), - canister.message_memory_usage(), - canister.memory_allocation(), - &round_limits.subnet_available_memory, + canister, + &mut round_limits.subnet_available_memory, &subnet_memory_saturation, - canister.compute_allocation(), round_limits.compute_allocation_used, - canister.system_state.freeze_threshold, - canister.system_state.balance(), subnet_size, cost_schedule, - canister.system_state.reserved_balance(), - canister.system_state.reserved_balance_limit(), - NumBytes::new(canister.system_state.log_memory_store.bytes_used() as u64), log_resize_needed, + Some(metrics), )?; - let old_usage = canister.memory_usage(); - let old_mem = canister.memory_allocation().allocated_bytes(old_usage); - let old_compute_allocation = canister.compute_allocation().as_percent(); - let old_log_bytes_used = canister.system_state.log_memory_store.bytes_used() as u64; - - self.do_update_settings(&validated_settings, canister, Some(metrics)); - let new_compute_allocation = canister.compute_allocation().as_percent(); if old_compute_allocation < new_compute_allocation { round_limits.compute_allocation_used = round_limits @@ -720,58 +683,21 @@ impl CanisterManager { .saturating_sub(old_compute_allocation - new_compute_allocation); } - let new_usage = canister.memory_usage(); - let new_mem = canister.memory_allocation().allocated_bytes(new_usage); - if new_mem >= old_mem { - // Settings were validated before so this should always succeed. - round_limits - .subnet_available_memory - .try_decrement(new_mem - old_mem, NumBytes::from(0), NumBytes::from(0)) - .expect("Error: Cannot fail to decrement SubnetAvailableMemory after validating the canister's settings"); - } else { - round_limits.subnet_available_memory.increment( - old_mem - new_mem, - NumBytes::from(0), - NumBytes::from(0), - ); - } - - // Deduct cycles and account instructions for log resize. - // Validation was done in validate_canister_settings; this is the actual deduction. - // Use consume_with_threshold with zero threshold instead of - // consume_cycles_for_management_canister_instructions to avoid - // recomputing the freezing threshold from post-mutation memory usage, - // which could fail despite validation having passed. + // Account instructions for log resize. + // Use pre-mutation bytes_used so that downsize operations (which drop + // old records) are charged for the actual work of reading/rewriting + // the original buffer, matching what validation checked. if log_resize_needed { - // Use pre-mutation bytes_used so that downsize operations (which drop - // old records) are charged for the actual work of reading/rewriting - // the original buffer, matching what validation checked. let log_resize_instructions = NumInstructions::new(old_log_bytes_used * LOG_RESIZE_COST_PER_BYTE); - let log_resize_cycles = self.cycles_account_manager.management_canister_cost( - log_resize_instructions, - subnet_size, - cost_schedule, - ); - let reveal_top_up = canister.system_state.controllers.contains(&sender); - self.cycles_account_manager - .consume_with_threshold( - &mut canister.system_state, - log_resize_cycles, - Cycles::zero(), - reveal_top_up, - ) - .expect( - "Consuming cycles for log resize should succeed because \ - the canister settings have been validated.", - ); round_limits.instructions -= as_round_instructions(log_resize_instructions); } canister.system_state.bump_canister_version(); - let new_controllers = match validated_settings.controllers() { - Some(_) => Some(canister.system_state.controllers.iter().copied().collect()), - None => None, + let new_controllers = if controllers_changed { + Some(canister.system_state.controllers.iter().copied().collect()) + } else { + None }; // For the sake of backward-compatibility, we do not record @@ -904,41 +830,29 @@ impl CanisterManager { .wasm_memory_limit .get_or_insert(self.config.default_wasm_memory_limit); - // Validate settings before `create_canister_helper` applies them - match self.validate_settings_for_canister_creation( + // Test coverage relies on the fact that + // the IC method `provisional_create_canister_with_cycles` + // implemented by `CanisterManager::create_canister_with_cycles` + // uses the same code (in `CanisterManager::create_canister_helper`) + // as the production IC method `create_canister` + // implemented by this function `CanisterManager::create_canister`. + let canister_id = match self.create_canister_helper( + origin, + cycles, + fee, settings, - round_limits.compute_allocation_used, - &round_limits.subnet_available_memory, - &subnet_memory_saturation, - cycles - fee.real(), + max_number_of_canisters, + state, + round_limits, + None, + subnet_memory_saturation, subnet_size, - state.get_own_cost_schedule(), + canister_creation_error, ) { - Err(err) => (Err(err.into()), cycles), - Ok(validate_settings) => { - // Test coverage relies on the fact that - // the IC method `provisional_create_canister_with_cycles` - // implemented by `CanisterManager::create_canister_with_cycles` - // uses the same code (in `CanisterManager::create_canister_helper`) - // as the production IC method `create_canister` - // implemented by this function `CanisterManager::create_canister`. - let canister_id = match self.create_canister_helper( - origin, - cycles, - fee, - validate_settings, - max_number_of_canisters, - state, - round_limits, - None, - canister_creation_error, - ) { - Ok(canister_id) => canister_id, - Err(err) => return (Err(err.into()), cycles), - }; - (Ok(canister_id), Cycles::zero()) - } - } + Ok(canister_id) => canister_id, + Err(err) => return (Err(err.into()), cycles), + }; + (Ok(canister_id), Cycles::zero()) } /// Installs code to a canister. @@ -1456,37 +1370,25 @@ impl CanisterManager { .wasm_memory_limit .get_or_insert(self.config.default_wasm_memory_limit); - // Validate settings before `create_canister_helper` applies them - // No creation fee applied. - // // Test coverage relies on the fact that // the IC method `provisional_create_canister_with_cycles` // implemented by this function `CanisterManager::create_canister_with_cycles` // uses the same code (in `CanisterManager::create_canister_helper`) // as the production IC method `create_canister` // implemented by `CanisterManager::create_canister`. - match self.validate_settings_for_canister_creation( - settings, - round_limits.compute_allocation_used, - &round_limits.subnet_available_memory, - &subnet_memory_saturation, + self.create_canister_helper( + origin, cycles, + CompoundCycles::new(Cycles::zero(), state.get_own_cost_schedule()), + settings, + max_number_of_canisters, + state, + round_limits, + specified_id, + subnet_memory_saturation, subnet_size, - state.get_own_cost_schedule(), - ) { - Err(err) => Err(err), - Ok(validated_settings) => self.create_canister_helper( - origin, - cycles, - CompoundCycles::new(Cycles::zero(), state.get_own_cost_schedule()), - validated_settings, - max_number_of_canisters, - state, - round_limits, - specified_id, - canister_creation_error, - ), - } + canister_creation_error, + ) } /// Validates specified ID is available for use. @@ -1496,7 +1398,7 @@ impl CanisterManager { /// Returns `Err` iff the `specified_id` is not valid. fn validate_specified_id( &self, - state: &mut ReplicatedState, + state: &ReplicatedState, specified_id: PrincipalId, ) -> Result { let new_canister_id = CanisterId::unchecked_from_principal(specified_id); @@ -1529,11 +1431,13 @@ impl CanisterManager { origin: CanisterChangeOrigin, cycles: Cycles, creation_fee: CompoundCycles, - settings: ValidatedCanisterSettings, + settings: CanisterSettings, max_number_of_canisters: u64, state: &mut ReplicatedState, round_limits: &mut RoundLimits, specified_id: Option, + subnet_memory_saturation: ResourceSaturation, + subnet_size: usize, canister_creation_error: &IntCounter, ) -> Result { let sender = origin.origin(); @@ -1550,9 +1454,14 @@ impl CanisterManager { let new_canister_id = match specified_id { Some(spec_id) => self.validate_specified_id(state, spec_id)?, - None => self.generate_new_canister_id(state, canister_creation_error)?, + None => self.peek_new_canister_id(state, canister_creation_error)?, }; + // Capture environment variables hash before settings is consumed. + let environment_variables_hash = settings + .environment_variables() + .map(|env_vars| env_vars.hash()); + // Canister id available. Create the new canister. let mut system_state = SystemState::new_running( new_canister_id, @@ -1574,18 +1483,21 @@ impl CanisterManager { // Canister creation's first-time allocation is a different event class // from user-triggered `log_memory_limit` resize — don't mix their // distributions. Pass `None` to skip observation here. - self.do_update_settings(&settings, &mut new_canister, None); - let new_usage = new_canister.memory_usage(); - let new_mem = new_canister - .system_state - .memory_allocation - .allocated_bytes(new_usage); - - // settings were validated before so this should always succeed - round_limits - .subnet_available_memory - .try_decrement(new_mem, NumBytes::from(0), NumBytes::from(0)) - .expect("Error: Cannot fail to decrement SubnetAvailableMemory after validating canister's settings"); + let round_limits_snapshot = round_limits.clone(); + if let Err(err) = self.validate_and_update_canister_settings( + settings, + &mut new_canister, + &mut round_limits.subnet_available_memory, + &subnet_memory_saturation, + round_limits.compute_allocation_used, + subnet_size, + state.get_own_cost_schedule(), + true, // New canister: resize always needed. + None, + ) { + *round_limits = round_limits_snapshot; + return Err(err); + } round_limits.compute_allocation_used = round_limits .compute_allocation_used @@ -1597,10 +1509,6 @@ impl CanisterManager { .iter() .copied() .collect(); - - let environment_variables_hash = settings - .environment_variables() - .map(|env_vars| env_vars.hash()); let available_execution_memory_change = new_canister.add_canister_change( state.time(), origin, @@ -1610,6 +1518,14 @@ impl CanisterManager { .subnet_available_memory .update_execution_memory_unchecked(available_execution_memory_change); + // Commit the canister ID now that settings validation has succeeded. + // For specified IDs the allocation counter is not involved. For + // auto-generated IDs, `peek_new_canister_id` did not mutate state, so + // we advance `last_generated_canister_id` only here. + if specified_id.is_none() { + state.metadata.commit_new_canister_id(new_canister_id); + } + // Add new canister to the replicated state. state.put_canister_state(new_canister); @@ -1698,21 +1614,23 @@ impl CanisterManager { Ok(()) } - /// Generates a new canister ID. + /// Returns the next canister ID that would be generated, without mutating + /// state. Returns `Err` if the subnet can generate no more canister IDs, + /// or if a canister with the peeked ID already exists. /// - /// Returns `Err` if the subnet can generate no more canister IDs; or a canister - /// with the newly generated ID already exists. + /// The caller must follow up with `state.metadata.commit_new_canister_id` + /// once canister creation succeeds. // // WARNING!!! If you change the logic here, please ensure that the sequence // of NNS canister ids as defined in nns/constants/src/lib.rs are also // updated. - fn generate_new_canister_id( + fn peek_new_canister_id( &self, - state: &mut ReplicatedState, + state: &ReplicatedState, canister_creation_error: &IntCounter, ) -> Result { - let canister_id = state.metadata.generate_new_canister_id().map_err(|err| { - error!(self.log, "Unable to generate new canister IDs: {}", err); + let canister_id = state.metadata.peek_new_canister_id().map_err(|err| { + error!(self.log, "Unable to peek new canister ID: {}", err); CanisterManagerError::SubnetOutOfCanisterIds })?; diff --git a/rs/execution_environment/src/canister_manager/tests.rs b/rs/execution_environment/src/canister_manager/tests.rs index 5006941b6e52..f6692167865e 100644 --- a/rs/execution_environment/src/canister_manager/tests.rs +++ b/rs/execution_environment/src/canister_manager/tests.rs @@ -7805,6 +7805,34 @@ fn create_canister_reserves_cycles_for_memory_allocation() { }); } +#[test] +fn create_canister_reverts_subnet_available_memory_on_failure() { + // subnet_available_memory is decremented by the memory allocation step in + // validate_and_update_canister_settings before the cycles check runs. + // A huge freezing threshold makes the required cycles exceed the balance, + // so the cycles check fails after subnet_available_memory was already changed. + // The caller must revert subnet_available_memory from the round_limits snapshot. + let mut test = ExecutionTestBuilder::new().build(); + + let initial_subnet_available_memory = test.subnet_available_memory().get_execution_memory(); + + let err = test + .create_canister_with_settings( + Cycles::new(1_000_000_000_000), + CanisterSettingsArgsBuilder::new() + .with_memory_allocation(MIB) + .with_freezing_threshold(u64::MAX) + .build(), + ) + .unwrap_err(); + + assert_eq!(err.code(), ErrorCode::InsufficientCyclesInMemoryAllocation); + assert_eq!( + test.subnet_available_memory().get_execution_memory(), + initial_subnet_available_memory, + ); +} + #[test] fn create_canister_fails_with_reserved_cycles_limit_exceeded() { const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); diff --git a/rs/execution_environment/src/canister_settings.rs b/rs/execution_environment/src/canister_settings.rs index d4430b0b69c7..dee881c722b8 100644 --- a/rs/execution_environment/src/canister_settings.rs +++ b/rs/execution_environment/src/canister_settings.rs @@ -415,101 +415,6 @@ impl From for UpdateSettingsError { } } -pub(crate) struct ValidatedCanisterSettings { - controllers: Option>, - compute_allocation: Option, - memory_allocation: Option, - wasm_memory_threshold: Option, - freezing_threshold: Option, - reserved_cycles_limit: Option, - reservation_cycles: Cycles, - log_visibility: Option, - snapshot_visibility: Option, - log_memory_limit: Option, - wasm_memory_limit: Option, - environment_variables: Option, -} - -impl ValidatedCanisterSettings { - pub fn new( - controllers: Option>, - compute_allocation: Option, - memory_allocation: Option, - wasm_memory_threshold: Option, - freezing_threshold: Option, - reserved_cycles_limit: Option, - reservation_cycles: Cycles, - log_visibility: Option, - snapshot_visibility: Option, - log_memory_limit: Option, - wasm_memory_limit: Option, - environment_variables: Option, - ) -> Self { - Self { - controllers, - compute_allocation, - memory_allocation, - wasm_memory_threshold, - freezing_threshold, - reserved_cycles_limit, - reservation_cycles, - log_visibility, - snapshot_visibility, - log_memory_limit, - wasm_memory_limit, - environment_variables, - } - } - - pub fn controllers(&self) -> Option> { - self.controllers.clone() - } - - pub fn compute_allocation(&self) -> Option { - self.compute_allocation - } - - pub fn memory_allocation(&self) -> Option { - self.memory_allocation - } - - pub fn wasm_memory_threshold(&self) -> Option { - self.wasm_memory_threshold - } - - pub fn freezing_threshold(&self) -> Option { - self.freezing_threshold - } - - pub fn reserved_cycles_limit(&self) -> Option { - self.reserved_cycles_limit - } - - pub fn reservation_cycles(&self) -> Cycles { - self.reservation_cycles - } - - pub fn log_visibility(&self) -> Option<&LogVisibilityV2> { - self.log_visibility.as_ref() - } - - pub fn snapshot_visibility(&self) -> Option<&SnapshotVisibility> { - self.snapshot_visibility.as_ref() - } - - pub fn log_memory_limit(&self) -> Option { - self.log_memory_limit - } - - pub fn wasm_memory_limit(&self) -> Option { - self.wasm_memory_limit - } - - pub fn environment_variables(&self) -> Option<&EnvironmentVariables> { - self.environment_variables.as_ref() - } -} - #[derive(Clone, Eq, PartialEq, Debug)] pub(crate) enum VisibilitySettings<'a> { Public, diff --git a/rs/execution_environment/tests/canister_settings.rs b/rs/execution_environment/tests/canister_settings.rs index 9b1ccf557a46..fcb62caf0f3e 100644 --- a/rs/execution_environment/tests/canister_settings.rs +++ b/rs/execution_environment/tests/canister_settings.rs @@ -1,12 +1,15 @@ +use assert_matches::assert_matches; use candid::Nat; use ic_error_types::ErrorCode; use ic_management_canister_types_private::{ CanisterIdRecord, CanisterSettingsArgs, CanisterSettingsArgsBuilder, CreateCanisterArgs, DefiniteCanisterSettingsArgs, IC_00, Method, Payload, UpdateSettingsArgs, }; -use ic_state_machine_tests::StateMachine; +use ic_registry_subnet_type::SubnetType; +use ic_state_machine_tests::{StateMachine, StateMachineBuilder}; use ic_test_utilities::universal_canister::{CallArgs, UNIVERSAL_CANISTER_WASM, wasm}; use ic_test_utilities_execution_environment::get_reply; +use ic_types::CanisterId; use ic_types::NumBytes; use ic_types::ingress::WasmResult; use ic_types_cycles::Cycles; @@ -201,3 +204,78 @@ fn canister_settings_ranges() { assert!(err.contains(&expected_err)); } } + +#[test] +fn failed_create_canister_does_not_reuse_canister_id() { + let env = StateMachineBuilder::new() + .with_subnet_type(SubnetType::Application) + .build(); + // Proxy universal canister gets ID 0. + let proxy_canister_id = env + .install_canister_with_cycles( + UNIVERSAL_CANISTER_WASM.to_vec(), + vec![], + None, + Cycles::new(100_000_000_000_000), + ) + .unwrap(); + + // On an application subnet, a freezing threshold of 2^60 seconds combined + // with a 1 MiB memory allocation requires an astronomically large cycle + // balance, causing the create_canister call to fail with + // InsufficientCyclesInMemoryAllocation during settings validation — before + // a canister ID is ever generated. + let failing_args = CreateCanisterArgs { + settings: Some( + CanisterSettingsArgsBuilder::new() + .with_freezing_threshold(1u64 << 60) + .with_memory_allocation(1 << 20) + .build(), + ), + sender_canister_version: None, + }; + // Send enough cycles to pass the creation-fee check so the calls reach + // the freeze-threshold validation (where they are actually rejected). + let cycles_with_call: u64 = 1_000_000_000_000; + + // Each call is rejected during validation, so no canister ID is consumed. + for _ in 0..3 { + let call_args = CallArgs::default() + .other_side(failing_args.encode()) + .on_reject(wasm().reject_message().reject().build()); + let res = env + .execute_ingress( + proxy_canister_id, + "update", + wasm() + .call_with_cycles(IC_00, Method::CreateCanister, call_args, cycles_with_call) + .build(), + ) + .unwrap(); + assert_matches!(res, WasmResult::Reject(_)); + } + + // Because the failures above consumed no IDs, the next successful call + // still gets ID 1 (one after the proxy at ID 0). + let success_args = CreateCanisterArgs { + settings: None, + sender_canister_version: None, + }; + let call_args = CallArgs::default() + .other_side(success_args.encode()) + .on_reject(wasm().reject_message().reject().build()); + let res = env + .execute_ingress( + proxy_canister_id, + "update", + wasm() + .call_with_cycles(IC_00, Method::CreateCanister, call_args, cycles_with_call) + .build(), + ) + .unwrap(); + let canister_id = match res { + WasmResult::Reply(bytes) => CanisterIdRecord::decode(&bytes).unwrap().get_canister_id(), + WasmResult::Reject(msg) => panic!("Unexpected reject: {}", msg), + }; + assert_eq!(canister_id, CanisterId::from_u64(1)); +} diff --git a/rs/replicated_state/src/metadata_state.rs b/rs/replicated_state/src/metadata_state.rs index da789d137ca6..b8a98f2c6ee2 100644 --- a/rs/replicated_state/src/metadata_state.rs +++ b/rs/replicated_state/src/metadata_state.rs @@ -595,8 +595,9 @@ impl SystemMetadata { /// If a canister ID from a second canister allocation range is generated, the /// first range is dropped. The last canister allocation range is never dropped. /// - /// Returns `Err` iff no more canister IDs can be generated. - pub fn generate_new_canister_id(&mut self) -> Result { + /// Returns the next canister ID that would be generated, without mutating + /// state. Returns `Err` iff no more canister IDs can be generated. + pub fn peek_new_canister_id(&self) -> Result { // Start off with // (canister_allocation_ranges // ∩ routing_table.ranges(own_subnet_id)) @@ -626,26 +627,38 @@ impl SystemMetadata { ) })?; - let res = canister_allocation_ranges.generate_canister_id(self.last_generated_canister_id); - - if let Some(res) = &res { - self.last_generated_canister_id = Some(*res); - - while self.canister_allocation_ranges.len() > 1 - && !self - .canister_allocation_ranges - .iter() - .next() - .unwrap() - .contains(res) - { - // Drop the first canister allocation range iff consumed and more allocation - // ranges are available. - self.canister_allocation_ranges.drop_first(); - } + canister_allocation_ranges + .generate_canister_id(self.last_generated_canister_id) + .ok_or_else(|| "Canister ID allocation was consumed".into()) + } + + /// Records `canister_id` as the last generated canister ID and drops any + /// exhausted allocation ranges. Must be called with the ID returned by the + /// immediately preceding `peek_new_canister_id` call, once canister creation + /// has succeeded and the ID should be permanently consumed. + pub fn commit_new_canister_id(&mut self, canister_id: CanisterId) { + self.last_generated_canister_id = Some(canister_id); + + while self.canister_allocation_ranges.len() > 1 + && !self + .canister_allocation_ranges + .iter() + .next() + .unwrap() + .contains(&canister_id) + { + // Drop the first canister allocation range iff consumed and more allocation + // ranges are available. + self.canister_allocation_ranges.drop_first(); } + } - res.ok_or_else(|| "Canister ID allocation was consumed".into()) + /// Generates and immediately commits the next canister ID. + /// Returns `Err` iff no more canister IDs can be generated. + pub fn generate_new_canister_id(&mut self) -> Result { + let canister_id = self.peek_new_canister_id()?; + self.commit_new_canister_id(canister_id); + Ok(canister_id) } /// Returns the number of canister IDs that can still be generated.