From 555d44462bcf964c950608380cc9ed893e114c33 Mon Sep 17 00:00:00 2001 From: Dimitris Sarlis Date: Wed, 19 Jul 2023 08:16:04 +0000 Subject: [PATCH] fix: Charge for message memory consumed by canisters --- rs/cycles_account_manager/src/lib.rs | 20 +++++- .../src/scheduler/test_utilities.rs | 13 +++- .../src/scheduler/tests.rs | 68 +++++++++++++++++-- 3 files changed, 93 insertions(+), 8 deletions(-) diff --git a/rs/cycles_account_manager/src/lib.rs b/rs/cycles_account_manager/src/lib.rs index 4fa81edc27a..7a3e4b9dc17 100644 --- a/rs/cycles_account_manager/src/lib.rs +++ b/rs/cycles_account_manager/src/lib.rs @@ -819,7 +819,7 @@ impl CyclesAccountManager { duration_between_blocks: Duration, subnet_size: usize, ) -> Result<(), CanisterOutOfCyclesError> { - let bytes_to_charge = match canister.memory_allocation() { + let canister_memory_bytes_to_charge = match canister.memory_allocation() { // The canister has explicitly asked for a memory allocation, so charge // based on it accordingly. MemoryAllocation::Reserved(bytes) => bytes, @@ -828,7 +828,7 @@ impl CyclesAccountManager { }; if let Err(err) = self.charge_for_memory( &mut canister.system_state, - bytes_to_charge, + canister_memory_bytes_to_charge, duration_between_blocks, subnet_size, ) { @@ -841,6 +841,22 @@ impl CyclesAccountManager { return Err(err); } + let message_memory_bytes_to_charge = canister.message_memory_usage(); + if let Err(err) = self.charge_for_memory( + &mut canister.system_state, + message_memory_bytes_to_charge, + duration_between_blocks, + subnet_size, + ) { + info!( + log, + "Charging canister {} for message memory usage failed with {}", + canister.canister_id(), + err + ); + return Err(err); + } + let compute_allocation = canister.compute_allocation(); if let Err(err) = self.charge_for_compute_allocation( &mut canister.system_state, diff --git a/rs/execution_environment/src/scheduler/test_utilities.rs b/rs/execution_environment/src/scheduler/test_utilities.rs index 84348268a9c..00c594ed79b 100644 --- a/rs/execution_environment/src/scheduler/test_utilities.rs +++ b/rs/execution_environment/src/scheduler/test_utilities.rs @@ -57,6 +57,7 @@ use ic_types::{ }; use ic_wasm_types::CanisterModule; use maplit::btreemap; +use std::time::Duration; use crate::{ as_round_instructions, ExecutionEnvironment, Hypervisor, IngressHistoryWriterImpl, RoundLimits, @@ -528,8 +529,12 @@ impl SchedulerTest { } pub fn charge_for_resource_allocations(&mut self) { + let subnet_size = self.subnet_size(); self.scheduler - .charge_canisters_for_resource_allocation_and_usage(self.state.as_mut().unwrap(), 1) + .charge_canisters_for_resource_allocation_and_usage( + self.state.as_mut().unwrap(), + subnet_size, + ) } pub fn induct_messages_on_same_subnet(&mut self) { @@ -579,6 +584,12 @@ impl SchedulerTest { self.subnet_size(), ) } + + pub fn memory_cost(&self, bytes: NumBytes, duration: Duration) -> Cycles { + self.scheduler + .cycles_account_manager + .memory_cost(bytes, duration, self.subnet_size()) + } } /// A builder for `SchedulerTest`. diff --git a/rs/execution_environment/src/scheduler/tests.rs b/rs/execution_environment/src/scheduler/tests.rs index 868658fb95a..e712dbc3160 100644 --- a/rs/execution_environment/src/scheduler/tests.rs +++ b/rs/execution_environment/src/scheduler/tests.rs @@ -1108,6 +1108,68 @@ fn only_charge_for_allocation_after_specified_duration() { ); } +#[test] +fn charging_for_message_memory_works() { + let mut test = SchedulerTestBuilder::new() + .with_scheduler_config(SchedulerConfig { + scheduler_cores: 2, + max_instructions_per_message: NumInstructions::from(1), + max_instructions_per_message_without_dts: NumInstructions::from(1), + max_instructions_per_slice: NumInstructions::new(1), + max_instructions_per_round: NumInstructions::from(1), + instruction_overhead_per_message: NumInstructions::from(0), + instruction_overhead_per_canister: NumInstructions::from(0), + instruction_overhead_per_canister_for_finalization: NumInstructions::from(0), + ..SchedulerConfig::application_subnet() + }) + .build(); + + // Charging handles time=0 as a special case, so it should be set to some + // non-zero time. + let initial_time = Time::from_nanos_since_unix_epoch(1_000_000_000_000); + test.set_time(initial_time); + + let initial_cycles = 1_000_000_000_000; + let canister = test.create_canister_with( + Cycles::new(initial_cycles), + ComputeAllocation::zero(), + MemoryAllocation::BestEffort, + None, + Some(initial_time), + None, + ); + + // Send an ingress that triggers an inter-canister call. Because of the scheduler + // configuration, we can only execute the ingress message but not the + // inter-canister message so this remain in the canister's input queue. + test.send_ingress( + canister, + ingress(1).call(other_side(canister, 1), on_response(1)), + ); + test.execute_round(ExecutionRoundType::OrdinaryRound); + let balance_before = test.canister_state(canister).system_state.balance(); + + // Set time to at least one interval between charges to trigger a charge + // because of message memory consumption. + let charge_duration = test + .scheduler() + .cycles_account_manager + .duration_between_allocation_charges(); + test.set_time(initial_time + charge_duration); + test.charge_for_resource_allocations(); + + // The balance of the canister should have been reduced by the cost of + // message memory during the charge period. + assert_eq!( + test.canister_state(canister).system_state.balance(), + balance_before + - test.memory_cost( + test.canister_state(canister).message_memory_usage(), + charge_duration, + ), + ); +} + #[test] fn dont_execute_any_canisters_if_not_enough_instructions_in_round() { let instructions_per_message = NumInstructions::from(5); @@ -1247,11 +1309,7 @@ fn dont_charge_allocations_for_long_running_canisters() { assert_eq!( test.canister_state(canister).system_state.balance(), canister_balance_before - - test.scheduler().cycles_account_manager.memory_cost( - NumBytes::from(1 << 30), - duration_between_allocation_charges, - 1 - ) + - test.memory_cost(NumBytes::from(1 << 30), duration_between_allocation_charges) ); }