From 57c35b42e356489ecde30c2a5ebe35b6c57abaa4 Mon Sep 17 00:00:00 2001 From: Martin Raszyk Date: Mon, 29 Jun 2026 14:19:05 +0000 Subject: [PATCH] fix: subnet deletion in PocketIC with registry canister --- packages/pocket-ic/tests/tests.rs | 27 +++++++++++++++++++--- rs/pocket_ic_server/src/pocket_ic.rs | 22 +++++++++++++++++- rs/registry/proto_data_provider/src/lib.rs | 12 ++++++++-- 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/packages/pocket-ic/tests/tests.rs b/packages/pocket-ic/tests/tests.rs index f51152870b5f..77569f04c707 100644 --- a/packages/pocket-ic/tests/tests.rs +++ b/packages/pocket-ic/tests/tests.rs @@ -3634,14 +3634,35 @@ fn cloud_engine_default_effective_canister_id() { #[test] fn test_delete_subnet() { - // Create a PocketIC instance with two application subnets. + // Create a PocketIC instance with an NNS subnet, two application subnets, + // and the `registry` ICP feature enabled. The `registry` ICP feature is + // important here: after every operation the server syncs its local registry + // from the registry canister, and deleting a subnet writes registry records + // directly to the local registry. Without keeping the registry canister in + // sync, this sync would loop forever (regression test). let pic = PocketIcBuilder::new() + .with_nns_subnet() + .with_icp_features(IcpFeatures { + registry: Some(IcpFeaturesConfig::DefaultConfig), + ..Default::default() + }) .with_application_subnet() .with_application_subnet() .build(); - let subnet_id_1 = pic.topology().get_app_subnets()[0]; - let subnet_id_2 = pic.topology().get_app_subnets()[1]; + // The subnet hosting the default effective canister ID cannot be deleted, so + // we keep it (as `subnet_id_1`) and delete the other application subnet. + let topology = pic.topology(); + let default_effective_canister_id: Principal = + topology.default_effective_canister_id.clone().into(); + let subnet_id_1 = topology + .get_subnet(default_effective_canister_id) + .expect("default effective canister ID must belong to a subnet"); + let subnet_id_2 = topology + .get_app_subnets() + .into_iter() + .find(|subnet_id| *subnet_id != subnet_id_1) + .expect("there must be a second application subnet"); assert_ne!(subnet_id_1, subnet_id_2); // Deploy test canisters on both subnets. diff --git a/rs/pocket_ic_server/src/pocket_ic.rs b/rs/pocket_ic_server/src/pocket_ic.rs index 2edb3b702bec..be7665cdd5b5 100644 --- a/rs/pocket_ic_server/src/pocket_ic.rs +++ b/rs/pocket_ic_server/src/pocket_ic.rs @@ -1200,6 +1200,17 @@ impl PocketIcSubnets { } // Upload registry to the registry canister. + self.sync_registry_to_canister(nns_subnet); + } + + /// Applies all registry data provider mutations that have not yet been + /// applied to the registry canister, advancing the registry canister to + /// the latest version of the local registry data provider. This keeps the + /// registry canister and the local registry data provider in sync so that + /// `sync_registry_from_canister` does not loop forever waiting for the + /// registry canister to reach a version that the local registry data + /// provider has already surpassed. + fn sync_registry_to_canister(&mut self, nns_subnet: Arc) { let mutation_requests: Vec<_> = self .registry_data_provider .export_versions_as_atomic_mutation_requests() @@ -2843,7 +2854,7 @@ impl PocketIcSubnets { } // Update global registry records to reflect the removed subnet. - if self.nns_subnet.is_some() { + if let Some(nns_subnet) = self.nns_subnet.clone() { let next_version = RegistryVersion::new(self.registry_data_provider.latest_version().get() + 1); remove_chain_key_registry_records( @@ -2871,6 +2882,15 @@ impl PocketIcSubnets { next_version, ); self.persist_registry_changes(); + // Apply the registry changes to the registry canister as well if the + // `registry` ICP feature is enabled so that the registry canister and + // the local registry data provider stay in sync (otherwise + // `sync_registry_from_canister` would loop forever). + if let Some(icp_features) = &self.icp_features + && icp_features.registry.is_some() + { + self.sync_registry_to_canister(nns_subnet); + } } // Drop the StateMachine, waiting until no other Arc holders remain. diff --git a/rs/registry/proto_data_provider/src/lib.rs b/rs/registry/proto_data_provider/src/lib.rs index 5768ac6ad990..20caca38ae3a 100644 --- a/rs/registry/proto_data_provider/src/lib.rs +++ b/rs/registry/proto_data_provider/src/lib.rs @@ -3,7 +3,7 @@ use ic_interfaces_registry::{RegistryDataProvider, RegistryRecord, RegistryValue use ic_registry_common_proto::pb::proto_registry::v1::{ProtoRegistry, ProtoRegistryRecord}; use ic_registry_transport::pb::v1::registry_mutation::Type; use ic_registry_transport::pb::v1::{RegistryAtomicMutateRequest, RegistryMutation}; -use ic_registry_transport::upsert; +use ic_registry_transport::{delete, upsert}; use ic_sys::fs::{Clobber, write_atomically}; use ic_types::{RegistryVersion, registry::RegistryDataProviderError}; use std::collections::HashMap; @@ -235,7 +235,15 @@ impl ProtoRegistryDataProvider { for record in records { let version = record.version; - let mutation = upsert(record.key, record.value.or_else(|| Some(vec![])).unwrap()); + // A record with no value represents a deletion (tombstone) and must + // be exported as a `delete` mutation rather than an `upsert` with an + // empty value; otherwise the registry canister would store an empty + // value for the key (e.g. a 0-byte crypto key) and fail its + // invariant checks. + let mutation = match record.value { + Some(value) => upsert(record.key, value), + None => delete(record.key), + }; if let Some(mutations_vec) = mutations_by_version.get_mut(&version) { mutations_vec.push(mutation);