From 45549b72620f80f20f835017d68d12990ea0378c Mon Sep 17 00:00:00 2001 From: luc-blaeser Date: Fri, 30 Aug 2024 15:18:28 +0200 Subject: [PATCH 1/6] Test that canister snapshots cover globals --- .../tests/canister_snapshots.rs | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs index d389641cccf..3c8eb85e2ab 100644 --- a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs +++ b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs @@ -1714,3 +1714,91 @@ fn load_canister_snapshot_charges_canister_cycles() { test.canister_state(canister_id).system_state.balance() < initial_balance - expected_charge ); } + +#[test] +fn snapshot_must_include_globals() { + let wat = r#" + (module + (import "ic0" "msg_reply" (func $msg_reply)) + (import "ic0" "msg_reply_data_append" + (func $msg_reply_data_append (param i32 i32))) + + (func $read_global + (i32.store + (i32.const 0) + (global.get 0) + ) + (call $msg_reply_data_append + (i32.const 0) + (i32.const 4)) + (call $msg_reply) + ) + + (func $increase_global + (global.set 0 + (i32.add + (global.get 0) + (i32.const 1) + ) + ) + (call $msg_reply) + ) + + (memory $memory 1) + (export "memory" (memory $memory)) + (global (export "counter") (mut i32) (i32.const 0)) + (export "canister_query read_global" (func $read_global)) + (export "canister_update increase_global" (func $increase_global)) + )"#; + let wasm = wat::parse_str(wat).unwrap(); + + const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); + let own_subnet = subnet_test_id(1); + let caller_canister = canister_test_id(1); + + let mut test = ExecutionTestBuilder::new() + .with_own_subnet_id(own_subnet) + .with_snapshots(FlagStatus::Enabled) + .with_caller(own_subnet, caller_canister) + .build(); + + // Create canister. + let canister_id = test.canister_from_cycles_and_binary(CYCLES, wasm).unwrap(); + test.canister_update_reserved_cycles_limit(canister_id, CYCLES) + .unwrap(); + + // Check that global is initially 0 + let result = test + .non_replicated_query(canister_id, "read_global", vec![]) + .unwrap(); + assert_eq!(result, WasmResult::Reply(vec![0, 0, 0, 0])); + + // Increase global to 1 + test.ingress(canister_id, "increase_global", vec![]) + .unwrap(); + + // Check that global is now 1 + let result = test + .non_replicated_query(canister_id, "read_global", vec![]) + .unwrap(); + assert_eq!(result, WasmResult::Reply(vec![1, 0, 0, 0])); + + // Take a snapshot. + let args = TakeCanisterSnapshotArgs::new(canister_id, None); + let result = test + .subnet_message("take_canister_snapshot", args.encode()) + .unwrap(); + let response = CanisterSnapshotResponse::decode(&result.bytes()).unwrap(); + let snapshot_id = response.snapshot_id(); + + // Load the snapshot. + let args = LoadCanisterSnapshotArgs::new(canister_id, snapshot_id, None); + test.subnet_message("load_canister_snapshot", args.encode()) + .unwrap(); + + // Check that global is still 1 + let result = test + .non_replicated_query(canister_id, "read_global", vec![]) + .unwrap(); + assert_eq!(result, WasmResult::Reply(vec![1, 0, 0, 0])); +} From 351c7ec8e69e12501517b85248c054702e2a2d42 Mon Sep 17 00:00:00 2001 From: luc-blaeser Date: Fri, 30 Aug 2024 17:24:33 +0200 Subject: [PATCH 2/6] Fix: Canister snapshots should include Wasm globals --- rs/execution_environment/src/canister_manager.rs | 1 + .../v1/canister_snapshot_bits.proto | 1 + .../gen/state/state.canister_snapshot_bits.v1.rs | 2 ++ rs/replicated_state/src/canister_snapshots.rs | 16 +++++++++++++--- .../src/canister_state/execution_state.rs | 2 ++ rs/state_layout/src/state_layout.rs | 14 ++++++++++++++ rs/state_layout/src/state_layout/tests.rs | 1 + rs/state_manager/src/checkpoint.rs | 3 +++ rs/state_manager/src/tip.rs | 1 + 9 files changed, 38 insertions(+), 3 deletions(-) diff --git a/rs/execution_environment/src/canister_manager.rs b/rs/execution_environment/src/canister_manager.rs index 99bbd402c02..b86455d8441 100644 --- a/rs/execution_environment/src/canister_manager.rs +++ b/rs/execution_environment/src/canister_manager.rs @@ -2140,6 +2140,7 @@ impl CanisterManager { } }; + new_execution_state.exported_globals = execution_snapshot.exported_globals.clone(); new_execution_state.stable_memory = Memory::from(&execution_snapshot.stable_memory); new_execution_state.wasm_memory = Memory::from(&execution_snapshot.wasm_memory); (instructions_used, Some(new_execution_state)) diff --git a/rs/protobuf/def/state/canister_snapshot_bits/v1/canister_snapshot_bits.proto b/rs/protobuf/def/state/canister_snapshot_bits/v1/canister_snapshot_bits.proto index 209c01736c5..975fc6fd0f4 100644 --- a/rs/protobuf/def/state/canister_snapshot_bits/v1/canister_snapshot_bits.proto +++ b/rs/protobuf/def/state/canister_snapshot_bits/v1/canister_snapshot_bits.proto @@ -15,4 +15,5 @@ message CanisterSnapshotBits { uint64 stable_memory_size = 8; uint64 wasm_memory_size = 9; uint64 total_size = 10; + repeated canister_state_bits.v1.Global exported_globals = 11; } diff --git a/rs/protobuf/src/gen/state/state.canister_snapshot_bits.v1.rs b/rs/protobuf/src/gen/state/state.canister_snapshot_bits.v1.rs index 1b761a2072e..4ba482b9855 100644 --- a/rs/protobuf/src/gen/state/state.canister_snapshot_bits.v1.rs +++ b/rs/protobuf/src/gen/state/state.canister_snapshot_bits.v1.rs @@ -22,4 +22,6 @@ pub struct CanisterSnapshotBits { pub wasm_memory_size: u64, #[prost(uint64, tag = "10")] pub total_size: u64, + #[prost(message, repeated, tag = "11")] + pub exported_globals: ::prost::alloc::vec::Vec, } diff --git a/rs/replicated_state/src/canister_snapshots.rs b/rs/replicated_state/src/canister_snapshots.rs index 5d65d01ee8c..4756528d53a 100644 --- a/rs/replicated_state/src/canister_snapshots.rs +++ b/rs/replicated_state/src/canister_snapshots.rs @@ -1,7 +1,7 @@ use crate::{ - canister_state::execution_state::Memory, - canister_state::system_state::wasm_chunk_store::WasmChunkStore, CanisterState, NumWasmPages, - PageMap, + canister_state::execution_state::{Global, Memory}, + canister_state::system_state::wasm_chunk_store::WasmChunkStore, + CanisterState, NumWasmPages, PageMap, }; use ic_sys::PAGE_SIZE; use ic_types::{CanisterId, NumBytes, SnapshotId, Time}; @@ -284,6 +284,10 @@ pub struct ExecutionStateSnapshot { /// The raw canister module. #[validate_eq(Ignore)] pub wasm_binary: CanisterModule, + /// The Wasm global variables. + /// Note: The hypervisor instrumentations exports all global variables, + /// including originally internal global variables. + pub exported_globals: Vec, /// Snapshot of stable memory. #[validate_eq(CompareWithValidateEq)] pub stable_memory: PageMemory, @@ -345,6 +349,7 @@ impl CanisterSnapshot { .ok_or(CanisterSnapshotError::EmptyExecutionState(canister_id))?; let execution_snapshot = ExecutionStateSnapshot { wasm_binary: execution_state.wasm_binary.binary.clone(), + exported_globals: execution_state.exported_globals.clone(), stable_memory: PageMemory::from(&execution_state.stable_memory), wasm_memory: PageMemory::from(&execution_state.wasm_memory), }; @@ -392,6 +397,10 @@ impl CanisterSnapshot { &self.execution_snapshot.wasm_binary } + pub fn exported_globals(&self) -> &Vec { + &self.execution_snapshot.exported_globals + } + pub fn chunk_store(&self) -> &WasmChunkStore { &self.chunk_store } @@ -457,6 +466,7 @@ mod tests { ) -> (SnapshotId, CanisterSnapshot) { let execution_snapshot = ExecutionStateSnapshot { wasm_binary: CanisterModule::new(vec![1, 2, 3]), + exported_globals: vec![Global::I32(1), Global::I64(2), Global::F64(0.1)], stable_memory: PageMemory { page_map: PageMap::new_for_testing(), size: NumWasmPages::new(10), diff --git a/rs/replicated_state/src/canister_state/execution_state.rs b/rs/replicated_state/src/canister_state/execution_state.rs index 70c43ce6550..c1c9f70889a 100644 --- a/rs/replicated_state/src/canister_state/execution_state.rs +++ b/rs/replicated_state/src/canister_state/execution_state.rs @@ -106,6 +106,8 @@ impl PartialEq for Global { } } +impl Eq for Global {} + impl From<&Global> for pb::Global { fn from(item: &Global) -> Self { match item { diff --git a/rs/state_layout/src/state_layout.rs b/rs/state_layout/src/state_layout.rs index 045f1a8491f..51402364e89 100644 --- a/rs/state_layout/src/state_layout.rs +++ b/rs/state_layout/src/state_layout.rs @@ -201,6 +201,8 @@ pub struct CanisterSnapshotBits { pub wasm_memory_size: NumWasmPages, /// The total size of the snapshot in bytes. pub total_size: NumBytes, + /// State of the exported Wasm globals. + pub exported_globals: Vec, } #[derive(Clone)] @@ -2513,6 +2515,11 @@ impl From for pb_canister_snapshot_bits::CanisterSnapshotB stable_memory_size: item.stable_memory_size.get() as u64, wasm_memory_size: item.wasm_memory_size.get() as u64, total_size: item.total_size.get(), + exported_globals: item + .exported_globals + .iter() + .map(|global| global.into()) + .collect(), } } } @@ -2537,6 +2544,12 @@ impl TryFrom for CanisterSnapsh } None => None, }; + + let mut exported_globals = Vec::with_capacity(item.exported_globals.len()); + for global in item.exported_globals.into_iter() { + exported_globals.push(global.try_into()?); + } + Ok(Self { snapshot_id: SnapshotId::from((canister_id, item.snapshot_id)), canister_id, @@ -2552,6 +2565,7 @@ impl TryFrom for CanisterSnapsh stable_memory_size: NumWasmPages::from(item.stable_memory_size as usize), wasm_memory_size: NumWasmPages::from(item.wasm_memory_size as usize), total_size: NumBytes::from(item.total_size), + exported_globals, }) } } diff --git a/rs/state_layout/src/state_layout/tests.rs b/rs/state_layout/src/state_layout/tests.rs index 6bd5f06c589..683331df094 100644 --- a/rs/state_layout/src/state_layout/tests.rs +++ b/rs/state_layout/src/state_layout/tests.rs @@ -212,6 +212,7 @@ fn test_canister_snapshots_decode() { stable_memory_size: NumWasmPages::new(10), wasm_memory_size: NumWasmPages::new(10), total_size: NumBytes::new(100), + exported_globals: vec![Global::I32(1), Global::I64(2), Global::F64(3.141)], }; let pb_bits = diff --git a/rs/state_manager/src/checkpoint.rs b/rs/state_manager/src/checkpoint.rs index 925418e8838..ba4351edf42 100644 --- a/rs/state_manager/src/checkpoint.rs +++ b/rs/state_manager/src/checkpoint.rs @@ -579,8 +579,11 @@ pub fn load_snapshot( .deserialize(canister_snapshot_bits.binary_hash)?; durations.insert("snapshot_canister_module", starting_time.elapsed()); + let exported_globals = canister_snapshot_bits.exported_globals.clone(); + ExecutionStateSnapshot { wasm_binary, + exported_globals, stable_memory, wasm_memory, } diff --git a/rs/state_manager/src/tip.rs b/rs/state_manager/src/tip.rs index 951c5cbe5ee..f152c155860 100644 --- a/rs/state_manager/src/tip.rs +++ b/rs/state_manager/src/tip.rs @@ -1076,6 +1076,7 @@ fn serialize_snapshot_to_tip( stable_memory_size: canister_snapshot.stable_memory().size, wasm_memory_size: canister_snapshot.wasm_memory().size, total_size: canister_snapshot.size(), + exported_globals: canister_snapshot.exported_globals().clone(), } .into(), )?; From a37dfcb44c713184dc94d27a348636316b37a913 Mon Sep 17 00:00:00 2001 From: luc-blaeser Date: Fri, 30 Aug 2024 18:20:43 +0200 Subject: [PATCH 3/6] Appease clippy --- rs/state_layout/src/state_layout/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/state_layout/src/state_layout/tests.rs b/rs/state_layout/src/state_layout/tests.rs index 683331df094..c9bd140a6e0 100644 --- a/rs/state_layout/src/state_layout/tests.rs +++ b/rs/state_layout/src/state_layout/tests.rs @@ -212,7 +212,7 @@ fn test_canister_snapshots_decode() { stable_memory_size: NumWasmPages::new(10), wasm_memory_size: NumWasmPages::new(10), total_size: NumBytes::new(100), - exported_globals: vec![Global::I32(1), Global::I64(2), Global::F64(3.141)], + exported_globals: vec![Global::I32(1), Global::I64(2), Global::F64(0.1)], }; let pb_bits = From a9407ef06969e04a6181aab02b8eb528be019032 Mon Sep 17 00:00:00 2001 From: luc-blaeser Date: Mon, 2 Sep 2024 13:22:28 +0200 Subject: [PATCH 4/6] Simplify `snapshot_must_include_globals` test --- .../src/execution_environment/tests/canister_snapshots.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs index 3c8eb85e2ab..892f813ef86 100644 --- a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs +++ b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs @@ -1752,17 +1752,12 @@ fn snapshot_must_include_globals() { )"#; let wasm = wat::parse_str(wat).unwrap(); - const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); - let own_subnet = subnet_test_id(1); - let caller_canister = canister_test_id(1); - let mut test = ExecutionTestBuilder::new() - .with_own_subnet_id(own_subnet) .with_snapshots(FlagStatus::Enabled) - .with_caller(own_subnet, caller_canister) .build(); // Create canister. + const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); let canister_id = test.canister_from_cycles_and_binary(CYCLES, wasm).unwrap(); test.canister_update_reserved_cycles_limit(canister_id, CYCLES) .unwrap(); From 4252315d1eaeff39e66c5c9d7e6ef926370654eb Mon Sep 17 00:00:00 2001 From: luc-blaeser Date: Mon, 2 Sep 2024 13:23:09 +0200 Subject: [PATCH 5/6] Add attribute for canister snapshot state (review feedback) --- rs/replicated_state/src/canister_snapshots.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rs/replicated_state/src/canister_snapshots.rs b/rs/replicated_state/src/canister_snapshots.rs index 4756528d53a..b5fb43787f7 100644 --- a/rs/replicated_state/src/canister_snapshots.rs +++ b/rs/replicated_state/src/canister_snapshots.rs @@ -287,6 +287,7 @@ pub struct ExecutionStateSnapshot { /// The Wasm global variables. /// Note: The hypervisor instrumentations exports all global variables, /// including originally internal global variables. + #[validate_eq(Ignore)] pub exported_globals: Vec, /// Snapshot of stable memory. #[validate_eq(CompareWithValidateEq)] From 3e8bc69b2d184c2c9deb589f96faab2af94df600 Mon Sep 17 00:00:00 2001 From: luc-blaeser Date: Mon, 2 Sep 2024 13:27:45 +0200 Subject: [PATCH 6/6] Further simplify `snapshot_must_include_globals` test --- .../src/execution_environment/tests/canister_snapshots.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs index 892f813ef86..ec4b974e4f3 100644 --- a/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs +++ b/rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs @@ -1757,10 +1757,7 @@ fn snapshot_must_include_globals() { .build(); // Create canister. - const CYCLES: Cycles = Cycles::new(1_000_000_000_000_000); - let canister_id = test.canister_from_cycles_and_binary(CYCLES, wasm).unwrap(); - test.canister_update_reserved_cycles_limit(canister_id, CYCLES) - .unwrap(); + let canister_id = test.canister_from_binary(wasm).unwrap(); // Check that global is initially 0 let result = test