Merge branch 'dimitris/charge-message-memory' into 'master'

fix: Charge for message memory consumed by canisters

Charging for message memory consumed by canisters was removed unintentionally in a previous change. Given that message memory is a shared resource across all canisters in a subnet, it makes sense to charge canisters for it, similarly to how we charge canisters for other memory they consume. 

See merge request dfinity-lab/public/ic!13577

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,

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`.

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)
     );
 }