Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
27 changes: 27 additions & 0 deletions rs/registry/canister/src/common/test_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,28 @@ pub fn prepare_registry_with_nodes_and_node_operator_id(
start_mutation_id: u8,
nodes: u64,
node_operator_id: PrincipalId,
) -> (RegistryAtomicMutateRequest, BTreeMap<NodeId, PublicKey>) {
prepare_registry_raw(start_mutation_id, nodes, node_operator_id, false)
}

/// Same as above, just with the possibility to have a chip_id.
pub fn prepare_registry_with_nodes_and_chip_id(
start_mutation_id: u8,
nodes: u64,
) -> (RegistryAtomicMutateRequest, BTreeMap<NodeId, PublicKey>) {
prepare_registry_raw(
start_mutation_id,
nodes,
PrincipalId::new_user_test_id(999),
true,
)
}

pub fn prepare_registry_raw(
start_mutation_id: u8,
nodes: u64,
node_operator_id: PrincipalId,
with_chip_id: bool,
) -> (RegistryAtomicMutateRequest, BTreeMap<NodeId, PublicKey>) {
// Prepare a transaction to add the nodes to the registry
let mut mutations = Vec::<RegistryMutation>::default();
Expand All @@ -135,6 +157,11 @@ pub fn prepare_registry_with_nodes_and_node_operator_id(
// Preset this field to Some(), in order to allow seamless creation of ApiBoundaryNodeRecord if needed.
domain: Some(format!("node{effective_id}.example.com")),
node_reward_type: Some(NodeRewardType::Type1 as i32),
chip_id: if with_chip_id {
Some(format!("chip-id-{effective_id}").into_bytes())
} else {
None
},
..Default::default()
};
mutations.append(&mut make_add_node_registry_mutations(
Expand Down
44 changes: 33 additions & 11 deletions rs/registry/canister/src/invariants/subnet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ use std::{
};

use crate::invariants::common::{
InvariantCheckError, RegistrySnapshot, get_subnet_ids_from_snapshot,
InvariantCheckError, RegistrySnapshot, get_node_record_from_snapshot,
get_subnet_ids_from_snapshot,
};

use ic_base_types::{NodeId, PrincipalId};
Expand Down Expand Up @@ -36,6 +37,7 @@ pub(crate) fn check_subnet_invariants(
panic!("Subnet {subnet_id:} is in subnet list but no record exists")
});

// Each SSH key access list does not contain more than 50 keys
if subnet_record.ssh_readonly_access.len() > MAX_NUM_SSH_KEYS
|| subnet_record.ssh_backup_access.len() > MAX_NUM_SSH_KEYS
{
Expand All @@ -52,42 +54,43 @@ pub(crate) fn check_subnet_invariants(
});
}

let num_nodes = subnet_record.membership.len();
let mut subnet_members: HashSet<NodeId> = subnet_record
let subnet_members: HashSet<NodeId> = subnet_record
.membership
.iter()
.map(|v| NodeId::from(PrincipalId::try_from(v).unwrap()))
.collect();

// Subnet membership must contain registered nodes only
subnet_members.retain(|&k| {
let node_key = make_node_record_key(k);
let node_exists = snapshot.contains_key(node_key.as_bytes());
if !node_exists {
panic!("Node {k} does not exist in Subnet {subnet_id}");
for &node_id in &subnet_members {
let node_key = make_node_record_key(node_id);
if !snapshot.contains_key(node_key.as_bytes()) {
panic!("Node {node_id} does not exist in Subnet {subnet_id}");
}
node_exists
});
}

// Each node appears at most once in a subnet membership
let num_nodes = subnet_record.membership.len();
if num_nodes > subnet_members.len() {
panic!("Repeated nodes in subnet {subnet_id:}");
}

// Each subnet contains at least one node
if subnet_members.is_empty() {
panic!("No node in subnet {subnet_id:}");
}

// Each node appears at most once in at most one subnet membership
let intersection = accumulated_nodes_in_subnets
.intersection(&subnet_members)
.collect::<HashSet<_>>();
// Each node appears at most once in at most one subnet membership
if !intersection.is_empty() {
return Err(InvariantCheckError {
msg: format!("Nodes in subnet {subnet_id:} also belong to other subnets"),
source: None,
});
}
accumulated_nodes_in_subnets.extend(&subnet_members);

// Count occurrence of system subnets
if subnet_record.subnet_type == i32::from(SubnetType::System) {
system_subnet_count += 1;
Expand All @@ -105,6 +108,25 @@ pub(crate) fn check_subnet_invariants(
source: None,
});
}

// SEV-enabled subnets consist of SEV-enabled nodes only (i.e. nodes with a chip ID in the node record)
if let Some(features) = subnet_record.features.as_ref()
&& features.sev_enabled == Some(true)
{
for &node_id in &subnet_members {
// handle missing node record
let node_record = get_node_record_from_snapshot(node_id, snapshot)?
.ok_or_else(|| InvariantCheckError {
msg: format!("Subnet {subnet_id} has node {node_id} in its membership but the node record does not exist"),
source: None,
})?;
// handle missing chip_id
node_record.chip_id.as_ref().ok_or_else(|| InvariantCheckError {
msg: format!("Subnet {subnet_id} is SEV-enabled but at least one of its nodes is not: {node_id} does not have a chip ID in its node record"),
source: None,
})?;
}
}
}

