diff --git a/rs/nns/integration_tests/src/node_operator_handler.rs b/rs/nns/integration_tests/src/node_operator_handler.rs index dec6b7b0541..3e5e36a5092 100644 --- a/rs/nns/integration_tests/src/node_operator_handler.rs +++ b/rs/nns/integration_tests/src/node_operator_handler.rs @@ -20,14 +20,17 @@ use ic_nns_test_utils::{ itest_helpers::{state_machine_test_on_nns_subnet, NnsCanisters}, registry::{get_value_or_panic, prepare_add_node_payload}, }; -use ic_protobuf::registry::node_operator::v1::{NodeOperatorRecord, RemoveNodeOperatorsPayload}; +use ic_protobuf::registry::node_operator::v1::NodeOperatorRecord; use ic_registry_keys::make_node_operator_record_key; use ic_registry_transport::{ deserialize_get_value_response, serialize_get_value_request, Error::KeyNotPresent, }; use ic_types::PrincipalId; use maplit::btreemap; -use registry_canister::mutations::do_add_node_operator::AddNodeOperatorPayload; +use registry_canister::mutations::{ + do_add_node_operator::AddNodeOperatorPayload, + do_remove_node_operators::RemoveNodeOperatorsPayload, +}; use std::time::Duration; /// Test that new Node Operator records can be added and removed to/from the @@ -123,11 +126,10 @@ fn test_node_operator_records_can_be_added_and_removed() { .await .unwrap(); - let node_operator_id_1: Vec = (*TEST_NEURON_1_OWNER_PRINCIPAL.into_vec()).to_vec(); - let node_operator_id_2: Vec = (*TEST_NEURON_2_OWNER_PRINCIPAL.into_vec()).to_vec(); - let proposal_payload = RemoveNodeOperatorsPayload { - node_operators_to_remove: vec![node_operator_id_1, node_operator_id_2], - }; + let proposal_payload = RemoveNodeOperatorsPayload::new(vec![ + *TEST_NEURON_1_OWNER_PRINCIPAL, + *TEST_NEURON_2_OWNER_PRINCIPAL, + ]); let node_operator_record_key_1 = make_node_operator_record_key(*TEST_NEURON_1_OWNER_PRINCIPAL).into_bytes(); diff --git a/rs/protobuf/def/registry/node_operator/v1/node_operator.proto b/rs/protobuf/def/registry/node_operator/v1/node_operator.proto index 91ae44a2ead..2b13634ba43 100644 --- a/rs/protobuf/def/registry/node_operator/v1/node_operator.proto +++ b/rs/protobuf/def/registry/node_operator/v1/node_operator.proto @@ -29,8 +29,3 @@ message NodeOperatorRecord { optional string ipv6 = 6; } - -// The payload of a request to remove Node Operator records from the Registry -message RemoveNodeOperatorsPayload { - repeated bytes node_operators_to_remove = 1; -} diff --git a/rs/protobuf/src/gen/registry/registry.node_operator.v1.rs b/rs/protobuf/src/gen/registry/registry.node_operator.v1.rs index 7016b9114ef..cd42ae996d2 100644 --- a/rs/protobuf/src/gen/registry/registry.node_operator.v1.rs +++ b/rs/protobuf/src/gen/registry/registry.node_operator.v1.rs @@ -39,18 +39,3 @@ pub struct NodeOperatorRecord { #[prost(string, optional, tag = "6")] pub ipv6: ::core::option::Option<::prost::alloc::string::String>, } -/// The payload of a request to remove Node Operator records from the Registry -#[derive( - candid::CandidType, - serde::Serialize, - candid::Deserialize, - Eq, - Hash, - Clone, - PartialEq, - ::prost::Message, -)] -pub struct RemoveNodeOperatorsPayload { - #[prost(bytes = "vec", repeated, tag = "1")] - pub node_operators_to_remove: ::prost::alloc::vec::Vec<::prost::alloc::vec::Vec>, -} diff --git a/rs/registry/admin/src/main.rs b/rs/registry/admin/src/main.rs index e080c8c0ab8..e43f738a557 100644 --- a/rs/registry/admin/src/main.rs +++ b/rs/registry/admin/src/main.rs @@ -77,7 +77,7 @@ use ic_protobuf::registry::{ dc::v1::{AddOrRemoveDataCentersProposalPayload, DataCenterRecord}, firewall::v1::{FirewallConfig, FirewallRule, FirewallRuleSet}, node::v1::{NodeRecord, NodeRewardType}, - node_operator::v1::{NodeOperatorRecord, RemoveNodeOperatorsPayload}, + node_operator::v1::NodeOperatorRecord, node_rewards::v2::{NodeRewardRate, UpdateNodeRewardsTableProposalPayload}, provisional_whitelist::v1::ProvisionalWhitelist as ProvisionalWhitelistProto, replica_version::v1::{BlessedReplicaVersions, ReplicaVersionRecord}, @@ -134,6 +134,7 @@ use registry_canister::mutations::{ do_deploy_guestos_to_all_subnet_nodes::DeployGuestosToAllSubnetNodesPayload, do_deploy_guestos_to_all_unassigned_nodes::DeployGuestosToAllUnassignedNodesPayload, do_remove_api_boundary_nodes::RemoveApiBoundaryNodesPayload, + do_remove_node_operators::RemoveNodeOperatorsPayload, do_revise_elected_replica_versions::ReviseElectedGuestosVersionsPayload, do_set_firewall_config::SetFirewallConfigPayload, do_update_api_boundary_nodes_version::DeployGuestosToSomeApiBoundaryNodes, @@ -701,14 +702,7 @@ impl ProposalTitle for ProposeToRemoveNodeOperatorsCmd { #[async_trait] impl ProposalPayload for ProposeToRemoveNodeOperatorsCmd { async fn payload(&self, _: &Agent) -> RemoveNodeOperatorsPayload { - RemoveNodeOperatorsPayload { - node_operators_to_remove: self - .node_operators_to_remove - .clone() - .iter() - .map(|x| x.to_vec()) - .collect(), - } + RemoveNodeOperatorsPayload::new(self.node_operators_to_remove.clone()) } } diff --git a/rs/registry/canister/canister/canister.rs b/rs/registry/canister/canister/canister.rs index 542e655187f..2796efce602 100644 --- a/rs/registry/canister/canister/canister.rs +++ b/rs/registry/canister/canister/canister.rs @@ -10,7 +10,7 @@ use ic_nervous_system_string::clamp_debug_len; use ic_nns_constants::{GOVERNANCE_CANISTER_ID, ROOT_CANISTER_ID}; use ic_protobuf::registry::{ dc::v1::{AddOrRemoveDataCentersProposalPayload, DataCenterRecord}, - node_operator::v1::{NodeOperatorRecord, RemoveNodeOperatorsPayload}, + node_operator::v1::NodeOperatorRecord, node_rewards::v2::UpdateNodeRewardsTableProposalPayload, }; use ic_registry_canister_api::{ @@ -43,6 +43,7 @@ use registry_canister::{ do_deploy_guestos_to_all_unassigned_nodes::DeployGuestosToAllUnassignedNodesPayload, do_recover_subnet::RecoverSubnetPayload, do_remove_api_boundary_nodes::RemoveApiBoundaryNodesPayload, + do_remove_node_operators::RemoveNodeOperatorsPayload, do_remove_nodes_from_subnet::RemoveNodesFromSubnetPayload, do_revise_elected_replica_versions::ReviseElectedGuestosVersionsPayload, do_set_firewall_config::SetFirewallConfigPayload, diff --git a/rs/registry/canister/canister/registry.did b/rs/registry/canister/canister/registry.did index 987abff684c..cd059d0ba1f 100644 --- a/rs/registry/canister/canister/registry.did +++ b/rs/registry/canister/canister/registry.did @@ -273,6 +273,11 @@ type RemoveNodeDirectlyPayload = record { node_id : principal }; type RemoveNodeOperatorsPayload = record { node_operators_to_remove : vec blob; + node_operator_principals_to_remove : opt NodeOperatorPrincipals; +}; + +type NodeOperatorPrincipals = record { + principals : vec principal; }; type RemoveNodesPayload = record { node_ids : vec principal }; diff --git a/rs/registry/canister/src/mutations/do_remove_node_operators.rs b/rs/registry/canister/src/mutations/do_remove_node_operators.rs index 28399444174..365af594de0 100644 --- a/rs/registry/canister/src/mutations/do_remove_node_operators.rs +++ b/rs/registry/canister/src/mutations/do_remove_node_operators.rs @@ -3,12 +3,11 @@ use crate::{common::LOG_PREFIX, registry::Registry}; #[cfg(target_arch = "wasm32")] use dfn_core::println; +use candid::CandidType; use ic_base_types::PrincipalId; -use ic_protobuf::registry::node_operator::v1::RemoveNodeOperatorsPayload; use ic_registry_keys::{make_node_operator_record_key, NODE_RECORD_KEY_PREFIX}; use ic_registry_transport::pb::v1::{registry_mutation, RegistryMutation}; - -use std::convert::TryFrom; +use serde::{Deserialize, Serialize}; use ic_protobuf::registry::node::v1::NodeRecord; use prost::Message; @@ -20,26 +19,21 @@ impl Registry { let mut mutations = vec![]; - // Node Operator IDs that are parsable as PrincipalIds and have an associated - // NodeOperatorRecord in the Registry - let mut valid_node_operator_ids = payload - .node_operators_to_remove + // Filter Node Operator IDs that have a NodeOperatorRecord in the Registry + let mut valid_node_operator_ids_to_remove: Vec = payload + .principal_ids_to_remove() .into_iter() - .filter_map(|bytes| { - PrincipalId::try_from(bytes) - .ok() - .filter(|node_operator_id| { - let node_operator_record_key = - make_node_operator_record_key(*node_operator_id).into_bytes(); - self.get(&node_operator_record_key, self.latest_version()) - .is_some() - }) + .filter(|node_operator_id| { + let node_operator_record_key = + make_node_operator_record_key(*node_operator_id).into_bytes(); + self.get(&node_operator_record_key, self.latest_version()) + .is_some() }) .collect(); - self.filter_node_operators_that_have_nodes(&mut valid_node_operator_ids); + self.filter_node_operators_that_have_nodes(&mut valid_node_operator_ids_to_remove); - for node_operator_id in valid_node_operator_ids { + for node_operator_id in valid_node_operator_ids_to_remove { let node_operator_record_key = make_node_operator_record_key(node_operator_id).into_bytes(); mutations.push(RegistryMutation { @@ -73,3 +67,50 @@ impl Registry { } } } + +/// The payload of a request to remove Node Operator records from the Registry +#[derive(Clone, Debug, Eq, PartialEq, CandidType, Deserialize, Serialize, Hash)] +pub struct RemoveNodeOperatorsPayload { + // Old compatibility field, required for Candid, to be removed in the future + pub node_operators_to_remove: Vec>, + + // New field, where the Node Operator IDs are passed as PrincipalIds instead of Vec + pub node_operator_principals_to_remove: Option, +} + +/// Wrapper message for the optional repeated field +#[derive(Clone, Debug, Eq, PartialEq, CandidType, Deserialize, Serialize, Hash)] +pub struct NodeOperatorPrincipals { + pub principals: Vec, +} + +impl RemoveNodeOperatorsPayload { + pub fn new(node_operators_to_remove: Vec) -> Self { + Self { + node_operators_to_remove: vec![], + node_operator_principals_to_remove: Some(NodeOperatorPrincipals { + principals: node_operators_to_remove, + }), + } + } + + pub fn principal_ids_to_remove(&self) -> Vec { + // Ensure only one of the fields is set to avoid confusing semantics. + // If the new field is present, panic if the old field is also set. + // This approach encourages clients to use the new field and allows for + // eventual deprecation of the old field. + match &self.node_operator_principals_to_remove { + Some(principals) if self.node_operators_to_remove.is_empty() => { + principals.principals.clone() + } + Some(_) => { + panic!("Cannot specify both node_operators_to_remove and node_operator_principals_to_remove"); + } + None => self + .node_operators_to_remove + .iter() + .filter_map(|bytes| PrincipalId::try_from(bytes.clone()).ok()) + .collect(), + } + } +} diff --git a/rs/registry/canister/tests/remove_node_operators.rs b/rs/registry/canister/tests/remove_node_operators.rs index 0ee9a9b8332..2436731b350 100644 --- a/rs/registry/canister/tests/remove_node_operators.rs +++ b/rs/registry/canister/tests/remove_node_operators.rs @@ -8,8 +8,10 @@ use ic_nns_test_utils::{ }, registry::invariant_compliant_mutation_as_atomic_req, }; -use ic_protobuf::registry::node_operator::v1::RemoveNodeOperatorsPayload; -use registry_canister::init::RegistryCanisterInitPayloadBuilder; +use registry_canister::{ + init::RegistryCanisterInitPayloadBuilder, + mutations::do_remove_node_operators::RemoveNodeOperatorsPayload, +}; #[test] fn test_the_anonymous_user_cannot_remove_node_operators() { @@ -22,9 +24,7 @@ fn test_the_anonymous_user_cannot_remove_node_operators() { ) .await; - let payload = RemoveNodeOperatorsPayload { - node_operators_to_remove: vec![], - }; + let payload = RemoveNodeOperatorsPayload::new(vec![]); // The anonymous end-user tries to remove node operators, bypassing // the Governance canister. This should be rejected. @@ -72,9 +72,7 @@ fn test_a_canister_other_than_the_governance_canister_cannot_remove_node_operato ) .await; - let payload = RemoveNodeOperatorsPayload { - node_operators_to_remove: vec![], - }; + let payload = RemoveNodeOperatorsPayload::new(vec![]); // The attacker canister tries to remove node operators, pretending // to be the Governance canister. This should have no effect.