diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 13dcf1d5b587..3cb87e6a1c33 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -1496,7 +1496,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); @@ -1550,7 +1550,7 @@ 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)?, }; // Canister id available. Create the new canister. @@ -1610,6 +1610,10 @@ impl CanisterManager { .subnet_available_memory .update_execution_memory_unchecked(available_execution_memory_change); + 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 +1702,25 @@ impl CanisterManager { Ok(()) } - /// Generates a new canister ID. + /// Returns the next canister ID that is available for canister creation, + /// without mutating state. /// - /// Returns `Err` if the subnet can generate no more canister IDs; or a canister - /// with the newly generated ID already exists. + /// Returns `Err` if the subnet can generate no more canister IDs, + /// or if a canister with the next canister 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/tests/canister_settings.rs b/rs/execution_environment/tests/canister_settings.rs index 9b1ccf557a46..816041bf0812 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,76 @@ 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. + let failing_args = CreateCanisterArgs { + settings: Some( + CanisterSettingsArgsBuilder::new() + .with_freezing_threshold(1_u64 << 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 freezing 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/nns/constants/src/lib.rs b/rs/nns/constants/src/lib.rs index c703ab6aff96..e97445e4377e 100644 --- a/rs/nns/constants/src/lib.rs +++ b/rs/nns/constants/src/lib.rs @@ -11,7 +11,7 @@ use maplit::btreemap; // NOTES (IMPORTANT!) // ~~~~~~~~~~~~~~~~~~ // - This is dependent on the implementation of function -// `CanisterManager::generate_new_canister_id`. +// `CanisterManager::peek_new_canister_id`. // - Unless you only add at the end, be sure to double check with // `rs/nns/canister_ids.json`. TODO: Write a test that enforces // that this file matches the .json file diff --git a/rs/registry/routing_table/src/lib.rs b/rs/registry/routing_table/src/lib.rs index 1e6316e28eb2..0208b2e94ec8 100644 --- a/rs/registry/routing_table/src/lib.rs +++ b/rs/registry/routing_table/src/lib.rs @@ -55,10 +55,7 @@ impl CanisterIdRange { /// * `None` if `previous_canister_id >= self.end` /// or the entire range of 64 bit integers is exhausted. /// * `previous_canister_id + 1` otherwise. - pub fn generate_canister_id( - &self, - previous_canister_id: Option, - ) -> Option { + pub fn next_canister_id(&self, previous_canister_id: Option) -> Option { let previous_canister_id = match previous_canister_id { Some(previous_canister_id) => previous_canister_id, @@ -215,17 +212,14 @@ impl CanisterIdRanges { self.0.last().map(|range| range.end) } - /// Generates the next canister ID after the (provided) previously generated - /// canister ID, if any is available. + /// Returns the next canister ID after `previous_canister_id`, if any is + /// available. /// /// Returns `None` if no more canister IDs can be generated. - pub fn generate_canister_id( - &self, - previous_canister_id: Option, - ) -> Option { + pub fn next_canister_id(&self, previous_canister_id: Option) -> Option { self.0 .iter() - .flat_map(|range| range.generate_canister_id(previous_canister_id)) + .flat_map(|range| range.next_canister_id(previous_canister_id)) .next() } diff --git a/rs/registry/routing_table/src/tests.rs b/rs/registry/routing_table/src/tests.rs index e5115ff11196..b519f1a18893 100644 --- a/rs/registry/routing_table/src/tests.rs +++ b/rs/registry/routing_table/src/tests.rs @@ -68,25 +68,25 @@ fn try_convert_string_into_canister_id_range() { } #[test] -fn canister_id_range_generate_canister_id() { +fn canister_id_range_next_canister_id() { let range = CanisterIdRange { start: 5.into(), end: 10.into(), }; // `previous_canister_id` is `None`: `range.start`. - assert_eq!(Some(5.into()), range.generate_canister_id(None)); + assert_eq!(Some(5.into()), range.next_canister_id(None)); // `previous_canister_id < range.start`: `range.start`. - assert_eq!(Some(5.into()), range.generate_canister_id(Some(0.into()))); + assert_eq!(Some(5.into()), range.next_canister_id(Some(0.into()))); // In-range `previous_canister_id`: next canister ID. - assert_eq!(Some(6.into()), range.generate_canister_id(Some(5.into()))); - assert_eq!(Some(10.into()), range.generate_canister_id(Some(9.into()))); + assert_eq!(Some(6.into()), range.next_canister_id(Some(5.into()))); + assert_eq!(Some(10.into()), range.next_canister_id(Some(9.into()))); // `previous_canister_id >= range.end`: `None`. - assert_eq!(None, range.generate_canister_id(Some(10.into()))); - assert_eq!(None, range.generate_canister_id(Some(11.into()))); + assert_eq!(None, range.next_canister_id(Some(10.into()))); + assert_eq!(None, range.next_canister_id(Some(11.into()))); } #[test] @@ -175,49 +175,31 @@ fn canister_id_ranges_are_not_disjoint() { } #[test] -fn canister_id_ranges_generate_canister_id() { +fn canister_id_ranges_next_canister_id() { let empty_ranges = new_canister_id_ranges(vec![]); // Empty ranges: `None`. - assert_eq!(None, empty_ranges.generate_canister_id(Some(1.into()))); - assert_eq!(None, empty_ranges.generate_canister_id(None)); + assert_eq!(None, empty_ranges.next_canister_id(Some(1.into()))); + assert_eq!(None, empty_ranges.next_canister_id(None)); let ranges = new_canister_id_ranges(vec![(10, 19), (30, 39)]); // `previous_canister_id` is `None`: `self.start`. - assert_eq!(Some(10.into()), ranges.generate_canister_id(None)); + assert_eq!(Some(10.into()), ranges.next_canister_id(None)); // `previous_canister_id < self.start`: `self.start`. - assert_eq!(Some(10.into()), ranges.generate_canister_id(Some(9.into()))); + assert_eq!(Some(10.into()), ranges.next_canister_id(Some(9.into()))); // `previous_canister_id < ranges.end()`: next canister ID. - assert_eq!( - Some(11.into()), - ranges.generate_canister_id(Some(10.into())) - ); - assert_eq!( - Some(19.into()), - ranges.generate_canister_id(Some(18.into())) - ); - assert_eq!( - Some(30.into()), - ranges.generate_canister_id(Some(19.into())) - ); - assert_eq!( - Some(30.into()), - ranges.generate_canister_id(Some(25.into())) - ); - assert_eq!( - Some(31.into()), - ranges.generate_canister_id(Some(30.into())) - ); - assert_eq!( - Some(39.into()), - ranges.generate_canister_id(Some(38.into())) - ); + assert_eq!(Some(11.into()), ranges.next_canister_id(Some(10.into()))); + assert_eq!(Some(19.into()), ranges.next_canister_id(Some(18.into()))); + assert_eq!(Some(30.into()), ranges.next_canister_id(Some(19.into()))); + assert_eq!(Some(30.into()), ranges.next_canister_id(Some(25.into()))); + assert_eq!(Some(31.into()), ranges.next_canister_id(Some(30.into()))); + assert_eq!(Some(39.into()), ranges.next_canister_id(Some(38.into()))); // `previous_canister_id == self.end`: `None`. - assert_eq!(None, ranges.generate_canister_id(Some(39.into()))); + assert_eq!(None, ranges.next_canister_id(Some(39.into()))); } #[test] diff --git a/rs/replicated_state/src/metadata_state.rs b/rs/replicated_state/src/metadata_state.rs index eead980f15d8..ae758aebfeac 100644 --- a/rs/replicated_state/src/metadata_state.rs +++ b/rs/replicated_state/src/metadata_state.rs @@ -590,13 +590,10 @@ impl SystemMetadata { Ok(()) } - /// Generates a new canister ID. + /// Computes a new canister ID for canister creation. /// - /// 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 `Err` iff no more canister IDs are available. + pub fn peek_new_canister_id(&self) -> Result { // Start off with // (canister_allocation_ranges // ∩ routing_table.ranges(own_subnet_id)) @@ -626,26 +623,32 @@ 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); + canister_allocation_ranges + .next_canister_id(self.last_generated_canister_id) + .ok_or_else(|| "Canister ID allocation was consumed".into()) + } - 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(); - } + /// Records `canister_id` as the last generated canister ID and drops any + /// exhausted allocation ranges. Must be called with the canister ID returned + /// by the immediately preceding `peek_new_canister_id` call, once canister + /// creation succeeded and the canister ID should be permanently consumed. + /// + /// Note. The last canister allocation range is never dropped. + 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()) } /// Returns the number of canister IDs that can still be generated. diff --git a/rs/replicated_state/src/metadata_state/tests.rs b/rs/replicated_state/src/metadata_state/tests.rs index 8efa21d2ed86..f4444f28ec21 100644 --- a/rs/replicated_state/src/metadata_state/tests.rs +++ b/rs/replicated_state/src/metadata_state/tests.rs @@ -202,12 +202,12 @@ fn init_allocation_ranges_if_empty() { } #[test] -fn generate_new_canister_id_no_allocation_ranges() { - let mut system_metadata = SystemMetadata::new(SUBNET_0, SubnetType::Application); +fn peek_new_canister_id_no_allocation_ranges() { + let system_metadata = SystemMetadata::new(SUBNET_0, SubnetType::Application); assert_eq!( Err("Canister ID allocation was consumed".into()), - system_metadata.generate_new_canister_id() + system_metadata.peek_new_canister_id() ); assert_eq!(None, system_metadata.last_generated_canister_id); } @@ -219,7 +219,7 @@ fn generate_new_canister_id_no_allocation_ranges() { /// \ canister_migrations.ranges() /// ``` #[test] -fn generate_new_canister_id() { +fn peek_and_commit_new_canister_id() { fn range(start: u64, end: u64) -> CanisterIdRange { CanisterIdRange { start: start.into(), @@ -269,10 +269,9 @@ fn generate_new_canister_id() { /// Asserts that the next generated canister ID is the expected one. /// And that `last_generated_canister_id` is updated accordingly. fn assert_next_generated(expected: u64, system_metadata: &mut SystemMetadata) { - assert_eq!( - Ok(expected.into()), - system_metadata.generate_new_canister_id() - ); + let canister_id = system_metadata.peek_new_canister_id().unwrap(); + assert_eq!(CanisterId::from(expected), canister_id); + system_metadata.commit_new_canister_id(canister_id); assert_eq!( Some(expected.into()), system_metadata.last_generated_canister_id @@ -300,7 +299,7 @@ fn generate_new_canister_id() { // No more canister IDs can be generated. assert_eq!( Err("Canister ID allocation was consumed".into()), - system_metadata.generate_new_canister_id() + system_metadata.peek_new_canister_id() ); // But last generated is the same. assert_eq!(Some(30.into()), system_metadata.last_generated_canister_id); diff --git a/rs/tests/execution/fill_execution_rounds_workload.rs b/rs/tests/execution/fill_execution_rounds_workload.rs index 5fe0b467db4c..a88b794c64a2 100644 --- a/rs/tests/execution/fill_execution_rounds_workload.rs +++ b/rs/tests/execution/fill_execution_rounds_workload.rs @@ -199,7 +199,7 @@ pub fn fill_execution_rounds( &log, )); canister_id = canister_id_ranges - .generate_canister_id(Some(canister_id)) + .next_canister_id(Some(canister_id)) .expect("Canister ID can be generated"); currently_installing_canisters += 1; @@ -219,7 +219,7 @@ pub fn fill_execution_rounds( &log, )); canister_id = canister_id_ranges - .generate_canister_id(Some(canister_id)) + .next_canister_id(Some(canister_id)) .expect("Canister ID can be generated"); currently_installing_canisters += 1; diff --git a/rs/tests/networking/http_endpoints_public_spec_test.rs b/rs/tests/networking/http_endpoints_public_spec_test.rs index 02bb3de214dd..8b22d51dc11f 100644 --- a/rs/tests/networking/http_endpoints_public_spec_test.rs +++ b/rs/tests/networking/http_endpoints_public_spec_test.rs @@ -719,17 +719,13 @@ fn get_canister_ids(snapshot: &TopologySnapshot) -> (CanisterId, CanisterId, Can let (sys_subnet, app_subnet) = get_subnets(snapshot); let sys_subnet_canister_id_range = sys_subnet.subnet_canister_ranges()[0]; - let sys_uc1_id = sys_subnet_canister_id_range - .generate_canister_id(None) - .unwrap(); + let sys_uc1_id = sys_subnet_canister_id_range.next_canister_id(None).unwrap(); let sys_uc2_id = sys_subnet_canister_id_range - .generate_canister_id(Some(sys_uc1_id)) + .next_canister_id(Some(sys_uc1_id)) .unwrap(); let app_subnet_canister_id_range = app_subnet.subnet_canister_ranges()[0]; - let app_uc_id = app_subnet_canister_id_range - .generate_canister_id(None) - .unwrap(); + let app_uc_id = app_subnet_canister_id_range.next_canister_id(None).unwrap(); (sys_uc1_id, sys_uc2_id, app_uc_id) }