From e30aaaf82da62769436519a1222a77183c3c94ad Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Wed, 31 Jan 2024 17:19:00 +0000 Subject: [PATCH] fix: only reveal cycles top up balance of frozen canisters to controllers --- rs/cycles_account_manager/src/lib.rs | 28 +++++- .../tests/cycles_account_manager.rs | 98 ++++++++++++++++--- .../src/canister_manager.rs | 4 + .../src/canister_manager/tests.rs | 45 +++++++++ .../src/execution/replicated_query.rs | 2 + .../src/execution/response.rs | 5 + .../src/execution/update.rs | 10 ++ .../src/execution_environment.rs | 5 + rs/ingress_manager/src/ingress_selector.rs | 1 + .../src/execution_environment/errors.rs | 13 ++- rs/messaging/src/scheduling/valid_set_rule.rs | 2 + rs/system_api/src/lib.rs | 1 + .../src/sandbox_safe_system_state.rs | 6 ++ rs/system_api/tests/system_api.rs | 2 + rs/types/types/src/messages.rs | 7 ++ 15 files changed, 210 insertions(+), 19 deletions(-) diff --git a/rs/cycles_account_manager/src/lib.rs b/rs/cycles_account_manager/src/lib.rs index 9d66cef6e28..642dc4c49b2 100644 --- a/rs/cycles_account_manager/src/lib.rs +++ b/rs/cycles_account_manager/src/lib.rs @@ -374,6 +374,7 @@ impl CyclesAccountManager { cycles: Cycles, subnet_size: usize, reserved_balance: Cycles, + reveal_top_up: bool, ) -> Result<(), CanisterOutOfCyclesError> { self.withdraw_with_threshold( canister_id, @@ -388,6 +389,7 @@ impl CyclesAccountManager { subnet_size, reserved_balance, ), + reveal_top_up, ) } @@ -408,6 +410,7 @@ impl CyclesAccountManager { canister_compute_allocation: ComputeAllocation, cycles: Cycles, subnet_size: usize, + reveal_top_up: bool, ) -> Result<(), CanisterOutOfCyclesError> { let threshold = self.freeze_threshold_cycles( canister.system_state.freeze_threshold, @@ -425,6 +428,7 @@ impl CyclesAccountManager { available: canister.system_state.debited_balance(), requested: cycles, threshold, + reveal_top_up, }); } canister @@ -437,6 +441,7 @@ impl CyclesAccountManager { cycles, threshold, CyclesUseCase::IngressInduction, + reveal_top_up, ) } } @@ -460,6 +465,7 @@ impl CyclesAccountManager { cycles: Cycles, subnet_size: usize, use_case: CyclesUseCase, + reveal_top_up: bool, ) -> Result<(), CanisterOutOfCyclesError> { let threshold = self.freeze_threshold_cycles( system_state.freeze_threshold, @@ -470,7 +476,7 @@ impl CyclesAccountManager { subnet_size, system_state.reserved_balance(), ); - self.consume_with_threshold(system_state, cycles, threshold, use_case) + self.consume_with_threshold(system_state, cycles, threshold, use_case, reveal_top_up) } /// Prepays the cost of executing a message with the given number of @@ -491,6 +497,7 @@ impl CyclesAccountManager { canister_compute_allocation: ComputeAllocation, num_instructions: NumInstructions, subnet_size: usize, + reveal_top_up: bool, ) -> Result { let cost = self.execution_cost(num_instructions, subnet_size); self.consume_with_threshold( @@ -506,6 +513,7 @@ impl CyclesAccountManager { system_state.reserved_balance(), ), CyclesUseCase::Instructions, + reveal_top_up, ) .map(|_| cost) } @@ -724,6 +732,7 @@ impl CyclesAccountManager { prepayment_for_response_transmission: Cycles, subnet_size: usize, reserved_balance: Cycles, + reveal_top_up: bool, ) -> Result, CanisterOutOfCyclesError> { // The total amount charged consists of: // - the fee to do the xnet call (request + response) @@ -751,6 +760,7 @@ impl CyclesAccountManager { subnet_size, reserved_balance, ), + reveal_top_up, )?; Ok(Vec::from([ @@ -830,6 +840,7 @@ impl CyclesAccountManager { canister_current_message_memory_usage: NumBytes, canister_compute_allocation: ComputeAllocation, subnet_size: usize, + reveal_top_up: bool, ) -> Result<(), CanisterOutOfCyclesError> { let threshold = self.freeze_threshold_cycles( system_state.freeze_threshold, @@ -847,6 +858,7 @@ impl CyclesAccountManager { available: system_state.balance(), requested, threshold, + reveal_top_up, }) } else { Ok(()) @@ -861,6 +873,7 @@ impl CyclesAccountManager { cycles: Cycles, threshold: Cycles, use_case: CyclesUseCase, + reveal_top_up: bool, ) -> Result<(), CanisterOutOfCyclesError> { let effective_cycles_balance = match use_case { CyclesUseCase::Memory | CyclesUseCase::ComputeAllocation | CyclesUseCase::Uninstall => { @@ -884,6 +897,7 @@ impl CyclesAccountManager { effective_cycles_balance, cycles, threshold, + reveal_top_up, )?; debug_assert_ne!(use_case, CyclesUseCase::NonConsumed); @@ -897,6 +911,7 @@ impl CyclesAccountManager { cycles_balance: Cycles, cycles: Cycles, threshold: Cycles, + reveal_top_up: bool, ) -> Result<(), CanisterOutOfCyclesError> { let cycles_available = if cycles_balance > threshold { cycles_balance - threshold @@ -910,6 +925,7 @@ impl CyclesAccountManager { available: cycles_balance, requested: cycles, threshold, + reveal_top_up, }); } Ok(()) @@ -930,8 +946,15 @@ impl CyclesAccountManager { cycles_balance: &mut Cycles, cycles: Cycles, threshold: Cycles, + reveal_top_up: bool, ) -> Result<(), CanisterOutOfCyclesError> { - self.verify_cycles_balance_with_threshold(canister_id, *cycles_balance, cycles, threshold)?; + self.verify_cycles_balance_with_threshold( + canister_id, + *cycles_balance, + cycles, + threshold, + reveal_top_up, + )?; *cycles_balance -= cycles; Ok(()) @@ -1052,6 +1075,7 @@ impl CyclesAccountManager { cycles, Cycles::zero(), use_case, + false, // caller is system => no need to reveal top up balance ) { info!( log, diff --git a/rs/cycles_account_manager/tests/cycles_account_manager.rs b/rs/cycles_account_manager/tests/cycles_account_manager.rs index a76affd50e7..769f2bc3f63 100644 --- a/rs/cycles_account_manager/tests/cycles_account_manager.rs +++ b/rs/cycles_account_manager/tests/cycles_account_manager.rs @@ -109,6 +109,7 @@ fn withdraw_cycles_with_not_enough_balance_returns_error() { amount, SMALL_APP_SUBNET_MAX_SIZE, system_state.reserved_balance(), + false, ), Ok(()) ); @@ -146,6 +147,7 @@ fn withdraw_cycles_with_not_enough_balance_returns_error() { amount, SMALL_APP_SUBNET_MAX_SIZE, system_state.reserved_balance(), + false, ), Ok(()) ); @@ -185,6 +187,7 @@ fn withdraw_cycles_with_not_enough_balance_returns_error() { amount, SMALL_APP_SUBNET_MAX_SIZE, system_state.reserved_balance(), + false, ), Ok(()) ); @@ -222,6 +225,7 @@ fn withdraw_cycles_with_not_enough_balance_returns_error() { amount, SMALL_APP_SUBNET_MAX_SIZE, system_state.reserved_balance(), + false, ), Err(CanisterOutOfCyclesError { canister_id: canister_test_id(1), @@ -235,7 +239,8 @@ fn withdraw_cycles_with_not_enough_balance_returns_error() { ComputeAllocation::default(), SMALL_APP_SUBNET_MAX_SIZE, system_state.reserved_balance(), - ) + ), + reveal_top_up: false, }) ); } @@ -258,6 +263,7 @@ fn verify_no_cycles_charged_for_message_execution_on_system_subnets() { ComputeAllocation::default(), NumInstructions::from(1_000_000), subnet_size, + false, ) .unwrap(); assert_eq!(system_state.balance(), initial_balance); @@ -387,7 +393,8 @@ fn cycles_withdraw_no_threshold() { system_state.canister_id, &mut balance, Cycles::zero(), - threshold + threshold, + false, ) .is_ok()); // unchanged cycles @@ -397,26 +404,50 @@ fn cycles_withdraw_no_threshold() { // withdraw i128::MAX and verify correctness let amount = Cycles::from(i128::MAX as u128); assert!(cycles_account_manager - .withdraw_with_threshold(system_state.canister_id, &mut balance, amount, threshold) + .withdraw_with_threshold( + system_state.canister_id, + &mut balance, + amount, + threshold, + false + ) .is_ok()); cycles_balance_expected -= amount; assert_eq!(balance, Cycles::from(i128::MAX as u128) + Cycles::new(1)); assert!(cycles_account_manager - .withdraw_with_threshold(system_state.canister_id, &mut balance, amount, threshold) + .withdraw_with_threshold( + system_state.canister_id, + &mut balance, + amount, + threshold, + false + ) .is_ok()); cycles_balance_expected -= amount; assert_eq!(balance, Cycles::new(1)); let amount = Cycles::new(1); assert!(cycles_account_manager - .withdraw_with_threshold(system_state.canister_id, &mut balance, amount, threshold) + .withdraw_with_threshold( + system_state.canister_id, + &mut balance, + amount, + threshold, + false + ) .is_ok()); cycles_balance_expected -= amount; assert_eq!(balance, Cycles::zero()); assert!(cycles_account_manager - .withdraw_with_threshold(system_state.canister_id, &mut balance, amount, threshold) + .withdraw_with_threshold( + system_state.canister_id, + &mut balance, + amount, + threshold, + false + ) .is_err()); cycles_balance_expected -= amount; assert_eq!(balance, Cycles::zero()); @@ -440,6 +471,7 @@ fn test_consume_with_threshold() { Cycles::zero(), threshold, CyclesUseCase::Memory, + false, ) .is_ok()); // unchanged cycles @@ -449,7 +481,13 @@ fn test_consume_with_threshold() { // withdraw i128::MAX and verify correctness let amount = Cycles::from(i128::MAX as u128); assert!(cycles_account_manager - .consume_with_threshold(&mut system_state, amount, threshold, CyclesUseCase::Memory,) + .consume_with_threshold( + &mut system_state, + amount, + threshold, + CyclesUseCase::Memory, + false + ) .is_ok()); cycles_balance_expected -= amount; assert_eq!( @@ -458,20 +496,38 @@ fn test_consume_with_threshold() { ); assert!(cycles_account_manager - .consume_with_threshold(&mut system_state, amount, threshold, CyclesUseCase::Memory,) + .consume_with_threshold( + &mut system_state, + amount, + threshold, + CyclesUseCase::Memory, + false + ) .is_ok()); cycles_balance_expected -= amount; assert_eq!(system_state.balance(), Cycles::new(1)); let amount = Cycles::new(1); assert!(cycles_account_manager - .consume_with_threshold(&mut system_state, amount, threshold, CyclesUseCase::Memory) + .consume_with_threshold( + &mut system_state, + amount, + threshold, + CyclesUseCase::Memory, + false + ) .is_ok()); cycles_balance_expected -= amount; assert_eq!(system_state.balance(), Cycles::zero()); assert!(cycles_account_manager - .consume_with_threshold(&mut system_state, amount, threshold, CyclesUseCase::Memory) + .consume_with_threshold( + &mut system_state, + amount, + threshold, + CyclesUseCase::Memory, + false + ) .is_err()); cycles_balance_expected -= amount; assert_eq!(system_state.balance(), Cycles::zero()); @@ -515,6 +571,7 @@ fn cycles_withdraw_for_execution() { amount, SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Instructions, + false, ) .is_ok()); assert_eq!(system_state.balance(), initial_cycles - amount); @@ -527,6 +584,7 @@ fn cycles_withdraw_for_execution() { amount, SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Instructions, + false, ) .is_err()); @@ -539,7 +597,8 @@ fn cycles_withdraw_for_execution() { memory_usage, message_memory_usage, compute_allocation, - SMALL_APP_SUBNET_MAX_SIZE + SMALL_APP_SUBNET_MAX_SIZE, + false, ) .is_ok()); assert!(cycles_account_manager @@ -551,6 +610,7 @@ fn cycles_withdraw_for_execution() { exec_cycles_max, SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Instructions, + false, ) .is_ok()); assert_eq!(system_state.balance(), freeze_threshold_cycles); @@ -561,13 +621,15 @@ fn cycles_withdraw_for_execution() { memory_usage, message_memory_usage, compute_allocation, - SMALL_APP_SUBNET_MAX_SIZE + SMALL_APP_SUBNET_MAX_SIZE, + false, ), Err(CanisterOutOfCyclesError { canister_id, available: freeze_threshold_cycles, requested: Cycles::new(10), threshold: freeze_threshold_cycles, + reveal_top_up: false, }) ); @@ -581,6 +643,7 @@ fn cycles_withdraw_for_execution() { exec_cycles_max, SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Instructions, + false, ) .is_err()); assert!(cycles_account_manager @@ -592,6 +655,7 @@ fn cycles_withdraw_for_execution() { Cycles::new(10), SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Instructions, + false, ) .is_err()); assert!(cycles_account_manager @@ -603,6 +667,7 @@ fn cycles_withdraw_for_execution() { Cycles::new(1), SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Instructions, + false, ) .is_err()); assert!(cycles_account_manager @@ -614,6 +679,7 @@ fn cycles_withdraw_for_execution() { Cycles::zero(), SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Instructions, + false, ) .is_ok()); assert_eq!(system_state.balance(), freeze_threshold_cycles); @@ -637,6 +703,7 @@ fn withdraw_execution_cycles_consumes_cycles() { ComputeAllocation::default(), NumInstructions::from(1_000_000), SMALL_APP_SUBNET_MAX_SIZE, + false, ) .unwrap(); let consumed_cycles_after = system_state @@ -667,6 +734,7 @@ fn withdraw_for_transfer_does_not_consume_cycles() { Cycles::new(1_000_000), SMALL_APP_SUBNET_MAX_SIZE, system_state.reserved_balance(), + false, ) .unwrap(); let consumed_cycles_after = system_state @@ -696,6 +764,7 @@ fn consume_cycles_updates_consumed_cycles() { Cycles::new(1_000_000), SMALL_APP_SUBNET_MAX_SIZE, CyclesUseCase::Memory, + false, ) .unwrap(); let consumed_cycles_after = system_state @@ -723,6 +792,7 @@ fn consume_cycles_for_memory_drains_reserved_balance() { Cycles::new(2_000_000), Cycles::new(0), CyclesUseCase::Memory, + false, ) .unwrap(); assert_eq!(system_state.reserved_balance(), Cycles::new(0)); @@ -744,6 +814,7 @@ fn consume_cycles_for_compute_drains_reserved_balance() { Cycles::new(2_000_000), Cycles::new(0), CyclesUseCase::ComputeAllocation, + false, ) .unwrap(); assert_eq!(system_state.reserved_balance(), Cycles::new(0)); @@ -765,6 +836,7 @@ fn consume_cycles_for_uninstall_drains_reserved_balance() { Cycles::new(2_000_000), Cycles::new(0), CyclesUseCase::Uninstall, + false, ) .unwrap(); assert_eq!(system_state.reserved_balance(), Cycles::new(0)); @@ -786,6 +858,7 @@ fn consume_cycles_for_execution_does_not_drain_reserved_balance() { Cycles::new(2_000_000), Cycles::new(0), CyclesUseCase::Instructions, + false, ) .unwrap(); assert_eq!(system_state.reserved_balance(), Cycles::new(1_000_000)); @@ -815,6 +888,7 @@ fn withdraw_cycles_for_transfer_checks_reserved_balance() { Cycles::new(1_000_000), SMALL_APP_SUBNET_MAX_SIZE, system_state.reserved_balance(), + false, ) .unwrap(); assert_eq!(Cycles::zero(), new_balance); diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index dff3d639d76..0765b8072c6 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -918,6 +918,7 @@ impl CanisterManager { None => { let memory_usage = canister.memory_usage(); let message_memory_usage = canister.message_memory_usage(); + let reveal_top_up = canister.controllers().contains(message.sender()); match self.cycles_account_manager.prepay_execution_cycles( &mut canister.system_state, memory_usage, @@ -925,6 +926,7 @@ impl CanisterManager { execution_parameters.compute_allocation, execution_parameters.instruction_limits.message(), subnet_size, + reveal_top_up, ) { Ok(cycles) => cycles, Err(err) => { @@ -1588,6 +1590,7 @@ impl CanisterManager { let current_memory_usage = canister.memory_usage(); let message_memory = canister.message_memory_usage(); let compute_allocation = canister.compute_allocation(); + let reveal_top_up = canister.controllers().contains(&sender); // Charge for the upload. let prepaid_cycles = self .cycles_account_manager @@ -1598,6 +1601,7 @@ impl CanisterManager { compute_allocation, instructions, subnet_size, + reveal_top_up, ) .map_err(|err| CanisterManagerError::WasmChunkStoreError { message: format!("Error charging for 'upload_chunk': {}", err), diff --git a/rs/execution_environment/src/canister_manager/tests.rs b/rs/execution_environment/src/canister_manager/tests.rs index 173c543ac7f..c4db977ca19 100644 --- a/rs/execution_environment/src/canister_manager/tests.rs +++ b/rs/execution_environment/src/canister_manager/tests.rs @@ -4475,6 +4475,51 @@ fn unfreezing_of_frozen_canister() { get_reply(result); } +#[test] +fn frozen_canister_reveal_top_up() { + let mut test = ExecutionTestBuilder::new().build(); + let canister_id = test + .universal_canister_with_cycles(Cycles::new(1_000_000_000_000)) + .unwrap(); + + // Set the freezing threshold high to freeze the canister. + let payload = UpdateSettingsArgs { + canister_id: canister_id.get(), + settings: CanisterSettingsArgsBuilder::new() + .with_freezing_threshold(1_000_000_000_000) + .build(), + sender_canister_version: None, + } + .encode(); + test.subnet_message(Method::UpdateSettings, payload) + .unwrap(); + + // Sending an ingress message to a frozen canister fails with a verbose error message. + let err = test + .ingress(canister_id, "update", wasm().reply().build()) + .unwrap_err(); + assert_eq!(ErrorCode::CanisterOutOfCycles, err.code()); + assert!(err.description().starts_with(&format!( + "Canister {} is out of cycles: please top up the canister with at least", + canister_id + ))); + + // Blackhole the canister. + test.canister_update_controller(canister_id, vec![]) + .unwrap(); + + // Sending an ingress message to a frozen canister fails without revealing + // top up balance to non-controllers. + let err = test + .ingress(canister_id, "update", wasm().reply().build()) + .unwrap_err(); + assert_eq!(ErrorCode::CanisterOutOfCycles, err.code()); + assert_eq!( + err.description(), + format!("Canister {} is out of cycles", canister_id) + ); +} + #[test] fn create_canister_fails_if_memory_capacity_exceeded() { let mut test = ExecutionTestBuilder::new() diff --git a/rs/execution_environment/src/execution/replicated_query.rs b/rs/execution_environment/src/execution/replicated_query.rs index 1494d44d37d..37681ac8383 100644 --- a/rs/execution_environment/src/execution/replicated_query.rs +++ b/rs/execution_environment/src/execution/replicated_query.rs @@ -41,6 +41,7 @@ pub fn execute_replicated_query( let message_memory_usage = canister.message_memory_usage(); let compute_allocation = canister.scheduler_state.compute_allocation; + let reveal_top_up = canister.controllers().contains(req.sender()); let prepaid_execution_cycles = match round.cycles_account_manager.prepay_execution_cycles( &mut canister.system_state, memory_usage, @@ -48,6 +49,7 @@ pub fn execute_replicated_query( compute_allocation, instruction_limit, subnet_size, + reveal_top_up, ) { Ok(cycles) => cycles, Err(err) => { diff --git a/rs/execution_environment/src/execution/response.rs b/rs/execution_environment/src/execution/response.rs index 5d0b963c3bd..805200cf1a8 100644 --- a/rs/execution_environment/src/execution/response.rs +++ b/rs/execution_environment/src/execution/response.rs @@ -372,11 +372,16 @@ impl ResponseHelper { let requested = state_changes.system_state_changes.removed_cycles(); // Note that we ignore the freezing threshold as required by the spec. if old_balance < requested { + let reveal_top_up = self + .canister + .controllers() + .contains(&original.call_origin.get_principal()); let err = CanisterOutOfCyclesError { canister_id: self.canister.canister_id(), available: old_balance, requested, threshold: original.freezing_threshold, + reveal_top_up, }; info!( round.log, diff --git a/rs/execution_environment/src/execution/update.rs b/rs/execution_environment/src/execution/update.rs index 25889af8e5a..84356d46b19 100644 --- a/rs/execution_environment/src/execution/update.rs +++ b/rs/execution_environment/src/execution/update.rs @@ -51,6 +51,10 @@ pub fn execute_update( let mut canister = clean_canister; let memory_usage = canister.memory_usage(); let message_memory_usage = canister.message_memory_usage(); + let reveal_top_up = call_or_task + .caller() + .map(|caller| canister.controllers().contains(&caller)) + .unwrap_or_default(); let prepaid_execution_cycles = match round.cycles_account_manager.prepay_execution_cycles( &mut canister.system_state, @@ -59,6 +63,7 @@ pub fn execute_update( execution_parameters.compute_allocation, execution_parameters.instruction_limits.message(), subnet_size, + reveal_top_up, ) { Ok(cycles) => cycles, Err(err) => { @@ -371,12 +376,17 @@ impl UpdateHelper { if let Some(state_changes) = &canister_state_changes { let old_balance = self.canister.system_state.balance(); let requested = state_changes.system_state_changes.removed_cycles(); + let reveal_top_up = self + .canister + .controllers() + .contains(&original.call_origin.get_principal()); if old_balance < requested + original.freezing_threshold { let err = CanisterOutOfCyclesError { canister_id: self.canister.canister_id(), available: old_balance, requested, threshold: original.freezing_threshold, + reveal_top_up, }; let err = UserError::new(ErrorCode::CanisterOutOfCycles, err); info!( diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index 255c585acf2..6e546eb5cea 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -696,6 +696,7 @@ impl ExecutionEnvironment { induction_cost, registry_settings.subnet_size, CyclesUseCase::IngressInduction, + false, // we ignore the error anyway => no need to reveal top up balance ); } } @@ -1837,6 +1838,9 @@ impl ExecutionEnvironment { if let IngressInductionCost::Fee { payer, cost } = induction_cost { let paying_canister = canister(payer)?; + let reveal_top_up = paying_canister + .controllers() + .contains(&ingress.sender().get()); if let Err(err) = self.cycles_account_manager.can_withdraw_cycles( &paying_canister.system_state, cost, @@ -1844,6 +1848,7 @@ impl ExecutionEnvironment { paying_canister.message_memory_usage(), paying_canister.scheduler_state.compute_allocation, subnet_size, + reveal_top_up, ) { return Err(UserError::new( ErrorCode::CanisterOutOfCycles, diff --git a/rs/ingress_manager/src/ingress_selector.rs b/rs/ingress_manager/src/ingress_selector.rs index 6dc03619f0e..adab966f0a5 100644 --- a/rs/ingress_manager/src/ingress_selector.rs +++ b/rs/ingress_manager/src/ingress_selector.rs @@ -480,6 +480,7 @@ impl IngressManager { canister.message_memory_usage(), canister.scheduler_state.compute_allocation, subnet_size, + false, // error here is not returned back to the user => no need to reveal top up balance ) { return Err(ValidationError::Permanent( IngressPermanentError::InsufficientCycles(err), diff --git a/rs/interfaces/src/execution_environment/errors.rs b/rs/interfaces/src/execution_environment/errors.rs index 75c17be79e2..550e1694c11 100644 --- a/rs/interfaces/src/execution_environment/errors.rs +++ b/rs/interfaces/src/execution_environment/errors.rs @@ -50,17 +50,20 @@ pub struct CanisterOutOfCyclesError { pub available: Cycles, pub requested: Cycles, pub threshold: Cycles, + pub reveal_top_up: bool, } impl std::error::Error for CanisterOutOfCyclesError {} impl std::fmt::Display for CanisterOutOfCyclesError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!( - f, - "Canister {} is out of cycles: please top up the canister with at least {} additional cycles", - self.canister_id, (self.threshold + self.requested) - self.available - ) + let msg = if self.reveal_top_up { + format!("Canister {} is out of cycles: please top up the canister with at least {} additional cycles", + self.canister_id, (self.threshold + self.requested) - self.available) + } else { + format!("Canister {} is out of cycles", self.canister_id) + }; + write!(f, "{}", msg) } } diff --git a/rs/messaging/src/scheduling/valid_set_rule.rs b/rs/messaging/src/scheduling/valid_set_rule.rs index 44b306a719b..ca7c0241168 100644 --- a/rs/messaging/src/scheduling/valid_set_rule.rs +++ b/rs/messaging/src/scheduling/valid_set_rule.rs @@ -294,6 +294,7 @@ impl ValidSetRuleImpl { let memory_usage = canister.memory_usage(); let message_memory_usage = canister.message_memory_usage(); let compute_allocation = canister.scheduler_state.compute_allocation; + let reveal_top_up = canister.controllers().contains(&ingress.source.get()); if let Err(err) = self.cycles_account_manager.charge_ingress_induction_cost( canister, memory_usage, @@ -301,6 +302,7 @@ impl ValidSetRuleImpl { compute_allocation, cost, subnet_size, + reveal_top_up, ) { return Err(StateError::CanisterOutOfCycles(err)); } diff --git a/rs/system_api/src/lib.rs b/rs/system_api/src/lib.rs index 6c5003c4030..0a055afc57c 100644 --- a/rs/system_api/src/lib.rs +++ b/rs/system_api/src/lib.rs @@ -1111,6 +1111,7 @@ impl SystemApiImpl { self.memory_usage.current_usage, self.memory_usage.current_message_usage, amount, + false, // synchronous error => no need to reveal top up balance )?; request.add_cycles(amount); Ok(()) diff --git a/rs/system_api/src/sandbox_safe_system_state.rs b/rs/system_api/src/sandbox_safe_system_state.rs index 96ad4c9fb3d..b93268a2068 100644 --- a/rs/system_api/src/sandbox_safe_system_state.rs +++ b/rs/system_api/src/sandbox_safe_system_state.rs @@ -886,6 +886,7 @@ impl SandboxSafeSystemState { canister_current_memory_usage: NumBytes, canister_current_message_memory_usage: NumBytes, amount: Cycles, + reveal_top_up: bool, ) -> HypervisorResult<()> { let mut new_balance = self.cycles_balance(); let result = self @@ -901,6 +902,7 @@ impl SandboxSafeSystemState { amount, self.subnet_size, self.reserved_balance(), + reveal_top_up, ) .map_err(HypervisorError::InsufficientCyclesBalance); self.update_balance_change(new_balance); @@ -930,6 +932,10 @@ impl SandboxSafeSystemState { prepayment_for_response_transmission, self.subnet_size, self.reserved_balance(), + // if the canister is frozen, the controller should call canister_status + // to learn the top up balance instead of getting it from an error + // message to a canister method making downstream call + false, ) { Ok(consumed_cycles) => consumed_cycles, Err(_) => return Err(msg), diff --git a/rs/system_api/tests/system_api.rs b/rs/system_api/tests/system_api.rs index 64dcdeab089..eac4846ae04 100644 --- a/rs/system_api/tests/system_api.rs +++ b/rs/system_api/tests/system_api.rs @@ -1093,6 +1093,7 @@ fn test_fail_add_cycles_when_not_enough_balance() { available: cycles_amount, threshold: Cycles::zero(), requested: amount, + reveal_top_up: false, }) ); //Check cycles balance after call_add_cycles. @@ -1142,6 +1143,7 @@ fn test_fail_adding_more_cycles_when_not_enough_balance() { available: Cycles::from(cycles_amount - amount), threshold: Cycles::zero(), requested: Cycles::from(amount), + reveal_top_up: false, }) ); // Balance unchanged after the second call_add_cycles. diff --git a/rs/types/types/src/messages.rs b/rs/types/types/src/messages.rs index 1daaef3cb02..161dba4a26f 100644 --- a/rs/types/types/src/messages.rs +++ b/rs/types/types/src/messages.rs @@ -468,6 +468,13 @@ impl CanisterCallOrTask { CanisterCallOrTask::Task(_) => Cycles::zero(), } } + + pub fn caller(&self) -> Option { + match self { + CanisterCallOrTask::Call(msg) => Some(*msg.sender()), + CanisterCallOrTask::Task(_) => None, + } + } } #[cfg(test)]