refactor: canister ID generation

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<CanisterId, CanisterManagerError> {
         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<CanisterId, CanisterManagerError> {
-        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
         })?;
 

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));
+}

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

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<CanisterId>,
-    ) -> Option<CanisterId> {
+    pub fn next_canister_id(&self, previous_canister_id: Option<CanisterId>) -> Option<CanisterId> {
         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<CanisterId>,
-    ) -> Option<CanisterId> {
+    pub fn next_canister_id(&self, previous_canister_id: Option<CanisterId>) -> Option<CanisterId> {
         self.0
             .iter()
-            .flat_map(|range| range.generate_canister_id(previous_canister_id))
+            .flat_map(|range| range.next_canister_id(previous_canister_id))
             .next()
     }
 

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]

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<CanisterId, String> {
+    /// Returns `Err` iff no more canister IDs are available.
+    pub fn peek_new_canister_id(&self) -> Result<CanisterId, String> {
         // 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.