From 54ad947439d3c0d56c824ccb4bd918c83cdb27cc Mon Sep 17 00:00:00 2001 From: Alin Sinpalean Date: Mon, 8 Jun 2026 12:02:19 +0000 Subject: [PATCH] chore: Drop SystemMetadata::full_topology Since now all subnets (including engines) always see each other, there is no need for a separate full_topology to be certified by the NNS. We can fall back on the regular topology and drop full_topology altogether. --- .../src/lazy_tree_conversion.rs | 9 +- rs/http_endpoints/public/tests/common/mod.rs | 1 - rs/messaging/src/message_routing.rs | 40 +----- rs/messaging/src/message_routing/tests.rs | 47 ++----- .../def/state/metadata/v1/metadata.proto | 8 +- .../src/gen/state/state.metadata.v1.rs | 10 -- rs/replicated_state/src/lib.rs | 2 +- rs/replicated_state/src/metadata_state.rs | 54 -------- .../src/metadata_state/proto.rs | 36 ----- .../src/metadata_state/tests.rs | 124 +----------------- 10 files changed, 27 insertions(+), 304 deletions(-) diff --git a/rs/canonical_state/src/lazy_tree_conversion.rs b/rs/canonical_state/src/lazy_tree_conversion.rs index 0cc601805f49..f534b9e25b6a 100644 --- a/rs/canonical_state/src/lazy_tree_conversion.rs +++ b/rs/canonical_state/src/lazy_tree_conversion.rs @@ -432,10 +432,7 @@ pub fn replicated_state_as_lazy_tree(state: &ReplicatedState, height: Height) -> ); let own_subnet_id = state.metadata.own_subnet_id; let inverted_routing_table = Arc::new(invert_routing_table( - state - .metadata - .network_topology - .routing_table_for_certification(), + state.metadata.network_topology.routing_table(), )); let split_routing_table = Arc::new(split_inverted_routing_table( &inverted_routing_table, @@ -465,7 +462,7 @@ pub fn replicated_state_as_lazy_tree(state: &ReplicatedState, height: Height) -> ) .with("subnet", move || { subnets_as_tree( - state.metadata.network_topology.subnets_for_certification(), + state.metadata.network_topology.subnets(), own_subnet_id, &state.metadata.node_public_keys, inverted_routing_table.clone(), @@ -482,7 +479,7 @@ pub fn replicated_state_as_lazy_tree(state: &ReplicatedState, height: Height) -> "canister_ranges", move || { canister_ranges_as_tree( - state.metadata.network_topology.subnets_for_certification(), + state.metadata.network_topology.subnets(), Arc::clone(&split_routing_table), certification_version, ) diff --git a/rs/http_endpoints/public/tests/common/mod.rs b/rs/http_endpoints/public/tests/common/mod.rs index 698c46010eeb..d832c0226233 100644 --- a/rs/http_endpoints/public/tests/common/mod.rs +++ b/rs/http_endpoints/public/tests/common/mod.rs @@ -256,7 +256,6 @@ pub fn default_get_latest_state() -> Labeled> { None, None, None, - None, ); metadata.network_topology = network_topology; diff --git a/rs/messaging/src/message_routing.rs b/rs/messaging/src/message_routing.rs index 42adbe00d77c..d2986f33c22e 100644 --- a/rs/messaging/src/message_routing.rs +++ b/rs/messaging/src/message_routing.rs @@ -35,7 +35,7 @@ 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, NetworkTopology, ReplicatedState, SubnetTopology, }; use ic_types::batch::{Batch, BatchContent, BatchSummary}; use ic_types::crypto::{KeyPurpose, threshold_sig::ThresholdSigPublicKey}; @@ -936,7 +936,7 @@ impl BatchProcessorImpl { let subnet_ids = subnet_ids_record.unwrap_or_default(); // Populate subnet topologies for all subnets. - let mut all_subnets = BTreeMap::new(); + let mut subnets = BTreeMap::new(); for subnet_id in &subnet_ids { let public_key = self @@ -1057,7 +1057,7 @@ impl BatchProcessorImpl { ); } - all_subnets.insert( + subnets.insert( *subnet_id, SubnetTopology { public_key, @@ -1071,23 +1071,12 @@ impl BatchProcessorImpl { ); } - let full_routing_table = self + // Every subnet sees the full topology, including CloudEngine subnets. + let routing_table = self .registry .get_routing_table(registry_version) .map_err(|err| registry_error("routing table", None, err))? .unwrap_or_default(); - - // All subnets see the full topology, including CloudEngine subnets. - let subnets: BTreeMap<_, _> = all_subnets - .iter() - .map(|(id, topo)| (*id, topo.clone())) - .collect(); - let routing_table = full_routing_table - .iter() - .map(|(range, id)| (*range, *id)) - .collect::>() - .try_into() - .map_err(|err| Persistent(format!("routing table err: {:?}", err)))?; let canister_migrations = self .registry .get_canister_migrations(registry_version) @@ -1101,10 +1090,8 @@ impl BatchProcessorImpl { .ok_or_else(|| not_found_error("NNS subnet ID", None))?; // Look up the default subnet for `SetupInitialDKG`. The key may be - // unset (in which case `SetupInitialDKG` requests fall back to the - // calling subnet), or the configured subnet may not be visible from - // this subnet (e.g. if it has been filtered out above), in which case - // we also fall back to the calling subnet. + // unset, in which case `SetupInitialDKG` requests fall back to the + // calling subnet. let default_initial_dkg_subnet_id = self .registry .get_default_initial_dkg_subnet_id(registry_version) @@ -1146,18 +1133,6 @@ impl BatchProcessorImpl { }) .collect(); - // Only the NNS subnet needs the full (unfiltered) topology so that its - // certified state tree contains entries for every subnet (including - // cloud engines). - let full_topology = if own_subnet_id == nns_subnet_id { - Some(FullTopology { - subnets: all_subnets, - routing_table: Arc::new(full_routing_table), - }) - } else { - None - }; - Ok(NetworkTopology::new( subnets, Arc::new(routing_table), @@ -1166,7 +1141,6 @@ impl BatchProcessorImpl { chain_key_enabled_subnets, self.bitcoin_config.testnet_canister_id, self.bitcoin_config.mainnet_canister_id, - full_topology, default_initial_dkg_subnet_id, )) } diff --git a/rs/messaging/src/message_routing/tests.rs b/rs/messaging/src/message_routing/tests.rs index 796993d9cff3..7ebf1858906e 100644 --- a/rs/messaging/src/message_routing/tests.rs +++ b/rs/messaging/src/message_routing/tests.rs @@ -1816,8 +1816,7 @@ fn setup_three_subnet_registry() -> (Arc, SubnetId, SubnetId } /// Tests that a CloudEngine subnet sees the full topology (all three subnets): -/// `subnets`, `routing_table`, `subnets_for_certification`, and -/// `routing_table_for_certification` all contain every subnet. +/// both `subnets` and `routing_table` contain every subnet. #[test] fn try_read_registry_engine_subnet_sees_full_topology() { with_test_replica_logger(|log| { @@ -1841,16 +1840,6 @@ fn try_read_registry_engine_subnet_sees_full_topology() { assert!(rt_subnets.contains(&app_subnet_id)); assert!(rt_subnets.contains(&engine_subnet_id)); assert!(rt_subnets.contains(&nns_subnet_id)); - - // No full_topology on engine subnets; certification accessors fall back to subnets(). - assert_eq!( - network_topology.subnets_for_certification(), - network_topology.subnets(), - ); - assert_eq!( - network_topology.routing_table_for_certification(), - network_topology.routing_table(), - ); }); } @@ -1880,21 +1869,11 @@ fn try_read_registry_application_subnet_sees_full_topology() { assert!(rt_subnets.contains(&app_subnet_id)); assert!(rt_subnets.contains(&nns_subnet_id)); assert!(rt_subnets.contains(&engine_subnet_id)); - - // No full_topology on non-NNS subnets; certification accessors fall back to subnets(). - assert_eq!( - network_topology.subnets_for_certification(), - network_topology.subnets(), - ); - assert_eq!( - network_topology.routing_table_for_certification(), - network_topology.routing_table(), - ); }); } /// Tests that the NNS subnet sees all subnets in both `subnets()` and -/// `subnets_for_certification()` (via `full_topology`), including engines. +/// `routing_table()`, including engines. #[test] fn try_read_registry_nns_subnet_has_full_topology_with_engines() { with_test_replica_logger(|log| { @@ -1911,25 +1890,15 @@ fn try_read_registry_nns_subnet_has_full_topology_with_engines() { assert!(subnet_keys.contains(&nns_subnet_id)); assert!(subnet_keys.contains(&engine_subnet_id)); - // subnets_for_certification also includes all three (via full_topology). - let cert_keys: Vec<_> = network_topology - .subnets_for_certification() - .keys() - .copied() - .collect(); - assert!(cert_keys.contains(&app_subnet_id)); - assert!(cert_keys.contains(&engine_subnet_id)); - assert!(cert_keys.contains(&nns_subnet_id)); - - // routing_table_for_certification includes ranges for all three subnets. - let rt_cert: BTreeSet<_> = network_topology - .routing_table_for_certification() + // routing_table() includes ranges for all three subnets. + let rt_subnets: BTreeSet<_> = network_topology + .routing_table() .iter() .map(|(_, sid)| *sid) .collect(); - assert!(rt_cert.contains(&app_subnet_id)); - assert!(rt_cert.contains(&engine_subnet_id)); - assert!(rt_cert.contains(&nns_subnet_id)); + assert!(rt_subnets.contains(&app_subnet_id)); + assert!(rt_subnets.contains(&engine_subnet_id)); + assert!(rt_subnets.contains(&nns_subnet_id)); }); } diff --git a/rs/protobuf/def/state/metadata/v1/metadata.proto b/rs/protobuf/def/state/metadata/v1/metadata.proto index 3f82bd48e05a..cd91b722b16f 100644 --- a/rs/protobuf/def/state/metadata/v1/metadata.proto +++ b/rs/protobuf/def/state/metadata/v1/metadata.proto @@ -55,15 +55,11 @@ message NetworkTopology { repeated types.v1.CanisterId bitcoin_testnet_canister_ids = 6; repeated types.v1.CanisterId bitcoin_mainnet_canister_ids = 7; repeated ChainKeySubnetEntry chain_key_enabled_subnets = 8; - FullTopology full_topology = 9; + reserved 9; + reserved "full_topology"; types.v1.SubnetId default_initial_dkg_subnet_id = 10; } -message FullTopology { - repeated SubnetsEntry subnets = 1; - registry.routing_table.v1.RoutingTable routing_table = 2; -} - message SetupInitialDkgContext { state.queues.v1.Request request = 1; repeated types.v1.NodeId nodes_in_subnet = 2; diff --git a/rs/protobuf/src/gen/state/state.metadata.v1.rs b/rs/protobuf/src/gen/state/state.metadata.v1.rs index 03331fafbd45..793b3b98f78f 100644 --- a/rs/protobuf/src/gen/state/state.metadata.v1.rs +++ b/rs/protobuf/src/gen/state/state.metadata.v1.rs @@ -71,21 +71,11 @@ pub struct NetworkTopology { ::prost::alloc::vec::Vec, #[prost(message, repeated, tag = "8")] pub chain_key_enabled_subnets: ::prost::alloc::vec::Vec, - #[prost(message, optional, tag = "9")] - pub full_topology: ::core::option::Option, #[prost(message, optional, tag = "10")] pub default_initial_dkg_subnet_id: ::core::option::Option, } #[derive(Clone, PartialEq, ::prost::Message)] -pub struct FullTopology { - #[prost(message, repeated, tag = "1")] - pub subnets: ::prost::alloc::vec::Vec, - #[prost(message, optional, tag = "2")] - pub routing_table: - ::core::option::Option, -} -#[derive(Clone, PartialEq, ::prost::Message)] pub struct SetupInitialDkgContext { #[prost(message, optional, tag = "1")] pub request: ::core::option::Option, diff --git a/rs/replicated_state/src/lib.rs b/rs/replicated_state/src/lib.rs index 1a484b163c30..1517a2965c33 100644 --- a/rs/replicated_state/src/lib.rs +++ b/rs/replicated_state/src/lib.rs @@ -75,7 +75,7 @@ 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, + IngressHistoryState, NetworkTopology, 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 dd25eacd1fb5..194f9bd3befa 100644 --- a/rs/replicated_state/src/metadata_state.rs +++ b/rs/replicated_state/src/metadata_state.rs @@ -195,27 +195,8 @@ pub struct SystemMetadata { pub logs_migrated: bool, } -/// Unfiltered topology, including all subnets and the full routing table. -/// -/// Only populated on the NNS subnet, where the certified state tree must -/// contain entries for every subnet in the network (including cloud engines). -/// On all other subnets this is `None` and the state tree falls back to the -/// (filtered) data in [`NetworkTopology`]. -#[derive(Clone, Eq, PartialEq, Debug, Deserialize, Serialize)] -pub struct FullTopology { - pub subnets: BTreeMap, - #[serde(serialize_with = "ic_utils::serde_arc::serialize_arc")] - #[serde(deserialize_with = "ic_utils::serde_arc::deserialize_arc")] - pub routing_table: Arc, -} - /// Full description of the IC network toplogy. /// -/// The `subnets` and `routing_table` fields contain the **filtered** view: -/// only the subnets this subnet may interact with. For the NNS subnet an -/// optional [`FullTopology`] stores the unfiltered data so that the certified -/// state tree can cover every subnet. -/// /// Contains [`Arc`] references, so it is only safe to serialize for read-only /// use. #[derive(Clone, Eq, PartialEq, Debug, Deserialize, Serialize)] @@ -239,10 +220,6 @@ pub struct NetworkTopology { /// The ID of the canister to forward bitcoin mainnet requests to. pub bitcoin_mainnet_canister_id: Option, - /// Unfiltered topology for the certified state tree. - /// Only set on the NNS subnet; `None` everywhere else. - full_topology: Option, - /// Subnet to which `SetupInitialDKG` management canister calls are routed /// by default, i.e., when no subnet id is specified explicitly in the /// request. If `None`, such requests are routed to the calling subnet. @@ -274,7 +251,6 @@ impl Default for NetworkTopology { chain_key_enabled_subnets: Default::default(), bitcoin_testnet_canister_id: None, bitcoin_mainnet_canister_id: None, - full_topology: None, default_initial_dkg_subnet_id: None, } } @@ -290,7 +266,6 @@ impl NetworkTopology { chain_key_enabled_subnets: BTreeMap>, bitcoin_testnet_canister_id: Option, bitcoin_mainnet_canister_id: Option, - full_topology: Option, default_initial_dkg_subnet_id: Option, ) -> Self { Self { @@ -301,7 +276,6 @@ impl NetworkTopology { chain_key_enabled_subnets, bitcoin_testnet_canister_id, bitcoin_mainnet_canister_id, - full_topology, default_initial_dkg_subnet_id, } } @@ -337,29 +311,6 @@ impl NetworkTopology { .map(|subnet_topology| subnet_topology.cost_schedule) } - /// Returns the subnets map used for the certified state tree. - /// - /// On the NNS subnet this returns the full, unfiltered map (including cloud - /// engines); on every other subnet it falls back to `subnets()`. - pub fn subnets_for_certification(&self) -> &BTreeMap { - self.full_topology - .as_ref() - .map(|ft| &ft.subnets) - .unwrap_or(&self.subnets) - } - - /// Returns the routing table used for the certified state tree. - /// - /// On the NNS subnet this returns the full, unfiltered table (including - /// cloud engine ranges); on every other subnet it falls back to - /// `routing_table()`. - pub fn routing_table_for_certification(&self) -> &Arc { - self.full_topology - .as_ref() - .map(|ft| &ft.routing_table) - .unwrap_or(&self.routing_table) - } - /// Find the subnet for `principal_id`. The input can either be a canister id, or a subnet id. pub fn route(&self, principal_id: PrincipalId) -> Option { let as_subnet_id = SubnetId::from(principal_id); @@ -2050,8 +2001,6 @@ pub mod testing { fn routing_table_mut(&mut self) -> &mut RoutingTable; /// Sets the routing table. fn set_routing_table(&mut self, routing_table: RoutingTable); - /// Sets the full (unfiltered) topology for the state tree. - fn set_full_topology(&mut self, full_topology: Option); } impl NetworkTopologyTesting for NetworkTopology { @@ -2067,9 +2016,6 @@ pub mod testing { fn set_routing_table(&mut self, routing_table: RoutingTable) { self.routing_table = Arc::new(routing_table); } - fn set_full_topology(&mut self, full_topology: Option) { - self.full_topology = full_topology; - } } pub trait StreamTesting { diff --git a/rs/replicated_state/src/metadata_state/proto.rs b/rs/replicated_state/src/metadata_state/proto.rs index d6a34c769340..6bff1811f0cd 100644 --- a/rs/replicated_state/src/metadata_state/proto.rs +++ b/rs/replicated_state/src/metadata_state/proto.rs @@ -52,20 +52,6 @@ impl From<&NetworkTopology> for pb_metadata::NetworkTopology { } }) .collect(), - full_topology: item - .full_topology - .as_ref() - .map(|ft| pb_metadata::FullTopology { - subnets: ft - .subnets - .iter() - .map(|(subnet_id, subnet_topology)| pb_metadata::SubnetsEntry { - subnet_id: Some(subnet_id_into_protobuf(*subnet_id)), - subnet_topology: Some(subnet_topology.into()), - }) - .collect(), - routing_table: Some(ft.routing_table.as_ref().into()), - }), default_initial_dkg_subnet_id: item .default_initial_dkg_subnet_id .map(subnet_id_into_protobuf), @@ -132,28 +118,6 @@ impl TryFrom for NetworkTopology { chain_key_enabled_subnets, bitcoin_testnet_canister_id, bitcoin_mainnet_canister_id, - full_topology: match item.full_topology { - None => None, - Some(ft) => { - let mut ft_subnets = BTreeMap::new(); - for entry in ft.subnets { - ft_subnets.insert( - subnet_id_try_from_option(entry.subnet_id, "FullTopology::subnets::K")?, - try_from_option_field( - entry.subnet_topology, - "FullTopology::subnets::V", - )?, - ); - } - let ft_routing_table: Arc = - try_from_option_field(ft.routing_table, "FullTopology::routing_table") - .map(Arc::new)?; - Some(FullTopology { - subnets: ft_subnets, - routing_table: ft_routing_table, - }) - } - }, default_initial_dkg_subnet_id, }) } diff --git a/rs/replicated_state/src/metadata_state/tests.rs b/rs/replicated_state/src/metadata_state/tests.rs index 6407dbca03e1..acf33b4720c9 100644 --- a/rs/replicated_state/src/metadata_state/tests.rs +++ b/rs/replicated_state/src/metadata_state/tests.rs @@ -451,14 +451,7 @@ fn network_topology_roundtrip_encoding() { ..Default::default() }; - let filtered_routing_table = Arc::new( - RoutingTable::try_from(btreemap! { - range(10, 19) => app_subnet_id, - }) - .unwrap(), - ); - - let full_routing_table = Arc::new( + let routing_table = Arc::new( RoutingTable::try_from(btreemap! { range(10, 19) => app_subnet_id, range(20, 29) => engine_subnet_id, @@ -484,45 +477,21 @@ fn network_topology_roundtrip_encoding() { let bitcoin_testnet_canister_id = Some(canister_test_id(100)); let bitcoin_mainnet_canister_id = Some(canister_test_id(101)); - // NetworkTopology without full_topology (non-NNS subnet). + // NetworkTopology round-trips through its protobuf representation. let network_topology = NetworkTopology::new( - btreemap! { app_subnet_id => app_subnet_topo.clone() }, - filtered_routing_table.clone(), - canister_migrations.clone(), - nns_subnet_id, - chain_key_enabled_subnets.clone(), - bitcoin_testnet_canister_id, - bitcoin_mainnet_canister_id, - None, - Some(app_subnet_id), - ); - - let proto = pb::NetworkTopology::from(&network_topology); - let round_trip = NetworkTopology::try_from(proto).unwrap(); - assert_eq!(network_topology, round_trip); - - // NetworkTopology with full_topology (NNS subnet). - let network_topology_with_full = NetworkTopology::new( - btreemap! { app_subnet_id => app_subnet_topo.clone() }, - filtered_routing_table, + btreemap! { app_subnet_id => app_subnet_topo, engine_subnet_id => engine_subnet_topo }, + routing_table, canister_migrations, nns_subnet_id, chain_key_enabled_subnets, bitcoin_testnet_canister_id, bitcoin_mainnet_canister_id, - Some(FullTopology { - subnets: btreemap! { - app_subnet_id => app_subnet_topo, - engine_subnet_id => engine_subnet_topo, - }, - routing_table: full_routing_table, - }), Some(app_subnet_id), ); - let proto = pb::NetworkTopology::from(&network_topology_with_full); + let proto = pb::NetworkTopology::from(&network_topology); let round_trip = NetworkTopology::try_from(proto).unwrap(); - assert_eq!(network_topology_with_full, round_trip); + assert_eq!(network_topology, round_trip); } #[test] @@ -1243,87 +1212,6 @@ fn network_topology_route_uses_filtered_topology() { assert_eq!(network_topology.route(subnet_b.get()), None); } -#[test] -fn subnets_for_certification_falls_back_to_filtered() { - let subnet_a = subnet_test_id(1); - - let routing_table = Arc::new( - RoutingTable::try_from(btreemap! { - CanisterIdRange { start: CanisterId::from(0_u64), end: CanisterId::from(99_u64) } => subnet_a, - }) - .unwrap(), - ); - - let network_topology = NetworkTopology { - subnets: btreemap! { - subnet_a => SubnetTopology::default(), - }, - routing_table: routing_table.clone(), - ..Default::default() - }; - - // Without full_topology, subnets_for_certification returns the filtered map. - assert_eq!( - network_topology.subnets_for_certification(), - network_topology.subnets() - ); - assert_eq!(network_topology.routing_table(), &routing_table); - assert_eq!( - network_topology.routing_table_for_certification(), - network_topology.routing_table() - ); -} - -#[test] -fn subnets_for_certification_returns_full_topology_when_set() { - use crate::metadata_state::testing::NetworkTopologyTesting; - - let subnet_a = subnet_test_id(1); - let subnet_b = subnet_test_id(2); // e.g., a cloud engine - - let full_subnets = btreemap! { - subnet_a => SubnetTopology::default(), - subnet_b => SubnetTopology::default(), - }; - let full_routing_table = Arc::new( - RoutingTable::try_from(btreemap! { - CanisterIdRange { start: CanisterId::from(0_u64), end: CanisterId::from(99_u64) } => subnet_a, - CanisterIdRange { start: CanisterId::from(100_u64), end: CanisterId::from(199_u64) } => subnet_b, - }) - .unwrap(), - ); - - let filtered_subnets = btreemap! { - subnet_a => SubnetTopology::default(), - }; - let filtered_routing_table = Arc::new( - RoutingTable::try_from(btreemap! { - CanisterIdRange { start: CanisterId::from(0_u64), end: CanisterId::from(99_u64) } => subnet_a, - }) - .unwrap(), - ); - - let mut network_topology = NetworkTopology { - subnets: filtered_subnets.clone(), - routing_table: filtered_routing_table.clone(), - ..Default::default() - }; - network_topology.set_full_topology(Some(FullTopology { - subnets: full_subnets.clone(), - routing_table: full_routing_table.clone(), - })); - - // subnets() and routing_table() return the filtered view. - assert_eq!(network_topology.subnets(), &filtered_subnets); - assert_eq!(network_topology.routing_table(), &filtered_routing_table); - // subnets_for_certification() and routing_table_for_certification() return the full view. - assert_eq!(network_topology.subnets_for_certification(), &full_subnets); - assert_eq!( - network_topology.routing_table_for_certification(), - &full_routing_table - ); -} - /// Test fixture that will produce an ingress status of type completed or failed, /// depending on whether `i % 2 == 0` (completed) or not (failed). Both statuses /// will have the same payload size.