Skip to content

refactor: introduce strum::EnumIter to protobuf's NodeRewardType #9702

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

Merged
pierugo-dfinity merged 3 commits into from
Apr 8, 2026
Merged

refactor: introduce strum::EnumIter to protobuf's NodeRewardType #9702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b50cc25
refactor(ic-interface-owners): introduce strum::EnumIter to NodeRewar…
pierugo-dfinity Apr 1, 2026
aae7040
refactor(governance-team): registry canister unit tests
pierugo-dfinity Apr 1, 2026
7eef2ac
refactor(consensus): firewall unit tests
pierugo-dfinity Apr 1, 2026
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

150 changes: 61 additions & 89 deletions rs/orchestrator/src/firewall.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ mod tests {
SubnetRecordBuilder, add_single_subnet_record, add_subnet_list_record,
};
use ic_test_utilities_types::ids::{SUBNET_1, node_test_id, subnet_test_id};
use rstest::rstest;
use strum::IntoEnumIterator;

use super::*;

Expand Down Expand Up @@ -983,28 +983,22 @@ mod tests {
);
}

#[rstest]
fn nftables_golden_assigned_replica_test(
#[test]
fn nftables_golden_assigned_replica_test() {
// For assigned replicas, only Type4 (cloud engine) nodes have a different firewall
#[values(
None,
Some(NodeRewardType::Unspecified),
Some(NodeRewardType::Type0),
Some(NodeRewardType::Type1),
Some(NodeRewardType::Type2),
Some(NodeRewardType::Type3),
Some(NodeRewardType::Type3dot1),
Some(NodeRewardType::Type1dot1)
)]
reward_type: Option<NodeRewardType>,
) {
golden_test(
Role::AssignedReplica(SUBNET_ID),
node_test_id(0),
reward_type,
NFTABLES_ASSIGNED_REPLICA_GOLDEN_BYTES,
"assigned_replica",
);
for reward_type in NodeRewardType::iter()
.filter(|reward_type| *reward_type != NodeRewardType::Type4)
.map(Some)
.chain(std::iter::once(None))
{
golden_test(
Role::AssignedReplica(SUBNET_ID),
node_test_id(0),
reward_type,
NFTABLES_ASSIGNED_REPLICA_GOLDEN_BYTES,
"assigned_replica",
);
}
}

#[test]
Expand All @@ -1018,28 +1012,22 @@ mod tests {
);
}

#[rstest]
fn nftables_unassigned_replica_golden_test(
#[test]
fn nftables_unassigned_replica_golden_test() {
// For unassigned replicas, only Type4 (cloud engine) nodes have a different firewall
#[values(
None,
Some(NodeRewardType::Unspecified),
Some(NodeRewardType::Type0),
Some(NodeRewardType::Type1),
Some(NodeRewardType::Type2),
Some(NodeRewardType::Type3),
Some(NodeRewardType::Type3dot1),
Some(NodeRewardType::Type1dot1)
)]
reward_type: Option<NodeRewardType>,
) {
golden_test(
Role::UnassignedReplica,
node_test_id(0),
reward_type,
NFTABLES_UNASSIGNED_REPLICA_GOLDEN_BYTES,
"unassigned_replica",
);
for reward_type in NodeRewardType::iter()
.filter(|reward_type| *reward_type != NodeRewardType::Type4)
.map(Some)
.chain(std::iter::once(None))
{
golden_test(
Role::UnassignedReplica,
node_test_id(0),
reward_type,
NFTABLES_UNASSIGNED_REPLICA_GOLDEN_BYTES,
"unassigned_replica",
);
}
}

#[test]
Expand All @@ -1053,64 +1041,48 @@ mod tests {
);
}

