From e8239be5ffa7a25578a38b88fb2edeceaea8d208 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 22 Jul 2026 12:48:04 +0000 Subject: [PATCH 1/4] Allow Cloud Engines to have blank replica_version_id. --- rs/registry/canister/src/flags.rs | 18 ++ .../src/invariants/replica_version.rs | 220 +++++++++++++++++- 2 files changed, 231 insertions(+), 7 deletions(-) diff --git a/rs/registry/canister/src/flags.rs b/rs/registry/canister/src/flags.rs index f999a0b6abcb..220c6a48becf 100644 --- a/rs/registry/canister/src/flags.rs +++ b/rs/registry/canister/src/flags.rs @@ -9,6 +9,7 @@ thread_local! { static IS_SUBNET_SPLITTING_ENABLED: Cell = const { Cell::new(false) }; static IS_CHUNKIFYING_LARGE_VALUES_ENABLED: Cell = const { Cell::new(true) }; static IS_NODE_SWAPPING_ENABLED: Cell = const { Cell::new(true) }; + static IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED: Cell = const { Cell::new(false) }; // Temporary flags related to the node swapping feature. // @@ -62,6 +63,23 @@ pub(crate) fn is_subnet_splitting_enabled() -> bool { IS_SUBNET_SPLITTING_ENABLED.get() } +pub(crate) fn is_blank_replica_version_id_for_cloud_engines_enabled() -> bool { + IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED.get() +} + +#[cfg(test)] +pub(crate) fn temporarily_enable_blank_replica_version_id_for_cloud_engines() -> Temporary { + Temporary::new(&IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED, true) +} + +#[cfg(test)] +pub(crate) fn temporarily_disable_blank_replica_version_id_for_cloud_engines() -> Temporary { + Temporary::new( + &IS_BLANK_REPLICA_VERSION_ID_FOR_CLOUD_ENGINES_ENABLED, + false, + ) +} + #[cfg(any(test, feature = "test"))] pub mod temporary_overrides { use super::*; diff --git a/rs/registry/canister/src/invariants/replica_version.rs b/rs/registry/canister/src/invariants/replica_version.rs index b7b51afcd551..ce5a6a5ec6d8 100644 --- a/rs/registry/canister/src/invariants/replica_version.rs +++ b/rs/registry/canister/src/invariants/replica_version.rs @@ -1,16 +1,20 @@ use std::collections::BTreeSet; -use crate::invariants::common::{ - InvariantCheckError, RegistrySnapshot, assert_valid_urls_and_hash, - get_all_replica_version_records, get_api_boundary_node_records_from_snapshot, - get_subnet_ids_from_snapshot, get_value_from_snapshot, +use crate::{ + flags::is_blank_replica_version_id_for_cloud_engines_enabled, + invariants::common::{ + InvariantCheckError, RegistrySnapshot, assert_valid_urls_and_hash, + get_all_replica_version_records, get_api_boundary_node_records_from_snapshot, + get_subnet_ids_from_snapshot, get_value_from_snapshot, + }, }; use ic_base_types::SubnetId; use ic_protobuf::registry::{ replica_version::v1::ReplicaVersionRecord, standard_engine_replica_version::v1::StandardEngineReplicaVersionRecord, - subnet::v1::SubnetRecord, unassigned_nodes_config::v1::UnassignedNodesConfigRecord, + subnet::v1::{SubnetRecord, SubnetType}, + unassigned_nodes_config::v1::UnassignedNodesConfigRecord, }; use ic_registry_keys::{ make_replica_version_key, make_standard_engine_replica_version_record_key, @@ -28,6 +32,11 @@ use prost::Message; /// * The corresponding ReplicaVersionRecord exists. /// * Each URL is well-formed. /// * Release package hash is a well-formed hex-encoded SHA256 value. +/// +/// Exception: a CloudEngine can have a blank replica_version_id in its +/// SubnetRecord if there is a StandardEngineReplicaVersionRecord. As of July +/// 22, 2026, this feature is disabled via a flag (but the plan is to enable it +/// in the not too distant future). pub(crate) fn check_replica_version_invariants( snapshot: &RegistrySnapshot, ) -> Result<(), InvariantCheckError> { @@ -93,9 +102,38 @@ fn get_subnet_record(snapshot: &RegistrySnapshot, subnet_id: SubnetId) -> Subnet /// Returns the list of replica versions where each version is referred to /// by at least one subnet. fn get_all_replica_versions_of_subnets(snapshot: &RegistrySnapshot) -> BTreeSet { + let cloud_engines_are_allowed_to_have_blank_replica_version_id = + is_blank_replica_version_id_for_cloud_engines_enabled() + && snapshot + .get(make_standard_engine_replica_version_record_key().as_bytes()) + .is_some(); + get_subnet_ids_from_snapshot(snapshot) .iter() - .map(|subnet_id| get_subnet_record(snapshot, *subnet_id).replica_version_id) + .filter_map(|subnet_id| { + let SubnetRecord { + replica_version_id, + subnet_type, + .. + } = get_subnet_record(snapshot, *subnet_id); + + if !replica_version_id.is_empty() { + // For non-CloudEngines, this is normal (because it is required). + return Some(replica_version_id); + } + + if subnet_type == SubnetType::CloudEngine as i32 + && cloud_engines_are_allowed_to_have_blank_replica_version_id + { + // For CloudEngines, this is normal (because this is allowed and typical). + return None; + } + + // Most likely, this will eventually lead to an explosion, because + // at this point, replica_version_id is empty, and in practice, we + // would have no elected replica version with an ID of length 0. + return Some(replica_version_id); + }) .collect() } @@ -124,7 +162,16 @@ fn get_all_standard_engine_replica_versions(snapshot: &RegistrySnapshot) -> BTre #[cfg(test)] mod tests { - use crate::common::test_helpers::invariant_compliant_registry; + use crate::{ + common::test_helpers::{ + invariant_compliant_registry, prepare_registry_with_cloud_engine_subnet, + }, + flags::{ + temporarily_disable_blank_replica_version_id_for_cloud_engines, + temporarily_enable_blank_replica_version_id_for_cloud_engines, + }, + registry::Registry, + }; use super::*; use canister_test::PrincipalId; @@ -293,6 +340,165 @@ mod tests { // The assertion is at the top: #[should_panic(...)]. } + /// Adds a CloudEngine subnet (subnet_type, cycles cost schedule, member + /// node reward types, and crypto/CUP material all set up) to the given + /// registry. Its replica_version_id is left as the default, which is + /// non-blank. Returns the new subnet's id. + fn add_cloud_engine_subnet(registry: &mut Registry) -> SubnetId { + let (cloud_engine_request, subnet_id) = prepare_registry_with_cloud_engine_subnet(1, 1); + registry.maybe_apply_mutation_internal(cloud_engine_request.mutations); + + subnet_id + } + + /// Returns a collection of mutations that blanks out the given subnet's + /// replica_version_id. + fn blank_replica_version_id_mutation( + registry: &Registry, + subnet_id: SubnetId, + ) -> Vec { + let mut subnet = registry.get_subnet_or_panic(subnet_id); + subnet.replica_version_id = "".to_string(); + + vec![upsert( + make_subnet_record_key(subnet_id).into_bytes(), + subnet.encode_to_vec(), + )] + } + + /// One thing not mentioned in the name is that + /// StandardEngineReplicaVersionRecord must exist (and this feature must be + /// enabled via flag). + #[test] + fn test_blank_replica_version_id_is_allowed() { + // Step 1: Prepare the world. + + let _restore_on_drop = temporarily_enable_blank_replica_version_id_for_cloud_engines(); + let mut registry = invariant_compliant_registry(0); + + // Elect replica versions 1 and 2. + registry.maybe_apply_mutation_internal(elect_version_mutations(vec![ + REPLICA_VERSION_ID_1.to_string(), + REPLICA_VERSION_ID_2.to_string(), + ])); + + // Upgrade 10% of CloudEngines to replica version 2 (from version 1). + registry.maybe_apply_mutation_internal(vec![insert( + make_standard_engine_replica_version_record_key().as_bytes(), + StandardEngineReplicaVersionRecord { + new_replica_version_id: REPLICA_VERSION_ID_1.to_string(), + old_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + deployment_progress: 0.1, + } + .encode_to_vec(), + )]); + + // Create a Cloud Engine. + let subnet_id = add_cloud_engine_subnet(&mut registry); + + // Step 2: Run the code under test. + let mutation = blank_replica_version_id_mutation(®istry, subnet_id); + registry.check_global_state_invariants(&mutation); + + // Step 3: Verify result(s). + // The implicit assertion is that the previous line did not panic. + } + + #[test] + #[should_panic(expected = "Using a version that isn't elected.")] + fn panic_when_blank_replica_version_id_is_not_enabled_yet() { + // Step 1: Prepare the world. The only difference compared to the + // previous test is that here, the feature is DISABLED. + let _restore_on_drop = temporarily_disable_blank_replica_version_id_for_cloud_engines(); + let mut registry = invariant_compliant_registry(0); + registry.maybe_apply_mutation_internal(elect_version_mutations(vec![ + REPLICA_VERSION_ID_1.to_string(), + REPLICA_VERSION_ID_2.to_string(), + ])); + registry.maybe_apply_mutation_internal(vec![insert( + make_standard_engine_replica_version_record_key().as_bytes(), + StandardEngineReplicaVersionRecord { + new_replica_version_id: REPLICA_VERSION_ID_1.to_string(), + old_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + deployment_progress: 0.1, + } + .encode_to_vec(), + )]); + let subnet_id = add_cloud_engine_subnet(&mut registry); + + // Step 2: Run the code under test. Same as the previous test. + let mutation = blank_replica_version_id_mutation(®istry, subnet_id); + registry.check_global_state_invariants(&mutation); + + // Step 3: Verify result(s). + // The assertion is at the top: #[should_panic(...)]. + // Unlike the previous test where the nominal behavior is NO panic. + } + + /// It is fine for there to be no standard engine replica version if there + /// are no CloudEngines, but in general, there would be, so the name of this + /// test does not mention this "and the sun must exist" condition. + #[test] + #[should_panic(expected = "Using a version that isn't elected.")] + fn panic_when_there_is_no_standard_replica_version() { + // Step 1: Prepare the world. + let _restore_on_drop = temporarily_enable_blank_replica_version_id_for_cloud_engines(); + let mut registry = invariant_compliant_registry(0); + let subnet_id = add_cloud_engine_subnet(&mut registry); + + // Step 2: Run the code under test. + let mutation = blank_replica_version_id_mutation(®istry, subnet_id); + registry.check_global_state_invariants(&mutation); + + // Step 3: Verify result(s). + // The assertion is at the top: #[should_panic(...)]. + } + + #[test] + #[should_panic(expected = "Using a version that isn't elected.")] + fn panic_when_non_cloud_engine_subnet_has_blank_replica_version_id() { + // Step 1: Prepare the world. + + let _restore_on_drop = temporarily_enable_blank_replica_version_id_for_cloud_engines(); + let mut registry = invariant_compliant_registry(0); + + // Like in previous tests, elect a couple of replica versions, and + // upgrade 10% of the CloudEngine fleet to version 2 (from version 1). + registry.maybe_apply_mutation_internal(elect_version_mutations(vec![ + REPLICA_VERSION_ID_1.to_string(), + REPLICA_VERSION_ID_2.to_string(), + ])); + registry.maybe_apply_mutation_internal(vec![insert( + make_standard_engine_replica_version_record_key().as_bytes(), + StandardEngineReplicaVersionRecord { + new_replica_version_id: REPLICA_VERSION_ID_1.to_string(), + old_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + deployment_progress: 0.1, + } + .encode_to_vec(), + )]); + + // Step 2: Run the code under test. + + // Blank the replica_version_id field of a (non-CloudEngine) subnet. + let list = registry.get_subnet_list_record(); + let subnet_id = + SubnetId::from(PrincipalId::try_from(list.subnets.first().unwrap()).unwrap()); + let mut subnet = registry.get_subnet_or_panic(subnet_id); + assert_ne!(subnet.subnet_type, SubnetType::CloudEngine as i32); + subnet.replica_version_id = "".to_string(); + + // Update the record. + let mutation = vec![upsert( + make_subnet_record_key(subnet_id).into_bytes(), + subnet.encode_to_vec(), + )]; + registry.check_global_state_invariants(&mutation); + + // Step 3: Verify result(s). + // The assertion is at the top: #[should_panic(...)]. + } + #[test] #[should_panic(expected = "Using a version that isn't elected.")] fn panic_when_retiring_a_version_in_use() { From 4f612403de192d29f76a3749ef268f3fd0719c01 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 22 Jul 2026 13:48:01 +0000 Subject: [PATCH 2/4] small comment enhancement --- rs/registry/canister/src/invariants/replica_version.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rs/registry/canister/src/invariants/replica_version.rs b/rs/registry/canister/src/invariants/replica_version.rs index ce5a6a5ec6d8..d094e1ea2ab3 100644 --- a/rs/registry/canister/src/invariants/replica_version.rs +++ b/rs/registry/canister/src/invariants/replica_version.rs @@ -118,14 +118,16 @@ fn get_all_replica_versions_of_subnets(snapshot: &RegistrySnapshot) -> BTreeSet< } = get_subnet_record(snapshot, *subnet_id); if !replica_version_id.is_empty() { - // For non-CloudEngines, this is normal (because it is required). + // For non-CloudEngines, this is normal (because it is + // required). CloudEngines can also end up here. return Some(replica_version_id); } if subnet_type == SubnetType::CloudEngine as i32 && cloud_engines_are_allowed_to_have_blank_replica_version_id { - // For CloudEngines, this is normal (because this is allowed and typical). + // For CloudEngines, this is normal (because this is allowed and + // typical). return None; } From 879e01566f163f452294f19addbfec5db751f7eb Mon Sep 17 00:00:00 2001 From: daniel-wong-dfinity-org-twin Date: Wed, 22 Jul 2026 15:56:35 +0200 Subject: [PATCH 3/4] More realistic test data (and match comments). Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../canister/src/invariants/replica_version.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/rs/registry/canister/src/invariants/replica_version.rs b/rs/registry/canister/src/invariants/replica_version.rs index d094e1ea2ab3..965897c0a460 100644 --- a/rs/registry/canister/src/invariants/replica_version.rs +++ b/rs/registry/canister/src/invariants/replica_version.rs @@ -388,8 +388,8 @@ mod tests { registry.maybe_apply_mutation_internal(vec![insert( make_standard_engine_replica_version_record_key().as_bytes(), StandardEngineReplicaVersionRecord { - new_replica_version_id: REPLICA_VERSION_ID_1.to_string(), - old_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + new_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + old_replica_version_id: REPLICA_VERSION_ID_1.to_string(), deployment_progress: 0.1, } .encode_to_vec(), @@ -420,8 +420,8 @@ mod tests { registry.maybe_apply_mutation_internal(vec![insert( make_standard_engine_replica_version_record_key().as_bytes(), StandardEngineReplicaVersionRecord { - new_replica_version_id: REPLICA_VERSION_ID_1.to_string(), - old_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + new_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + old_replica_version_id: REPLICA_VERSION_ID_1.to_string(), deployment_progress: 0.1, } .encode_to_vec(), @@ -473,8 +473,8 @@ mod tests { registry.maybe_apply_mutation_internal(vec![insert( make_standard_engine_replica_version_record_key().as_bytes(), StandardEngineReplicaVersionRecord { - new_replica_version_id: REPLICA_VERSION_ID_1.to_string(), - old_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + new_replica_version_id: REPLICA_VERSION_ID_2.to_string(), + old_replica_version_id: REPLICA_VERSION_ID_1.to_string(), deployment_progress: 0.1, } .encode_to_vec(), From 88e839022a7af99e4f96f02c7262dcaaef3ec392 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 22 Jul 2026 14:14:40 +0000 Subject: [PATCH 4/4] tiny clippy fix --- rs/registry/canister/src/invariants/replica_version.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rs/registry/canister/src/invariants/replica_version.rs b/rs/registry/canister/src/invariants/replica_version.rs index 965897c0a460..0328f7b2d3a5 100644 --- a/rs/registry/canister/src/invariants/replica_version.rs +++ b/rs/registry/canister/src/invariants/replica_version.rs @@ -134,7 +134,7 @@ fn get_all_replica_versions_of_subnets(snapshot: &RegistrySnapshot) -> BTreeSet< // Most likely, this will eventually lead to an explosion, because // at this point, replica_version_id is empty, and in practice, we // would have no elected replica version with an ID of length 0. - return Some(replica_version_id); + Some(replica_version_id) }) .collect() }