From 88160bf345c5661a80e21ff1936f5f3dc6f4f340 Mon Sep 17 00:00:00 2001 From: Adam Bratschi-Kaye Date: Wed, 10 Jan 2024 11:20:10 +0000 Subject: [PATCH] RUN-875: Charge for chunked install on hash mismatch --- .../src/canister_manager.rs | 143 +++++++++++++---- .../src/canister_manager/tests.rs | 54 ++++--- .../src/execution/install.rs | 20 ++- .../src/execution/install_code.rs | 10 +- .../src/execution/install_code/tests.rs | 145 +++++++++++++++++- .../src/execution/upgrade.rs | 22 ++- .../src/execution_environment.rs | 6 +- .../system_state/wasm_chunk_store.rs | 11 ++ rs/types/wasm_types/src/lib.rs | 9 ++ 9 files changed, 351 insertions(+), 69 deletions(-) diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 47e02581799..8284de17b61 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -49,7 +49,7 @@ use ic_types::{ InvalidMemoryAllocationError, MemoryAllocation, NumBytes, NumInstructions, PrincipalId, SubnetId, Time, }; -use ic_wasm_types::CanisterModule; +use ic_wasm_types::{CanisterModule, WasmHash}; use num_traits::cast::ToPrimitive; use prometheus::IntCounter; use serde::{Deserialize, Serialize}; @@ -153,12 +153,109 @@ impl CanisterMgrConfig { } } +#[derive(Clone, Debug)] +pub enum WasmSource { + CanisterModule(CanisterModule), + ChunkStore { + wasm_chunk_store: WasmChunkStore, + chunk_hashes_list: Vec>, + wasm_module_hash: WasmHash, + }, +} + +impl From<&WasmSource> for WasmHash { + fn from(item: &WasmSource) -> Self { + match item { + WasmSource::CanisterModule(canister_module) => { + Self::from(canister_module.module_hash()) + } + WasmSource::ChunkStore { + wasm_module_hash, .. + } => wasm_module_hash.clone(), + } + } +} + +impl WasmSource { + pub fn module_hash(&self) -> [u8; 32] { + WasmHash::from(self).to_slice() + } + + /// The number of instructions to be charged each time we try to convert to + /// a canister module. + pub fn instructions_to_assemble(&self) -> NumInstructions { + match self { + Self::CanisterModule(_module) => NumInstructions::from(0), + // Charge one instruction per byte, assuming each chunk is the + // maximum size. + Self::ChunkStore { + chunk_hashes_list, .. + } => NumInstructions::from( + (wasm_chunk_store::chunk_size() * chunk_hashes_list.len() as u64).get(), + ), + } + } + + /// Convert the source to a canister module (assembling chunks if required). + pub(crate) fn into_canister_module(self) -> Result { + match self { + Self::CanisterModule(module) => Ok(module), + Self::ChunkStore { + wasm_chunk_store, + chunk_hashes_list, + wasm_module_hash, + } => { + // Assume each chunk uses the full chunk size even though the actual + // size might be smaller. + let mut wasm_module = Vec::with_capacity( + chunk_hashes_list.len() * wasm_chunk_store::chunk_size().get() as usize, + ); + for hash in chunk_hashes_list { + let hash = hash.as_slice().try_into().map_err(|_| { + CanisterManagerError::WasmChunkStoreError { + message: "Chunk hash is invalid. The length is not 32".to_string(), + } + })?; + for page in wasm_chunk_store.get_chunk_data(&hash).ok_or_else(|| { + CanisterManagerError::WasmChunkStoreError { + message: format!("Chunk hash {:?} was not found", &hash[..32]), + } + })? { + wasm_module.extend_from_slice(page) + } + } + let canister_module = CanisterModule::new(wasm_module); + + if canister_module.module_hash()[..] != wasm_module_hash.to_slice() { + return Err(CanisterManagerError::WasmChunkStoreError { + message: format!( + "Wasm module hash {:?} does not match given hash {:?}", + canister_module.module_hash(), + wasm_module_hash + ), + }); + } + Ok(canister_module) + } + } + } + + #[allow(dead_code)] + /// Only used for tests. + fn unwrap_as_slice_for_testing(&self) -> &[u8] { + match self { + Self::CanisterModule(module) => module.as_slice(), + Self::ChunkStore { .. } => panic!("Can't convert WasmSource::ChunkStore to slice"), + } + } +} + #[derive(Clone, Debug)] pub struct InstallCodeContext { pub origin: CanisterChangeOrigin, pub mode: CanisterInstallModeV2, pub canister_id: CanisterId, - pub wasm_module: CanisterModule, + pub wasm_source: WasmSource, pub arg: Vec, pub compute_allocation: Option, pub memory_allocation: Option, @@ -232,38 +329,22 @@ impl InstallCodeContext { store: &WasmChunkStore, ) -> Result { let canister_id = args.target_canister_id(); - // Assume each chunk uses the full chunk size even though the actual - // size might be smaller. - let mut wasm_module = Vec::with_capacity( - args.chunk_hashes_list.len() * wasm_chunk_store::chunk_size().get() as usize, - ); - for hash in args.chunk_hashes_list { - let hash = hash.as_slice().try_into().map_err(|_| { - InstallCodeContextError::InvalidHash( - "Chunk hash is invalid. The length is not 32".to_string(), - ) - })?; - for page in store.get_chunk_data(&hash).ok_or_else(|| { - InstallCodeContextError::InvalidHash(format!( - "Chunk hash {:?} was not found", - &hash[..32] - )) - })? { - wasm_module.extend_from_slice(page) - } - } - let hash = ic_crypto_sha2::Sha256::hash(&wasm_module); - if hash[..] != args.wasm_module_hash { - return Err(InstallCodeContextError::InvalidHash(format!( - "Wasm module hash {:?} does not match given hash {:?}", - hash, args.wasm_module_hash - ))); - } + let wasm_module_hash = args.wasm_module_hash.try_into().map_err(|hash| { + InstallCodeContextError::InvalidHash(format!("Invalid wasm hash {:?}", hash)) + })?; Ok(InstallCodeContext { origin, mode: args.mode, canister_id, - wasm_module: CanisterModule::new(wasm_module), + wasm_source: WasmSource::ChunkStore { + wasm_chunk_store: store.clone(), + chunk_hashes_list: args + .chunk_hashes_list + .into_iter() + .map(|h| h.to_vec()) + .collect(), + wasm_module_hash, + }, arg: args.arg, compute_allocation: None, memory_allocation: None, @@ -300,7 +381,7 @@ impl TryFrom<(CanisterChangeOrigin, InstallCodeArgsV2)> for InstallCodeContext { origin, mode: args.mode, canister_id, - wasm_module: CanisterModule::new(args.wasm_module), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(args.wasm_module)), arg: args.arg, compute_allocation, memory_allocation, diff --git a/rs/execution_environment/src/canister_manager/tests.rs b/rs/execution_environment/src/canister_manager/tests.rs index 1e07a8d5eb8..fe312fe365c 100644 --- a/rs/execution_environment/src/canister_manager/tests.rs +++ b/rs/execution_environment/src/canister_manager/tests.rs @@ -2,7 +2,7 @@ use crate::{ as_num_instructions, canister_manager::{ uninstall_canister, AddCanisterChangeToHistory, CanisterManager, CanisterManagerError, - CanisterMgrConfig, InstallCodeContext, StopCanisterResult, + CanisterMgrConfig, InstallCodeContext, StopCanisterResult, WasmSource, }, canister_settings::{CanisterSettings, CanisterSettingsBuilder}, execution_environment::{as_round_instructions, RoundCounters}, @@ -156,7 +156,7 @@ impl InstallCodeContextBuilder { } pub fn wasm_module(mut self, wasm_module: Vec) -> Self { - self.ctx.wasm_module = CanisterModule::new(wasm_module); + self.ctx.wasm_source = WasmSource::CanisterModule(CanisterModule::new(wasm_module)); self } @@ -192,7 +192,9 @@ impl Default for InstallCodeContextBuilder { ctx: InstallCodeContext { origin: canister_change_origin_from_principal(&PrincipalId::new_user_test_id(0)), canister_id: canister_test_id(0), - wasm_module: CanisterModule::new(wat::parse_str(EMPTY_WAT).unwrap()), + wasm_source: WasmSource::CanisterModule(CanisterModule::new( + wat::parse_str(EMPTY_WAT).unwrap(), + )), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -333,7 +335,7 @@ fn install_code( let args = InstallCodeArgsV2::new( context.mode, context.canister_id, - context.wasm_module.as_slice().into(), + context.wasm_source.unwrap_as_slice_for_testing().into(), context.arg.clone(), None, None, @@ -3064,7 +3066,7 @@ fn failed_upgrade_hooks_consume_instructions() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(initial_wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(initial_wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3088,7 +3090,7 @@ fn failed_upgrade_hooks_consume_instructions() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(upgrade_wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(upgrade_wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3208,7 +3210,7 @@ fn failed_install_hooks_consume_instructions() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3326,7 +3328,7 @@ fn install_code_respects_instruction_limit() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm.clone()), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm.clone())), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3357,7 +3359,7 @@ fn install_code_respects_instruction_limit() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm.clone()), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm.clone())), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3381,7 +3383,7 @@ fn install_code_respects_instruction_limit() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm.clone()), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm.clone())), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3411,7 +3413,7 @@ fn install_code_respects_instruction_limit() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3471,14 +3473,18 @@ fn install_code_preserves_system_state_and_scheduler_state() { .sender(controller.into()) .canister_id(canister_id) .build(); - let compilation_cost = wasm_compilation_cost(install_code_context.wasm_module.as_slice()); + let compilation_cost = wasm_compilation_cost( + install_code_context + .wasm_source + .unwrap_as_slice_for_testing(), + ); let ctxt = InstallCodeContextBuilder::default() .mode(CanisterInstallModeV2::Install) .sender(controller.into()) .canister_id(canister_id) .build(); - let module_hash = ctxt.wasm_module.module_hash(); + let module_hash = ctxt.wasm_source.module_hash(); let (instructions_left, res, canister) = install_code(&canister_manager, ctxt, &mut state, &mut round_limits); state.put_canister_state(canister.unwrap()); @@ -3520,7 +3526,7 @@ fn install_code_preserves_system_state_and_scheduler_state() { .sender(controller.into()) .canister_id(canister_id) .build(); - let module_hash = ctxt.wasm_module.module_hash(); + let module_hash = ctxt.wasm_source.module_hash(); let (instructions_left, res, canister) = install_code(&canister_manager, ctxt, &mut state, &mut round_limits); state.put_canister_state(canister.unwrap()); @@ -3571,7 +3577,7 @@ fn install_code_preserves_system_state_and_scheduler_state() { .sender(controller.into()) .canister_id(canister_id) .build(); - let module_hash = ctxt.wasm_module.module_hash(); + let (instructions_left, res, canister) = install_code(&canister_manager, ctxt, &mut state, &mut round_limits); state.put_canister_state(canister.unwrap()); @@ -3644,7 +3650,7 @@ fn lower_memory_allocation_than_usage_fails() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3713,7 +3719,7 @@ fn test_install_when_updating_memory_allocation_via_canister_settings() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm.clone()), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm.clone())), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3755,7 +3761,7 @@ fn test_install_when_updating_memory_allocation_via_canister_settings() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3814,7 +3820,7 @@ fn test_upgrade_when_updating_memory_allocation_via_canister_settings() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3839,7 +3845,7 @@ fn test_upgrade_when_updating_memory_allocation_via_canister_settings() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm.clone()), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm.clone())), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -3886,7 +3892,7 @@ fn test_upgrade_when_updating_memory_allocation_via_canister_settings() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -4023,7 +4029,7 @@ fn test_install_when_setting_memory_allocation_to_zero() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -4074,7 +4080,7 @@ fn test_upgrade_when_setting_memory_allocation_to_zero() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm.clone()), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm.clone())), arg: vec![], compute_allocation: None, memory_allocation: None, @@ -4110,7 +4116,7 @@ fn test_upgrade_when_setting_memory_allocation_to_zero() { InstallCodeContext { origin: canister_change_origin_from_principal(&sender), canister_id, - wasm_module: CanisterModule::new(wasm), + wasm_source: WasmSource::CanisterModule(CanisterModule::new(wasm)), arg: vec![], compute_allocation: None, memory_allocation: None, diff --git a/rs/execution_environment/src/execution/install.rs b/rs/execution_environment/src/execution/install.rs index deb98a88144..dee63efd2b8 100644 --- a/rs/execution_environment/src/execution/install.rs +++ b/rs/execution_environment/src/execution/install.rs @@ -4,6 +4,7 @@ use std::sync::Arc; +use crate::as_round_instructions; use crate::canister_manager::{ DtsInstallCodeResult, InstallCodeContext, PausedInstallCodeExecution, }; @@ -92,9 +93,24 @@ pub(crate) fn execute_install( let canister_id = helper.canister().canister_id(); let layout = canister_layout(&original.canister_layout_path, &canister_id); let context_sender = context.sender(); - let module_hash = context.wasm_module.module_hash(); + let instructions_to_assemble = context.wasm_source.instructions_to_assemble(); + helper.reduce_instructions_by(instructions_to_assemble); + round_limits.instructions -= as_round_instructions(instructions_to_assemble); + let wasm_module = match context.wasm_source.into_canister_module() { + Ok(wasm_module) => wasm_module, + Err(err) => { + return finish_err( + clean_canister, + helper.instructions_left(), + original, + round, + err, + ); + } + }; + let module_hash = wasm_module.module_hash(); let (instructions_from_compilation, result) = round.hypervisor.create_execution_state( - context.wasm_module, + wasm_module, layout.raw_path(), canister_id, round_limits, diff --git a/rs/execution_environment/src/execution/install_code.rs b/rs/execution_environment/src/execution/install_code.rs index 117724eaf0d..b921b7394ac 100644 --- a/rs/execution_environment/src/execution/install_code.rs +++ b/rs/execution_environment/src/execution/install_code.rs @@ -172,6 +172,12 @@ impl InstallCodeHelper { self.canister.message_memory_usage() } + pub fn reduce_instructions_by(&mut self, instructions: NumInstructions) { + self.execution_parameters + .instruction_limits + .reduce_by(instructions); + } + /// Returns a struct with all the necessary information to replay the /// performed `install_code` steps in subsequent rounds. pub fn pause(self) -> PausedInstallCodeHelper { @@ -489,9 +495,7 @@ impl InstallCodeHelper { stable_memory_handling, }); - self.execution_parameters - .instruction_limits - .reduce_by(instructions_from_compilation); + self.reduce_instructions_by(instructions_from_compilation); let old_memory_usage = self.canister.memory_usage(); let old_memory_allocation = self.canister.system_state.memory_allocation; diff --git a/rs/execution_environment/src/execution/install_code/tests.rs b/rs/execution_environment/src/execution/install_code/tests.rs index 8b50012797e..cc547a44213 100644 --- a/rs/execution_environment/src/execution/install_code/tests.rs +++ b/rs/execution_environment/src/execution/install_code/tests.rs @@ -2,6 +2,7 @@ use assert_matches::assert_matches; use ic_base_types::PrincipalId; use ic_error_types::{ErrorCode, UserError}; use ic_registry_routing_table::{CanisterIdRange, RoutingTable}; +use ic_replicated_state::canister_state::system_state::wasm_chunk_store; use ic_replicated_state::{ExecutionTask, ReplicatedState}; use ic_state_machine_tests::{IngressState, IngressStatus}; use ic_types::{ @@ -10,8 +11,8 @@ use ic_types::{ use ic_ic00_types::{ CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, CanisterInstallMode, - CanisterInstallModeV2, EmptyBlob, InstallChunkedCodeArgs, InstallCodeArgs, Method, Payload, - UploadChunkArgs, UploadChunkReply, + CanisterInstallModeV2, EmptyBlob, InstallChunkedCodeArgs, InstallCodeArgs, InstallCodeArgsV2, + Method, Payload, UploadChunkArgs, UploadChunkReply, }; use ic_replicated_state::canister_state::NextExecution; use ic_test_utilities_execution_environment::{ @@ -1834,7 +1835,7 @@ fn install_chunked_works_from_controller_of_store() { } #[test] -fn install_chunked_works_fails_from_noncontroller_of_store() { +fn install_chunked_fails_from_noncontroller_of_store() { const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); let mut test = ExecutionTestBuilder::new().with_wasm_chunk_store().build(); @@ -2161,3 +2162,141 @@ fn upgrade_with_dts_correctly_updates_system_state() { assert_eq!(history_entries_before + 1, history_entries_after); } + +#[test] +fn failed_install_chunked_charges_for_wasm_assembly() { + const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); + + let mut test = ExecutionTestBuilder::new().with_wasm_chunk_store().build(); + + let canister_id = test.create_canister(CYCLES); + + let wasm = wat::parse_str("(module)").unwrap(); + let hash = UploadChunkReply::decode(&get_reply( + test.subnet_message( + "upload_chunk", + UploadChunkArgs { + canister_id: canister_id.into(), + chunk: wasm.to_vec(), + } + .encode(), + ), + )) + .unwrap() + .hash; + + let mut wrong_hash = hash.clone(); + wrong_hash[0] = wrong_hash[0].wrapping_add(1); + + let initial_cycles = test.canister_state(canister_id).system_state.balance(); + + let method_name = "install_chunked_code"; + let arg = InstallChunkedCodeArgs::new( + CanisterInstallModeV2::Install, + canister_id, + Some(canister_id), + vec![hash], + wrong_hash, + vec![], + ) + .encode(); + + let expected_cost = test.cycles_account_manager().execution_cost( + NumInstructions::from(wasm_chunk_store::chunk_size().get()), + test.subnet_size(), + ); + + // Install the universal canister + let install_err = test.subnet_message(method_name, arg).unwrap_err(); + assert_eq!(install_err.code(), ErrorCode::CanisterContractViolation); + let final_cycles = test.canister_state(canister_id).system_state.balance(); + let charged_cycles = initial_cycles - final_cycles; + // There seems to be a rounding difference from prepay and refund. + assert!( + charged_cycles - expected_cost <= Cycles::from(1_u64) + && expected_cost - charged_cycles <= Cycles::from(1_u64), + "Charged cycles {} differs from expected cost {}", + charged_cycles, + expected_cost + ); +} + +#[test] +fn successful_install_chunked_charges_for_wasm_assembly() { + const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); + + let mut test = ExecutionTestBuilder::new().with_wasm_chunk_store().build(); + let wasm = wat::parse_str("(module)").unwrap(); + + // Get the charges for a normal install + let charge_for_regular_install = { + let canister_id = test.create_canister(CYCLES); + let initial_cycles = test.canister_state(canister_id).system_state.balance(); + let method_name = "install_code"; + let arg = InstallCodeArgsV2::new( + CanisterInstallModeV2::Install, + canister_id, + wasm.clone(), + vec![], + None, + None, + None, + ) + .encode(); + let _response = test.subnet_message(method_name, arg).unwrap(); + let final_cycles = test.canister_state(canister_id).system_state.balance(); + initial_cycles - final_cycles + }; + + let canister_id = test.create_canister(CYCLES); + + let hash = UploadChunkReply::decode(&get_reply( + test.subnet_message( + "upload_chunk", + UploadChunkArgs { + canister_id: canister_id.into(), + chunk: wasm.to_vec(), + } + .encode(), + ), + )) + .unwrap() + .hash; + + let initial_cycles = test.canister_state(canister_id).system_state.balance(); + + let method_name = "install_chunked_code"; + let arg = InstallChunkedCodeArgs::new( + CanisterInstallModeV2::Install, + canister_id, + Some(canister_id), + vec![hash.clone()], + hash, + vec![], + ) + .encode(); + + // There is a fixed overhead in the `execution_cost` which we don't want to + // double count. + let fixed_execution_overhead = test + .cycles_account_manager() + .execution_cost(NumInstructions::from(0), test.subnet_size()); + let expected_cost = test.cycles_account_manager().execution_cost( + NumInstructions::from(wasm_chunk_store::chunk_size().get()), + test.subnet_size(), + ) - fixed_execution_overhead + + charge_for_regular_install; + + // Install the universal canister + let _response = test.subnet_message(method_name, arg).unwrap(); + let final_cycles = test.canister_state(canister_id).system_state.balance(); + let charged_cycles = initial_cycles - final_cycles; + // There seems to be a rounding difference from prepay and refund. + assert!( + charged_cycles - expected_cost <= Cycles::from(1_u64) + && expected_cost - charged_cycles <= Cycles::from(1_u64), + "Charged cycles {} differs from expected cost {}", + charged_cycles, + expected_cost + ); +} diff --git a/rs/execution_environment/src/execution/upgrade.rs b/rs/execution_environment/src/execution/upgrade.rs index 291b33d2051..16a8e2bcb6d 100644 --- a/rs/execution_environment/src/execution/upgrade.rs +++ b/rs/execution_environment/src/execution/upgrade.rs @@ -5,6 +5,7 @@ use std::sync::Arc; +use crate::as_round_instructions; use crate::canister_manager::{ DtsInstallCodeResult, InstallCodeContext, PausedInstallCodeExecution, }; @@ -245,13 +246,30 @@ fn upgrade_stage_2_and_3a_create_execution_state_and_call_start( ) -> DtsInstallCodeResult { let canister_id = helper.canister().canister_id(); let context_sender = context.sender(); - let module_hash = context.wasm_module.module_hash(); + + let instructions_to_assemble = context.wasm_source.instructions_to_assemble(); + helper.reduce_instructions_by(instructions_to_assemble); + round_limits.instructions -= as_round_instructions(instructions_to_assemble); + let wasm_module = match context.wasm_source.into_canister_module() { + Ok(wasm_module) => wasm_module, + Err(err) => { + return finish_err( + clean_canister, + helper.instructions_left(), + original, + round, + err, + ); + } + }; + + let module_hash = wasm_module.module_hash(); // Stage 2: create a new execution state based on the new Wasm code, deactivate global timer, and bump canister version. // Replace the execution state of the canister with a new execution state, but // persist the stable memory (if it exists). let layout = canister_layout(&original.canister_layout_path, &canister_id); let (instructions_from_compilation, result) = round.hypervisor.create_execution_state( - context.wasm_module, + wasm_module, layout.raw_path(), canister_id, round_limits, diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index db3e8e7aae7..a1505e936d0 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -2390,7 +2390,7 @@ impl ExecutionEnvironment { } let canister_id = old_canister.canister_id(); - let new_wasm_hash = WasmHash::from(&install_context.wasm_module); + let new_wasm_hash = (&install_context.wasm_source).into(); let compilation_cost_handling = if state .metadata .expected_compiled_wasms @@ -2402,9 +2402,7 @@ impl ExecutionEnvironment { }; info!( self.log, - "Start executing install_code message on canister {:?}, contains module {:?}", - canister_id, - install_context.wasm_module.is_empty().to_string(), + "Start executing install_code message on canister {:?}", canister_id, ); let execution_parameters = self.execution_parameters( diff --git a/rs/replicated_state/src/canister_state/system_state/wasm_chunk_store.rs b/rs/replicated_state/src/canister_state/system_state/wasm_chunk_store.rs index 7f620744952..0ce6124a91f 100644 --- a/rs/replicated_state/src/canister_state/system_state/wasm_chunk_store.rs +++ b/rs/replicated_state/src/canister_state/system_state/wasm_chunk_store.rs @@ -33,6 +33,8 @@ struct ChunkInfo { length: u64, } +/// Uploaded chunks which can be assembled to create a Wasm module. +/// It is cheap to clone because the data is stored in a [`PageMap`]. #[derive(Clone, Debug, PartialEq, Eq)] pub struct WasmChunkStore { data: PageMap, @@ -183,6 +185,8 @@ impl WasmChunkStore { } } +/// Mapping from chunk hash to location in the store. It is cheap to clone +/// because the size is limited to 100 entries. #[derive(Clone, Debug, PartialEq, Eq, Default)] pub struct WasmChunkStoreMetadata { /// Maps each chunk to its chunk index and length. @@ -191,6 +195,13 @@ pub struct WasmChunkStoreMetadata { size: NumPages, } +// In order to make the metadata cheap to clone, we should ensure that the size +// of the Map is limited to a small number of entries. +#[test] +fn wasm_chunk_store_cheap_clone() { + assert!(DEFAULT_MAX_SIZE / CHUNK_SIZE <= 100.into()); +} + impl From<&WasmChunkStoreMetadata> for pb::WasmChunkStoreMetadata { fn from(item: &WasmChunkStoreMetadata) -> Self { let chunks = item diff --git a/rs/types/wasm_types/src/lib.rs b/rs/types/wasm_types/src/lib.rs index 96246bf3671..cf9c45bc72d 100644 --- a/rs/types/wasm_types/src/lib.rs +++ b/rs/types/wasm_types/src/lib.rs @@ -159,6 +159,15 @@ impl From<[u8; WASM_HASH_LENGTH]> for WasmHash { } } +impl TryFrom> for WasmHash { + type Error = Vec; + + fn try_from(value: Vec) -> Result { + let array: [u8; WASM_HASH_LENGTH] = value.try_into()?; + Ok(Self::from(array)) + } +} + impl CountBytes for WasmHash { fn count_bytes(&self) -> usize { self.0.len()