Skip to content

fix: check the cycles balance and charge executed instructions on DTS resume #11039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
mraszyk wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix: check the cycles balance and charge executed instructions on DTS resume #11039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions rs/execution_environment/src/execution/call_or_task.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ pub fn execute_call_or_task(
};
let paused_execution = Box::new(PausedCallOrTaskExecution {
paused_wasm_execution,
paused_helper: helper.pause(),
paused_helper: helper.pause(slice.executed_instructions),
original,
});
ExecuteMessageResult::Paused {
Expand Down Expand Up @@ -322,6 +322,7 @@ struct OriginalContext {
struct PausedCallOrTaskHelper {
call_context_id: CallContextId,
initial_cycles_balance: Cycles,
executed_wasm_instructions: NumInstructions,
}

/// A helper that implements and keeps track of update call steps.
Expand All @@ -330,6 +331,12 @@ struct CallOrTaskHelper {
canister: CanisterState,
call_context_id: CallContextId,
initial_cycles_balance: Cycles,
// Instructions already executed by a Wasm execution that has been paused and
// has not finished yet. Such instructions are not reflected in the
// instruction limits because those are updated only when the Wasm execution
// finishes, so they are tracked here in order to be charged if the execution
// fails before the Wasm execution finishes.
executed_wasm_instructions: NumInstructions,
deallocation_sender: DeallocationSender,
}

Expand Down Expand Up @@ -412,17 +419,22 @@ impl CallOrTaskHelper {
canister,
call_context_id,
initial_cycles_balance,
executed_wasm_instructions: NumInstructions::new(0),
deallocation_sender: deallocation_sender.clone(),
})
}

