diff --git a/core/station/impl/src/services/cycle_manager.rs b/core/station/impl/src/services/cycle_manager.rs index 1ba091663..e53a1a002 100644 --- a/core/station/impl/src/services/cycle_manager.rs +++ b/core/station/impl/src/services/cycle_manager.rs @@ -7,7 +7,9 @@ use canfund::api::ledger::{CyclesLedgerCanister, IcLedgerCanister}; use canfund::manager::options::{FundManagerOptions, ObtainCyclesOptions}; use canfund::manager::record::CanisterRecord; use canfund::manager::RegisterOpts; -use canfund::operations::obtain::{MintCycles, WithdrawFromCyclesLedger}; +use canfund::operations::obtain::{ + MintCycles, ObtainCycles, ObtainCyclesError, WithdrawFromCyclesLedger, +}; use canfund::FundManager; use ic_cdk::api::management_canister::main::CanisterId; use ic_cdk::print; @@ -64,11 +66,36 @@ impl CycleManager { manager.borrow_mut().register(canister_id, register_opts); }); - ic_cdk::print(format!( + crate::core::ic_cdk::api::print(format!( "Cycle manager: canister {canister_id} added to cycle monitoring." )); } + /// Registers an external canister for monitoring. + /// + /// External canisters can only be funded from the station's own cycles balance. They are + /// registered with explicit obtain options that never obtain cycles, so canfund does not + /// fall back to the station's global obtain strategy (e.g. minting from a treasury account). + pub fn add_external_canister( + &self, + canister_id: CanisterId, + fund_strategy: MonitorExternalCanisterStrategy, + ) { + let register_opts = RegisterOpts::new() + .with_strategy(fund_strategy.into()) + .with_obtain_cycles_options(ObtainCyclesOptions { + obtain_cycles: Arc::new(NoObtainCycles), + }); + + FUND_MANAGER.with(|manager| { + manager.borrow_mut().register(canister_id, register_opts); + }); + + crate::core::ic_cdk::api::print(format!( + "Cycle manager: external canister {canister_id} added to cycle monitoring." + )); + } + pub fn get_canister(&self, canister_id: &CanisterId) -> Option { FUND_MANAGER.with(|manager| manager.borrow().get_canisters().get(canister_id).cloned()) } @@ -78,7 +105,7 @@ impl CycleManager { manager.borrow_mut().unregister(canister_id); }); - ic_cdk::print(format!( + crate::core::ic_cdk::api::print(format!( "Cycle manager: canister {canister_id} removed from cycle monitoring." )); } @@ -88,7 +115,7 @@ impl CycleManager { manager.borrow_mut().start(); }); - ic_cdk::print("Cycle manager: monitoring started."); + crate::core::ic_cdk::api::print("Cycle manager: monitoring started."); } pub fn set_global_cycle_obtain_strategy(&self, strategy: &CycleObtainStrategy) { @@ -99,12 +126,31 @@ impl CycleManager { fund_manager.with_options(options); }); - ic_cdk::print(format!( + crate::core::ic_cdk::api::print(format!( "Cycle manager: obtain cycles strategy changed to {strategy:?}." )); } } +/// Obtain cycles operation that never obtains cycles. +struct NoObtainCycles; + +#[async_trait::async_trait] +impl ObtainCycles for NoObtainCycles { + async fn obtain_cycles( + &self, + _amount: u128, + target_canister_id: Principal, + ) -> Result { + Err(ObtainCyclesError { + details: format!( + "Obtaining cycles is not supported for external canister {target_canister_id}" + ), + can_retry: false, + }) + } +} + fn get_obtain_cycle_config(strategy: &CycleObtainStrategy) -> Option { match strategy { CycleObtainStrategy::Disabled => None, diff --git a/core/station/impl/src/services/external_canister.rs b/core/station/impl/src/services/external_canister.rs index 69920387f..90ebea710 100644 --- a/core/station/impl/src/services/external_canister.rs +++ b/core/station/impl/src/services/external_canister.rs @@ -1038,10 +1038,9 @@ impl ExternalCanisterService { pub fn canister_monitor_restart(&self) { for canister in self.external_canister_repository.find_all() { if let Some(monitoring) = &canister.monitoring { - self.cycle_manager.add_canister( + self.cycle_manager.add_external_canister( canister.canister_id, monitoring.funding_strategy.clone(), - monitoring.cycle_obtain_strategy, ); } } @@ -1053,10 +1052,13 @@ impl ExternalCanisterService { strategy: MonitorExternalCanisterStrategy, cycle_obtain_strategy: Option, ) -> ServiceResult<()> { - if let Some(CycleObtainStrategy::MintFromNativeToken { .. }) = cycle_obtain_strategy { + if !matches!( + cycle_obtain_strategy, + None | Some(CycleObtainStrategy::Disabled) + ) { Err(ExternalCanisterError::Failed { reason: format!( - "Minting from native token is not yet supported for canister {}", + "Obtaining cycles is not supported for canister {}", canister_id.to_text() ), })?; @@ -1073,7 +1075,7 @@ impl ExternalCanisterService { } self.cycle_manager - .add_canister(canister_id, strategy.clone(), cycle_obtain_strategy); + .add_external_canister(canister_id, strategy.clone()); external_canister.monitoring = Some(ExternalCanisterMonitoring { funding_strategy: strategy, @@ -2360,6 +2362,106 @@ mod tests { }) ); } + + #[test] + fn canister_monitor_start_rejects_cycle_obtain_strategies() { + setup(); + let account = crate::models::account_test_utils::mock_account(); + crate::repositories::ACCOUNT_REPOSITORY.insert(account.to_key(), account.clone()); + let external_canister = + crate::models::external_canister_test_utils::mock_external_canister(); + EXTERNAL_CANISTER_REPOSITORY.insert(external_canister.key(), external_canister.clone()); + + for cycle_obtain_strategy in [ + CycleObtainStrategy::MintFromNativeToken { + account_id: account.id, + }, + CycleObtainStrategy::WithdrawFromCyclesLedger { + account_id: account.id, + }, + ] { + let result = EXTERNAL_CANISTER_SERVICE.canister_monitor_start( + external_canister.canister_id, + MonitorExternalCanisterStrategy::Always(1_000_000_000_000), + Some(cycle_obtain_strategy), + ); + + assert!(result.is_err()); + assert!(CYCLE_MANAGER + .get_canister(&external_canister.canister_id) + .is_none()); + } + + for cycle_obtain_strategy in [None, Some(CycleObtainStrategy::Disabled)] { + let external_canister = + crate::models::external_canister_test_utils::mock_external_canister(); + EXTERNAL_CANISTER_REPOSITORY.insert(external_canister.key(), external_canister.clone()); + + EXTERNAL_CANISTER_SERVICE + .canister_monitor_start( + external_canister.canister_id, + MonitorExternalCanisterStrategy::Always(1_000_000_000_000), + cycle_obtain_strategy, + ) + .expect("Failed to start monitoring the external canister"); + } + } + + #[tokio::test] + async fn monitored_external_canisters_never_obtain_cycles() { + setup(); + let account = crate::models::account_test_utils::mock_account(); + crate::repositories::ACCOUNT_REPOSITORY.insert(account.to_key(), account.clone()); + CYCLE_MANAGER.set_global_cycle_obtain_strategy(&CycleObtainStrategy::MintFromNativeToken { + account_id: account.id, + }); + + // a monitor started through the service + let started = crate::models::external_canister_test_utils::mock_external_canister(); + EXTERNAL_CANISTER_REPOSITORY.insert(started.key(), started.clone()); + EXTERNAL_CANISTER_SERVICE + .canister_monitor_start( + started.canister_id, + MonitorExternalCanisterStrategy::Always(1_000_000_000_000), + None, + ) + .expect("Failed to start monitoring the external canister"); + + // a monitor restored from storage with a per-canister obtain strategy + let mut restored = crate::models::external_canister_test_utils::mock_external_canister(); + restored.monitoring = Some(ExternalCanisterMonitoring { + funding_strategy: MonitorExternalCanisterStrategy::Always(1_000_000_000_000), + cycle_obtain_strategy: Some(CycleObtainStrategy::WithdrawFromCyclesLedger { + account_id: account.id, + }), + }); + EXTERNAL_CANISTER_REPOSITORY.insert(restored.key(), restored.clone()); + EXTERNAL_CANISTER_SERVICE.canister_monitor_restart(); + + // changing the global strategy afterwards must not affect the external canisters + CYCLE_MANAGER.set_global_cycle_obtain_strategy( + &CycleObtainStrategy::WithdrawFromCyclesLedger { + account_id: account.id, + }, + ); + + for canister_id in [started.canister_id, restored.canister_id] { + let record = CYCLE_MANAGER + .get_canister(&canister_id) + .expect("External canister is not monitored"); + let obtain_cycles_options = record + .get_obtain_cycles_options() + .clone() + .expect("External canister has no obtain cycles options"); + + let error = obtain_cycles_options + .obtain_cycles + .obtain_cycles(1_000_000_000_000, canister_id) + .await + .expect_err("External canister must not obtain cycles"); + assert!(!error.can_retry); + } + } } #[cfg(feature = "canbench")] diff --git a/tests/integration/src/cycles_monitor_tests.rs b/tests/integration/src/cycles_monitor_tests.rs index 8f3419435..01fce080d 100644 --- a/tests/integration/src/cycles_monitor_tests.rs +++ b/tests/integration/src/cycles_monitor_tests.rs @@ -3,8 +3,9 @@ use crate::setup::{ get_canister_wasm, setup_new_env, setup_new_env_with_config, SetupConfig, WALLET_ADMIN_USER, }; use crate::utils::{ - advance_time_to_burn_cycles, create_icp_account, get_core_canister_health_status, - get_icp_account_identifier, get_system_info, get_user, user_test_id, NNS_ROOT_CANISTER_ID, + advance_time_to_burn_cycles, create_icp_account, execute_request, + get_core_canister_health_status, get_icp_account_identifier, get_system_info, get_user, + upgrade_station, user_test_id, NNS_ROOT_CANISTER_ID, }; use crate::TestEnv; use control_panel_api::{ @@ -14,9 +15,19 @@ use control_panel_api::{ use ic_ledger_types::AccountIdentifier; use orbit_essentials::api::ApiResult; +use candid::Principal; use pocket_ic::update_candid_as; +use pocket_ic::PocketIc; use sha2::{Digest, Sha256}; -use station_api::{HealthStatus, SystemInfoResponse}; +use station_api::{ + AllowDTO, AuthScopeDTO, CreateExternalCanisterOperationInput, + CreateExternalCanisterOperationKindCreateNewDTO, CreateExternalCanisterOperationKindDTO, + CycleObtainStrategyInput, ExternalCanisterPermissionsCreateInput, + ExternalCanisterRequestPoliciesCreateInput, HealthStatus, + MonitorExternalCanisterOperationInput, MonitorExternalCanisterOperationKindDTO, + MonitorExternalCanisterStartInput, MonitorExternalCanisterStrategyDTO, RequestOperationDTO, + RequestOperationInput, SystemInfoResponse, +}; use std::time::Duration; #[test] @@ -242,3 +253,173 @@ fn can_mint_cycles_to_top_up_self() { && post_cycle_balance - pre_cycle_balance < 300_000_000_000 ); } + +fn create_external_canister(env: &PocketIc, station_id: Principal, name: &str) -> Principal { + let restricted = AllowDTO { + auth_scope: AuthScopeDTO::Restricted, + user_groups: vec![], + users: vec![], + }; + let request = execute_request( + env, + WALLET_ADMIN_USER, + station_id, + RequestOperationInput::CreateExternalCanister(CreateExternalCanisterOperationInput { + kind: CreateExternalCanisterOperationKindDTO::CreateNew( + CreateExternalCanisterOperationKindCreateNewDTO { + initial_cycles: Some(500_000_000_000), + subnet_selection: None, + }, + ), + name: name.to_string(), + description: None, + labels: None, + metadata: None, + permissions: ExternalCanisterPermissionsCreateInput { + calls: vec![], + read: restricted.clone(), + change: restricted, + }, + request_policies: ExternalCanisterRequestPoliciesCreateInput { + change: vec![], + calls: vec![], + }, + }), + ) + .unwrap(); + + match request.operation { + RequestOperationDTO::CreateExternalCanister(operation) => operation.canister_id.unwrap(), + operation => panic!("Unexpected request operation type: {operation:?}"), + } +} + +fn monitor_external_canister( + env: &PocketIc, + station_id: Principal, + canister_id: Principal, + fund_cycles: u128, + cycle_obtain_strategy: Option, +) -> bool { + execute_request( + env, + WALLET_ADMIN_USER, + station_id, + RequestOperationInput::MonitorExternalCanister(MonitorExternalCanisterOperationInput { + canister_id, + kind: MonitorExternalCanisterOperationKindDTO::Start( + MonitorExternalCanisterStartInput { + funding_strategy: MonitorExternalCanisterStrategyDTO::Always(fund_cycles), + cycle_obtain_strategy, + }, + ), + }), + ) + .is_ok() +} + +fn run_fund_manager(env: &PocketIc) { + env.advance_time(Duration::from_secs(24 * 60 * 60)); + for _ in 0..20 { + env.tick(); + } +} + +#[test] +fn external_canisters_are_funded_only_from_station_balance() { + let TestEnv { + env, + canister_ids, + controller, + .. + } = setup_new_env_with_config(SetupConfig { + start_cycles: Some(10_000_000_000_000), + ..Default::default() + }); + let station_id = canister_ids.station; + + // keep the station and upgrader well funded so that neither needs to obtain cycles + let upgrader_id = get_system_info(&env, WALLET_ADMIN_USER, station_id).upgrader_id; + env.add_cycles(upgrader_id, 100_000_000_000_000); + env.add_cycles(station_id, 100_000_000_000_000); + + // the station can afford to fund this canister from its own balance + let funded = create_external_canister(&env, station_id, "funded"); + assert!(monitor_external_canister( + &env, + station_id, + funded, + 1_000_000_000_000, + None + )); + + // the station cannot afford to fund this canister from its own balance, but minting from + // the ICP account could + let unfunded = create_external_canister(&env, station_id, "unfunded"); + assert!(monitor_external_canister( + &env, + station_id, + unfunded, + 200_000_000_000_000, + Some(CycleObtainStrategyInput::Disabled) + )); + + // creating the first ICP account enables minting cycles for the station after the + // external canisters are monitored + let user = get_user(&env, WALLET_ADMIN_USER, station_id); + let account = create_icp_account(&env, station_id, user.id); + let account_identifier = AccountIdentifier::from_hex( + &get_icp_account_identifier(&account.addresses).expect("no icp address found"), + ) + .unwrap(); + send_icp_to_account( + &env, + controller, + account_identifier, + 100 * ICP, + 0, + None, + None, + ) + .unwrap(); + + // external canisters cannot be configured to obtain cycles + let rejected = create_external_canister(&env, station_id, "rejected"); + for cycle_obtain_strategy in [ + CycleObtainStrategyInput::MintFromNativeToken { + account_id: account.id.clone(), + }, + CycleObtainStrategyInput::WithdrawFromCyclesLedger { + account_id: account.id.clone(), + }, + ] { + assert!(!monitor_external_canister( + &env, + station_id, + rejected, + 1_000_000_000_000, + Some(cycle_obtain_strategy) + )); + } + + let assert_funding_round = |env: &PocketIc| { + let pre_funded_balance = env.cycle_balance(funded); + let pre_unfunded_balance = env.cycle_balance(unfunded); + + run_fund_manager(env); + + let funded_top_up = env.cycle_balance(funded) - pre_funded_balance; + assert!( + funded_top_up > 990_000_000_000 && funded_top_up <= 1_000_000_000_000, + "unexpected top up of {funded_top_up} cycles" + ); + assert!(env.cycle_balance(unfunded) <= pre_unfunded_balance); + assert_eq!(get_icp_account_balance(env, account_identifier), 100 * ICP); + }; + + assert_funding_round(&env); + + // monitoring is restored after an upgrade with the same behavior + upgrade_station(&env, WALLET_ADMIN_USER, station_id, None); + assert_funding_round(&env); +}