diff --git a/rs/execution_environment/src/scheduler.rs b/rs/execution_environment/src/scheduler.rs index 5901de86c5e1..9affe545e5f5 100644 --- a/rs/execution_environment/src/scheduler.rs +++ b/rs/execution_environment/src/scheduler.rs @@ -808,14 +808,30 @@ impl SchedulerImpl { } /// Charge canisters for their resource allocation and usage. Canisters - /// that did not manage to pay are uninstalled. - /// This function is expected to be called at the end of a round. + /// that cannot pay are uninstalled. + /// + /// This function is expected to be called at the end of a round and + /// particularly after paused executions were aborted on checkpoint rounds. fn charge_canisters_for_resource_allocation_and_usage( &self, state: &mut ReplicatedState, subnet_size: usize, current_round: ExecutionRound, + current_round_type: ExecutionRoundType, ) { + // Because it is relatively expensive to make every canister mutable just to + // check whether 10 seconds have passed since it was last charged, only + // charge canisters every 50 rounds and/or on checkpoint rounds. + // + // The latter ensures that (because we skip canisters with paused + // executions), every canister is charged at least once per checkpoint + // interval. + if !current_round.get().is_multiple_of(50) + && current_round_type != ExecutionRoundType::CheckpointRound + { + return; + } + let cost_schedule = state.get_own_cost_schedule(); let state_time = state.time(); let threshold_last_allocation_charge = state_time.saturating_sub( @@ -1464,15 +1480,17 @@ impl Scheduler for SchedulerImpl { round_schedule.finish_round(&mut final_state, current_round, &self.metrics); } + // Abort (some) paused executions. self.finish_round(&mut final_state, current_round_type); - // Charge canisters after (some) paused executions were aborted. + // Charge canisters after paused executions were aborted. { let _timer = self.metrics.round_finalization_charge.start_timer(); self.charge_canisters_for_resource_allocation_and_usage( &mut final_state, registry_settings.subnet_size, current_round, + current_round_type, ); } diff --git a/rs/execution_environment/src/scheduler/test_utilities.rs b/rs/execution_environment/src/scheduler/test_utilities.rs index 1fedc17b0e79..dd2a87d07bc9 100644 --- a/rs/execution_environment/src/scheduler/test_utilities.rs +++ b/rs/execution_environment/src/scheduler/test_utilities.rs @@ -675,6 +675,7 @@ impl SchedulerTest { self.state.as_mut().unwrap(), subnet_size, ExecutionRound::from(0), + ExecutionRoundType::CheckpointRound, ) } diff --git a/rs/execution_environment/src/scheduler/tests/charging.rs b/rs/execution_environment/src/scheduler/tests/charging.rs index 83f6d0cc74b1..949b20621769 100644 --- a/rs/execution_environment/src/scheduler/tests/charging.rs +++ b/rs/execution_environment/src/scheduler/tests/charging.rs @@ -56,8 +56,9 @@ fn only_charge_for_allocation_after_specified_duration() { ); // Don't charge because the time since the last charge is too small. + // Checkpoint round, to force charging for storage. test.set_time(initial_time + time_between_batches); - test.execute_round(ExecutionRoundType::OrdinaryRound); + test.execute_round(ExecutionRoundType::CheckpointRound); assert_eq!( test.canister_state(canister).system_state.balance().get(), @@ -65,9 +66,9 @@ fn only_charge_for_allocation_after_specified_duration() { ); // The time of the current batch is now long enough that allocation charging - // should be triggered. + // should be triggered. Checkpoint round, to force charging for storage. test.set_time(initial_time + 2 * time_between_batches); - test.execute_round(ExecutionRoundType::OrdinaryRound); + test.execute_round(ExecutionRoundType::CheckpointRound); assert_eq!( test.canister_state(canister).system_state.balance().get(), initial_cycles - 10, @@ -106,7 +107,7 @@ fn charging_for_message_memory_works() { // 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. + // inter-canister message so this remains in the canister's input queue. test.send_ingress( canister, ingress(1).call(other_side(canister, 1), on_response(1)), @@ -220,7 +221,8 @@ fn canisters_with_insufficient_cycles_are_uninstalled() { .duration_between_allocation_charges(), ); - test.execute_round(ExecutionRoundType::OrdinaryRound); + // Checkpoint round, to force charging for storage. + test.execute_round(ExecutionRoundType::CheckpointRound); for canister in test.state().canisters_iter() { assert!(canister.execution_state.is_none()); @@ -455,7 +457,9 @@ fn snapshot_is_deleted_when_canister_is_out_of_cycles() { .cycles_account_manager .duration_between_allocation_charges(), ); - test.execute_round(ExecutionRoundType::OrdinaryRound); + // Checkpoint round, to force charging for storage. + test.execute_round(ExecutionRoundType::CheckpointRound); + assert_eq!( test.scheduler() .metrics @@ -591,7 +595,9 @@ fn snapshot_is_deleted_when_uninstalled_canister_is_out_of_cycles() { .cycles_account_manager .duration_between_allocation_charges(), ); - test.execute_round(ExecutionRoundType::OrdinaryRound); + // Checkpoint round, to force charging for storage. + test.execute_round(ExecutionRoundType::CheckpointRound); + assert_eq!( test.scheduler() .metrics diff --git a/rs/execution_environment/tests/canister_history.rs b/rs/execution_environment/tests/canister_history.rs index 28c080b305c9..749ee8de2ed6 100644 --- a/rs/execution_environment/tests/canister_history.rs +++ b/rs/execution_environment/tests/canister_history.rs @@ -635,7 +635,9 @@ fn canister_history_cleared_if_canister_out_of_cycles() { / compute_percent_allocated_per_second_fee.get() as u64; now += Duration::from_secs(seconds_to_burn_balance + 1); env.set_time(now); - env.tick(); + // canisters are always charged for storage on checkpoint rounds + env.checkpointed_tick(); + // check canister history let total_num_change_entries = reference_change_entries.len(); reference_change_entries.clear(); diff --git a/rs/execution_environment/tests/execution_test.rs b/rs/execution_environment/tests/execution_test.rs index 5aa63293b48b..97c443116c46 100644 --- a/rs/execution_environment/tests/execution_test.rs +++ b/rs/execution_environment/tests/execution_test.rs @@ -476,7 +476,8 @@ fn canister_has_zero_balance_when_uninstalled_due_to_low_cycles() { let seconds_to_burn_balance = env.cycle_balance(canister_id) as u64 / compute_percent_allocated_per_second_fee.get() as u64; env.advance_time(Duration::from_secs(seconds_to_burn_balance + 1)); - env.tick(); + // Checkpoint round, to force charging for storage. + env.checkpointed_tick(); // Verify the original canister still exists but it's uninstalled and has a // zero cycle balance. diff --git a/rs/execution_environment/tests/subnet_size_test.rs b/rs/execution_environment/tests/subnet_size_test.rs index 7ea7c172be73..95d1811c87f7 100644 --- a/rs/execution_environment/tests/subnet_size_test.rs +++ b/rs/execution_environment/tests/subnet_size_test.rs @@ -304,7 +304,8 @@ fn simulate_one_gib_per_second_cost( env.advance_time(duration_between_allocation_charges); let balance_before = env.cycle_balance(canister_id); - env.tick(); + // Checkpoint round, to force charging for storage. + env.checkpointed_tick(); let balance_after = env.cycle_balance(canister_id); // Scale the cost from a defined in config value to a 1 second duration.