/// Returns a struct with all the necessary information to replay the
/// performed update call steps in subsequent rounds.
fn pause(self) -> PausedCallOrTaskHelper {
/// The given `executed_wasm_instructions` are the instructions executed by
/// the Wasm slice that is being paused.
fn pause(self, executed_wasm_instructions: NumInstructions) -> PausedCallOrTaskHelper {
self.deallocation_sender.send(Box::new(self.canister));
PausedCallOrTaskHelper {
call_context_id: self.call_context_id,
initial_cycles_balance: self.initial_cycles_balance,
executed_wasm_instructions: self.executed_wasm_instructions
+ executed_wasm_instructions,
}
}

Expand All @@ -435,7 +447,8 @@ impl CallOrTaskHelper {
paused: PausedCallOrTaskHelper,
deallocation_sender: &DeallocationSender,
) -> Result<Self, UserError> {
let helper = Self::new(clean_canister, original, deallocation_sender)?;
let mut helper = Self::new(clean_canister, original, deallocation_sender)?;
helper.executed_wasm_instructions = paused.executed_wasm_instructions;
if helper.initial_cycles_balance != paused.initial_cycles_balance {
let msg = match original.call_or_task {
CanisterCallOrTask::Update(_) => {
Expand Down Expand Up @@ -717,6 +730,11 @@ impl PausedExecution for PausedCallOrTaskExecution {
self.original.method,
clean_canister.canister_id(),
);
// If resuming fails, then the instructions already executed by the paused
// Wasm execution are still charged: they have been executed and hence
// consumed round instructions, but the instruction limits are updated
// only when the Wasm execution finishes.
let executed_wasm_instructions = self.paused_helper.executed_wasm_instructions;
let helper = match CallOrTaskHelper::resume(
&clean_canister,
&self.original,
Expand All @@ -733,16 +751,15 @@ impl PausedExecution for PausedCallOrTaskExecution {
err,
);
self.paused_wasm_execution.abort();
return finish_err(
clean_canister,
let instructions_left = NumInstructions::new(
self.original
.execution_parameters
.instruction_limits
.message(),
err,
self.original,
round,
.message()
.get()
.saturating_sub(executed_wasm_instructions.get()),
);
return finish_err(clean_canister, instructions_left, err, self.original, round);
}
};

Expand All @@ -760,7 +777,7 @@ impl PausedExecution for PausedCallOrTaskExecution {
update_round_limits(round_limits, &slice);
let paused_execution = Box::new(PausedCallOrTaskExecution {
paused_wasm_execution,
paused_helper: helper.pause(),
paused_helper: helper.pause(slice.executed_instructions),
original: self.original,
});
ExecuteMessageResult::Paused {
Expand Down
67 changes: 0 additions & 67 deletions rs/execution_environment/src/execution/call_or_task/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,73 +720,6 @@ fn hitting_access_limit_fails_non_replicated_query() {
);
}

#[test]
fn dts_replicated_execution_resume_fails_due_to_cycles_change() {
with_update_and_replicated_query(|method| {
// Test steps:
// 1. Canister A starts running the update|query method.
// 2. While canister A is paused, we change its cycles balance.
// 3. The update|query method resumes, detects the cycles balance mismatch, and
// fails.
let instruction_limit = 1_000_000;
let mut test = ExecutionTestBuilder::new()
.with_instruction_limit(instruction_limit)
.with_slice_instruction_limit(200_000)
.with_manual_execution()
.build();

let a_id = test.universal_canister().unwrap();

let a = wasm()
.stable64_grow(1)
.stable64_fill(0, 0, 50_000)
.stable64_fill(0, 0, 50_000)
.stable64_fill(0, 0, 50_000)
.stable64_fill(0, 0, 50_000)
.stable64_fill(0, 0, 50_000)
.stable64_fill(0, 0, 50_000)
.stable64_fill(0, 0, 50_000)
.stable64_fill(0, 0, 50_000)
.build();

let (ingress_id, _) = test.ingress_raw(a_id, method, a);

test.execute_slice(a_id);
assert_eq!(
test.canister_state(a_id).next_execution(),
NextExecution::ContinueLong,
);

// Change the cycles balance of the clean canister.
let balance = test.canister_state(a_id).system_state.balance();
test.canister_state_mut(a_id)
.system_state
.add_cycles(balance + Cycles::new(1));

test.execute_slice(a_id);

assert_eq!(
test.canister_state(a_id).next_execution(),
NextExecution::None,
);

let err = check_ingress_status(test.ingress_status(&ingress_id)).unwrap_err();
let message = if method == "update" {
"an update call"
} else {
"a replicated query"
};
err.assert_contains(
ErrorCode::CanisterWasmEngineError,
&format!(
"Error from Canister {a_id}: Canister encountered a Wasm engine error: \
Failed to apply system changes: Mismatch in cycles \
balance when resuming {message}"
),
);
});
}

#[test]
fn dts_replicated_execution_resume_fails_due_to_call_context_change() {
with_update_and_replicated_query(|method| {
Expand Down
8 changes: 4 additions & 4 deletions rs/execution_environment/src/execution/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ pub(crate) fn execute_install(
ingress_status_with_processing_state(&original.message, original.time);
let paused_execution = Box::new(PausedStartExecutionDuringInstall {
paused_wasm_execution,
paused_helper: helper.pause(),
paused_helper: helper.pause(slice.executed_instructions),
context_sender,
context_arg: context.arg,
original,
Expand Down Expand Up @@ -322,7 +322,7 @@ fn install_stage_2b_continue_install_after_start(
let ingress_status =
ingress_status_with_processing_state(&original.message, original.time);
let paused_execution = Box::new(PausedInitExecution {
paused_helper: helper.pause(),
paused_helper: helper.pause(slice.executed_instructions),
paused_wasm_execution,
original,
});
Expand Down Expand Up @@ -440,7 +440,7 @@ impl PausedInstallCodeExecution for PausedInitExecution {
update_round_limits(round_limits, &slice);
let paused_execution = Box::new(PausedInitExecution {
paused_wasm_execution,
paused_helper: helper.pause(),
paused_helper: helper.pause(slice.executed_instructions),
..*self
});
DtsInstallCodeResult::Paused {
Expand Down Expand Up @@ -550,7 +550,7 @@ impl PausedInstallCodeExecution for PausedStartExecutionDuringInstall {
);
let paused_execution = Box::new(PausedStartExecutionDuringInstall {
paused_wasm_execution,
paused_helper: helper.pause(),
paused_helper: helper.pause(slice.executed_instructions),
..*self
});
DtsInstallCodeResult::Paused {
Expand Down
55 changes: 51 additions & 4 deletions rs/execution_environment/src/execution/install_code.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use ic_types::{
CanisterLog, CanisterTimer, MemoryAllocation, NumInstructions, Time, messages::CanisterCall,
};
use ic_types_cycles::{CompoundCycles, Cycles, CyclesUseCase, Instructions};
use ic_wasm_types::WasmEngineError::FailedToApplySystemChanges;
use ic_wasm_types::WasmHash;

use crate::{
Expand Down Expand Up @@ -103,6 +104,8 @@ pub(crate) enum InstallCodeStep {
pub(crate) struct PausedInstallCodeHelper {
steps: Vec<InstallCodeStep>,
instructions_left: NumInstructions,
initial_cycles_balance: Cycles,
executed_wasm_instructions: NumInstructions,
}

/// A helper that implements and keeps track of `install_code` steps.
Expand All @@ -124,12 +127,21 @@ pub(crate) struct InstallCodeHelper {
deallocated_wasm_custom_sections_bytes: NumBytes,
// The total heap delta of all steps.
total_heap_delta: NumBytes,
// The cycles balance of the clean canister state this helper was built from.
initial_cycles_balance: Cycles,
// Instructions already executed by a Wasm execution that has been paused and
// has not finished yet. Such instructions are not reflected in
// `execution_parameters` because the instruction limits are updated only
// when the Wasm execution finishes, so they are tracked here in order to be
// charged if the execution fails before the Wasm execution finishes.
executed_wasm_instructions: NumInstructions,
}

impl InstallCodeHelper {
pub fn new(clean_canister: &CanisterState, original: &OriginalContext) -> Self {
Self {
steps: vec![],
initial_cycles_balance: clean_canister.system_state.balance(),
canister: clean_canister.clone(),
message_instruction_limit: original.execution_parameters.instruction_limits.message(),
execution_parameters: original.execution_parameters.clone(),
Expand All @@ -139,6 +151,7 @@ impl InstallCodeHelper {
deallocated_bytes: NumBytes::new(0),
deallocated_wasm_custom_sections_bytes: NumBytes::new(0),
total_heap_delta: NumBytes::new(0),
executed_wasm_instructions: NumInstructions::new(0),
}
}

Expand Down Expand Up @@ -233,16 +246,23 @@ impl InstallCodeHelper {

/// Returns a struct with all the necessary information to replay the
/// performed `install_code` steps in subsequent rounds.
pub fn pause(self) -> PausedInstallCodeHelper {
/// The given `executed_wasm_instructions` are the instructions executed by
/// the Wasm slice that is being paused.
pub fn pause(self, executed_wasm_instructions: NumInstructions) -> PausedInstallCodeHelper {
PausedInstallCodeHelper {
instructions_left: self.instructions_left(),
steps: self.steps,
initial_cycles_balance: self.initial_cycles_balance,
executed_wasm_instructions: self.executed_wasm_instructions
+ executed_wasm_instructions,
}
}

/// Replays the previous `install_code` steps on the given clean canister.
/// Returns an error if any step fails. Otherwise, it returns an instance of
/// the helper that can be used to continue the `install_code` execution.
/// Returns an error if the cycles balance of the clean canister differs from
/// the cycles balance at the start of the DTS execution or if any step
/// fails. Otherwise, it returns an instance of the helper that can be used
/// to continue the `install_code` execution.
#[allow(clippy::result_large_err)]
pub fn resume(
clean_canister: &CanisterState,
Expand All @@ -259,12 +279,36 @@ impl InstallCodeHelper {
> {
let mut helper = Self::new(clean_canister, original);
let paused_instructions_left = paused.instructions_left;
let executed_wasm_instructions = paused.executed_wasm_instructions;
// If resuming fails, then the instructions already executed by the paused
// Wasm execution are charged in addition to the instructions accounted
// for in `paused_instructions_left`: they have been executed and hence
// consumed round instructions, but the instruction limits of the helper
// are updated only when the Wasm execution finishes.
let instructions_left_on_error = NumInstructions::new(
paused_instructions_left
.get()
.saturating_sub(executed_wasm_instructions.get()),
);

// The cycles balance of the clean canister must not change during the
// DTS execution.
if helper.initial_cycles_balance != paused.initial_cycles_balance {
let msg = "Mismatch in cycles balance when resuming an install code".to_string();
let err = HypervisorError::WasmEngineError(FailedToApplySystemChanges(msg));
let err = (clean_canister.canister_id(), err).into();
return Err((err, instructions_left_on_error, helper.take_canister_log()));
}

for state_change in paused.steps.into_iter() {
helper
.replay_step(state_change, original, round)
.map_err(|err| (err, paused_instructions_left, helper.take_canister_log()))?;
.map_err(|err| (err, instructions_left_on_error, helper.take_canister_log()))?;
}
debug_assert_eq!(paused_instructions_left, helper.instructions_left());
// Replaying the steps of a Wasm execution that has already finished resets
// this counter, so it is restored after replaying all the steps.
helper.executed_wasm_instructions = executed_wasm_instructions;
Ok(helper)
}

Expand Down Expand Up @@ -654,6 +698,9 @@ impl InstallCodeHelper {
self.execution_parameters
.instruction_limits
.update(output.num_instructions_left);
// The instructions of all the slices of this Wasm execution are now
// reflected in the instruction limits above.
self.executed_wasm_instructions = NumInstructions::new(0);

debug_assert!(
output
Expand Down
Loading
Loading