From 2e5e9ee8214d4af91006c2abfc371effa29c25fe Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Wed, 1 Jul 2026 15:47:56 +0000 Subject: [PATCH 1/3] refactor: Move api_boundary_nodes into NetworkTopology This is the first step of consolidating the various SystemMetadata fields populated from the registry under NetworkTopology (for network-wide information), on the one hand; and a similar SubnetTopology (for subnet-related information), on the other. Once all this consolidation is complete, we can reuse the two structs (wrapped in Arcs in SystemMetadata) from one ReplicatedState to the next, iff they use the same registry version. --- .../src/lazy_tree_conversion.rs | 2 +- rs/canonical_state/src/traversal.rs | 2 +- rs/http_endpoints/public/tests/common/mod.rs | 1 + rs/messaging/src/message_routing.rs | 9 +-- rs/messaging/src/message_routing/tests.rs | 58 +++++++++-------- rs/messaging/src/state_machine.rs | 5 +- rs/messaging/src/state_machine/tests.rs | 5 -- .../def/state/metadata/v1/metadata.proto | 1 + .../src/gen/state/state.metadata.v1.rs | 2 + rs/replicated_state/src/metadata_state.rs | 13 ++-- .../src/metadata_state/proto.rs | 62 ++++++++++++++----- .../src/metadata_state/tests.rs | 18 +++--- rs/state_manager/src/tree_hash.rs | 2 +- rs/state_manager/tests/state_manager.rs | 2 +- 14 files changed, 102 insertions(+), 80 deletions(-) diff --git a/rs/canonical_state/src/lazy_tree_conversion.rs b/rs/canonical_state/src/lazy_tree_conversion.rs index 21fd100c06a7..732a5ad4f80d 100644 --- a/rs/canonical_state/src/lazy_tree_conversion.rs +++ b/rs/canonical_state/src/lazy_tree_conversion.rs @@ -459,7 +459,7 @@ pub fn replicated_state_as_lazy_tree(state: &ReplicatedState, height: Height) -> FiniteMap::default() .with("api_boundary_nodes", move || { api_boundary_nodes_as_tree( - &state.metadata.api_boundary_nodes, + &state.metadata.network_topology.api_boundary_nodes, certification_version, ) }) diff --git a/rs/canonical_state/src/traversal.rs b/rs/canonical_state/src/traversal.rs index 24b996f43f8e..122aadafe5dd 100644 --- a/rs/canonical_state/src/traversal.rs +++ b/rs/canonical_state/src/traversal.rs @@ -1282,7 +1282,7 @@ mod tests { #[test] fn test_traverse_api_boundary_nodes() { let mut state = ReplicatedState::new(subnet_test_id(1), SubnetType::Application); - state.metadata.api_boundary_nodes = btreemap! { + std::sync::Arc::make_mut(&mut state.metadata.network_topology).api_boundary_nodes = btreemap! { node_test_id(11) => ApiBoundaryNodeEntry { domain: "api-bn11-example.com".to_string(), ipv4_address: Some("127.0.0.1".to_string()), diff --git a/rs/http_endpoints/public/tests/common/mod.rs b/rs/http_endpoints/public/tests/common/mod.rs index f511b262d828..18ef37c96995 100644 --- a/rs/http_endpoints/public/tests/common/mod.rs +++ b/rs/http_endpoints/public/tests/common/mod.rs @@ -257,6 +257,7 @@ pub fn default_get_latest_state() -> Labeled> { None, None, None, + Default::default(), ); metadata.network_topology = Arc::new(network_topology); diff --git a/rs/messaging/src/message_routing.rs b/rs/messaging/src/message_routing.rs index b3431044ccaf..b257d0102832 100644 --- a/rs/messaging/src/message_routing.rs +++ b/rs/messaging/src/message_routing.rs @@ -730,7 +730,6 @@ impl BatchProcessorImpl { ResourceLimits, RegistryExecutionSettings, NodePublicKeys, - ApiBoundaryNodes, ) { loop { match self.try_to_read_registry(registry_version, own_subnet_id) { @@ -777,7 +776,6 @@ impl BatchProcessorImpl { ResourceLimits, RegistryExecutionSettings, NodePublicKeys, - ApiBoundaryNodes, ), ReadRegistryError, > { @@ -792,7 +790,6 @@ impl BatchProcessorImpl { .try_into() .unwrap_or(SubnetType::CloudEngine); - let api_boundary_nodes = self.try_to_populate_api_boundary_nodes(registry_version)?; let network_topology = self.try_to_populate_network_topology( registry_version, own_subnet_id, @@ -914,7 +911,6 @@ impl BatchProcessorImpl { registry_version, }, node_public_keys, - api_boundary_nodes, )) } @@ -1156,6 +1152,8 @@ impl BatchProcessorImpl { None }; + let api_boundary_nodes = self.try_to_populate_api_boundary_nodes(registry_version)?; + Ok(NetworkTopology::new( subnets, Arc::new(routing_table), @@ -1166,6 +1164,7 @@ impl BatchProcessorImpl { self.bitcoin_config.mainnet_canister_id, full_topology, default_initial_dkg_subnet_id, + api_boundary_nodes, )) } @@ -1395,7 +1394,6 @@ impl BatchProcessor for BatchProcessorImpl BatchProcessor for BatchProcessorImpl ReplicatedState { state.metadata.network_topology = Arc::new(network_topology); state.metadata.own_subnet_features = subnet_features; state.metadata.own_resource_limits = resource_limits; state.metadata.node_public_keys = node_public_keys; - state.metadata.api_boundary_nodes = api_boundary_nodes; state.put_canister_state( CanisterStateBuilder::new() .with_canister_id(canister_test_id(1)) @@ -696,7 +694,6 @@ fn try_to_read_registry( ResourceLimits, RegistryExecutionSettings, NodePublicKeys, - ApiBoundaryNodes, ), ReadRegistryError, > { @@ -894,7 +891,6 @@ fn try_read_registry_succeeds_with_fully_specified_registry_records() { own_resource_limits, registry_execution_settings, node_public_keys, - api_boundary_nodes, ) = batch_processor .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id) .unwrap(); @@ -951,6 +947,30 @@ fn try_read_registry_succeeds_with_fully_specified_registry_records() { assert_eq!(&routing_table, network_topology.routing_table().as_ref()); assert_eq!(canister_migrations, *network_topology.canister_migrations); + // Check API Boundary Nodes. + let api_boundary_nodes = &network_topology.api_boundary_nodes; + assert_eq!(api_boundary_nodes.len(), 2); + let entry_1 = api_boundary_nodes.get(&node_test_id(11)).unwrap(); + assert_eq!( + entry_1, + &ApiBoundaryNodeEntry { + domain: "api-bn11.example.org".to_string(), + ipv4_address: Some("127.0.0.1".to_string()), + ipv6_address: "2001:0db8:85a3:0000:0000:8a2e:0370:7334".to_string(), + pubkey: None, + } + ); + let entry_2 = api_boundary_nodes.get(&node_test_id(12)).unwrap(); + assert_eq!( + entry_2, + &ApiBoundaryNodeEntry { + domain: "api-bn12.example.org".to_string(), + ipv4_address: None, + ipv6_address: "2001:0db8:85a3:0000:0000:8a2e:0370:7335".to_string(), + pubkey: None, + } + ); + // Check registry execution settings. assert_eq!( own_subnet_record.max_number_of_canisters, @@ -990,29 +1010,6 @@ fn try_read_registry_succeeds_with_fully_specified_registry_records() { ); } - // Check API Boundary Nodes. - assert_eq!(api_boundary_nodes.len(), 2); - let entry_1 = api_boundary_nodes.get(&node_test_id(11)).unwrap(); - assert_eq!( - entry_1, - &ApiBoundaryNodeEntry { - domain: "api-bn11.example.org".to_string(), - ipv4_address: Some("127.0.0.1".to_string()), - ipv6_address: "2001:0db8:85a3:0000:0000:8a2e:0370:7334".to_string(), - pubkey: None, - } - ); - let entry_2 = api_boundary_nodes.get(&node_test_id(12)).unwrap(); - assert_eq!( - entry_2, - &ApiBoundaryNodeEntry { - domain: "api-bn12.example.org".to_string(), - ipv4_address: None, - ipv6_address: "2001:0db8:85a3:0000:0000:8a2e:0370:7335".to_string(), - pubkey: None, - } - ); - // Commit a state with `own_subnet_id` in its metadata to ensure the latest // state corresponds to the registry records written above. let (height, mut state) = state_manager.take_tip(); @@ -1128,7 +1125,7 @@ fn try_read_registry_succeeds_with_minimal_registry_records() { // critical error for `subnet_size` has incremented. assert_eq!(metrics.critical_error_missing_subnet_size.get(), 1); // Check the subnet size was set to the maximum for a small app subnet. - let (_, _, _, registry_execution_settings, _, _) = result.unwrap(); + let (_, _, _, registry_execution_settings, _) = result.unwrap(); assert_eq!( registry_execution_settings.subnet_size, SMALL_APP_SUBNET_MAX_SIZE @@ -1453,7 +1450,7 @@ fn try_read_registry_can_skip_missing_or_invalid_node_public_keys() { 2 ); - let (_, _, _, _, node_public_keys, _) = res.unwrap(); + let (_, _, _, _, node_public_keys) = res.unwrap(); assert_eq!(node_public_keys.len(), 1); assert!(!node_public_keys.contains_key(&node_test_id(1))); assert!(!node_public_keys.contains_key(&node_test_id(2))); @@ -1592,7 +1589,8 @@ fn try_read_registry_can_skip_missing_or_invalid_fields_of_api_boundary_nodes() // There are six API BNs in the registry. However, five nodes have missing or invalid fields of NodeRecord. // Hence, only one nodes are retrieved. - let (_, _, _, _, _, api_boundary_nodes) = res.unwrap(); + let (network_topology, _, _, _, _) = res.unwrap(); + let api_boundary_nodes = &network_topology.api_boundary_nodes; assert_eq!(api_boundary_nodes.len(), 1); assert!(api_boundary_nodes.contains_key(&node_test_id(11))); diff --git a/rs/messaging/src/state_machine.rs b/rs/messaging/src/state_machine.rs index acb57a963a52..4dff2ed94863 100644 --- a/rs/messaging/src/state_machine.rs +++ b/rs/messaging/src/state_machine.rs @@ -1,5 +1,5 @@ use crate::message_routing::{ - ApiBoundaryNodes, CRITICAL_ERROR_INDUCT_RESPONSE_FAILED, MessageRoutingMetrics, NodePublicKeys, + CRITICAL_ERROR_INDUCT_RESPONSE_FAILED, MessageRoutingMetrics, NodePublicKeys, }; use crate::routing::demux::Demux; use crate::routing::stream_builder::{ @@ -39,7 +39,6 @@ pub(crate) trait StateMachine: Send { resource_limits: ResourceLimits, registry_settings: &RegistryExecutionSettings, node_public_keys: NodePublicKeys, - api_boundary_nodes: ApiBoundaryNodes, ) -> ReplicatedState; } pub(crate) struct StateMachineImpl { @@ -113,7 +112,6 @@ impl StateMachine for StateMachineImpl { resource_limits: ResourceLimits, registry_settings: &RegistryExecutionSettings, node_public_keys: NodePublicKeys, - api_boundary_nodes: ApiBoundaryNodes, ) -> ReplicatedState { let time_out_messages_timer = self.metrics.start_phase_timer(PHASE_TIME_OUT_MESSAGES); @@ -133,7 +131,6 @@ impl StateMachine for StateMachineImpl { state.metadata.own_subnet_features = subnet_features; state.metadata.own_resource_limits = resource_limits; state.metadata.node_public_keys = node_public_keys; - state.metadata.api_boundary_nodes = api_boundary_nodes; if let Err(message) = state.metadata.init_allocation_ranges_if_empty() { self.metrics .observe_no_canister_allocation_range(&self.log, message); diff --git a/rs/messaging/src/state_machine/tests.rs b/rs/messaging/src/state_machine/tests.rs index 0461efdeef1f..6b93eefaae0c 100644 --- a/rs/messaging/src/state_machine/tests.rs +++ b/rs/messaging/src/state_machine/tests.rs @@ -193,7 +193,6 @@ fn state_machine_populates_network_topology() { Default::default(), &test_registry_settings(), Default::default(), - Default::default(), ); assert_eq!( @@ -227,7 +226,6 @@ fn test_delivered_batch(provided_batch: Batch) -> ReplicatedState { Default::default(), &test_registry_settings(), Default::default(), - Default::default(), ) }) } @@ -660,7 +658,6 @@ fn state_machine_handles_messages_to_deleted_subnet() { Default::default(), &test_registry_settings(), Default::default(), - Default::default(), ); // Stream to the deleted subnet (8 messages) is gone — all dropped silently. @@ -854,7 +851,6 @@ fn test_online_split(new_subnet_id: SubnetId, other_subnet_id: SubnetId) -> Repl Default::default(), &test_registry_settings(), Default::default(), - Default::default(), ) }); @@ -972,7 +968,6 @@ fn test_batch_time_impl( Default::default(), &test_registry_settings(), Default::default(), - Default::default(), ); assert_eq!( diff --git a/rs/protobuf/def/state/metadata/v1/metadata.proto b/rs/protobuf/def/state/metadata/v1/metadata.proto index 8ea570b59984..d1d0c0ee25e1 100644 --- a/rs/protobuf/def/state/metadata/v1/metadata.proto +++ b/rs/protobuf/def/state/metadata/v1/metadata.proto @@ -57,6 +57,7 @@ message NetworkTopology { repeated ChainKeySubnetEntry chain_key_enabled_subnets = 8; FullTopology full_topology = 9; types.v1.SubnetId default_initial_dkg_subnet_id = 10; + repeated ApiBoundaryNodeEntry api_boundary_nodes = 11; } message FullTopology { diff --git a/rs/protobuf/src/gen/state/state.metadata.v1.rs b/rs/protobuf/src/gen/state/state.metadata.v1.rs index 4bf44c20e8c6..6359aa3ae175 100644 --- a/rs/protobuf/src/gen/state/state.metadata.v1.rs +++ b/rs/protobuf/src/gen/state/state.metadata.v1.rs @@ -76,6 +76,8 @@ pub struct NetworkTopology { #[prost(message, optional, tag = "10")] pub default_initial_dkg_subnet_id: ::core::option::Option, + #[prost(message, repeated, tag = "11")] + pub api_boundary_nodes: ::prost::alloc::vec::Vec, } #[derive(Clone, PartialEq, ::prost::Message)] pub struct FullTopology { diff --git a/rs/replicated_state/src/metadata_state.rs b/rs/replicated_state/src/metadata_state.rs index bf55749b9314..8827fd143492 100644 --- a/rs/replicated_state/src/metadata_state.rs +++ b/rs/replicated_state/src/metadata_state.rs @@ -100,8 +100,6 @@ pub struct SystemMetadata { /// DER-encoded public keys of the subnet's nodes. pub node_public_keys: BTreeMap>, - pub api_boundary_nodes: BTreeMap, - /// "Subnet split in progress" marker: `Some(original_subnet_id)` if this /// replicated state is in the process of being split from `original_subnet_id`; /// `None` otherwise. @@ -255,6 +253,9 @@ pub struct NetworkTopology { /// by default, i.e., when no subnet ID is specified explicitly in the /// request. If `None`, such requests are routed to the calling subnet. pub default_initial_dkg_subnet_id: Option, + + /// API boundary nodes, indexed by `NodeId`. + pub api_boundary_nodes: BTreeMap, } /// Full description of the API Boundary Node, which is saved in the metadata. @@ -284,6 +285,7 @@ impl Default for NetworkTopology { bitcoin_mainnet_canister_id: None, full_topology: None, default_initial_dkg_subnet_id: None, + api_boundary_nodes: Default::default(), } } } @@ -300,6 +302,7 @@ impl NetworkTopology { bitcoin_mainnet_canister_id: Option, full_topology: Option, default_initial_dkg_subnet_id: Option, + api_boundary_nodes: BTreeMap, ) -> Self { Self { subnets, @@ -311,6 +314,7 @@ impl NetworkTopology { bitcoin_mainnet_canister_id, full_topology, default_initial_dkg_subnet_id, + api_boundary_nodes, } } @@ -555,7 +559,6 @@ impl SystemMetadata { own_subnet_features: SubnetFeatures::default(), own_resource_limits: Default::default(), node_public_keys: Default::default(), - api_boundary_nodes: Default::default(), split_from: None, subnet_split_from: None, @@ -868,7 +871,6 @@ impl SystemMetadata { own_resource_limits: _, // Overwritten as soon as the round begins, no explicit action needed. node_public_keys: _, - api_boundary_nodes: _, ref mut split_from, subnet_split_from, subnet_call_context_manager: _, @@ -976,7 +978,6 @@ impl SystemMetadata { own_subnet_features, own_resource_limits, node_public_keys, - api_boundary_nodes, split_from, mut subnet_split_from, mut subnet_call_context_manager, @@ -1083,7 +1084,6 @@ impl SystemMetadata { own_resource_limits, // Already populated from the registry. node_public_keys, - api_boundary_nodes, split_from, subnet_split_from, subnet_call_context_manager, @@ -2209,7 +2209,6 @@ pub mod testing { own_subnet_features: SubnetFeatures::default(), own_resource_limits: Default::default(), node_public_keys: Default::default(), - api_boundary_nodes: Default::default(), split_from: None, subnet_split_from: None, prev_state_hash: Default::default(), diff --git a/rs/replicated_state/src/metadata_state/proto.rs b/rs/replicated_state/src/metadata_state/proto.rs index 7ce1215f5e1b..e0c44ba2dc40 100644 --- a/rs/replicated_state/src/metadata_state/proto.rs +++ b/rs/replicated_state/src/metadata_state/proto.rs @@ -69,6 +69,19 @@ impl From<&NetworkTopology> for pb_metadata::NetworkTopology { default_initial_dkg_subnet_id: item .default_initial_dkg_subnet_id .map(subnet_id_into_protobuf), + api_boundary_nodes: item + .api_boundary_nodes + .iter() + .map( + |(node_id, api_boundary_node_entry)| pb_metadata::ApiBoundaryNodeEntry { + node_id: Some(node_id_into_protobuf(*node_id)), + domain: api_boundary_node_entry.domain.clone(), + ipv4_address: api_boundary_node_entry.ipv4_address.clone(), + ipv6_address: api_boundary_node_entry.ipv6_address.clone(), + pubkey: api_boundary_node_entry.pubkey.clone(), + }, + ) + .collect(), } } } @@ -114,6 +127,19 @@ impl TryFrom for NetworkTopology { .map(subnet_id_try_from_protobuf) .transpose()?; + let mut api_boundary_nodes = BTreeMap::::new(); + for entry in item.api_boundary_nodes { + api_boundary_nodes.insert( + node_id_try_from_option(entry.node_id)?, + ApiBoundaryNodeEntry { + domain: entry.domain, + ipv4_address: entry.ipv4_address, + ipv6_address: entry.ipv6_address, + pubkey: entry.pubkey, + }, + ); + } + Ok(Self { subnets, routing_table: try_from_option_field( @@ -155,6 +181,7 @@ impl TryFrom for NetworkTopology { } }, default_initial_dkg_subnet_id, + api_boundary_nodes, }) } } @@ -391,6 +418,7 @@ impl From<&SystemMetadata> for pb_metadata::SystemMetadata { }) .collect(), api_boundary_nodes: item + .network_topology .api_boundary_nodes .iter() .map( @@ -501,17 +529,23 @@ impl TryFrom<(pb_metadata::SystemMetadata, &dyn CheckpointLoadingMetrics)> for S node_public_keys.insert(node_id_try_from_option(entry.node_id)?, entry.public_key); } - let mut api_boundary_nodes = BTreeMap::::new(); - for entry in item.api_boundary_nodes { - api_boundary_nodes.insert( - node_id_try_from_option(entry.node_id)?, - ApiBoundaryNodeEntry { - domain: entry.domain, - ipv4_address: entry.ipv4_address, - ipv6_address: entry.ipv6_address, - pubkey: entry.pubkey, - }, - ); + let mut network_topology: NetworkTopology = + try_from_option_field(item.network_topology, "SystemMetadata::network_topology")?; + // Backward compatibility: checkpoints written before `api_boundary_nodes` + // was moved into `NetworkTopology` stored it directly in `SystemMetadata`. + // Fall back to the old location when the new one is empty. + if network_topology.api_boundary_nodes.is_empty() { + for entry in item.api_boundary_nodes { + network_topology.api_boundary_nodes.insert( + node_id_try_from_option(entry.node_id)?, + ApiBoundaryNodeEntry { + domain: entry.domain, + ipv4_address: entry.ipv4_address, + ipv6_address: entry.ipv6_address, + pubkey: entry.pubkey, + }, + ); + } } Ok(Self { @@ -526,7 +560,6 @@ impl TryFrom<(pb_metadata::SystemMetadata, &dyn CheckpointLoadingMetrics)> for S own_subnet_features: item.own_subnet_features.unwrap_or_default().into(), own_resource_limits: item.own_resource_limits.unwrap_or_default().into(), node_public_keys, - api_boundary_nodes, // Note: `load_checkpoint()` will set this to the contents of `split_marker.pbuf`, // when present. split_from: None, @@ -543,10 +576,7 @@ impl TryFrom<(pb_metadata::SystemMetadata, &dyn CheckpointLoadingMetrics)> for S ingress_history: Default::default(), streams: Arc::new(streams), subnet_schedule, - network_topology: Arc::new(try_from_option_field( - item.network_topology, - "SystemMetadata::network_topology", - )?), + network_topology: Arc::new(network_topology), state_sync_version: item .state_sync_version .try_into() diff --git a/rs/replicated_state/src/metadata_state/tests.rs b/rs/replicated_state/src/metadata_state/tests.rs index 8258755e9e49..da58184f9d07 100644 --- a/rs/replicated_state/src/metadata_state/tests.rs +++ b/rs/replicated_state/src/metadata_state/tests.rs @@ -347,6 +347,14 @@ fn system_metadata_roundtrip_encoding() { routing_table, canister_migrations, nns_subnet_id: other_subnet_id, + api_boundary_nodes: btreemap! { + node_test_id(1) => ApiBoundaryNodeEntry { + domain: "api-example.com".to_string(), + ipv4_address: Some("127.0.0.1".to_string()), + ipv6_address: "2001:0db8:85a3:0000:0000:8a2e:0370:7334".to_string(), + pubkey: None, + }, + }, ..Default::default() }; system_metadata.network_topology = Arc::new(network_topology); @@ -359,14 +367,6 @@ fn system_metadata_roundtrip_encoding() { system_metadata.node_public_keys = btreemap! { node_test_id(1) => pk_der, }; - system_metadata.api_boundary_nodes = btreemap! { - node_test_id(1) => ApiBoundaryNodeEntry { - domain: "api-example.com".to_string(), - ipv4_address: Some("127.0.0.1".to_string()), - ipv6_address: "2001:0db8:85a3:0000:0000:8a2e:0370:7334".to_string(), - pubkey: None, - }, - }; system_metadata.bitcoin_get_successors_follow_up_responses = btreemap! { 10.into() => vec![vec![1], vec![2]] }; @@ -496,6 +496,7 @@ fn network_topology_roundtrip_encoding() { bitcoin_mainnet_canister_id, None, Some(app_subnet_id), + Default::default(), ); let proto = pb::NetworkTopology::from(&network_topology); @@ -519,6 +520,7 @@ fn network_topology_roundtrip_encoding() { routing_table: full_routing_table, }), Some(app_subnet_id), + Default::default(), ); let proto = pb::NetworkTopology::from(&network_topology_with_full); diff --git a/rs/state_manager/src/tree_hash.rs b/rs/state_manager/src/tree_hash.rs index 09b854749195..297ff680ffd5 100644 --- a/rs/state_manager/src/tree_hash.rs +++ b/rs/state_manager/src/tree_hash.rs @@ -288,7 +288,7 @@ mod tests { node_test_id(2) => vec![2; 44], }; - state.metadata.api_boundary_nodes = btreemap! { + std::sync::Arc::make_mut(&mut state.metadata.network_topology).api_boundary_nodes = btreemap! { node_test_id(11) => ApiBoundaryNodeEntry { domain: "api-bn11-example.com".to_string(), ipv4_address: Some("127.0.0.1".to_string()), diff --git a/rs/state_manager/tests/state_manager.rs b/rs/state_manager/tests/state_manager.rs index c65787d62b7c..3b397ebd49ff 100644 --- a/rs/state_manager/tests/state_manager.rs +++ b/rs/state_manager/tests/state_manager.rs @@ -5589,7 +5589,7 @@ fn certified_read_can_certify_api_boundary_nodes_since_v16() { state_manager_test(|_metrics, state_manager| { let (_, mut state) = state_manager.take_tip(); - state.metadata.api_boundary_nodes = btreemap! { + std::sync::Arc::make_mut(&mut state.metadata.network_topology).api_boundary_nodes = btreemap! { node_test_id(11) => ApiBoundaryNodeEntry { domain: "api-bn11-example.com".to_string(), ipv4_address: Some("127.0.0.1".to_string()), From 4b3050a720fa88646188f5caf6a6944cdf8b66eb Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Thu, 2 Jul 2026 12:58:16 +0000 Subject: [PATCH 2/3] refactor: Bundle `SystemMetadata` subnet fields into `OwnSubnetInfo` struct Bundle `SystemMetadata` fields `own_subnet_features`, `own_resource_limits` and `node_public_keys` into an `OwnSubnetInfo` struct, in preparation for easy reuse across `ReplicatedStates` using the same registry version. --- .../src/lazy_tree_conversion.rs | 2 +- rs/canonical_state/src/traversal.rs | 2 +- .../src/execution_environment.rs | 2 +- .../src/execution_environment/tests.rs | 16 +++-- .../src/scheduler/tests/metrics.rs | 8 ++- rs/messaging/src/message_routing.rs | 47 ++++--------- rs/messaging/src/message_routing/tests.rs | 60 ++++++----------- rs/messaging/src/state_machine.rs | 24 ++----- rs/messaging/src/state_machine/tests.rs | 20 ++---- .../def/state/metadata/v1/metadata.proto | 12 ++++ .../src/gen/state/state.metadata.v1.rs | 17 +++++ rs/replicated_state/src/lib.rs | 3 +- rs/replicated_state/src/metadata_state.rs | 43 ++++++------ .../src/metadata_state/proto.rs | 66 ++++++++++++++++--- .../src/metadata_state/tests.rs | 2 +- rs/replicated_state/src/replicated_state.rs | 7 +- rs/state_manager/src/manifest/split/tests.rs | 12 ++-- rs/state_manager/src/tree_hash.rs | 2 +- rs/state_manager/tests/state_manager.rs | 3 +- .../execution_environment/src/lib.rs | 5 +- rs/test_utilities/state/src/lib.rs | 3 +- 21 files changed, 195 insertions(+), 161 deletions(-) diff --git a/rs/canonical_state/src/lazy_tree_conversion.rs b/rs/canonical_state/src/lazy_tree_conversion.rs index 732a5ad4f80d..7fe13171c93e 100644 --- a/rs/canonical_state/src/lazy_tree_conversion.rs +++ b/rs/canonical_state/src/lazy_tree_conversion.rs @@ -480,7 +480,7 @@ pub fn replicated_state_as_lazy_tree(state: &ReplicatedState, height: Height) -> subnets_as_tree( state.metadata.network_topology.subnets_for_certification(), own_subnet_id, - &state.metadata.node_public_keys, + &state.metadata.own_subnet_info.node_public_keys, inverted_routing_table.clone(), &state.metadata.subnet_metrics, certification_version, diff --git a/rs/canonical_state/src/traversal.rs b/rs/canonical_state/src/traversal.rs index 122aadafe5dd..6f1a0a2f13b8 100644 --- a/rs/canonical_state/src/traversal.rs +++ b/rs/canonical_state/src/traversal.rs @@ -820,7 +820,7 @@ mod tests { .unwrap(), ); }); - state.metadata.node_public_keys = btreemap! { + std::sync::Arc::make_mut(&mut state.metadata.own_subnet_info).node_public_keys = btreemap! { node_test_id(2) => vec![9, 10, 11, 12], }; diff --git a/rs/execution_environment/src/execution_environment.rs b/rs/execution_environment/src/execution_environment.rs index dbff1a82785d..0c6b6b85e4fb 100644 --- a/rs/execution_environment/src/execution_environment.rs +++ b/rs/execution_environment/src/execution_environment.rs @@ -1268,7 +1268,7 @@ impl ExecutionEnvironment { }, }, - Ok(Ic00Method::HttpRequest) => match state.metadata.own_subnet_features.http_requests { + Ok(Ic00Method::HttpRequest) => match state.subnet_features().http_requests { true => match &msg { CanisterCall::Request(request) => { match CanisterHttpRequestArgs::decode(payload) { diff --git a/rs/execution_environment/src/execution_environment/tests.rs b/rs/execution_environment/src/execution_environment/tests.rs index 6f76dcd5e6e9..fbf981bb5088 100644 --- a/rs/execution_environment/src/execution_environment/tests.rs +++ b/rs/execution_environment/src/execution_environment/tests.rs @@ -2296,7 +2296,9 @@ fn management_canister_xnet_to_nns_called_from_non_nns() { .with_nns_subnet_id(own_subnet) .with_caller(other_subnet, other_canister) .build(); - test.state_mut().metadata.own_subnet_features.http_requests = true; + std::sync::Arc::make_mut(&mut test.state_mut().metadata.own_subnet_info) + .subnet_features + .http_requests = true; test.inject_call_to_ic00( Method::CreateCanister, @@ -2330,7 +2332,9 @@ fn http_request_bound_holds() { // set number of max in-flight calls to 10 .with_max_canister_http_requests_in_flight(10) .build(); - test.state_mut().metadata.own_subnet_features.http_requests = true; + std::sync::Arc::make_mut(&mut test.state_mut().metadata.own_subnet_info) + .subnet_features + .http_requests = true; // Create payload of the request. let url = "https://".to_string(); @@ -2395,7 +2399,9 @@ fn management_canister_xnet_called_from_non_nns() { .with_nns_subnet_id(nns_subnet) .with_caller(other_subnet, other_canister) .build(); - test.state_mut().metadata.own_subnet_features.http_requests = true; + std::sync::Arc::make_mut(&mut test.state_mut().metadata.own_subnet_info) + .subnet_features + .http_requests = true; test.inject_call_to_ic00( Method::CreateCanister, @@ -3345,7 +3351,9 @@ fn execute_canister_http_request_disabled() { .with_own_subnet_id(own_subnet) .with_caller(own_subnet, caller_canister) .build(); - test.state_mut().metadata.own_subnet_features.http_requests = false; + std::sync::Arc::make_mut(&mut test.state_mut().metadata.own_subnet_info) + .subnet_features + .http_requests = false; // Create payload of the request. let url = "https://".to_string(); diff --git a/rs/execution_environment/src/scheduler/tests/metrics.rs b/rs/execution_environment/src/scheduler/tests/metrics.rs index 9be5e9d8feb9..8c25e4423491 100644 --- a/rs/execution_environment/src/scheduler/tests/metrics.rs +++ b/rs/execution_environment/src/scheduler/tests/metrics.rs @@ -935,7 +935,9 @@ fn consumed_cycles_http_outcalls_are_added_to_consumed_cycles_total() { .build(); let caller_canister = test.create_canister(); - test.state_mut().metadata.own_subnet_features.http_requests = true; + std::sync::Arc::make_mut(&mut test.state_mut().metadata.own_subnet_info) + .subnet_features + .http_requests = true; test.state_metrics().observe( test.state().metadata.own_subnet_id, @@ -1047,7 +1049,9 @@ fn http_outcalls_free() { .build(); let caller_canister = test.create_canister(); - test.state_mut().metadata.own_subnet_features.http_requests = true; + std::sync::Arc::make_mut(&mut test.state_mut().metadata.own_subnet_info) + .subnet_features + .http_requests = true; let cycles_before = test.canister_state(caller_canister).system_state.balance(); diff --git a/rs/messaging/src/message_routing.rs b/rs/messaging/src/message_routing.rs index b257d0102832..f3ca2c383dc6 100644 --- a/rs/messaging/src/message_routing.rs +++ b/rs/messaging/src/message_routing.rs @@ -30,12 +30,12 @@ use ic_registry_client_helpers::subnet::{ SubnetListRegistry, SubnetRegistry, get_node_ids_from_subnet_record, }; use ic_registry_provisional_whitelist::ProvisionalWhitelist; -use ic_registry_resource_limits::ResourceLimits; use ic_registry_subnet_features::{ChainKeyConfig, SubnetFeatures}; use ic_registry_subnet_type::SubnetType; use ic_replicated_state::metadata_state::ApiBoundaryNodeEntry; use ic_replicated_state::{ - DroppedMessageMetrics, FullTopology, NetworkTopology, ReplicatedState, SubnetTopology, + DroppedMessageMetrics, FullTopology, NetworkTopology, OwnSubnetInfo, ReplicatedState, + SubnetTopology, }; use ic_types::batch::{Batch, BatchContent, BatchSummary}; use ic_types::crypto::{KeyPurpose, threshold_sig::ThresholdSigPublicKey}; @@ -724,13 +724,7 @@ impl BatchProcessorImpl { &self, registry_version: RegistryVersion, own_subnet_id: SubnetId, - ) -> ( - NetworkTopology, - SubnetFeatures, - ResourceLimits, - RegistryExecutionSettings, - NodePublicKeys, - ) { + ) -> (NetworkTopology, OwnSubnetInfo, RegistryExecutionSettings) { loop { match self.try_to_read_registry(registry_version, own_subnet_id) { Ok(result) => return result, @@ -769,16 +763,8 @@ impl BatchProcessorImpl { &self, registry_version: RegistryVersion, own_subnet_id: SubnetId, - ) -> Result< - ( - NetworkTopology, - SubnetFeatures, - ResourceLimits, - RegistryExecutionSettings, - NodePublicKeys, - ), - ReadRegistryError, - > { + ) -> Result<(NetworkTopology, OwnSubnetInfo, RegistryExecutionSettings), ReadRegistryError> + { let subnet_record = self .registry .get_subnet_record(own_subnet_id, registry_version) @@ -900,8 +886,11 @@ impl BatchProcessorImpl { Ok(( network_topology, - subnet_features, - resource_limits, + OwnSubnetInfo { + subnet_features, + resource_limits, + node_public_keys, + }, RegistryExecutionSettings { max_number_of_canisters, provisional_whitelist, @@ -910,7 +899,6 @@ impl BatchProcessorImpl { node_ids: nodes, registry_version, }, - node_public_keys, )) } @@ -1388,13 +1376,8 @@ impl BatchProcessor for BatchProcessorImpl BatchProcessor for BatchProcessorImpl ReplicatedState { state.metadata.network_topology = Arc::new(network_topology); - state.metadata.own_subnet_features = subnet_features; - state.metadata.own_resource_limits = resource_limits; - state.metadata.node_public_keys = node_public_keys; + state.metadata.own_subnet_info = Arc::new(own_subnet_info); state.put_canister_state( CanisterStateBuilder::new() .with_canister_id(canister_test_id(1)) @@ -687,16 +683,7 @@ fn try_to_read_registry( registry: Arc, log: ReplicaLogger, own_subnet_id: SubnetId, -) -> Result< - ( - NetworkTopology, - SubnetFeatures, - ResourceLimits, - RegistryExecutionSettings, - NodePublicKeys, - ), - ReadRegistryError, -> { +) -> Result<(NetworkTopology, OwnSubnetInfo, RegistryExecutionSettings), ReadRegistryError> { let (batch_processor, _, _, _) = make_batch_processor(registry.clone(), log); batch_processor.try_to_read_registry(registry.get_latest_version(), own_subnet_id) } @@ -885,15 +872,14 @@ fn try_read_registry_succeeds_with_fully_specified_registry_records() { // Reading from the registry must succeed for fully specified records. let (batch_processor, metrics, state_manager, registry_settings) = make_batch_processor(fixture.registry.clone(), log); - let ( - network_topology, - own_subnet_features, - own_resource_limits, - registry_execution_settings, - node_public_keys, - ) = batch_processor + let (network_topology, own_subnet_info, registry_execution_settings) = batch_processor .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id) .unwrap(); + let OwnSubnetInfo { + subnet_features: own_subnet_features, + resource_limits: own_resource_limits, + node_public_keys, + } = own_subnet_info; // Full specification includes the subnet size of `own_subnet_id`. Check the corresponding // critical error counter is untouched. @@ -1028,7 +1014,7 @@ fn try_read_registry_succeeds_with_fully_specified_registry_records() { ); assert_ne!( own_subnet_features, - latest_state.metadata.own_subnet_features + latest_state.metadata.own_subnet_info.subnet_features ); assert_ne!( *registry_settings.lock().unwrap(), @@ -1054,23 +1040,14 @@ fn try_read_registry_succeeds_with_fully_specified_registry_records() { &network_topology, latest_state.metadata.network_topology.as_ref() ); + assert_eq!(own_subnet_features, latest_state.subnet_features()); + assert_eq!(own_resource_limits, latest_state.resource_limits()); assert_eq!( - own_subnet_features, - latest_state.metadata.own_subnet_features - ); - assert_eq!( - own_resource_limits, - latest_state.metadata.own_resource_limits - ); - assert_eq!( - latest_state.metadata.own_resource_limits.maximum_state_size, + latest_state.resource_limits().maximum_state_size, Some(own_maximum_state_size) ); assert_eq!( - latest_state - .metadata - .own_resource_limits - .maximum_state_delta, + latest_state.resource_limits().maximum_state_delta, Some(own_maximum_state_delta) ); assert_eq!( @@ -1125,7 +1102,7 @@ fn try_read_registry_succeeds_with_minimal_registry_records() { // critical error for `subnet_size` has incremented. assert_eq!(metrics.critical_error_missing_subnet_size.get(), 1); // Check the subnet size was set to the maximum for a small app subnet. - let (_, _, _, registry_execution_settings, _) = result.unwrap(); + let (_, _, registry_execution_settings) = result.unwrap(); assert_eq!( registry_execution_settings.subnet_size, SMALL_APP_SUBNET_MAX_SIZE @@ -1450,7 +1427,8 @@ fn try_read_registry_can_skip_missing_or_invalid_node_public_keys() { 2 ); - let (_, _, _, _, node_public_keys) = res.unwrap(); + let (_, own_subnet_info, _) = res.unwrap(); + let node_public_keys = &own_subnet_info.node_public_keys; assert_eq!(node_public_keys.len(), 1); assert!(!node_public_keys.contains_key(&node_test_id(1))); assert!(!node_public_keys.contains_key(&node_test_id(2))); @@ -1589,7 +1567,7 @@ fn try_read_registry_can_skip_missing_or_invalid_fields_of_api_boundary_nodes() // There are six API BNs in the registry. However, five nodes have missing or invalid fields of NodeRecord. // Hence, only one nodes are retrieved. - let (network_topology, _, _, _, _) = res.unwrap(); + let (network_topology, _, _) = res.unwrap(); let api_boundary_nodes = &network_topology.api_boundary_nodes; assert_eq!(api_boundary_nodes.len(), 1); assert!(api_boundary_nodes.contains_key(&node_test_id(11))); diff --git a/rs/messaging/src/state_machine.rs b/rs/messaging/src/state_machine.rs index 4dff2ed94863..047c1b7086e6 100644 --- a/rs/messaging/src/state_machine.rs +++ b/rs/messaging/src/state_machine.rs @@ -1,6 +1,4 @@ -use crate::message_routing::{ - CRITICAL_ERROR_INDUCT_RESPONSE_FAILED, MessageRoutingMetrics, NodePublicKeys, -}; +use crate::message_routing::{CRITICAL_ERROR_INDUCT_RESPONSE_FAILED, MessageRoutingMetrics}; use crate::routing::demux::Demux; use crate::routing::stream_builder::{ StreamBuilder, generate_reject_responses_for_deleted_subnets, @@ -12,9 +10,7 @@ use ic_interfaces::execution_environment::{ use ic_interfaces::time_source::system_time_now; use ic_logger::{ReplicaLogger, error, fatal}; use ic_query_stats::deliver_query_stats; -use ic_registry_resource_limits::ResourceLimits; -use ic_registry_subnet_features::SubnetFeatures; -use ic_replicated_state::{NetworkTopology, ReplicatedState}; +use ic_replicated_state::{NetworkTopology, OwnSubnetInfo, ReplicatedState}; use ic_types::batch::{Batch, BatchContent}; use ic_types::{ExecutionRound, NumBytes, SubnetId}; use std::sync::Arc; @@ -33,12 +29,10 @@ pub(crate) trait StateMachine: Send { fn execute_round( &self, state: ReplicatedState, - network_topology: NetworkTopology, batch: Batch, - subnet_features: SubnetFeatures, - resource_limits: ResourceLimits, + network_topology: NetworkTopology, + own_subnet_info: OwnSubnetInfo, registry_settings: &RegistryExecutionSettings, - node_public_keys: NodePublicKeys, ) -> ReplicatedState; } pub(crate) struct StateMachineImpl { @@ -106,12 +100,10 @@ impl StateMachine for StateMachineImpl { fn execute_round( &self, mut state: ReplicatedState, - network_topology: NetworkTopology, batch: Batch, - subnet_features: SubnetFeatures, - resource_limits: ResourceLimits, + network_topology: NetworkTopology, + own_subnet_info: OwnSubnetInfo, registry_settings: &RegistryExecutionSettings, - node_public_keys: NodePublicKeys, ) -> ReplicatedState { let time_out_messages_timer = self.metrics.start_phase_timer(PHASE_TIME_OUT_MESSAGES); @@ -128,9 +120,7 @@ impl StateMachine for StateMachineImpl { } state.metadata.network_topology = Arc::new(network_topology); - state.metadata.own_subnet_features = subnet_features; - state.metadata.own_resource_limits = resource_limits; - state.metadata.node_public_keys = node_public_keys; + state.metadata.own_subnet_info = Arc::new(own_subnet_info); if let Err(message) = state.metadata.init_allocation_ranges_if_empty() { self.metrics .observe_no_canister_allocation_range(&self.log, message); diff --git a/rs/messaging/src/state_machine/tests.rs b/rs/messaging/src/state_machine/tests.rs index 6b93eefaae0c..e71f059e8081 100644 --- a/rs/messaging/src/state_machine/tests.rs +++ b/rs/messaging/src/state_machine/tests.rs @@ -187,12 +187,10 @@ fn state_machine_populates_network_topology() { let state = state_machine.execute_round( fixture.initial_state, - fixture.network_topology.clone(), provided_batch, - Default::default(), + fixture.network_topology.clone(), Default::default(), &test_registry_settings(), - Default::default(), ); assert_eq!( @@ -220,12 +218,10 @@ fn test_delivered_batch(provided_batch: Batch) -> ReplicatedState { state_machine.execute_round( fixture.initial_state, - fixture.network_topology.clone(), provided_batch, - Default::default(), + fixture.network_topology.clone(), Default::default(), &test_registry_settings(), - Default::default(), ) }) } @@ -652,12 +648,10 @@ fn state_machine_handles_messages_to_deleted_subnet() { let mut state = state_machine.execute_round( initial_state, - network_topology, provided_batch, - Default::default(), + network_topology, Default::default(), &test_registry_settings(), - Default::default(), ); // Stream to the deleted subnet (8 messages) is gone — all dropped silently. @@ -845,12 +839,10 @@ fn test_online_split(new_subnet_id: SubnetId, other_subnet_id: SubnetId) -> Repl state_machine.execute_round( fixture.initial_state, - fixture.network_topology.clone(), split_batch, - Default::default(), + fixture.network_topology.clone(), Default::default(), &test_registry_settings(), - Default::default(), ) }); @@ -962,12 +954,10 @@ fn test_batch_time_impl( let state = state_machine.execute_round( fixture.initial_state, - fixture.network_topology.clone(), provided_batch, - Default::default(), + fixture.network_topology.clone(), Default::default(), &test_registry_settings(), - Default::default(), ); assert_eq!( diff --git a/rs/protobuf/def/state/metadata/v1/metadata.proto b/rs/protobuf/def/state/metadata/v1/metadata.proto index d1d0c0ee25e1..7cd953a6b7e7 100644 --- a/rs/protobuf/def/state/metadata/v1/metadata.proto +++ b/rs/protobuf/def/state/metadata/v1/metadata.proto @@ -339,6 +339,12 @@ message NodePublicKeyEntry { bytes public_key = 2; } +message OwnSubnetInfo { + registry.subnet.v1.SubnetFeatures subnet_features = 1; + registry.subnet.v1.ResourceLimits resource_limits = 2; + repeated NodePublicKeyEntry node_public_keys = 3; +} + message ApiBoundaryNodeEntry { types.v1.NodeId node_id = 1; string domain = 2; @@ -378,6 +384,8 @@ message SystemMetadata { NetworkTopology network_topology = 6; types.v1.SubnetId own_subnet_id = 7; SubnetCallContextManager subnet_call_context_manager = 8; + OwnSubnetInfo own_subnet_info = 26; + types.v1.SubnetId subnet_split_from = 23; // Canister ID ranges allocated (exclusively) to this subnet, to generate @@ -400,6 +408,10 @@ message SystemMetadata { uint64 heap_delta_estimate = 11; + // `own_subnet_features`, `node_public_keys` and `own_resource_limits` moved + // into `own_subnet_info` (field 26); these are kept as legacy mirrors (still + // written and read back) so that checkpoints remain compatible in both + // directions across the transition. registry.subnet.v1.SubnetFeatures own_subnet_features = 13; SubnetMetrics subnet_metrics = 15; diff --git a/rs/protobuf/src/gen/state/state.metadata.v1.rs b/rs/protobuf/src/gen/state/state.metadata.v1.rs index 6359aa3ae175..24009c5a08ee 100644 --- a/rs/protobuf/src/gen/state/state.metadata.v1.rs +++ b/rs/protobuf/src/gen/state/state.metadata.v1.rs @@ -502,6 +502,17 @@ pub struct NodePublicKeyEntry { pub public_key: ::prost::alloc::vec::Vec, } #[derive(Clone, PartialEq, ::prost::Message)] +pub struct OwnSubnetInfo { + #[prost(message, optional, tag = "1")] + pub subnet_features: + ::core::option::Option, + #[prost(message, optional, tag = "2")] + pub resource_limits: + ::core::option::Option, + #[prost(message, repeated, tag = "3")] + pub node_public_keys: ::prost::alloc::vec::Vec, +} +#[derive(Clone, PartialEq, ::prost::Message)] pub struct ApiBoundaryNodeEntry { #[prost(message, optional, tag = "1")] pub node_id: ::core::option::Option, @@ -558,6 +569,8 @@ pub struct SystemMetadata { pub own_subnet_id: ::core::option::Option, #[prost(message, optional, tag = "8")] pub subnet_call_context_manager: ::core::option::Option, + #[prost(message, optional, tag = "26")] + pub own_subnet_info: ::core::option::Option, #[prost(message, optional, tag = "23")] pub subnet_split_from: ::core::option::Option, /// Canister ID ranges allocated (exclusively) to this subnet, to generate @@ -583,6 +596,10 @@ pub struct SystemMetadata { pub certification_version: u32, #[prost(uint64, tag = "11")] pub heap_delta_estimate: u64, + /// `own_subnet_features`, `node_public_keys` and `own_resource_limits` moved + /// into `own_subnet_info` (field 26); these are kept as legacy mirrors (still + /// written and read back) so that checkpoints remain compatible in both + /// directions across the transition. #[prost(message, optional, tag = "13")] pub own_subnet_features: ::core::option::Option, diff --git a/rs/replicated_state/src/lib.rs b/rs/replicated_state/src/lib.rs index 97702f82728b..e10da83ef135 100644 --- a/rs/replicated_state/src/lib.rs +++ b/rs/replicated_state/src/lib.rs @@ -75,7 +75,8 @@ pub use canister_state::{ pub use canister_states::CanisterStates; pub use metadata_state::subnet_schedule::{CanisterPriority, SubnetSchedule}; pub use metadata_state::{ - FullTopology, IngressHistoryState, NetworkTopology, Stream, SubnetTopology, SystemMetadata, + FullTopology, IngressHistoryState, NetworkTopology, OwnSubnetInfo, Stream, SubnetTopology, + SystemMetadata, }; pub use page_map::{PageIndex, PageMap}; pub use replicated_state::{InputQueueType, InputSource, ReplicatedState, StateError}; diff --git a/rs/replicated_state/src/metadata_state.rs b/rs/replicated_state/src/metadata_state.rs index 8827fd143492..6461f1f81e63 100644 --- a/rs/replicated_state/src/metadata_state.rs +++ b/rs/replicated_state/src/metadata_state.rs @@ -93,12 +93,9 @@ pub struct SystemMetadata { pub own_subnet_type: SubnetType, - pub own_subnet_features: SubnetFeatures, - - pub own_resource_limits: ResourceLimits, - - /// DER-encoded public keys of the subnet's nodes. - pub node_public_keys: BTreeMap>, + /// Registry-derived information about this subnet (its features, resource + /// limits and node public keys). + pub own_subnet_info: Arc, /// "Subnet split in progress" marker: `Some(original_subnet_id)` if this /// replicated state is in the process of being split from `original_subnet_id`; @@ -437,6 +434,18 @@ pub fn can_have_subnet_admins( && cost_schedule == CanisterCyclesCostSchedule::Free) } +/// Registry-derived information about the subnet that this replicated state +/// belongs to. Repopulated from the registry at the start of every round. +#[derive(Clone, Eq, PartialEq, Debug, Default)] +pub struct OwnSubnetInfo { + pub subnet_features: SubnetFeatures, + + pub resource_limits: ResourceLimits, + + /// DER-encoded public keys of the subnet's nodes. + pub node_public_keys: BTreeMap>, +} + #[derive(Clone, Eq, PartialEq, Debug, Default)] pub struct SubnetMetrics { consumed_cycles_by_deleted_canisters: NominalCycles, @@ -556,9 +565,7 @@ impl SystemMetadata { batch_time: UNIX_EPOCH, network_topology: Default::default(), subnet_call_context_manager: Default::default(), - own_subnet_features: SubnetFeatures::default(), - own_resource_limits: Default::default(), - node_public_keys: Default::default(), + own_subnet_info: Default::default(), split_from: None, subnet_split_from: None, @@ -866,11 +873,7 @@ impl SystemMetadata { // subnet registry record, do not touch it. own_subnet_type: _, // Overwritten as soon as the round begins, no explicit action needed. - own_subnet_features: _, - // Overwritten as soon as the round begins, no explicit action needed. - own_resource_limits: _, - // Overwritten as soon as the round begins, no explicit action needed. - node_public_keys: _, + own_subnet_info: _, ref mut split_from, subnet_split_from, subnet_call_context_manager: _, @@ -975,9 +978,7 @@ impl SystemMetadata { network_topology, own_subnet_id, own_subnet_type, - own_subnet_features, - own_resource_limits, - node_public_keys, + own_subnet_info, split_from, mut subnet_split_from, mut subnet_call_context_manager, @@ -1080,10 +1081,8 @@ impl SystemMetadata { // New subnet ID. own_subnet_id: subnet_id, own_subnet_type, - own_subnet_features, - own_resource_limits, // Already populated from the registry. - node_public_keys, + own_subnet_info, split_from, subnet_split_from, subnet_call_context_manager, @@ -2206,9 +2205,7 @@ pub mod testing { network_topology: Default::default(), // Covered in `super::subnet_call_context_manager::testing`. subnet_call_context_manager: Default::default(), - own_subnet_features: SubnetFeatures::default(), - own_resource_limits: Default::default(), - node_public_keys: Default::default(), + own_subnet_info: Default::default(), split_from: None, subnet_split_from: None, prev_state_hash: Default::default(), diff --git a/rs/replicated_state/src/metadata_state/proto.rs b/rs/replicated_state/src/metadata_state/proto.rs index e0c44ba2dc40..e63da0ae3c2b 100644 --- a/rs/replicated_state/src/metadata_state/proto.rs +++ b/rs/replicated_state/src/metadata_state/proto.rs @@ -186,6 +186,38 @@ impl TryFrom for NetworkTopology { } } +impl From<&OwnSubnetInfo> for pb_metadata::OwnSubnetInfo { + fn from(item: &OwnSubnetInfo) -> Self { + Self { + subnet_features: Some(item.subnet_features.into()), + resource_limits: Some(item.resource_limits.into()), + node_public_keys: item + .node_public_keys + .iter() + .map(|(node_id, public_key)| pb_metadata::NodePublicKeyEntry { + node_id: Some(node_id_into_protobuf(*node_id)), + public_key: public_key.clone(), + }) + .collect(), + } + } +} + +impl TryFrom for OwnSubnetInfo { + type Error = ProxyDecodeError; + fn try_from(item: pb_metadata::OwnSubnetInfo) -> Result { + let mut node_public_keys = BTreeMap::>::new(); + for entry in item.node_public_keys { + node_public_keys.insert(node_id_try_from_option(entry.node_id)?, entry.public_key); + } + Ok(Self { + subnet_features: item.subnet_features.unwrap_or_default().into(), + resource_limits: item.resource_limits.unwrap_or_default().into(), + node_public_keys, + }) + } +} + impl From<&SubnetTopology> for pb_metadata::SubnetTopology { fn from(item: &SubnetTopology) -> Self { Self { @@ -395,8 +427,11 @@ impl From<&SystemMetadata> for pb_metadata::SystemMetadata { state_sync_version: item.state_sync_version as u32, certification_version: item.certification_version as u32, heap_delta_estimate: item.heap_delta_estimate.get(), - own_subnet_features: Some(item.own_subnet_features.into()), - own_resource_limits: Some(item.own_resource_limits.into()), + // `own_subnet_features`, `own_resource_limits` and `node_public_keys` are + // legacy mirrors of the fields now held in `own_subnet_info`. + own_subnet_features: Some(item.own_subnet_info.subnet_features.into()), + own_resource_limits: Some(item.own_subnet_info.resource_limits.into()), + own_subnet_info: Some((&*item.own_subnet_info).into()), subnet_metrics: Some((&item.subnet_metrics).into()), bitcoin_get_successors_follow_up_responses: item .bitcoin_get_successors_follow_up_responses @@ -410,6 +445,7 @@ impl From<&SystemMetadata> for pb_metadata::SystemMetadata { ) .collect(), node_public_keys: item + .own_subnet_info .node_public_keys .iter() .map(|(node_id, public_key)| pb_metadata::NodePublicKeyEntry { @@ -524,10 +560,24 @@ impl TryFrom<(pb_metadata::SystemMetadata, &dyn CheckpointLoadingMetrics)> for S let batch_time = Time::from_nanos_since_unix_epoch(item.batch_time_nanos); - let mut node_public_keys = BTreeMap::>::new(); - for entry in item.node_public_keys { - node_public_keys.insert(node_id_try_from_option(entry.node_id)?, entry.public_key); - } + // `own_subnet_info` was previously stored as the top-level `own_subnet_features`, + // `own_resource_limits` and `node_public_keys` fields. Fall back to those when the + // new field is absent (i.e. reading a checkpoint written before the move). + let own_subnet_info = match item.own_subnet_info { + Some(own_subnet_info) => OwnSubnetInfo::try_from(own_subnet_info)?, + None => { + let mut node_public_keys = BTreeMap::>::new(); + for entry in item.node_public_keys { + node_public_keys + .insert(node_id_try_from_option(entry.node_id)?, entry.public_key); + } + OwnSubnetInfo { + subnet_features: item.own_subnet_features.unwrap_or_default().into(), + resource_limits: item.own_resource_limits.unwrap_or_default().into(), + node_public_keys, + } + } + }; let mut network_topology: NetworkTopology = try_from_option_field(item.network_topology, "SystemMetadata::network_topology")?; @@ -557,9 +607,7 @@ impl TryFrom<(pb_metadata::SystemMetadata, &dyn CheckpointLoadingMetrics)> for S // actual value when we serialize SystemMetadata. We rely on `load_checkpoint()` to // properly set this value. own_subnet_type: SubnetType::default(), - own_subnet_features: item.own_subnet_features.unwrap_or_default().into(), - own_resource_limits: item.own_resource_limits.unwrap_or_default().into(), - node_public_keys, + own_subnet_info: Arc::new(own_subnet_info), // Note: `load_checkpoint()` will set this to the contents of `split_marker.pbuf`, // when present. split_from: None, diff --git a/rs/replicated_state/src/metadata_state/tests.rs b/rs/replicated_state/src/metadata_state/tests.rs index da58184f9d07..4dd9c53475d4 100644 --- a/rs/replicated_state/src/metadata_state/tests.rs +++ b/rs/replicated_state/src/metadata_state/tests.rs @@ -364,7 +364,7 @@ fn system_metadata_roundtrip_encoding() { .unwrap() .serialize_rfc8410_der(); - system_metadata.node_public_keys = btreemap! { + std::sync::Arc::make_mut(&mut system_metadata.own_subnet_info).node_public_keys = btreemap! { node_test_id(1) => pk_der, }; system_metadata.bitcoin_get_successors_follow_up_responses = diff --git a/rs/replicated_state/src/replicated_state.rs b/rs/replicated_state/src/replicated_state.rs index c8f6c9b72846..56976d4a5fc0 100644 --- a/rs/replicated_state/src/replicated_state.rs +++ b/rs/replicated_state/src/replicated_state.rs @@ -24,6 +24,7 @@ use ic_management_canister_types_private::CanisterStatusType; use ic_protobuf::state::queues::v1::canister_queues::NextInputQueue; use ic_registry_resource_limits::ResourceLimits; use ic_registry_routing_table::RoutingTable; +use ic_registry_subnet_features::SubnetFeatures; use ic_registry_subnet_type::SubnetType; use ic_types::{ CanisterId, NumBytes, SubnetId, Time, @@ -987,8 +988,12 @@ impl ReplicatedState { self.canister_states.callback_count() } + pub fn subnet_features(&self) -> SubnetFeatures { + self.metadata.own_subnet_info.subnet_features + } + pub fn resource_limits(&self) -> ResourceLimits { - self.metadata.own_resource_limits + self.metadata.own_subnet_info.resource_limits } /// Returns the `SubnetId` hosting the given `principal_id` (canister or diff --git a/rs/state_manager/src/manifest/split/tests.rs b/rs/state_manager/src/manifest/split/tests.rs index a54109bcdce7..c68230eaa6da 100644 --- a/rs/state_manager/src/manifest/split/tests.rs +++ b/rs/state_manager/src/manifest/split/tests.rs @@ -402,19 +402,19 @@ fn expected_subnet_1_system_metadata() -> (FileInfo, ChunkInfo) { ( FileInfo { relative_path: PathBuf::from(SYSTEM_METADATA_FILE), - size_bytes: 68, + size_bytes: 77, hash: [ - 78, 238, 78, 17, 184, 53, 165, 62, 217, 51, 114, 244, 68, 143, 232, 32, 90, 63, 45, - 134, 51, 144, 248, 106, 212, 211, 58, 23, 10, 95, 146, 65, + 64, 72, 236, 108, 113, 120, 247, 40, 204, 128, 5, 106, 154, 2, 146, 207, 182, 107, + 57, 125, 220, 99, 92, 236, 128, 191, 68, 150, 125, 140, 170, 64, ], }, ChunkInfo { file_index: 13, - size_bytes: 68, + size_bytes: 77, offset: 0, hash: [ - 13, 106, 85, 104, 0, 31, 105, 192, 113, 124, 153, 142, 236, 229, 176, 163, 16, 20, - 52, 79, 170, 36, 197, 189, 52, 66, 254, 27, 85, 81, 135, 44, + 155, 195, 127, 250, 170, 36, 220, 116, 17, 32, 104, 209, 232, 238, 87, 228, 65, + 194, 58, 87, 153, 54, 84, 15, 237, 170, 214, 91, 22, 52, 52, 42, ], }, ) diff --git a/rs/state_manager/src/tree_hash.rs b/rs/state_manager/src/tree_hash.rs index 297ff680ffd5..2c43f03c8b23 100644 --- a/rs/state_manager/src/tree_hash.rs +++ b/rs/state_manager/src/tree_hash.rs @@ -283,7 +283,7 @@ mod tests { |_| {}, ); - state.metadata.node_public_keys = btreemap! { + std::sync::Arc::make_mut(&mut state.metadata.own_subnet_info).node_public_keys = btreemap! { node_test_id(1) => vec![1; 44], node_test_id(2) => vec![2; 44], }; diff --git a/rs/state_manager/tests/state_manager.rs b/rs/state_manager/tests/state_manager.rs index 3b397ebd49ff..0df27efa45fe 100644 --- a/rs/state_manager/tests/state_manager.rs +++ b/rs/state_manager/tests/state_manager.rs @@ -5537,7 +5537,8 @@ fn certified_read_can_certify_node_public_keys_since_v12() { network_topology.set_subnets(subnets); state.metadata.network_topology = Arc::new(network_topology); - state.metadata.node_public_keys = node_public_keys; + std::sync::Arc::make_mut(&mut state.metadata.own_subnet_info).node_public_keys = + node_public_keys; state_manager.commit_and_certify_sync(state, CertificationScope::Metadata, None); diff --git a/rs/test_utilities/execution_environment/src/lib.rs b/rs/test_utilities/execution_environment/src/lib.rs index 36673112fb99..abc34aabbe5e 100644 --- a/rs/test_utilities/execution_environment/src/lib.rs +++ b/rs/test_utilities/execution_environment/src/lib.rs @@ -2982,10 +2982,11 @@ impl ExecutionTestBuilder { ); } + let own_subnet_info = std::sync::Arc::make_mut(&mut state.metadata.own_subnet_info); if self.subnet_features.is_empty() { - state.metadata.own_subnet_features = SubnetFeatures::default(); + own_subnet_info.subnet_features = SubnetFeatures::default(); } else { - state.metadata.own_subnet_features = + own_subnet_info.subnet_features = SubnetFeatures::from_str(&self.subnet_features).unwrap(); } diff --git a/rs/test_utilities/state/src/lib.rs b/rs/test_utilities/state/src/lib.rs index dd494624c698..15b80abb6c0a 100644 --- a/rs/test_utilities/state/src/lib.rs +++ b/rs/test_utilities/state/src/lib.rs @@ -165,7 +165,8 @@ impl ReplicatedStateBuilder { }); state.metadata.batch_time = self.batch_time; - state.metadata.own_subnet_features = self.subnet_features; + std::sync::Arc::make_mut(&mut state.metadata.own_subnet_info).subnet_features = + self.subnet_features; state.epoch_query_stats = self.query_stats; From 0ee310e18543077bd86567f8587411a2dd1c4099 Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Thu, 2 Jul 2026 14:15:55 +0000 Subject: [PATCH 3/3] feat: Cache and reuse registry data across rounds Cache previously read registry values (`NetworkTopology`, `OwnSubnetInfo` and `RegistryExecutionSettings`) along with the registry version they were read at. On the next registry read (during the next execution round), if the same registry version is being requested, return the cached values. Reading the registry is now about 50% of the (admittedly already low) DSM overhead (~20 ms of a total ~40 ms; which is still 10% of the average 400 ms round). --- rs/messaging/src/message_routing.rs | 114 ++++++++++++++++---- rs/messaging/src/message_routing/tests.rs | 121 +++++++++++++++++++--- rs/messaging/src/state_machine.rs | 12 +-- rs/messaging/src/state_machine/tests.rs | 17 ++- 4 files changed, 218 insertions(+), 46 deletions(-) diff --git a/rs/messaging/src/message_routing.rs b/rs/messaging/src/message_routing.rs index f3ca2c383dc6..3db6082cb9fa 100644 --- a/rs/messaging/src/message_routing.rs +++ b/rs/messaging/src/message_routing.rs @@ -55,6 +55,7 @@ use prometheus::{ Gauge, Histogram, HistogramTimer, HistogramVec, IntCounter, IntCounterVec, IntGauge, IntGaugeVec, }; +use std::cell::RefCell; use std::collections::{BTreeMap, BTreeSet, VecDeque}; use std::convert::{AsRef, TryFrom}; use std::net::{Ipv4Addr, Ipv6Addr}; @@ -582,14 +583,10 @@ trait BatchProcessor: Send { } /// Implementation of [`BatchProcessor`]. -struct BatchProcessorImpl -where - RegistryClient_: RegistryClient, -{ +struct BatchProcessorImpl { state_manager: Arc>, state_machine: Box, - registry: Arc, - bitcoin_config: BitcoinConfig, + registry_reader: RegistryReader, metrics: MessageRoutingMetrics, log: ReplicaLogger, #[allow(dead_code)] @@ -704,18 +701,68 @@ impl BatchProcessorImpl { metrics.clone(), )); + let registry_reader = RegistryReader::new( + registry, + hypervisor_config.bitcoin, + metrics.clone(), + log.clone(), + ); + Self { state_manager, state_machine, - registry, - bitcoin_config: hypervisor_config.bitcoin, + registry_reader, metrics, log, malicious_flags, } } +} + +/// The registry-derived values read at a given registry version, cached by +/// [`RegistryReader`] so that repeated reads at the same version are cheap. +struct CachedRegistryData { + registry_version: RegistryVersion, + network_topology: Arc, + own_subnet_info: Arc, + registry_execution_settings: Arc, +} + +/// Reads the registry contents required by `BatchProcessorImpl::process_batch()`. +/// +/// Caches the values read at the most recent registry version, so that a +/// subsequent read at the same version returns `Arc` clones of the cached +/// values instead of reading the registry again. +struct RegistryReader { + registry: Arc, + bitcoin_config: BitcoinConfig, + /// Cache holding the values read at the most recent registry version. + cache: RefCell>, + metrics: MessageRoutingMetrics, + log: ReplicaLogger, +} + +impl RegistryReader { + fn new( + registry: Arc, + bitcoin_config: BitcoinConfig, + metrics: MessageRoutingMetrics, + log: ReplicaLogger, + ) -> Self { + Self { + registry, + bitcoin_config, + cache: RefCell::new(None), + metrics, + log, + } + } /// Reads registry contents required by `BatchProcessorImpl::process_batch()`. + /// + /// Returns `Arc` clones of the cached values if they were previously read at + /// the same `registry_version`; otherwise reads the registry, caches the + /// results and returns them. // /// # Warning /// If the registry is unavailable, this method loops until it becomes @@ -724,10 +771,25 @@ impl BatchProcessorImpl { &self, registry_version: RegistryVersion, own_subnet_id: SubnetId, - ) -> (NetworkTopology, OwnSubnetInfo, RegistryExecutionSettings) { - loop { + ) -> ( + Arc, + Arc, + Arc, + ) { + // Return the cached values if they were read at the requested version. + if let Some(cached) = self.cache.borrow().as_ref() + && cached.registry_version == registry_version + { + return ( + Arc::clone(&cached.network_topology), + Arc::clone(&cached.own_subnet_info), + Arc::clone(&cached.registry_execution_settings), + ); + } + + let (network_topology, own_subnet_info, registry_execution_settings) = loop { match self.try_to_read_registry(registry_version, own_subnet_id) { - Ok(result) => return result, + Ok(result) => break result, Err(ReadRegistryError::Persistent(error_message)) => { // Increment the critical error counter in case of a persistent error. self.metrics.critical_error_failed_to_read_registry.inc(); @@ -749,11 +811,28 @@ impl BatchProcessorImpl { } } sleep(std::time::Duration::from_millis(100)); - } + }; + + let network_topology = Arc::new(network_topology); + let own_subnet_info = Arc::new(own_subnet_info); + let registry_execution_settings = Arc::new(registry_execution_settings); + + *self.cache.borrow_mut() = Some(CachedRegistryData { + registry_version, + network_topology: Arc::clone(&network_topology), + own_subnet_info: Arc::clone(&own_subnet_info), + registry_execution_settings: Arc::clone(®istry_execution_settings), + }); + + ( + network_topology, + own_subnet_info, + registry_execution_settings, + ) } - /// Loads the `NetworkTopology`, `SubnetFeatures`, execution settings and - /// own subnet node public keys from the registry. + /// Loads the `NetworkTopology`, `OwnSubnetInfo` and execution settings from the + /// registry. /// /// All of the above are required for deterministic processing, so if any /// entry is missing or cannot be decoded; or reading the registry fails; the @@ -1373,11 +1452,10 @@ impl BatchProcessor for BatchProcessorImpl, + own_subnet_info: Arc, registry_settings: &RegistryExecutionSettings, ) -> ReplicatedState { - state.metadata.network_topology = Arc::new(network_topology); - state.metadata.own_subnet_info = Arc::new(own_subnet_info); + state.metadata.network_topology = network_topology; + state.metadata.own_subnet_info = own_subnet_info; state.put_canister_state( CanisterStateBuilder::new() .with_canister_id(canister_test_id(1)) @@ -668,8 +668,12 @@ fn make_batch_processor( let batch_processor = BatchProcessorImpl { state_manager: state_manager.clone(), state_machine: Box::new(FakeStateMachine(registry_settings.clone())), - registry, - bitcoin_config: BitcoinConfig::default(), + registry_reader: RegistryReader::new( + registry, + BitcoinConfig::default(), + metrics.clone(), + log.clone(), + ), metrics: metrics.clone(), log, malicious_flags: MaliciousFlags::default(), @@ -685,7 +689,9 @@ fn try_to_read_registry( own_subnet_id: SubnetId, ) -> Result<(NetworkTopology, OwnSubnetInfo, RegistryExecutionSettings), ReadRegistryError> { let (batch_processor, _, _, _) = make_batch_processor(registry.clone(), log); - batch_processor.try_to_read_registry(registry.get_latest_version(), own_subnet_id) + batch_processor + .registry_reader + .try_to_read_registry(registry.get_latest_version(), own_subnet_id) } /// Tests that `BatchProcessorImpl::try_to_read_registry()` returns `Ok(_)`; and checks that the @@ -873,6 +879,7 @@ fn try_read_registry_succeeds_with_fully_specified_registry_records() { let (batch_processor, metrics, state_manager, registry_settings) = make_batch_processor(fixture.registry.clone(), log); let (network_topology, own_subnet_info, registry_execution_settings) = batch_processor + .registry_reader .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id) .unwrap(); let OwnSubnetInfo { @@ -1095,6 +1102,7 @@ fn try_read_registry_succeeds_with_minimal_registry_records() { let (batch_processor, metrics, _, _) = make_batch_processor(fixture.registry.clone(), log.clone()); let result = batch_processor + .registry_reader .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id); assert_matches!(result, Ok(_)); @@ -1156,6 +1164,81 @@ fn try_read_registry_succeeds_with_minimal_registry_records() { }); } +/// Tests that `RegistryReader::read_registry()` caches the values read at the most +/// recent registry version: a second read at the same version returns `Arc` clones +/// of the cached values, while a read at a different version reads the registry anew. +#[test] +fn read_registry_caches_values_by_registry_version() { + with_test_replica_logger(|log| { + use Integrity::*; + + let own_subnet_id = subnet_test_id(13); + let own_subnet_record = SubnetRecord { + max_number_of_canisters: 784, + ..Default::default() + }; + let own_transcript = dummy_transcript_for_tests(); + let nns_subnet_id = subnet_test_id(42); + + let input = TestRecords { + subnet_ids: Valid([own_subnet_id]), + subnet_records: [Valid(&own_subnet_record)], + ni_dkg_transcripts: [Valid(Some(&own_transcript))], + nns_subnet_id: Valid(nns_subnet_id), + chain_key_enabled_subnets: &BTreeMap::default(), + provisional_whitelist: Missing, + routing_table: Missing, + canister_migrations: Missing, + node_public_keys: &BTreeMap::default(), + api_boundary_node_records: &BTreeMap::default(), + node_records: &BTreeMap::default(), + }; + + let fixture = RegistryFixture::new(); + fixture.write_test_records(&input).unwrap(); + let version_1 = fixture.registry.get_latest_version(); + + let (batch_processor, _, _, _) = + make_batch_processor(fixture.registry.clone(), log.clone()); + + // Two reads at the same registry version return `Arc` clones of the same + // (cached) values. + let (nt_1, osi_1, res_1) = batch_processor + .registry_reader + .read_registry(version_1, own_subnet_id); + let (nt_2, osi_2, res_2) = batch_processor + .registry_reader + .read_registry(version_1, own_subnet_id); + assert!(Arc::ptr_eq(&nt_1, &nt_2)); + assert!(Arc::ptr_eq(&osi_1, &osi_2)); + assert!(Arc::ptr_eq(&res_1, &res_2)); + + // Writing the same records again bumps the registry to a new version. + fixture.write_test_records(&input).unwrap(); + let version_2 = fixture.registry.get_latest_version(); + assert_ne!(version_1, version_2); + + // A read at a different version bypasses the cache and returns freshly read + // values: distinct `Arc` instances, but equal in content. + let (nt_3, osi_3, res_3) = batch_processor + .registry_reader + .read_registry(version_2, own_subnet_id); + assert!(!Arc::ptr_eq(&nt_1, &nt_3)); + assert!(!Arc::ptr_eq(&osi_1, &osi_3)); + assert!(!Arc::ptr_eq(&res_1, &res_3)); + assert_eq!(nt_1, nt_3); + assert_eq!(osi_1, osi_3); + + // The freshly read values are now the cached ones. + let (nt_4, osi_4, res_4) = batch_processor + .registry_reader + .read_registry(version_2, own_subnet_id); + assert!(Arc::ptr_eq(&nt_3, &nt_4)); + assert!(Arc::ptr_eq(&osi_3, &osi_4)); + assert!(Arc::ptr_eq(&res_3, &res_4)); + }); +} + /// Tests that `BatchProcessorImpl::try_to_read_registry()` returns `Err(_)` if records in the /// registry hold corrupted data. Corrupted data is simulated by writing a string of prime numbers /// (almost anything would do) rather than an actual struct into registry. @@ -1416,6 +1499,7 @@ fn try_read_registry_can_skip_missing_or_invalid_node_public_keys() { let (batch_processor, metrics, _, _) = make_batch_processor(fixture.registry.clone(), log.clone()); let res = batch_processor + .registry_reader .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id); assert_matches!(res, Ok(_)); @@ -1562,6 +1646,7 @@ fn try_read_registry_can_skip_missing_or_invalid_fields_of_api_boundary_nodes() let (batch_processor, metrics, _, _) = make_batch_processor(fixture.registry.clone(), log.clone()); let res = batch_processor + .registry_reader .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id); assert_matches!(res, Ok(_)); @@ -1639,6 +1724,7 @@ fn try_read_registry_succeeds_and_populates_subnet_admins() { let (batch_processor, _, _, _) = make_batch_processor(fixture.registry.clone(), log.clone()); let network_topology = batch_processor + .registry_reader .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id) .unwrap() .0; @@ -1718,6 +1804,7 @@ fn try_read_registry_succeeds_and_resets_subnet_admins() { let (batch_processor, metrics, _, _) = make_batch_processor(fixture.registry.clone(), log.clone()); let network_topology = batch_processor + .registry_reader .try_to_read_registry(fixture.registry.get_latest_version(), own_subnet_id) .unwrap() .0; @@ -2091,13 +2178,17 @@ fn check_critical_error_counter_is_not_incremented_for_transient_error() { // Try reading the registry at the next version; should return `Err(_)`. assert_matches!( - batch_processor.try_to_read_registry(next_registry_version, own_subnet_id), + batch_processor + .registry_reader + .try_to_read_registry(next_registry_version, own_subnet_id), Err(ReadRegistryError::Transient(_)) ); // Write minimal records to the registry, reading the registry should now work. fixture.write_test_records(&minimal_input).unwrap(); assert_matches!( - batch_processor.try_to_read_registry(next_registry_version, own_subnet_id), + batch_processor + .registry_reader + .try_to_read_registry(next_registry_version, own_subnet_id), Ok(_) ); @@ -2108,7 +2199,9 @@ fn check_critical_error_counter_is_not_incremented_for_transient_error() { // Spawn a thread trying to read from the registry at the next version; this will fail // until we update the registry. let handle = std::thread::spawn(move || { - batch_processor.read_registry(next_registry_version, own_subnet_id) + batch_processor + .registry_reader + .read_registry(next_registry_version, own_subnet_id) }); // Wait 150ms, then update the registry and join the thread above. std::thread::sleep(Duration::from_millis(150)); @@ -2146,11 +2239,15 @@ fn reading_mainnet_registry_succeeds() { let (batch_processor, _, _, _) = make_batch_processor(registry, log); assert_matches!( - batch_processor.try_to_read_registry(registry_version, mainnet_nns_subnet()), + batch_processor + .registry_reader + .try_to_read_registry(registry_version, mainnet_nns_subnet()), Ok(_) ); assert_matches!( - batch_processor.try_to_read_registry(registry_version, mainnet_app_subnet()), + batch_processor + .registry_reader + .try_to_read_registry(registry_version, mainnet_app_subnet()), Ok(_) ); }); diff --git a/rs/messaging/src/state_machine.rs b/rs/messaging/src/state_machine.rs index 047c1b7086e6..d49c68d4dca3 100644 --- a/rs/messaging/src/state_machine.rs +++ b/rs/messaging/src/state_machine.rs @@ -30,8 +30,8 @@ pub(crate) trait StateMachine: Send { &self, state: ReplicatedState, batch: Batch, - network_topology: NetworkTopology, - own_subnet_info: OwnSubnetInfo, + network_topology: Arc, + own_subnet_info: Arc, registry_settings: &RegistryExecutionSettings, ) -> ReplicatedState; } @@ -101,8 +101,8 @@ impl StateMachine for StateMachineImpl { &self, mut state: ReplicatedState, batch: Batch, - network_topology: NetworkTopology, - own_subnet_info: OwnSubnetInfo, + network_topology: Arc, + own_subnet_info: Arc, registry_settings: &RegistryExecutionSettings, ) -> ReplicatedState { let time_out_messages_timer = self.metrics.start_phase_timer(PHASE_TIME_OUT_MESSAGES); @@ -119,8 +119,8 @@ impl StateMachine for StateMachineImpl { ) } - state.metadata.network_topology = Arc::new(network_topology); - state.metadata.own_subnet_info = Arc::new(own_subnet_info); + state.metadata.network_topology = network_topology; + state.metadata.own_subnet_info = own_subnet_info; if let Err(message) = state.metadata.init_allocation_ranges_if_empty() { self.metrics .observe_no_canister_allocation_range(&self.log, message); diff --git a/rs/messaging/src/state_machine/tests.rs b/rs/messaging/src/state_machine/tests.rs index e71f059e8081..43f5c294b8c6 100644 --- a/rs/messaging/src/state_machine/tests.rs +++ b/rs/messaging/src/state_machine/tests.rs @@ -63,7 +63,7 @@ struct StateMachineTestFixture { demux: Box, stream_builder: Box, initial_state: ReplicatedState, - network_topology: NetworkTopology, + network_topology: Arc, metrics: MessageRoutingMetrics, metrics_registry: MetricsRegistry, } @@ -151,7 +151,7 @@ fn test_fixture(provided_batch: &Batch) -> StateMachineTestFixture { demux, stream_builder, initial_state, - network_topology, + network_topology: Arc::new(network_topology), metrics, metrics_registry, } @@ -181,8 +181,8 @@ fn state_machine_populates_network_topology() { )); assert_ne!( - fixture.initial_state.metadata.network_topology.as_ref(), - &fixture.network_topology + fixture.initial_state.metadata.network_topology, + fixture.network_topology ); let state = state_machine.execute_round( @@ -193,10 +193,7 @@ fn state_machine_populates_network_topology() { &test_registry_settings(), ); - assert_eq!( - state.metadata.network_topology.as_ref(), - &fixture.network_topology - ); + assert_eq!(state.metadata.network_topology, fixture.network_topology); }); } @@ -649,7 +646,7 @@ fn state_machine_handles_messages_to_deleted_subnet() { let mut state = state_machine.execute_round( initial_state, provided_batch, - network_topology, + Arc::new(network_topology), Default::default(), &test_registry_settings(), ); @@ -800,7 +797,7 @@ fn split_fixture() -> StateMachineTestFixture { demux, stream_builder, initial_state, - network_topology, + network_topology: Arc::new(network_topology), metrics, metrics_registry, }