#[rstest]
fn nftables_golden_boundary_node_system_subnet_test(
// For boundary nodes, the node reward type has no effect on the firewall
#[values(
None,
Some(NodeRewardType::Unspecified),
Some(NodeRewardType::Type0),
Some(NodeRewardType::Type1),
Some(NodeRewardType::Type2),
Some(NodeRewardType::Type3),
Some(NodeRewardType::Type3dot1),
Some(NodeRewardType::Type1dot1),
Some(NodeRewardType::Type4)
)]
reward_type: Option<NodeRewardType>,
) {
#[test]
fn nftables_golden_boundary_node_system_subnet_test() {
// pick the node id such that the API BN's SOCKS proxy serves system subnet nodes
// the assert checks that
let api_bn_id_for_system_subnet = node_test_id(0);
assert!(api_bn_id_for_system_subnet < node_test_id(API_BOUNDARY_NODE_ID));

golden_test(
Role::BoundaryNode,
api_bn_id_for_system_subnet,
reward_type,
NFTABLES_BOUNDARY_NODE_SYSTEM_SUBNET_GOLDEN_BYTES,
"boundary_node_system_subnet",
);
// For boundary nodes, the node reward type has no effect on the firewall
for reward_type in NodeRewardType::iter()
.map(Some)
.chain(std::iter::once(None))
{
golden_test(
Role::BoundaryNode,
api_bn_id_for_system_subnet,
reward_type,
NFTABLES_BOUNDARY_NODE_SYSTEM_SUBNET_GOLDEN_BYTES,
"boundary_node_system_subnet",
);
}
}

#[rstest]
fn nftables_golden_boundary_node_app_subnet_test(
// For boundary nodes, the node reward type has no effect on the firewall
#[values(
None,
Some(NodeRewardType::Unspecified),
Some(NodeRewardType::Type0),
Some(NodeRewardType::Type1),
Some(NodeRewardType::Type2),
Some(NodeRewardType::Type3),
Some(NodeRewardType::Type3dot1),
Some(NodeRewardType::Type1dot1),
Some(NodeRewardType::Type4)
)]
reward_type: Option<NodeRewardType>,
) {
#[test]
fn nftables_golden_boundary_node_app_subnet_test() {
// pick the node id such that the API BN's SOCKS proxy serves app subnet nodes
// the assert checks that
let api_bn_id_for_app_subnet = node_test_id(1234);
assert!(api_bn_id_for_app_subnet > node_test_id(API_BOUNDARY_NODE_ID));

golden_test(
Role::BoundaryNode,
api_bn_id_for_app_subnet,
reward_type,
NFTABLES_BOUNDARY_NODE_APP_SUBNET_GOLDEN_BYTES,
"boundary_node_app_subnet",
);
// For boundary nodes, the node reward type has no effect on the firewall
for reward_type in NodeRewardType::iter()
.map(Some)
.chain(std::iter::once(None))
{
golden_test(
Role::BoundaryNode,
api_bn_id_for_app_subnet,
reward_type,
NFTABLES_BOUNDARY_NODE_APP_SUBNET_GOLDEN_BYTES,
"boundary_node_app_subnet",
);
}
}

/// Runs [`Firewall::check_for_firewall_config`] and compares the output against the specified
Expand Down
4 changes: 4 additions & 0 deletions rs/protobuf/generator/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ fn build_registry_proto(def: &Path, out: &Path) {
".registry.node",
"#[derive(serde::Serialize, serde::Deserialize)]",
);
config.type_attribute(
".registry.node.v1.NodeRewardType",
"#[derive(strum::EnumIter)]",
);
config.type_attribute(
".registry.firewall",
"#[derive(candid::CandidType, serde::Serialize, serde::Deserialize)]",
Expand Down
1 change: 1 addition & 0 deletions rs/protobuf/src/gen/registry/registry.node.v1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ pub struct NodeRecord {
#[derive(
serde::Serialize,
serde::Deserialize,
strum::EnumIter,
Clone,
Copy,
Debug,
Expand Down
1 change: 1 addition & 0 deletions rs/registry/canister/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ DEPENDENCIES = [
"@crate_index//:maplit",
"@crate_index//:prost",
"@crate_index//:serde",
"@crate_index//:strum",
"@crate_index//:url",
]

Expand Down
1 change: 1 addition & 0 deletions rs/registry/canister/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ maplit = "1.0.2"
on_wire = { path = "../../rust_canisters/on_wire" }
prost = { workspace = true }
serde = { workspace = true }
strum = { workspace = true }
url = { workspace = true }
canbench-rs = { workspace = true, optional = true }
rand = { workspace = true }
Expand Down
16 changes: 6 additions & 10 deletions rs/registry/canister/src/invariants/subnet/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use ic_protobuf::{
};
use ic_registry_keys::{make_node_record_key, make_subnet_list_record_key, make_subnet_record_key};
use ic_test_utilities_types::ids::{node_test_id, subnet_test_id, user_test_id};
use strum::IntoEnumIterator;

#[test]
fn only_application_subnets_and_engines_can_be_free_cycles_cost_schedule() {
Expand Down Expand Up @@ -290,16 +291,11 @@ fn cloud_engine_subnets_must_have_type4_nodes() {
check_subnet_invariants(&snapshot).unwrap();

// Sad case: CloudEngine subnet with nodes that have a non-Type4 reward type.
for reward_type in [
None,
Some(NodeRewardType::Unspecified),
Some(NodeRewardType::Type0),
Some(NodeRewardType::Type1),
Some(NodeRewardType::Type2),
Some(NodeRewardType::Type3),
Some(NodeRewardType::Type3dot1),
Some(NodeRewardType::Type1dot1),
] {
for reward_type in NodeRewardType::iter()
.filter(|reward_type| *reward_type != NodeRewardType::Type4)
.map(Some)
.chain(std::iter::once(None))
{
println!(
"Ensuring that a node with reward type {reward_type:?} cannot be part of a CloudEngine subnet"
);
Expand Down
Loading