diff --git a/Cargo.lock b/Cargo.lock index 80864728a32..1d174ce81bc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3100,6 +3100,7 @@ dependencies = [ "icp-ledger", "icrc-ledger-types", "lazy_static", + "maplit", "on_wire", "prost 0.13.4", "rand 0.8.5", @@ -10549,6 +10550,7 @@ dependencies = [ "ic-stable-structures", "ic-state-machine-tests", "ic-test-utilities", + "ic-test-utilities-metrics", "ic-types", "ic-types-test-utils", "ic-xrc-types", diff --git a/rs/nns/cmc/BUILD.bazel b/rs/nns/cmc/BUILD.bazel index 270763dd66e..8f550372a6a 100644 --- a/rs/nns/cmc/BUILD.bazel +++ b/rs/nns/cmc/BUILD.bazel @@ -55,6 +55,7 @@ DEV_DEPENDENCIES = [ "//rs/types/types_test_utils", "@crate_index//:candid_parser", "@crate_index//:futures", + "@crate_index//:maplit", "@crate_index//:serde_bytes", ] diff --git a/rs/nns/cmc/Cargo.toml b/rs/nns/cmc/Cargo.toml index a6d775f9d4e..74901743519 100644 --- a/rs/nns/cmc/Cargo.toml +++ b/rs/nns/cmc/Cargo.toml @@ -45,6 +45,7 @@ yansi = "0.5.0" [dev-dependencies] candid_parser = { workspace = true } +maplit = "1.0.2" futures = { workspace = true } ic-types-test-utils = { path = "../../types/types_test_utils" } serde_bytes = { workspace = true } diff --git a/rs/nns/cmc/src/lib.rs b/rs/nns/cmc/src/lib.rs index f2e5b1b3593..1e61b3906af 100644 --- a/rs/nns/cmc/src/lib.rs +++ b/rs/nns/cmc/src/lib.rs @@ -12,6 +12,8 @@ use icp_ledger::{ use icrc_ledger_types::icrc1::account::Account; use serde::{Deserialize, Serialize}; +pub const IS_AUTOMATIC_REFUND_ENABLED: bool = false; + pub const DEFAULT_CYCLES_PER_XDR: u128 = 1_000_000_000_000u128; // 1T cycles = 1 XDR pub const PERMYRIAD_DECIMAL_PLACES: u32 = 4; @@ -313,10 +315,23 @@ pub struct CyclesLedgerDepositResult { pub block_index: Nat, } +// When a user sends us ICP, they indicate via memo (or icrc1_memo) what +// operation they want to perform. +// +// We promise that we will NEVER use 0 as one of these values. (This would be +// very bad, because if no memo is explicitly supplied, the Transaction might +// implicitly have 0 in the memo field.) +// +// Note to developers: If you add new values, update MEANINGFUL_MEMOS. pub const MEMO_CREATE_CANISTER: Memo = Memo(0x41455243); // == 'CREA' pub const MEMO_TOP_UP_CANISTER: Memo = Memo(0x50555054); // == 'TPUP' pub const MEMO_MINT_CYCLES: Memo = Memo(0x544e494d); // == 'MINT' +// New values might be added to this later. Do NOT assume that values won't be +// added to this array later. +pub const MEANINGFUL_MEMOS: [Memo; 3] = + [MEMO_CREATE_CANISTER, MEMO_TOP_UP_CANISTER, MEMO_MINT_CYCLES]; + pub fn create_canister_txn( amount: Tokens, from_subaccount: Option, diff --git a/rs/nns/cmc/src/main.rs b/rs/nns/cmc/src/main.rs index 4d11dfce023..3cd394b28eb 100644 --- a/rs/nns/cmc/src/main.rs +++ b/rs/nns/cmc/src/main.rs @@ -139,6 +139,9 @@ pub enum NotificationStatus { NotifiedCreateCanister(Result), /// The cached result of a completed cycles mint. NotifiedMint(NotifyMintCyclesResult), + /// The transaction did not have a supported memo (or icrc1_memo). + /// Therefore, we decided to send the ICP back to its source (minus fee). + AutomaticallyRefunded(Option), } /// Version of the State type. @@ -224,6 +227,16 @@ pub struct StateV1 { pub total_cycles_minted: Cycles, + // We use this for synchronization/journaling. + // + // Because our operations (e.g. minting cycles) require calling other + // canister(s), in particular ledger, it is possible for duplicate requests + // to interleave. In such cases, we want subsequent operations to see that + // an operation is already in flight. Therefore, before making any canister + // calls, we check that the block does not already have a status, and set + // its status to Processing. Only then do we proceed with calling the other + // canister (i.e. ledger). Once that comes back, we update the block's + // status. This avoids using the same ICP to perform multiple operations. pub blocks_notified: BTreeMap, pub last_purged_notification: BlockIndex, @@ -1167,17 +1180,21 @@ async fn notify_top_up( canister_id, }: NotifyTopUp, ) -> Result { - let cmc_id = dfn_core::api::id(); - let sub = Subaccount::from(&canister_id); - let expected_destination_account = AccountIdentifier::new(cmc_id.get(), Some(sub)); - let (amount, from) = fetch_transaction( block_index, - expected_destination_account, + Subaccount::from(&canister_id), MEMO_TOP_UP_CANISTER, ) .await?; + // Try to set the status of this block to Processing. In order for this to + // succeed, two conditions must hold: + // + // 1. It must not already have a status. + // + // 2. The block is "sufficiently recent". More precisely, it must be + // more recent than last_purged_notification. (To avoid unbounded + // growth of the journal.) let maybe_early_result = with_state_mut(|state| { state.purge_old_notifications(MAX_NOTIFY_HISTORY); @@ -1190,6 +1207,9 @@ async fn notify_top_up( match state.blocks_notified.entry(block_index) { Entry::Occupied(entry) => match entry.get() { NotificationStatus::Processing => Some(Err(NotifyError::Processing)), + + // If the user makes a duplicate request, we respond as though + // the current request is the original one. NotificationStatus::NotifiedTopUp(result) => Some(result.clone()), NotificationStatus::NotifiedCreateCanister(_) => { Some(Err(NotifyError::InvalidTransaction( @@ -1199,6 +1219,11 @@ async fn notify_top_up( NotificationStatus::NotifiedMint(_) => Some(Err(NotifyError::InvalidTransaction( "The same payment is already processed as mint request".into(), ))), + NotificationStatus::AutomaticallyRefunded(_) => { + Some(Err(NotifyError::InvalidTransaction( + "The same payment is already processed as automatic refund".into(), + ))) + } }, Entry::Vacant(entry) => { entry.insert(NotificationStatus::Processing); @@ -1244,9 +1269,7 @@ async fn notify_mint_cycles( deposit_memo, }: NotifyMintCyclesArg, ) -> NotifyMintCyclesResult { - let cmc_id = dfn_core::api::id(); let subaccount = Subaccount::from(&caller()); - let expected_destination_account = AccountIdentifier::new(cmc_id.get(), Some(subaccount)); let to_account = Account { owner: caller().into(), subaccount: to_subaccount, @@ -1263,8 +1286,7 @@ async fn notify_mint_cycles( }); } - let (amount, from) = - fetch_transaction(block_index, expected_destination_account, MEMO_MINT_CYCLES).await?; + let (amount, from) = fetch_transaction(block_index, subaccount, MEMO_MINT_CYCLES).await?; let maybe_early_result = with_state_mut(|state| { state.purge_old_notifications(MAX_NOTIFY_HISTORY); @@ -1288,6 +1310,11 @@ async fn notify_mint_cycles( NotificationStatus::NotifiedTopUp(_) => Some(Err(NotifyError::InvalidTransaction( "The same payment is already processed as a top up request.".into(), ))), + NotificationStatus::AutomaticallyRefunded(_) => { + Some(Err(NotifyError::InvalidTransaction( + "The same payment is already processed as an automatic refund.".into(), + ))) + } }, Entry::Vacant(entry) => { entry.insert(NotificationStatus::Processing); @@ -1355,9 +1382,6 @@ async fn notify_create_canister( ) -> Result { authorize_caller_to_call_notify_create_canister_on_behalf_of_creator(caller(), controller)?; - let cmc_id = dfn_core::api::id(); - let sub = Subaccount::from(&controller); - let expected_destination_account = AccountIdentifier::new(cmc_id.get(), Some(sub)); let subnet_selection = get_subnet_selection(subnet_type, subnet_selection).map_err(|error_message| { NotifyError::Other { @@ -1368,7 +1392,7 @@ async fn notify_create_canister( let (amount, from) = fetch_transaction( block_index, - expected_destination_account, + Subaccount::from(&controller), MEMO_CREATE_CANISTER, ) .await?; @@ -1392,6 +1416,11 @@ async fn notify_create_canister( NotificationStatus::NotifiedMint(_) => Some(Err(NotifyError::InvalidTransaction( "The same payment is already processed as a mint request.".into(), ))), + NotificationStatus::AutomaticallyRefunded(_) => { + Some(Err(NotifyError::InvalidTransaction( + "The same payment is already processed as an automatic refund.".into(), + ))) + } }, Entry::Vacant(entry) => { entry.insert(NotificationStatus::Processing); @@ -1571,9 +1600,7 @@ fn memo_to_intent_str(memo: Memo) -> String { /// Returns Ok if transaction matches expected_memo. /// -/// First, looks at the memo field. If that does not match, falls back to the -/// icrc1_field. If that is of length 8, converts assuming little endian, and if -/// that matches, returns Ok. +/// memo and icrc1_memo are used. See get_u64_memo. fn transaction_has_expected_memo( transaction: &Transaction, expected_memo: Memo, @@ -1582,42 +1609,15 @@ fn transaction_has_expected_memo( format!("{} ({})", memo_to_intent_str(memo), memo.0) } - if transaction.memo == expected_memo { - return Ok(()); - } - - // Fall back to icrc1_memo. - - // Read the field. - let Some(icrc1_memo) = &transaction.icrc1_memo else { - return Err(NotifyError::InvalidTransaction(format!( - "The transaction's memo ({}) does not have the required value ({}).", - stringify_memo(transaction.memo), - stringify_memo(expected_memo), - ))); - }; - - // Convert it to Memo. - type U64Array = [u8; std::mem::size_of::()]; - let observed_icrc1_memo = U64Array::try_from(icrc1_memo.as_ref()).map_err(|_err| { - NotifyError::InvalidTransaction(format!( - "The transaction's memo ({}) does not have the required value ({}).", - stringify_memo(transaction.memo), - stringify_memo(expected_memo), - )) - })?; - let observed_icrc1_memo = Memo(u64::from_le_bytes(observed_icrc1_memo)); - - // Compare to the required value. - if observed_icrc1_memo == expected_memo { + let observed_memo = get_u64_memo(transaction); + if observed_memo == expected_memo { return Ok(()); } Err(NotifyError::InvalidTransaction(format!( - "Neither the memo ({}) nor the icrc1_memo ({}) of the transaction \ - has the required value ({}).", - stringify_memo(transaction.memo), - stringify_memo(observed_icrc1_memo), + "The memo ({}) in the transaction does not match the expected memo \ + ({}) for the operation.", + stringify_memo(observed_memo), stringify_memo(expected_memo), ))) } @@ -1627,7 +1627,7 @@ fn transaction_has_expected_memo( /// Returns Ok if the arguments are matched. (Otherwise, returns Err). async fn fetch_transaction( block_index: BlockIndex, - expected_destination_account: AccountIdentifier, + expected_to_subaccount: Subaccount, expected_memo: Memo, ) -> Result<(Tokens, AccountIdentifier), NotifyError> { let ledger_id = with_state(|state| state.ledger_canister_id); @@ -1645,18 +1645,284 @@ async fn fetch_transaction( } }; - if to != expected_destination_account { + let expected_to = + AccountIdentifier::new(dfn_core::api::id().get(), Some(expected_to_subaccount)); + if to != expected_to { return Err(NotifyError::InvalidTransaction(format!( "Destination account in the block ({}) different than in the notification ({})", - to, expected_destination_account + to, expected_to_subaccount, ))); } + if IS_AUTOMATIC_REFUND_ENABLED { + issue_automatic_refund_if_memo_not_offerred( + block_index, + expected_to_subaccount, + block.transaction().as_ref(), + ) + .await?; + } + transaction_has_expected_memo(block.transaction().as_ref(), expected_memo)?; Ok((amount, from)) } +/// If transaction.memo is nonzero, returns that. Otherwise, falls back to +/// icrc1_memo. More precisely, if icrc1_memo is of length 8 (64 bits), then, +/// then that is returned, assuming little-endian. Otherwise, Memo(0) is +/// returned. +fn get_u64_memo(transaction: &Transaction) -> Memo { + if transaction.memo != Memo(0) { + return transaction.memo; + } + + // Fall back to icrc1_memo. + + let Some(icrc1_memo) = transaction.icrc1_memo.as_ref() else { + // icrc1_memo is absent. + return Memo(0); + }; + + type U64Array = [u8; std::mem::size_of::()]; + let Ok(icrc1_memo) = U64Array::try_from(icrc1_memo.as_ref()) else { + // icrc1_memo has the wrong size. + return Memo(0); + }; + + Memo(u64::from_le_bytes(icrc1_memo)) +} + +/// "Normally", sets the block's status to Processing. However, if the block is +/// too old (<= last_purged_notification), or it already has a status, no +/// changes are made, and the block's current status is returned. (None +/// indicates that the block is too old to have a status.) +fn set_block_status_to_processing( + block_index: BlockIndex, +) -> Result<(), Option> { + with_state_mut(|state| { + if block_index <= state.last_purged_notification { + return Err(None); + } + + match state.blocks_notified.entry(block_index) { + Entry::Occupied(entry) => Err(Some(entry.get().clone())), + + Entry::Vacant(entry) => { + entry.insert(NotificationStatus::Processing); + Ok(()) + } + } + }) +} + +/// If the block's status in blocks_notified is Processing, clear it. Otherwise, +/// makes no changes (and logs an error). +fn clear_block_processing_status(block_index: BlockIndex) { + with_state_mut(|state| { + // Fetch the block's status. + let occupied_entry = match state.blocks_notified.entry(block_index) { + Entry::Occupied(ok) => ok, + + Entry::Vacant(_entry) => { + println!( + "[cycles] ERROR: Tried to clear the status of block {}, \ + but it already has no status?!", + block_index, + ); + return; + } + }; + + // Make sure the block's status is currently Processing. + if &NotificationStatus::Processing != occupied_entry.get() { + // Otherwise, do not touch the block's status (and log). + println!( + "[cycles] ERROR: Tried to clear Processing status of block {} \ + but its current status is {:?}", + block_index, + occupied_entry.get(), + ); + return; + } + + occupied_entry.remove(); + }); +} + +/// Ok is returned if the transaction is not eligible for an automatic refund +/// (because its memo indicates one of the supported operations). This is so +/// that the caller can use the `?` operator to return early in the case where +/// automatic refund should be issued. +/// +/// Otherwise, transaction is eligible for an automatic refund. The rest of +/// these comments assume that we are in this (interesting) case. +/// +/// Attempts to transfer the ICP (minus fees) back to the sender (by calling +/// ledger). +/// +/// Regardless of whether that ledger call succeeds, Err is returned, but the +/// value in the Err depends on how the ledger call turns out. +/// +/// If the ledger call failed, the user can retry. +/// +/// Like the rest of this canister, uses blocks_notified for "journaling". More +/// precisely, before calling ledger, there are two things: +/// +/// 1. The block MUST have no status. If it does, this returns Err, and no +/// ledger call is attempted. +/// +/// 2. The block's status is set to Processing. +/// +/// If the ledger call succeeds, then the block's status is updated to +/// AutomaticallyRefunded. Otherwise, if the ledger call fails, then the block's +/// status is cleared to allow the user to try again. Some reasons the call +/// might fail: +/// +/// 1. Ledger is unavailable. This could be cause by it being upgraded. +/// +/// 2. Ledger is up, but there is something wrong with our request (e.g. +/// wrong fee). +/// +/// It is generally assumed that the arguments are consistent with one another. +/// E.g. we assume that fetching the block (using incoming_block_index), would +/// give us the same value as incoming_transaction. +async fn issue_automatic_refund_if_memo_not_offerred( + incoming_block_index: BlockIndex, + // This is needed because transaction only has an AccountIdentifier. + // Although it is possible to go from PrincipalId + Subaccount to + // AccountIdentifier, the reverse is not possible. This is a super confusing + // feature of the ICP ledger, but for better or worse, it is intentional. + incoming_to_subaccount: Subaccount, + incoming_transaction: &Transaction, +) -> Result<(), NotifyError> { + let memo = get_u64_memo(incoming_transaction); + if MEANINGFUL_MEMOS.contains(&memo) { + // Not eligible for refund. + return Ok(()); + } + + // Extract (from incoming_transaction) where the ICP came from, and how much + // was transferred. + let (incoming_from, incoming_amount) = match &incoming_transaction.operation { + Operation::Transfer { + from, + to, + amount, + + fee: _, + spender: _, + } => { + let incoming_to_account_identifier = + AccountIdentifier::new(dfn_core::api::id().get(), Some(incoming_to_subaccount)); + if to != &incoming_to_account_identifier { + // As long as callers always pass us Transfers where the + // destination matches incoming_to_subaccount, this code will + // never be executed. + println!( + "[cycles] WARNING: Destination in transfer ({}) passed to + issue_automatic_refund_if_memo_not_offerred does NOT match. \ + This indicates that we have some kind of bug. No refund will \ + be issued. {} (AccountIdentifier) vs. {:?} (Subaccount)", + incoming_block_index, to, incoming_to_subaccount, + ); + return Ok(()); + } + + (*from, *amount) + } + + _invalid_operation => { + // As long as callers always pass us Transfers, this code will never + // be executed. + println!( + "[cycles] WARNING: A non-transfer transaction ({}) was passed to \ + issue_automatic_refund_if_memo_not_offerred. This indicates that \ + we have some kind of bug. No refund will be issued.", + incoming_block_index, + ); + + return Ok(()); + } + }; + + // Set block's status to Processing before calling ledger. + let reason_for_refund = format!( + "Memo ({:#08X}) in the incoming ICP transfer does not correspond to \ + any of the operations that the Cycles Minting canister offers.", + memo.0, + ); + if let Err(prior_block_status) = set_block_status_to_processing(incoming_block_index) { + let Some(prior_block_status) = prior_block_status else { + // Callers of fetch_transaction generally do this already. + return Err(NotifyError::TransactionTooOld(with_state(|state| { + state.last_purged_notification + 1 + }))); + }; + + // Do not proceed, because block is either being processed, or was + // finished being processed earlier. + use NotificationStatus::{ + AutomaticallyRefunded, NotifiedCreateCanister, NotifiedMint, NotifiedTopUp, Processing, + }; + return match prior_block_status { + Processing => Err(NotifyError::Processing), + + AutomaticallyRefunded(block_index) => Err(NotifyError::Refunded { + block_index, + reason: reason_for_refund, + }), + + // There is no (known) way to reach this case, since we already + // verified that memo is in MEANINGFUL_MEMOS. + NotifiedCreateCanister(_) | NotifiedMint(_) | NotifiedTopUp(_) => { + Err(NotifyError::InvalidTransaction(format!( + "Block has already been processed: {:?}", + prior_block_status, + ))) + } + }; + } + + // Now, it is safe to call ledger to send the ICP back, so do it. + let refund_block_index = refund_icp( + incoming_to_subaccount, + incoming_from, + incoming_amount, + Tokens::from_e8s(0), // extra_fee + ) + .await + .inspect_err(|_err| { + // This allows the user to retry. + clear_block_processing_status(incoming_block_index); + })?; + + // Sending the ICP back succeeded. Therefore, update the block's status to + // AutomaticallyRefunded. + let old_entry_value = with_state_mut(|state| { + state.blocks_notified.insert( + incoming_block_index, + NotificationStatus::AutomaticallyRefunded(refund_block_index), + ) + }); + // Log if the block's previous status somehow changed out from under us + // while we were waiting for the ledger call to return. There is no known + // way for this to happen (except, ofc, bugs). + if old_entry_value != Some(NotificationStatus::Processing) { + println!( + "[cycles] ERROR: After issuing an automatic refund, the \ + incoming block's status was not Processing, even though \ + we checked this before calling ledger! {:?}", + old_entry_value, + ); + } + + Err(NotifyError::Refunded { + reason: reason_for_refund, + block_index: refund_block_index, + }) +} + /// Processes a legacy notification from the Ledger canister. async fn transaction_notification(tn: TransactionNotification) -> Result { let caller = caller(); @@ -1692,6 +1958,9 @@ async fn transaction_notification(tn: TransactionNotification) -> Result Err(format!("Already notified: {:?}", resp)), + NotificationStatus::AutomaticallyRefunded(resp) => { + Err(format!("Already notified: {:?}", resp)) + } }, Entry::Vacant(entry) => { entry.insert(NotificationStatus::Processing); @@ -2491,6 +2760,7 @@ fn get_subnet_selection( mod tests { use super::*; use ic_types_test_utils::ids::{subnet_test_id, user_test_id}; + use maplit::btreemap; use rand::Rng; use serde_bytes::ByteBuf; use std::str::FromStr; @@ -3376,4 +3646,155 @@ mod tests { } } } + + #[test] + fn test_set_block_status_to_processing_happy() { + let red_herring_block_index = 0xDEADBEEF; + STATE.with(|state| { + state.replace(Some(State { + blocks_notified: btreemap! { + red_herring_block_index => NotificationStatus::Processing, + }, + ..Default::default() + })) + }); + + let target_block_index = 42; + let result = set_block_status_to_processing(target_block_index); + + assert_eq!(result, Ok(())); + assert_eq!( + with_state(|state| state.blocks_notified.clone()), + btreemap! { + // Existing data untouched. + red_herring_block_index => NotificationStatus::Processing, + // New entry. + target_block_index => NotificationStatus::Processing, + }, + ); + } + + #[test] + fn test_set_block_status_to_processing_already_has_status() { + let target_block_index = 42; + let red_herring_block_index = 0xDEADBEEF; + let original_blocks_notified = btreemap! { + red_herring_block_index => NotificationStatus::Processing, + // Danger! Block ALREADY has status. + target_block_index => NotificationStatus::Processing, + }; + STATE.with(|state| { + state.replace(Some(State { + blocks_notified: original_blocks_notified.clone(), + ..Default::default() + })) + }); + + let result = set_block_status_to_processing(target_block_index); + + assert_eq!(result, Err(Some(NotificationStatus::Processing))); + assert_eq!( + with_state(|state| state.blocks_notified.clone()), + original_blocks_notified, + ); + } + + #[test] + fn test_set_block_status_to_processing_too_old() { + let target_block_index = 42; + let red_herring_block_index = 0xDEADBEEF; + let original_blocks_notified = btreemap! { + red_herring_block_index => NotificationStatus::Processing, + }; + STATE.with(|state| { + state.replace(Some(State { + blocks_notified: original_blocks_notified.clone(), + // We only know the status of blocks that are newer than this. + last_purged_notification: 42, + ..Default::default() + })) + }); + + let result = set_block_status_to_processing(target_block_index); + + assert_eq!(result, Err(None)); + assert_eq!( + with_state(|state| state.blocks_notified.clone()), + original_blocks_notified, + ); + } + + #[test] + fn test_clear_block_processing_status_happy() { + let target_block_index = 42; + let red_herring_block_index = 0xDEADBEEF; + let original_blocks_notified = btreemap! { + red_herring_block_index => NotificationStatus::Processing, + target_block_index => NotificationStatus::Processing, + }; + STATE.with(|state| { + state.replace(Some(State { + blocks_notified: original_blocks_notified.clone(), + ..Default::default() + })) + }); + + clear_block_processing_status(target_block_index); + + // Assert that target block was deleted. + assert_eq!( + with_state(|state| state.blocks_notified.clone()), + btreemap! { + red_herring_block_index => NotificationStatus::Processing, + // target_block_index no longer present. + }, + ); + } + + #[test] + fn test_clear_block_processing_status_not_processing() { + let target_block_index = 42; + let red_herring_block_index = 0xDEADBEEF; + let original_blocks_notified = btreemap! { + red_herring_block_index => NotificationStatus::Processing, + target_block_index => NotificationStatus::NotifiedTopUp(Ok(Cycles::new(1_000_000_000_000))), + }; + STATE.with(|state| { + state.replace(Some(State { + blocks_notified: original_blocks_notified.clone(), + ..Default::default() + })) + }); + + clear_block_processing_status(target_block_index); + + // Assert that blocks_notified not changed. + assert_eq!( + with_state(|state| state.blocks_notified.clone()), + original_blocks_notified, + ); + } + + #[test] + fn test_clear_block_processing_status_absent_entirely() { + let target_block_index = 42; + let red_herring_block_index = 0xDEADBEEF; + let original_blocks_notified = btreemap! { + red_herring_block_index => NotificationStatus::Processing, + }; + STATE.with(|state| { + state.replace(Some(State { + blocks_notified: original_blocks_notified.clone(), + ..Default::default() + })) + }); + + clear_block_processing_status(target_block_index); + + // Assert that blocks_notified not changed. + assert_eq!( + with_state(|state| state.blocks_notified.clone()), + original_blocks_notified, + ); + } } diff --git a/rs/nns/integration_tests/BUILD.bazel b/rs/nns/integration_tests/BUILD.bazel index ae0be0d5bca..78a89f37694 100644 --- a/rs/nns/integration_tests/BUILD.bazel +++ b/rs/nns/integration_tests/BUILD.bazel @@ -121,6 +121,7 @@ DEV_DEPENDENCIES = [ "//rs/nervous_system/common/test_utils", "//rs/nervous_system/integration_tests:nervous_system_integration_tests", "//rs/nns/test_utils/golden_nns_state", + "//rs/test_utilities/metrics", "//rs/types/types_test_utils", "@crate_index//:ic-cbor", "@crate_index//:ic-certificate-verification", diff --git a/rs/nns/integration_tests/Cargo.toml b/rs/nns/integration_tests/Cargo.toml index f507c85ca47..55aed4b3f53 100644 --- a/rs/nns/integration_tests/Cargo.toml +++ b/rs/nns/integration_tests/Cargo.toml @@ -55,6 +55,7 @@ ic-nns-governance-init = { path = "../governance/init" } ic-sns-root = { path = "../../sns/root" } ic-sns-swap = { path = "../../sns/swap" } ic-stable-structures = { workspace = true } +ic-test-utilities-metrics = { path = "../../test_utilities/metrics" } icp-ledger = { path = "../../ledger_suite/icp" } icrc-ledger-types = { path = "../../../packages/icrc-ledger-types" } lazy_static = { workspace = true } diff --git a/rs/nns/integration_tests/src/cycles_minting_canister.rs b/rs/nns/integration_tests/src/cycles_minting_canister.rs index 2ab4a6185c0..38df59b1c1d 100644 --- a/rs/nns/integration_tests/src/cycles_minting_canister.rs +++ b/rs/nns/integration_tests/src/cycles_minting_canister.rs @@ -1,12 +1,13 @@ use assert_matches::assert_matches; -use candid::{Decode, Encode, Nat}; +use candid::{Decode, Encode, Nat, Principal}; use canister_test::Canister; use cycles_minting_canister::{ CanisterSettingsArgs, ChangeSubnetTypeAssignmentArgs, CreateCanister, CreateCanisterError, IcpXdrConversionRateCertifiedResponse, NotifyCreateCanister, NotifyError, NotifyErrorCode, NotifyMintCyclesArg, NotifyMintCyclesSuccess, NotifyTopUp, SubnetListWithType, SubnetTypesToSubnetsResponse, UpdateSubnetTypeArgs, BAD_REQUEST_CYCLES_PENALTY, - MEMO_CREATE_CANISTER, MEMO_MINT_CYCLES, MEMO_TOP_UP_CANISTER, + IS_AUTOMATIC_REFUND_ENABLED, MEANINGFUL_MEMOS, MEMO_CREATE_CANISTER, MEMO_MINT_CYCLES, + MEMO_TOP_UP_CANISTER, }; use dfn_candid::candid_one; use dfn_protobuf::protobuf; @@ -18,6 +19,7 @@ use ic_management_canister_types::{ CanisterSettingsArgsBuilder, CanisterStatusResultV2, }; use ic_nervous_system_clients::canister_status::CanisterStatusResult; +use ic_nervous_system_common::E8; use ic_nervous_system_common_test_keys::{ TEST_NEURON_1_ID, TEST_NEURON_1_OWNER_KEYPAIR, TEST_USER1_KEYPAIR, TEST_USER1_PRINCIPAL, TEST_USER2_PRINCIPAL, TEST_USER3_PRINCIPAL, @@ -25,7 +27,7 @@ use ic_nervous_system_common_test_keys::{ use ic_nns_common::types::{NeuronId, ProposalId, UpdateIcpXdrConversionRatePayload}; use ic_nns_constants::{ CYCLES_LEDGER_CANISTER_ID, CYCLES_MINTING_CANISTER_ID, GOVERNANCE_CANISTER_ID, - LEDGER_CANISTER_INDEX_IN_NNS_SUBNET, ROOT_CANISTER_ID, + LEDGER_CANISTER_ID, LEDGER_CANISTER_INDEX_IN_NNS_SUBNET, ROOT_CANISTER_ID, }; use ic_nns_governance_api::pb::v1::{NnsFunction, ProposalStatus}; use ic_nns_test_utils::{ @@ -34,12 +36,14 @@ use ic_nns_test_utils::{ itest_helpers::{state_machine_test_on_nns_subnet, NnsCanisters}, neuron_helpers::get_neuron_1, state_test_helpers::{ - cmc_set_default_authorized_subnetworks, set_up_universal_canister, setup_cycles_ledger, - setup_nns_canisters, state_machine_builder_for_nns_tests, update_with_sender, + cmc_set_default_authorized_subnetworks, icrc1_balance, icrc1_transfer, + set_up_universal_canister, setup_cycles_ledger, setup_nns_canisters, + state_machine_builder_for_nns_tests, update_with_sender, }, }; use ic_state_machine_tests::{StateMachine, WasmResult}; use ic_test_utilities::universal_canister::{call_args, wasm}; +use ic_test_utilities_metrics::fetch_int_gauge_vec; use ic_types::{CanisterId, Cycles, PrincipalId}; use ic_types_test_utils::ids::subnet_test_id; use icp_ledger::{ @@ -47,7 +51,9 @@ use icp_ledger::{ NotifyCanisterArgs, SendArgs, Subaccount, Tokens, TransferArgs, TransferError, DEFAULT_TRANSFER_FEE, }; -use icrc_ledger_types::icrc1::account::Account; +use icrc_ledger_types::icrc1::{self, account::Account}; +use maplit::btreemap; +use serde_bytes::ByteBuf; use std::time::Duration; /// Test that the CMC's `icp_xdr_conversion_rate` can be updated via Governance @@ -778,6 +784,255 @@ fn test_cmc_cycles_create_with_settings() { ); } +#[test] +fn test_cmc_automatically_refunds_when_memo_is_garbage() { + // Step 1: Prepare the world. + + // USER1 has some ICP (in their default subaccount). + let account_identifier = AccountIdentifier::new(*TEST_USER1_PRINCIPAL, None); + let initial_balance = Tokens::new(100, 0).unwrap(); + let state_machine = state_machine_builder_for_nns_tests().build(); + let nns_init_payloads = NnsInitPayloadsBuilder::new() + .with_ledger_account(account_identifier, initial_balance) + .build(); + setup_nns_canisters(&state_machine, nns_init_payloads); + + let assert_canister_statuses_fixed = |narrative_phase| { + assert_eq!( + btreemap! { + btreemap! { "status".to_string() => "running".to_string() } => 11, + btreemap! { "status".to_string() => "stopped".to_string() } => 0, + btreemap! { "status".to_string() => "stopping".to_string() } => 0, + }, + fetch_int_gauge_vec( + state_machine.metrics_registry(), + "replicated_state_registered_canisters" + ), + "{}", + narrative_phase + ); + }; + // This will be called again later to verify that no canisters were added. + // Here, we just make sure that assert_canister_statuses_fixed has a correct + // understanding of how many canisters there are originally. + assert_canister_statuses_fixed("start"); + + let assert_balance = |nominal_amount_tokens: u64, fee_count: u64, narrative_phase: &str| { + let observed_balance = icrc1_balance( + &state_machine, + LEDGER_CANISTER_ID, + Account { + owner: Principal::from(*TEST_USER1_PRINCIPAL), + subaccount: None, + }, + ); + + let total_fees = Tokens::new(0, fee_count * 10_000).unwrap(); + let expected_balance = Tokens::new(nominal_amount_tokens, 0) + .unwrap() + .checked_sub(&total_fees) + .unwrap(); + assert_eq!(observed_balance, expected_balance, "{}", narrative_phase); + }; + // This is more to gain confidence that assert_balance works; there is very + // little risk that USER1's balance is not 100. + assert_balance(100, 0, "start"); + + // Step 2: Run code under test. + + // Step 2.1: Send ICP from USER1 to CMC, but use a garbage memo. There is no + // immediate explosion; "the bomb is only being planted" so to speak. + + let garbage_memo = [0xEF, 0xBE, 0xAD, 0xDE, 0, 0, 0, 0]; // little endian 0x_DEAD_BEEF + assert!(!MEANINGFUL_MEMOS.contains(&Memo(u64::from_le_bytes(garbage_memo)))); + let transfer_arg = icrc1::transfer::TransferArg { + to: icrc1::account::Account { + owner: Principal::from(CYCLES_MINTING_CANISTER_ID.get()), + subaccount: Some(Subaccount::from(&TEST_USER1_PRINCIPAL.clone()).0), + }, + amount: Nat::from(10 * E8), + fee: Some(Nat::from(10_000_u64)), + + // Here, the "bomb is being planted". + memo: Some(icrc1::transfer::Memo(ByteBuf::from(garbage_memo))), + + from_subaccount: None, // source from USER1's the default subaccount + created_at_time: None, + }; + let create_canister_block_index = icrc1_transfer( + &state_machine, + LEDGER_CANISTER_ID, + *TEST_USER1_PRINCIPAL, + transfer_arg.clone(), + ) + .expect("transfer failed"); + assert_balance(90, 1, "ICP sent to CMC"); + + // This is to make it so that CMC has more ICP. That way, when we later try + // to duplicate the automatic refund, we can verify that CMC refrains from + // sending back more ICP. + let transfer_arg = icrc1::transfer::TransferArg { + amount: Nat::from(50 * E8), + ..transfer_arg + }; + let _red_herring = icrc1_transfer( + &state_machine, + LEDGER_CANISTER_ID, + *TEST_USER1_PRINCIPAL, + transfer_arg, + ) + .expect("transfer failed"); + // Balance has gone down by 10 + 10 (plus fees). Later, we will see balance + // go back up, but only by 10 (minus fee). + assert_balance(40, 2, "additional red herring ICP sent to CMC"); + + // Step 2.2: Ask CMC to create a canister using the ICP that was just sent + // to it (from USER1). This is where it is noticed (by CMC) that USER1 did + // something wrong. Many requests are sent concurrently to verify that + // journaling/locking/deduplication works. + #[allow(deprecated)] + let notify_create_canister = Encode!(&NotifyCreateCanister { + block_index: create_canister_block_index, + controller: *TEST_USER1_PRINCIPAL, + + // Nothing fancy. + subnet_type: None, + subnet_selection: None, + settings: None, + }) + .unwrap(); + let results = (0..100) + .map(|_i| { + // Launch (another) notify_create_canister call, but crucially, do + // NOT wait for it to complete. That takes place a little further + // down. + state_machine + .send_ingress_safe( + *TEST_USER1_PRINCIPAL, + CYCLES_MINTING_CANISTER_ID, + "notify_create_canister", + notify_create_canister.clone(), + ) + .unwrap() + }) + // It might seem silly to call collect, and then immediately after that + // call into_iter, but this is to ensure that await_ingress is not + // called until after ALL send_ingress_safe calls are made. + .collect::>() + .into_iter() + .map(|message_id| { + let result = state_machine.await_ingress(message_id, 500).unwrap(); + + let result = match result { + WasmResult::Reply(ok) => ok, + _ => panic!("{:?}", result), + }; + + Decode!(&result, Result).unwrap() + }) + .collect::>(); + + // Step 3: Verify results. + + // Step 3.1: Verify that no canisters were created. + assert_canister_statuses_fixed("end"); + + // Step 3.2: Inspect USER1's balance. + if IS_AUTOMATIC_REFUND_ENABLED { + // Verify that CMC sent 10 ICP back to USER1 (minus fee, ofc). Here, we + // also see that CMC refrained from refunding the same transfer more + // than once, even though multiple its notify_create_canister method was + // called a couple of times. + assert_balance(50, 3, "end"); + } else { + assert_balance(40, 2, "end"); + } + + // Step 3.3: Verify that CMC returned Err. + + // Step 3.3.1: Filter out Err(Processing), and freak out if there are any Ok. + let mut errs = results + .into_iter() + .filter_map(|result| match result { + Err(NotifyError::Processing) => None, + Ok(_) => panic!("{:?}", result), + Err(err) => Some(err), + }) + .collect::>(); + + // Step 3.3.2: Assert that all errs are the same. + let last_err = errs.pop().unwrap(); + assert!( + errs.iter().all(|other_err| other_err == &last_err), + "{:?}\nvs.\n{:#?}", + last_err, + errs, + ); + assert!( + errs.len() >= 2, // If errs is empty, then the previous assert is trivial. + "{}: {:#?}", + errs.len(), + errs, + ); + // I tried cranking up the concurrent calls, but I could never get + // Processing to occur. That is, this would always print concurrency - 1. + println!("errs.len() == {}", errs.len()); + + // Step 3.3.3: Verify that the errors are of the right type. This depends on + // whether automatic refund is enabled. If so, then they errs should be + // Refunded; otherwise, they should be InvalidTransaction. + // + // (Most of the code here is for inspecting the reason.) + if IS_AUTOMATIC_REFUND_ENABLED { + match &last_err { + NotifyError::Refunded { + reason, + block_index: refund_block_index, + } => { + // There should be a block_index. + refund_block_index.as_ref().unwrap(); + + // Inspect reason. + let lower_reason = reason.to_lowercase(); + for key_word in ["memo", "0xdeadbeef", "does not correspond", "offer"] { + assert!( + lower_reason.contains(key_word), + r#""{}" not in {:?}"#, + key_word, + last_err + ); + } + } + + _ => panic!("{:?}", last_err), + }; + } else { + match &last_err { + NotifyError::InvalidTransaction(reason) => { + // Inspect reason. + let lower_reason = reason.to_lowercase(); + for key_word in [ + "memo", + "3735928559", // 0xDEAD_BEEF + "not match", + "expected", + "1095062083", // MEMO_CREATE_CANISTER + "createcanister", + ] { + assert!( + lower_reason.contains(key_word), + r#""{}" not in {:?}"#, + key_word, + last_err + ); + } + } + + _ => panic!("{:?}", last_err), + }; + } +} + fn send_transfer(env: &StateMachine, arg: &TransferArgs) -> Result { let ledger = CanisterId::from_u64(LEDGER_CANISTER_INDEX_IN_NNS_SUBNET); let from = *TEST_USER1_PRINCIPAL;