fix(cmc): Merging CMC hotfix back to master (#1368)

Merge the [hotfix](https://dashboard.internetcomputer.org/proposal/132262) back to master --------- Co-authored-by: Max Summe <[email protected]> Co-authored-by: Marko Kosmerl <[email protected]> Co-authored-by: max-dfinity <[email protected]>
dfinity · Sep 6, 2024 · c2b4a0a · c2b4a0a
1 parent 5d25ae6
commit c2b4a0a
Show file tree

Hide file tree

Showing 8 changed files with 469 additions and 69 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/rs/nns/cmc/cmc.did b/rs/nns/cmc/cmc.did
@@ -86,13 +86,6 @@ type CreateCanisterError = variant {
     // The reason why creating a canister failed.
     create_error : text;
   };
-  RefundFailed : record {
-    // The reason why creating a canister failed.
-    create_error : text;
-
-    // The reason why refunding cycles failed.
-    refund_error : text;
-  };
 };
 
 type NotifyError = variant {

diff --git a/rs/nns/cmc/src/lib.rs b/rs/nns/cmc/src/lib.rs
@@ -208,10 +208,6 @@ pub enum CreateCanisterError {
         refund_amount: u128,
         create_error: String,
     },
-    RefundFailed {
-        create_error: String,
-        refund_error: String,
-    },
 }
 
 /// Options to select subnets when creating a canister
@@ -239,6 +235,8 @@ pub enum NotifyErrorCode {
     BadSubnetSelection = 4,
     /// The caller is not allowed to perform the operation.
     Unauthorized = 5,
+    /// Deposit memo field is too long.
+    DepositMemoTooLong = 6,
 }
 
 impl NotifyError {

diff --git a/rs/nns/cmc/src/main.rs b/rs/nns/cmc/src/main.rs
@@ -15,8 +15,7 @@ use ic_crypto_tree_hash::{
     flatmap, HashTreeBuilder, HashTreeBuilderImpl, Label, LabeledTree, WitnessGenerator,
     WitnessGeneratorImpl,
 };
-use ic_ledger_core::block::BlockType;
-use ic_ledger_core::tokens::CheckedSub;
+use ic_ledger_core::{block::BlockType, tokens::CheckedSub};
 // TODO(EXC-1687): remove temporary aliases `Ic00CanisterSettingsArgs` and `Ic00CanisterSettingsArgsBuilder`.
 use ic_management_canister_types::{
     BoundedVec, CanisterIdRecord, CanisterSettingsArgs as Ic00CanisterSettingsArgs,
@@ -66,6 +65,8 @@ const ONE_MINUTE_SECONDS: u64 = 60;
 const MAX_NOTIFY_HISTORY: usize = 1_000_000;
 /// The maximum number of old notification statuses we purge in one go.
 const MAX_NOTIFY_PURGE: usize = 100_000;
+/// The maximum memo length.
+const MAX_MEMO_LENGTH: usize = 32;
 
 /// Calls to create_canister get rejected outright if they have obviously too few cycles attached.
 /// This is the minimum amount needed for creating a canister as of October 2023.
@@ -1308,6 +1309,17 @@ async fn notify_mint_cycles(
         subaccount: to_subaccount,
     };
 
+    let deposit_memo_len = deposit_memo.as_ref().map_or(0, |memo| memo.len());
+    if deposit_memo_len > MAX_MEMO_LENGTH {
+        return Err(NotifyError::Other {
+            error_code: NotifyErrorCode::DepositMemoTooLong as u64,
+            error_message: format!(
+                "Memo length {} exceeds the maximum length of {}",
+                deposit_memo_len, MAX_MEMO_LENGTH
+            ),
+        });
+    }
+
     let (amount, from) =
         fetch_transaction(block_index, expected_destination_account, MEMO_MINT_CYCLES).await?;
 
@@ -1528,6 +1540,7 @@ async fn create_canister(
     }: CreateCanister,
 ) -> Result<CanisterId, CreateCanisterError> {
     let cycles = dfn_core::api::msg_cycles_available();
+
     if cycles < CREATE_CANISTER_MIN_CYCLES {
         return Err(CreateCanisterError::Refunded {
             refund_amount: cycles.into(),
@@ -1542,24 +1555,18 @@ async fn create_canister(
             }
         })?;
 
-    // will always succeed because only calls from canisters can have cycles attached
-    let calling_canister = caller().try_into().unwrap();
-
-    dfn_core::api::msg_cycles_accept(cycles);
-    match do_create_canister(caller(), cycles.into(), subnet_selection, settings, false).await {
-        Ok(canister_id) => Ok(canister_id),
+    match do_create_canister(caller(), cycles.into(), subnet_selection, settings).await {
+        Ok(canister_id) => {
+            dfn_core::api::msg_cycles_accept(cycles);
+            Ok(canister_id)
+        }
         Err(create_error) => {
-            let refund_amount = cycles.saturating_sub(BAD_REQUEST_CYCLES_PENALTY as u64);
-            match deposit_cycles(calling_canister, refund_amount.into(), false).await {
-                Ok(()) => Err(CreateCanisterError::Refunded {
-                    refund_amount: refund_amount.into(),
-                    create_error,
-                }),
-                Err(refund_error) => Err(CreateCanisterError::RefundFailed {
-                    create_error,
-                    refund_error,
-                }),
-            }
+            dfn_core::api::msg_cycles_accept(BAD_REQUEST_CYCLES_PENALTY as u64);
+            let refund_amount = dfn_core::api::msg_cycles_available();
+            Err(CreateCanisterError::Refunded {
+                refund_amount: refund_amount.into(),
+                create_error,
+            })
         }
     }
 }
@@ -1859,7 +1866,7 @@ async fn process_create_canister(
     // Create the canister. If this fails, refund. Either way,
     // return a result so that the notification cannot be retried.
     // If refund fails, we allow to retry.
-    match do_create_canister(controller, cycles, subnet_selection, settings, true).await {
+    match do_create_canister(controller, cycles, subnet_selection, settings).await {
         Ok(canister_id) => {
             burn_and_log(sub, amount).await;
             Ok(canister_id)
@@ -2100,7 +2107,6 @@ async fn do_create_canister(
     cycles: Cycles,
     subnet_selection: Option<SubnetSelection>,
     settings: Option<CanisterSettingsArgs>,
-    mint_cycles: bool,
 ) -> Result<CanisterId, String> {
     // Retrieve randomness from the system to use later to get a random
     // permutation of subnets. Performing the asynchronous call before
@@ -2164,12 +2170,13 @@ async fn do_create_canister(
 
     let mut last_err = None;
 
-    if mint_cycles && !subnets.is_empty() {
-        // TODO(NNS1-503): If CreateCanister fails, then we still have minted
-        // these cycles.
-        ensure_balance(cycles)?;
+    if subnets.is_empty() {
+        return Err("No subnets in which to create a canister.".to_owned());
     }
 
+    // We have subnets available, so we can now mint the cycles and create the canister.
+    ensure_balance(cycles)?;
+
     let canister_settings = settings
         .map(|mut settings| {
             if settings.controllers.is_none() {
@@ -2221,31 +2228,34 @@ async fn do_create_canister(
         return Ok(canister_id);
     }
 
-    Err(last_err.unwrap_or_else(|| "No subnets in which to create a canister.".to_owned()))
+    Err(last_err.unwrap_or_else(|| "Unknown problem attempting to create a canister.".to_owned()))
 }
 
 fn ensure_balance(cycles: Cycles) -> Result<(), String> {
     let now = dfn_core::api::now();
 
+    let current_balance = Cycles::from(dfn_core::api::canister_cycle_balance());
+    let cycles_to_mint = cycles - current_balance;
+
     with_state_mut(|state| {
         state.limiter.purge_old(now);
         let count = state.limiter.get_count();
 
-        if count + cycles > state.cycles_limit {
+        if count + cycles_to_mint > state.cycles_limit {
             return Err(format!(
                 "More than {} cycles have been minted in the last {} seconds, please try again later.",
                 state.cycles_limit,
                 state.limiter.get_max_age().as_secs(),
             ));
         }
 
-        state.limiter.add(now, cycles);
-        state.total_cycles_minted += cycles;
+        state.limiter.add(now, cycles_to_mint);
+        state.total_cycles_minted += cycles_to_mint;
         Ok(())
     })?;
 
     dfn_core::api::mint_cycles(
-        cycles
+        cycles_to_mint
             .get()
             .try_into()
             .map_err(|_| "Cycles u64 overflow".to_owned())?,