Skip to content

chore: Drop SystemMetadata::full_topology #10411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
alin-at-dfinity wants to merge 1 commit into
base: stschnei/no-cycles-or-unbounded-xnet-on-engines
Choose a base branch
from
Draft

chore: Drop SystemMetadata::full_topology #10411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions rs/canonical_state/src/lazy_tree_conversion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(),
Expand All @@ -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,
)
Expand Down
1 change: 0 additions & 1 deletion rs/http_endpoints/public/tests/common/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,6 @@ pub fn default_get_latest_state() -> Labeled<Arc<ReplicatedState>> {
None,
None,
None,
None,
);

metadata.network_topology = network_topology;
Expand Down
40 changes: 7 additions & 33 deletions rs/messaging/src/message_routing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -936,7 +936,7 @@ impl<RegistryClient_: RegistryClient> BatchProcessorImpl<RegistryClient_> {
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
Expand Down Expand Up @@ -1057,7 +1057,7 @@ impl<RegistryClient_: RegistryClient> BatchProcessorImpl<RegistryClient_> {
);
}

all_subnets.insert(
subnets.insert(
*subnet_id,
SubnetTopology {
public_key,
Expand All @@ -1071,23 +1071,12 @@ impl<RegistryClient_: RegistryClient> BatchProcessorImpl<RegistryClient_> {
);
}

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::<BTreeMap<_, _>>()
.try_into()
.map_err(|err| Persistent(format!("routing table err: {:?}", err)))?;
let canister_migrations = self
.registry
.get_canister_migrations(registry_version)
Expand All @@ -1101,10 +1090,8 @@ impl<RegistryClient_: RegistryClient> BatchProcessorImpl<RegistryClient_> {
.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)
Expand Down Expand Up @@ -1146,18 +1133,6 @@ impl<RegistryClient_: RegistryClient> BatchProcessorImpl<RegistryClient_> {
})
.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),
Expand All @@ -1166,7 +1141,6 @@ impl<RegistryClient_: RegistryClient> BatchProcessorImpl<RegistryClient_> {
chain_key_enabled_subnets,
self.bitcoin_config.testnet_canister_id,
self.bitcoin_config.mainnet_canister_id,
full_topology,
default_initial_dkg_subnet_id,
))
}
Expand Down
47 changes: 8 additions & 39 deletions rs/messaging/src/message_routing/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1816,8 +1816,7 @@ fn setup_three_subnet_registry() -> (Arc<FakeRegistryClient>, 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| {
Expand All @@ -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(),
);
});
}

Expand Down Expand Up @@ -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| {
Expand All @@ -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));
});
}

Expand Down
8 changes: 2 additions & 6 deletions rs/protobuf/def/state/metadata/v1/metadata.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions rs/protobuf/src/gen/state/state.metadata.v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,21 +71,11 @@ pub struct NetworkTopology {
::prost::alloc::vec::Vec<super::super::super::types::v1::CanisterId>,
#[prost(message, repeated, tag = "8")]
pub chain_key_enabled_subnets: ::prost::alloc::vec::Vec<ChainKeySubnetEntry>,
#[prost(message, optional, tag = "9")]
pub full_topology: ::core::option::Option<FullTopology>,
#[prost(message, optional, tag = "10")]
pub default_initial_dkg_subnet_id:
::core::option::Option<super::super::super::types::v1::SubnetId>,
}
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct FullTopology {
#[prost(message, repeated, tag = "1")]
pub subnets: ::prost::alloc::vec::Vec<SubnetsEntry>,
#[prost(message, optional, tag = "2")]
pub routing_table:
::core::option::Option<super::super::super::registry::routing_table::v1::RoutingTable>,
}
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct SetupInitialDkgContext {
#[prost(message, optional, tag = "1")]
pub request: ::core::option::Option<super::super::queues::v1::Request>,
Expand Down
2 changes: 1 addition & 1 deletion rs/replicated_state/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
54 changes: 0 additions & 54 deletions rs/replicated_state/src/metadata_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<SubnetId, SubnetTopology>,
#[serde(serialize_with = "ic_utils::serde_arc::serialize_arc")]
#[serde(deserialize_with = "ic_utils::serde_arc::deserialize_arc")]
pub routing_table: Arc<RoutingTable>,
}

/// 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)]
Expand All @@ -239,10 +220,6 @@ pub struct NetworkTopology {
/// The ID of the canister to forward bitcoin mainnet requests to.
pub bitcoin_mainnet_canister_id: Option<CanisterId>,

/// Unfiltered topology for the certified state tree.
/// Only set on the NNS subnet; `None` everywhere else.
full_topology: Option<FullTopology>,

/// 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.
Expand Down Expand Up @@ -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,
}
}
Expand All @@ -290,7 +266,6 @@ impl NetworkTopology {
chain_key_enabled_subnets: BTreeMap<MasterPublicKeyId, Vec<SubnetId>>,
bitcoin_testnet_canister_id: Option<CanisterId>,
bitcoin_mainnet_canister_id: Option<CanisterId>,
full_topology: Option<FullTopology>,
default_initial_dkg_subnet_id: Option<SubnetId>,
) -> Self {
Self {
Expand All @@ -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,
}
}
Expand Down Expand Up @@ -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<SubnetId, SubnetTopology> {
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<RoutingTable> {
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<SubnetId> {
let as_subnet_id = SubnetId::from(principal_id);
Expand Down Expand Up @@ -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<FullTopology>);
}

impl NetworkTopologyTesting for NetworkTopology {
Expand All @@ -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<FullTopology>) {
self.full_topology = full_topology;
}
}

pub trait StreamTesting {
Expand Down
Loading
Loading