From df8af1e0b6d4c2b880736dcf5f47a4121d9684c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 15:54:13 +0000 Subject: [PATCH 1/2] fix(station): fund external canisters only from the station balance External canisters registered for cycle monitoring without their own obtain-cycles options inherited the station's global obtain strategy. Register them with explicit obtain options that never obtain cycles, both when monitoring starts and when it is restored after an upgrade, and only accept an empty or `Disabled` obtain strategy when starting to monitor an external canister. The station and upgrader keep using the global strategy. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Rr5M1gYD2ZN6MC7Zs2UFxf --- .../impl/src/services/cycle_manager.rs | 48 ++++++++++++++++++- .../impl/src/services/external_canister.rs | 12 +++-- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/core/station/impl/src/services/cycle_manager.rs b/core/station/impl/src/services/cycle_manager.rs index 1ba091663..2a8e07982 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; @@ -69,6 +71,31 @@ impl CycleManager { )); } + /// 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); + }); + + ic_cdk::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()) } @@ -105,6 +132,25 @@ impl CycleManager { } } +/// 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..de04dd5fa 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, From 1b467674606d3a8381f43bdcfd25d8f51f0c716c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 19 Oct 2026 17:28:54 +0000 Subject: [PATCH 2/2] test(station): cover funding of monitored external canisters Add unit tests for starting and restoring external canister monitors with cycle obtain strategies, and an integration test that checks monitored external canisters are topped up from the station's balance, including after an upgrade, and never obtain cycles from the station's ICP account. Use the test-safe print helper in the cycle manager so that it can be exercised in unit tests. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Rr5M1gYD2ZN6MC7Zs2UFxf --- .../impl/src/services/cycle_manager.rs | 10 +- .../impl/src/services/external_canister.rs | 100 ++++++++++ tests/integration/src/cycles_monitor_tests.rs | 187 +++++++++++++++++- 3 files changed, 289 insertions(+), 8 deletions(-) diff --git a/core/station/impl/src/services/cycle_manager.rs b/core/station/impl/src/services/cycle_manager.rs index 2a8e07982..e53a1a002 100644 --- a/core/station/impl/src/services/cycle_manager.rs +++ b/core/station/impl/src/services/cycle_manager.rs @@ -66,7 +66,7 @@ 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." )); } @@ -91,7 +91,7 @@ impl CycleManager { manager.borrow_mut().register(canister_id, register_opts); }); - ic_cdk::print(format!( + crate::core::ic_cdk::api::print(format!( "Cycle manager: external canister {canister_id} added to cycle monitoring." )); } @@ -105,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." )); } @@ -115,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) { @@ -126,7 +126,7 @@ 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:?}." )); } diff --git a/core/station/impl/src/services/external_canister.rs b/core/station/impl/src/services/external_canister.rs index de04dd5fa..90ebea710 100644 --- a/core/station/impl/src/services/external_canister.rs +++ b/core/station/impl/src/services/external_canister.rs @@ -2362,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); +}