// There is at least one system subnet. Note that we disable this invariant for benchmarks, as
Expand Down
121 changes: 114 additions & 7 deletions rs/registry/canister/src/invariants/subnet/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use crate::invariants::common::RegistrySnapshot;
use ic_base_types::SubnetId;
use ic_protobuf::registry::{
node::v1::NodeRecord,
subnet::v1::{CanisterCyclesCostSchedule, SubnetListRecord, SubnetRecord, SubnetType},
subnet::v1::{
CanisterCyclesCostSchedule, SubnetFeatures, SubnetListRecord, SubnetRecord, SubnetType,
},
};
use ic_registry_keys::{make_subnet_list_record_key, make_subnet_record_key};
use ic_test_utilities_types::ids::{node_test_id, subnet_test_id};
Expand All @@ -17,6 +19,8 @@ fn only_application_subnets_can_be_free_cycles_cost_schedule() {
setup_minimal_registry_snapshot_for_check_subnet_invariants(
system_subnet_id,
test_subnet_id,
1,
false,
);

// Trivial case. (Never forget the trivial case, because this is an edge
Expand Down Expand Up @@ -54,9 +58,100 @@ fn only_application_subnets_can_be_free_cycles_cost_schedule() {
);
}

#[test]
fn only_sev_enabled_subnets_consist_of_sev_enabled_nodes() {
let system_subnet_id = subnet_test_id(1);
let test_subnet_id = subnet_test_id(2);
let test_node_id = node_test_id(103);

// get a snapshot without chip IDs
let (mut snapshot, mut test_subnet_record) =
setup_minimal_registry_snapshot_for_check_subnet_invariants(
system_subnet_id,
test_subnet_id,
4,
false,
);

// a non SEV-enabled subnet only with nodes without chip ID is compliant
check_subnet_invariants(&snapshot).unwrap();

// a non SEV-enabled subnet with some nodes with and some without chip IDs is compliant
let test_node_record = NodeRecord {
chip_id: Some("a chip id".into()),
..Default::default()
};
snapshot.insert(
make_node_record_key(test_node_id).into_bytes(),
test_node_record.encode_to_vec(),
);
check_subnet_invariants(&snapshot).unwrap();

// an SEV-enabled subnet only with nodes without chip ID is NOT compliant
test_subnet_record.features = Some(SubnetFeatures {
sev_enabled: Some(true),
..Default::default()
});
snapshot.insert(
make_subnet_record_key(test_subnet_id).into_bytes(),
test_subnet_record.encode_to_vec(),
);
let test_node_record = NodeRecord {
chip_id: None,
..Default::default()
};
snapshot.insert(
make_node_record_key(test_node_id).into_bytes(),
test_node_record.encode_to_vec(),
);
assert_non_compliant_record(
&snapshot,
"subnet fbysm-3acaa-aaaaa-aaaap-yai is sev-enabled but at least one of its nodes is not",
);

// get a snapshot with chip IDs
let (mut snapshot, mut test_subnet_record) =
setup_minimal_registry_snapshot_for_check_subnet_invariants(
system_subnet_id,
test_subnet_id,
4,
true,
);

// a non SEV-enabled subnet only with nodes without chip ID is compliant
check_subnet_invariants(&snapshot).unwrap();

// an SEV-enabled subnet only with nodes with chip ID is compliant
test_subnet_record.features = Some(SubnetFeatures {
sev_enabled: Some(true),
..Default::default()
});
snapshot.insert(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name insert makes it look like you are adding a NEW ENTRY to snapshot, but iiuc, you are actually CHANGING an EXISTING entry. Ofc, the fact that a method named insert actually performs an UPSERT is not your fault. Still, it would be helpful to clarify that that we are CHANGING an EXISTING entry here (and elsewhere) with a comment or something. If you wanted to, you could assert that insert returns Some, which indicates that there was already a value, which got kicked out by the call.

make_subnet_record_key(test_subnet_id).into_bytes(),
test_subnet_record.encode_to_vec(),
);
check_subnet_invariants(&snapshot).unwrap();

// an SEV-enabled subnet with some nodes with and some without chip ID is NOT compliant
let test_node_record = NodeRecord {
chip_id: None,
..Default::default()
};
snapshot.insert(
make_node_record_key(test_node_id).into_bytes(),
test_node_record.encode_to_vec(),
);
assert_non_compliant_record(
&snapshot,
"subnet fbysm-3acaa-aaaaa-aaaap-yai is sev-enabled but at least one of its nodes is not",
);
}

