From 0ff9c386096a3d4101ac3730037e91b9352b4923 Mon Sep 17 00:00:00 2001 From: mraszyk <31483726+mraszyk@users.noreply.github.com> Date: Fri, 25 Oct 2024 13:17:29 +0200 Subject: [PATCH] chore(PocketIC): use sequence numbers as state labels (#2157) This PR replaces a state label derived from the certified state by a sequence number. The reason for this change is that parts of a PocketIC instance state are not certified (e.g., ingress and canister http pools) and thus they are not reflected in the state label. Because it is not clear why having the same state label for two different instances with the same state would be beneficial at the moment, this PR uses sequence numbers as state labels. This PR also fixes a bug when deleting an instance: an instance should not be deleted if it is still busy with a computation. Finally, - the size of the test `//packages/pocket-ic:test` is reduced back to small (this test is fast again); - outdated comments are removed; - unnecessary Debug implementations are removed. --------- Co-authored-by: IDX GitHub Automation --- packages/pocket-ic/BUILD.bazel | 2 +- rs/pocket_ic_server/src/pocket_ic.rs | 297 +++----------------- rs/pocket_ic_server/src/state_api/routes.rs | 28 +- rs/pocket_ic_server/src/state_api/state.rs | 88 +++--- 4 files changed, 89 insertions(+), 326 deletions(-) diff --git a/packages/pocket-ic/BUILD.bazel b/packages/pocket-ic/BUILD.bazel index 6a085ff5af5a..d6b0948ceb8b 100644 --- a/packages/pocket-ic/BUILD.bazel +++ b/packages/pocket-ic/BUILD.bazel @@ -46,7 +46,7 @@ rust_library( rust_test_suite( name = "test", - size = "medium", + size = "small", srcs = ["tests/tests.rs"], data = [ "//packages/pocket-ic/test_canister:test_canister.wasm", diff --git a/rs/pocket_ic_server/src/pocket_ic.rs b/rs/pocket_ic_server/src/pocket_ic.rs index b2ff44996ebf..f93d54965567 100644 --- a/rs/pocket_ic_server/src/pocket_ic.rs +++ b/rs/pocket_ic_server/src/pocket_ic.rs @@ -231,10 +231,7 @@ pub struct PocketIc { routing_table: RoutingTable, /// Created on initialization and updated if a new subnet is created. topology: TopologyInternal, - // The initial state hash used for computing the state label - // to distinguish PocketIC instances with different initial configs. - initial_state_hash: [u8; 32], - // The following fields are used to create a new subnet. + state_label: StateLabel, range_gen: RangeGen, registry_data_provider: Arc, runtime: Arc, @@ -389,6 +386,7 @@ impl PocketIc { pub(crate) fn new( runtime: Arc, + seed: u64, subnet_configs: ExtendedSubnetConfigSet, state_dir: Option, nonmainnet_features: bool, @@ -656,15 +654,6 @@ impl PocketIc { subnet.execute_round(); } - let mut hasher = Sha256::new(); - let subnet_configs_string = format!("{:?}", subnet_configs); - hasher.write(subnet_configs_string.as_bytes()); - let initial_state_hash = compute_state_label( - &hasher.finish(), - subnets.read().unwrap().values().cloned().collect(), - ) - .0; - let canister_http_adapters = Arc::new(TokioMutex::new( subnets .read() @@ -699,13 +688,15 @@ impl PocketIc { default_effective_canister_id, }; + let state_label = StateLabel::new(seed); + Self { state_dir, subnets, canister_http_adapters, routing_table, topology, - initial_state_hash, + state_label, range_gen, registry_data_provider, runtime, @@ -716,6 +707,10 @@ impl PocketIc { } } + pub(crate) fn bump_state_label(&mut self) { + self.state_label.bump(); + } + fn try_route_canister(&self, canister_id: CanisterId) -> Option> { let subnet_id = self.routing_table.route(canister_id.into()); subnet_id.map(|subnet_id| self.get_subnet_with_id(subnet_id).unwrap()) @@ -765,6 +760,7 @@ impl Default for PocketIc { fn default() -> Self { Self::new( Runtime::new().unwrap().into(), + 0, ExtendedSubnetConfigSet { application: vec![SubnetSpec::default()], ..Default::default() @@ -777,31 +773,9 @@ impl Default for PocketIc { } } -fn compute_state_label( - initial_state_hash: &[u8; 32], - subnets: Vec>, -) -> StateLabel { - let mut hasher = Sha256::new(); - hasher.write(initial_state_hash); - for subnet in subnets { - let subnet_state_hash = subnet - .state_manager - .latest_state_certification_hash() - .map(|(_, h)| h.0) - .unwrap_or_else(|| [0u8; 32].to_vec()); - let nanos = systemtime_to_unix_epoch_nanos(subnet.time()); - hasher.write(&subnet_state_hash[..]); - hasher.write(&nanos.to_be_bytes()); - } - StateLabel(hasher.finish()) -} - impl HasStateLabel for PocketIc { fn get_state_label(&self) -> StateLabel { - compute_state_label( - &self.initial_state_hash, - self.subnets.read().unwrap().values().cloned().collect(), - ) + self.state_label.clone() } } @@ -2597,165 +2571,11 @@ mod tests { #[test] fn state_label_test() { // State label changes. - let pic = PocketIc::default(); - let state0 = pic.get_state_label(); - let canister_id = pic.any_subnet().create_canister(None); - pic.any_subnet().add_cycles(canister_id, 2_000_000_000_000); - let state1 = pic.get_state_label(); - pic.any_subnet().stop_canister(canister_id).unwrap(); - pic.any_subnet().delete_canister(canister_id).unwrap(); - let state2 = pic.get_state_label(); - - assert_ne!(state0, state1); - assert_ne!(state1, state2); - assert_ne!(state0, state2); - - // Empyt IC. - let pic = PocketIc::default(); - let state1 = pic.get_state_label(); - let pic = PocketIc::default(); - let state2 = pic.get_state_label(); - - assert_eq!(state1, state2); - - // Two ICs with the same state. - let pic = PocketIc::default(); - let cid = pic.any_subnet().create_canister(None); - pic.any_subnet().add_cycles(cid, 2_000_000_000_000); - pic.any_subnet().stop_canister(cid).unwrap(); - let state3 = pic.get_state_label(); - - let pic = PocketIc::default(); - let cid = pic.any_subnet().create_canister(None); - pic.any_subnet().add_cycles(cid, 2_000_000_000_000); - pic.any_subnet().stop_canister(cid).unwrap(); - let state4 = pic.get_state_label(); - - assert_eq!(state3, state4); - } - - #[test] - fn test_time() { - let mut pic = PocketIc::default(); - - let unix_time_ns = 1640995200000000000; // 1st Jan 2022 - let time = Time::from_nanos_since_unix_epoch(unix_time_ns); - compute_assert_state_change(&mut pic, SetTime { time }); - let actual_time = compute_assert_state_immutable(&mut pic, GetTime {}); - - match actual_time { - OpOut::Time(actual_time_ns) => assert_eq!(unix_time_ns, actual_time_ns), - _ => panic!("Unexpected OpOut: {:?}", actual_time), - }; - } - - #[test] - fn test_execute_message() { - let (mut pic, canister_id) = new_pic_counter_installed(); - let amount: u128 = 20_000_000_000_000; - let add_cycles = AddCycles { - canister_id, - amount, - }; - add_cycles.compute(&mut pic); - - let update = ExecuteIngressMessage(CanisterCall { - sender: PrincipalId::new_anonymous(), - canister_id, - method: "write".into(), - payload: vec![], - effective_principal: EffectivePrincipal::None, - }); - - compute_assert_state_change(&mut pic, update); - } - - #[test] - fn test_cycles_burn_app_subnet() { - let (mut pic, canister_id) = new_pic_counter_installed(); - let (_, update) = query_update_constructors(canister_id); - let cycles_balance = GetCyclesBalance { canister_id }; - let OpOut::Cycles(initial_balance) = - compute_assert_state_immutable(&mut pic, cycles_balance.clone()) - else { - unreachable!() - }; - compute_assert_state_change(&mut pic, update("write")); - let OpOut::Cycles(new_balance) = compute_assert_state_immutable(&mut pic, cycles_balance) - else { - unreachable!() - }; - assert_ne!(initial_balance, new_balance); - } - - #[test] - fn test_cycles_burn_system_subnet() { - let (mut pic, canister_id) = new_pic_counter_installed_system_subnet(); - let (_, update) = query_update_constructors(canister_id); - - let cycles_balance = GetCyclesBalance { canister_id }; - let OpOut::Cycles(initial_balance) = - compute_assert_state_immutable(&mut pic, cycles_balance.clone()) - else { - unreachable!() - }; - compute_assert_state_change(&mut pic, update("write")); - let OpOut::Cycles(new_balance) = compute_assert_state_immutable(&mut pic, cycles_balance) - else { - unreachable!() - }; - assert_eq!(initial_balance, new_balance); - } - - fn query_update_constructors( - canister_id: CanisterId, - ) -> ( - impl Fn(&str) -> Query, - impl Fn(&str) -> ExecuteIngressMessage, - ) { - let call = move |method: &str| CanisterCall { - sender: PrincipalId::new_anonymous(), - canister_id, - method: method.into(), - payload: vec![], - effective_principal: EffectivePrincipal::None, - }; - - let update = move |m: &str| ExecuteIngressMessage(call(m)); - let query = move |m: &str| Query(call(m)); - - (query, update) - } - - fn new_pic_counter_installed() -> (PocketIc, CanisterId) { - let mut pic = PocketIc::default(); - let canister_id = pic.any_subnet().create_canister(None); - - let amount: u128 = 20_000_000_000_000; - let add_cycles = AddCycles { - canister_id, - amount, - }; - add_cycles.compute(&mut pic); - - let module = counter_wasm(); - let install_op = InstallCanisterAsController { - canister_id, - mode: CanisterInstallMode::Install, - module, - payload: vec![], - }; - - compute_assert_state_change(&mut pic, install_op); - - (pic, canister_id) - } - - fn new_pic_counter_installed_system_subnet() -> (PocketIc, CanisterId) { - let mut pic = PocketIc::new( + let mut pic0 = PocketIc::new( Runtime::new().unwrap().into(), + 0, ExtendedSubnetConfigSet { - ii: Some(SubnetSpec::default()), + application: vec![SubnetSpec::default()], ..Default::default() }, None, @@ -2763,74 +2583,29 @@ mod tests { None, None, ); - let canister_id = pic.any_subnet().create_canister(None); - - let module = counter_wasm(); - let install_op = InstallCanisterAsController { - canister_id, - mode: CanisterInstallMode::Install, - module, - payload: vec![], - }; - - compute_assert_state_change(&mut pic, install_op); - - (pic, canister_id) - } - - fn compute_assert_state_change(pic: &mut PocketIc, op: impl Operation) -> OpOut { - let state0 = pic.get_state_label(); - let res = op.compute(pic); - let state1 = pic.get_state_label(); - assert_ne!(state0, state1); - res - } + let mut pic1 = PocketIc::new( + Runtime::new().unwrap().into(), + 1, + ExtendedSubnetConfigSet { + application: vec![SubnetSpec::default()], + ..Default::default() + }, + None, + false, + None, + None, + ); + assert_ne!(pic0.get_state_label(), pic1.get_state_label()); - fn compute_assert_state_immutable(pic: &mut PocketIc, op: impl Operation) -> OpOut { - let state0 = pic.get_state_label(); - let res = op.compute(pic); - let state1 = pic.get_state_label(); - assert_eq!(state0, state1); - res - } + let pic0_state_label = pic0.get_state_label(); + pic0.bump_state_label(); + assert_ne!(pic0.get_state_label(), pic0_state_label); + assert_ne!(pic0.get_state_label(), pic1.get_state_label()); - fn counter_wasm() -> Vec { - wat::parse_str(COUNTER_WAT).unwrap().as_slice().to_vec() + let pic1_state_label = pic1.get_state_label(); + pic1.bump_state_label(); + assert_ne!(pic1.get_state_label(), pic0_state_label); + assert_ne!(pic1.get_state_label(), pic1_state_label); + assert_ne!(pic1.get_state_label(), pic0.get_state_label()); } - - const COUNTER_WAT: &str = r#" -;; Counter with global variable ;; -(module - (import "ic0" "msg_reply" (func $msg_reply)) - (import "ic0" "msg_reply_data_append" - (func $msg_reply_data_append (param i32 i32))) - - (func $read - (i32.store - (i32.const 0) - (global.get 0) - ) - (call $msg_reply_data_append - (i32.const 0) - (i32.const 4)) - (call $msg_reply)) - - (func $write - (global.set 0 - (i32.add - (global.get 0) - (i32.const 1) - ) - ) - (call $read) - ) - - (memory $memory 1) - (export "memory" (memory $memory)) - (global (export "counter_global") (mut i32) (i32.const 0)) - (export "canister_query read" (func $read)) - (export "canister_query inc_read" (func $write)) - (export "canister_update write" (func $write)) -) - "#; } diff --git a/rs/pocket_ic_server/src/state_api/routes.rs b/rs/pocket_ic_server/src/state_api/routes.rs index a66f2e62be5d..a2d196f5cf09 100644 --- a/rs/pocket_ic_server/src/state_api/routes.rs +++ b/rs/pocket_ic_server/src/state_api/routes.rs @@ -1112,21 +1112,19 @@ pub async fn create_instance( None }; - let pocket_ic = tokio::task::spawn_blocking(move || { - PocketIc::new( - runtime, - subnet_configs, - instance_config.state_dir, - instance_config.nonmainnet_features, - log_level, - instance_config.bitcoind_addr, - ) - }) - .await - .expect("Failed to launch PocketIC"); - - let topology = pocket_ic.topology().clone(); - let instance_id = api_state.add_instance(pocket_ic).await; + let (instance_id, topology) = api_state + .add_instance(move |seed| { + PocketIc::new( + runtime, + seed, + subnet_configs, + instance_config.state_dir, + instance_config.nonmainnet_features, + log_level, + instance_config.bitcoind_addr, + ) + }) + .await; ( StatusCode::CREATED, Json(rest::CreateInstanceResponse::Created { diff --git a/rs/pocket_ic_server/src/state_api/state.rs b/rs/pocket_ic_server/src/state_api/state.rs index 9ac7a5b08ebd..976ef947e5f9 100644 --- a/rs/pocket_ic_server/src/state_api/state.rs +++ b/rs/pocket_ic_server/src/state_api/state.rs @@ -52,7 +52,10 @@ use pocket_ic::common::rest::{ }; use pocket_ic::{ErrorCode, UserError, WasmResult}; use serde::{Deserialize, Serialize}; -use std::{collections::HashMap, fmt, path::PathBuf, str::FromStr, sync::Arc, time::Duration}; +use std::{ + collections::HashMap, fmt, path::PathBuf, str::FromStr, sync::atomic::AtomicU64, sync::Arc, + time::Duration, +}; use tokio::{ sync::mpsc::error::TryRecvError, sync::mpsc::Receiver, @@ -74,12 +77,26 @@ const MIN_OPERATION_DELAY: Duration = Duration::from_millis(100); // The minimum delay between consecutive attempts to read the graph in auto progress mode. const READ_GRAPH_DELAY: Duration = Duration::from_millis(100); -pub const STATE_LABEL_HASH_SIZE: usize = 32; +pub const STATE_LABEL_HASH_SIZE: usize = 16; /// Uniquely identifies a state. -#[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Default, Deserialize)] +#[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Default)] pub struct StateLabel(pub [u8; STATE_LABEL_HASH_SIZE]); +impl StateLabel { + pub fn new(seed: u64) -> Self { + let mut seq_no: u128 = seed.into(); + seq_no <<= 64; + Self(seq_no.to_le_bytes()) + } + + pub fn bump(&mut self) { + let mut seq_no: u128 = u128::from_le_bytes(self.0); + seq_no += 1; + self.0 = seq_no.to_le_bytes(); + } +} + // The only error condition is if the vector has the wrong size. pub struct InvalidSize; @@ -116,18 +133,12 @@ struct Instance { state: InstanceState, } -impl std::fmt::Debug for Instance { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - writeln!(f, "{:?}", self.state)?; - Ok(()) - } -} - /// The state of the PocketIC API. pub struct ApiState { // impl note: If locks are acquired on both fields, acquire first on `instances` and then on `graph`. instances: Arc>>>, graph: Arc>>, + seed: AtomicU64, sync_wait_time: Duration, // PocketIC server port port: Option, @@ -194,6 +205,7 @@ impl PocketIcApiStateBuilder { Arc::new(ApiState { instances, graph, + seed: AtomicU64::new(0), sync_wait_time, port: self.port, http_gateways: Arc::new(RwLock::new(Vec::new())), @@ -711,14 +723,23 @@ impl ApiState { Self::read_result(self.graph.clone(), state_label, op_id) } - pub async fn add_instance(&self, instance: PocketIc) -> InstanceId { + pub async fn add_instance(&self, f: F) -> (InstanceId, Topology) + where + F: FnOnce(u64) -> PocketIc + std::marker::Send + 'static, + { + let seed = self.seed.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + // create the instance using `spawn_blocking` before acquiring a lock + let instance = tokio::task::spawn_blocking(move || f(seed)) + .await + .expect("Failed to create PocketIC instance"); + let topology = instance.topology(); let mut instances = self.instances.write().await; let instance_id = instances.len(); instances.push(Mutex::new(Instance { progress_thread: None, state: InstanceState::Available(instance), })); - instance_id + (instance_id, topology) } pub async fn delete_instance(&self, instance_id: InstanceId) { @@ -726,9 +747,9 @@ impl ApiState { loop { let instances = self.instances.read().await; let mut instance = instances[instance_id].lock().await; - match std::mem::replace(&mut instance.state, InstanceState::Deleted) { - InstanceState::Available(pocket_ic) => { - std::mem::drop(pocket_ic); + match &instance.state { + InstanceState::Available(_) => { + let _ = std::mem::replace(&mut instance.state, InstanceState::Deleted); break; } InstanceState::Deleted => { @@ -1407,6 +1428,7 @@ impl ApiState { op_id.0, ); let result = op.compute(&mut pocket_ic); + pocket_ic.bump_state_label(); let new_state_label = pocket_ic.get_state_label(); // add result to graph, but grab instance lock first! let instances = instances.blocking_read(); @@ -1424,8 +1446,7 @@ impl ApiState { instance.state = InstanceState::Available(pocket_ic); } trace!("bg_task::end instance_id={} op_id={}", instance_id, op_id.0); - // also return old_state_label so we can prune graph if we return quickly - (result, old_state_label) + result } }; @@ -1458,7 +1479,7 @@ impl ApiState { // note: this assumes that cancelling the JoinHandle does not stop the execution of the // background task. This only works because the background thread, in this case, is a // kernel thread. - if let Ok(Ok((op_out, _old_state_label))) = time::timeout(sync_wait_time, bg_handle).await { + if let Ok(Ok(op_out)) = time::timeout(sync_wait_time, bg_handle).await { trace!( "update_with_timeout::synchronous instance_id={} op_id={}", instance_id, @@ -1475,34 +1496,3 @@ impl ApiState { Ok(busy_outcome) } } - -impl std::fmt::Debug for InstanceState { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Busy { state_label, op_id } => { - write!(f, "Busy {{ {state_label:?}, {op_id:?} }}")? - } - Self::Available(pic) => write!(f, "Available({:?})", pic.get_state_label())?, - Self::Deleted => write!(f, "Deleted")?, - } - Ok(()) - } -} - -impl std::fmt::Debug for ApiState { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let instances = self.instances.blocking_read(); - let graph = self.graph.blocking_read(); - - writeln!(f, "Instances:")?; - for (idx, instance) in instances.iter().enumerate() { - writeln!(f, " [{idx}] {instance:?}")?; - } - - writeln!(f, "Graph:")?; - for (k, v) in graph.iter() { - writeln!(f, " {k:?} => {v:?}")?; - } - Ok(()) - } -}