fix: subnet deletion in PocketIC with registry canister

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.

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<Subnet>) {
         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.

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);