fn setup_minimal_registry_snapshot_for_check_subnet_invariants(
system_subnet_id: SubnetId,
test_subnet_id: SubnetId,
num_nodes_in_test_subnet: usize,
with_chip_id: bool,
) -> (RegistrySnapshot, SubnetRecord) {
let mut snapshot = RegistrySnapshot::new();

Expand Down Expand Up @@ -84,11 +179,18 @@ fn setup_minimal_registry_snapshot_for_check_subnet_invariants(
);

// Add a test subnet in the subnet list.
let test_node_id = node_test_id(100);
snapshot.insert(
make_node_record_key(test_node_id.to_owned()).into_bytes(),
NodeRecord::default().encode_to_vec(),
);
for i in 0..num_nodes_in_test_subnet {
let node_id = node_test_id((i + 100) as u64);
let mut node_record = NodeRecord::default();
if with_chip_id {
node_record.chip_id = Some(format!("chip-id-{i}").into_bytes());
}
snapshot.insert(
make_node_record_key(node_id.to_owned()).into_bytes(),
node_record.encode_to_vec(),
);
}

let subnet_list_record = SubnetListRecord {
subnets: vec![
system_subnet_id.get().into_vec(),
Expand All @@ -99,8 +201,12 @@ fn setup_minimal_registry_snapshot_for_check_subnet_invariants(
make_subnet_list_record_key().into_bytes(),
subnet_list_record.encode_to_vec(),
);
let membership = (0..num_nodes_in_test_subnet)
.map(|i| node_test_id((i + 100) as u64).get().to_vec())
.collect();

let test_subnet_record = SubnetRecord {
membership: vec![test_node_id.get().to_vec()],
membership,
..Default::default()
};
snapshot.insert(
Expand All @@ -116,5 +222,6 @@ fn assert_non_compliant_record(snapshot: &RegistrySnapshot, error_msg: &str) {
panic!("Expected Err, but got Ok!");
};
let message = err.msg.to_lowercase();
println!("Error message: {message}");
assert!(message.contains(error_msg));
}
Loading
Loading