From de07076ba59ead5410158ea06681dc4d20f7df5f Mon Sep 17 00:00:00 2001
From: Ruediger Birkner <ruediger.birkner@dfinity.org>
Date: Fri, 31 Jan 2025 12:18:52 +0000
Subject: [PATCH 1/3] allow replacing a node if it is active as an API BN
---
.../mutations/node_management/do_add_node.rs | 70 +++++-
.../do_remove_node_directly.rs | 204 ++++++++++++------
2 files changed, 203 insertions(+), 71 deletions(-)
diff --git a/rs/registry/canister/src/mutations/node_management/do_add_node.rs b/rs/registry/canister/src/mutations/node_management/do_add_node.rs
index b7bfeb0d5ce..65f4c4d8e31 100644
--- a/rs/registry/canister/src/mutations/node_management/do_add_node.rs
+++ b/rs/registry/canister/src/mutations/node_management/do_add_node.rs
@@ -55,9 +55,7 @@ impl Registry {
println!("{}do_add_node: The node id is {:?}", LOG_PREFIX, node_id);
// 2. Clear out any nodes that already exist at this IP.
- // This will only succeed if:
- // - the same NO was in control of the original nodes.
- // - the nodes are no longer in subnets.
+ // This will only succeed if the same NO was in control of the original nodes.
//
// (We use the http endpoint to be in line with what is used by the
// release dashboard.)
@@ -118,7 +116,7 @@ impl Registry {
.transpose()?
.map(|node_reward_type| node_reward_type as i32);
- // 5. Validate the domain is valid
+ // 5. Validate the domain
let domain: Option<String> = payload
.domain
.as_ref()
@@ -132,7 +130,7 @@ impl Registry {
})
.transpose()?;
- // 6. If there is an IPv4 config, make sure that the IPv4 is not used by anyone else
+ // 6. If there is an IPv4 config, make sure that the same IPv4 address is not used by any other node
let ipv4_intf_config = payload.public_ipv4_config.clone().map(|ipv4_config| {
ipv4_config.panic_on_invalid();
IPv4InterfaceConfig {
@@ -322,10 +320,15 @@ mod tests {
use ic_base_types::{NodeId, PrincipalId};
use ic_config::crypto::CryptoConfig;
use ic_crypto_node_key_generation::generate_node_keys_once;
- use ic_protobuf::registry::node_operator::v1::NodeOperatorRecord;
+ use ic_protobuf::registry::{
+ api_boundary_node::v1::ApiBoundaryNodeRecord, node_operator::v1::NodeOperatorRecord,
+ };
use ic_registry_canister_api::IPv4Config;
- use ic_registry_keys::{make_node_operator_record_key, make_node_record_key};
+ use ic_registry_keys::{
+ make_api_boundary_node_record_key, make_node_operator_record_key, make_node_record_key,
+ };
use ic_registry_transport::insert;
+ use ic_types::ReplicaVersion;
use itertools::Itertools;
use lazy_static::lazy_static;
use prost::Message;
@@ -859,6 +862,59 @@ mod tests {
}
}
+ #[test]
+ fn should_add_node_and_replace_existing_api_boundary_node() {
+ // This test verifies that adding a new node replaces an existing node in a subnet
+ let mut registry = invariant_compliant_registry(0);
+
+ // Add a node to the registry
+ let (mutate_request, node_ids_and_dkg_pks) = prepare_registry_with_nodes(1, 1);
+ registry.maybe_apply_mutation_internal(mutate_request.mutations);
+ let node_ids: Vec<NodeId> = node_ids_and_dkg_pks.keys().cloned().collect();
+
+ let old_node_id = node_ids[0];
+ let old_node = registry.get_node(old_node_id).unwrap();
+
+ let node_operator_id = registry_add_node_operator_for_node(&mut registry, old_node_id, 0);
+
+ // Turn that node into an API boundary node
+ let api_bn = ApiBoundaryNodeRecord {
+ version: ReplicaVersion::default().to_string(),
+ };
+ registry.maybe_apply_mutation_internal(vec![insert(
+ make_api_boundary_node_record_key(old_node_id),
+ api_bn.encode_to_vec(),
+ )]);
+
+ // Add a new node with the same IP address and port as an existing node, which should replace the existing node
+ let (mut payload, _valid_pks) = prepare_add_node_payload(2);
+ let http = old_node.http.unwrap();
+ payload
+ .http_endpoint
+ .clone_from(&format!("[{}]:{}", http.ip_addr, http.port));
+ let new_node_id = registry
+ .do_add_node_(payload.clone(), node_operator_id)
+ .expect("failed to add a node");
+
+ // Verify that there is an API boundary node record for the new node
+ assert!(registry
+ .get(
+ make_api_boundary_node_record_key(new_node_id).as_bytes(),
+ registry.latest_version()
+ )
+ .is_some());
+
+ // Verify the old node is removed from the registry
+ assert!(registry.get_node(old_node_id).is_none());
+
+ // Verify the new node is present in the registry
+ assert!(registry.get_node(new_node_id).is_some());
+
+ // Verify node operator allowance is unchanged
+ let updated_operator = get_node_operator_record(®istry, node_operator_id).unwrap();
+ assert_eq!(updated_operator.node_allowance, 0);
+ }
+
#[test]
#[should_panic(expected = "Node allowance for this Node Operator is exhausted")]
fn should_panic_if_node_allowance_is_exhausted() {
diff --git a/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs b/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs
index 2fc11389747..a8db24d16c0 100644
--- a/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs
+++ b/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs
@@ -10,7 +10,7 @@ use dfn_core::println;
use ic_base_types::{NodeId, PrincipalId};
use ic_registry_keys::{make_api_boundary_node_record_key, make_subnet_record_key};
use ic_registry_transport::pb::v1::RegistryMutation;
-use ic_registry_transport::upsert;
+use ic_registry_transport::{delete, insert, upsert};
use prost::Message;
impl Registry {
@@ -51,8 +51,8 @@ impl Registry {
}
// Prepare mutations for removing or replacing a node in the registry.
- // If new_node_id is Some, the old node is in-place replaced with the new node, even if the old node is in a subnet.
- // If new_node_id is None, the old node is only removed from the registry and is not allowed to be in a subnet.
+ // If new_node_id is Some, the old node is in-place replaced with the new node, even if the old node is in active use (i.e., assigned to a subnet or acts as an API boundary node).
+ // If new_node_id is None, the old node is only removed from the registry and is not allowed to be in active use.
pub fn make_remove_or_replace_node_mutations(
&mut self,
payload: RemoveNodeDirectlyPayload,
@@ -121,21 +121,30 @@ impl Registry {
);
}
- // 3. Ensure the node is not an API Boundary Node.
- // In order to succeed, a corresponding ApiBoundaryNodeRecord should be removed first via proposal.
- let api_bn_id = self.get_api_boundary_node_record(payload.node_id);
- if api_bn_id.is_some() {
- panic!(
- "{}do_remove_node_directly: Cannot remove a node, as it has ApiBoundaryNodeRecord with record_key: {}",
- LOG_PREFIX,
- make_api_boundary_node_record_key(payload.node_id)
- );
+ let mut mutations = vec![];
+
+ // 3. Check if the node is an API Boundary Node. If there is a replacement node, remove the existing node and try to assign the new one to act as API boundary node. This will only .
+ if let Some(api_bn_record) = self.get_api_boundary_node_record(payload.node_id) {
+ if let Some(node_id) = new_node_id {
+ // remove the existing API boundary node record
+ let old_key = make_api_boundary_node_record_key(payload.node_id);
+ mutations.push(delete(old_key));
+
+ // create the new API boundary node record by just cloning the old one and inserting it with the new key
+ let new_key = make_api_boundary_node_record_key(node_id);
+ mutations.push(insert(new_key, api_bn_record.clone().encode_to_vec()));
+ } else {
+ panic!(
+ "{}do_remove_node_directly: Cannot remove this node, as it is an active API boundary node: {}",
+ LOG_PREFIX,
+ make_api_boundary_node_record_key(payload.node_id)
+ );
+ }
}
// 4. Check if node is in a subnet, and if so, replace it in the subnet by updating the membership in the subnet record.
let subnet_list_record = get_subnet_list_record(self);
let is_node_in_subnet = find_subnet_for_node(self, payload.node_id, &subnet_list_record);
- let mut mutations = vec![];
if let Some(subnet_id) = is_node_in_subnet {
if new_node_id.is_some() {
// The node is in a subnet and is being replaced with a new node.
@@ -157,10 +166,10 @@ impl Registry {
&mut subnet_record,
subnet_membership,
);
- mutations = vec![upsert(
+ mutations.push(upsert(
make_subnet_record_key(subnet_id),
subnet_record.encode_to_vec(),
- )];
+ ));
} else {
panic!("{}do_remove_node_directly: Cannot remove a node that is a member of a subnet. This node is a member of Subnet: {}",
LOG_PREFIX,
@@ -217,7 +226,7 @@ mod tests {
api_boundary_node::v1::ApiBoundaryNodeRecord, node_operator::v1::NodeOperatorRecord,
};
use ic_registry_keys::{make_node_operator_record_key, make_node_record_key};
- use ic_registry_transport::insert;
+ use ic_registry_transport::{insert, update};
use ic_types::ReplicaVersion;
use prost::Message;
use std::str::FromStr;
@@ -234,8 +243,8 @@ mod tests {
}
#[test]
- #[should_panic(expected = "Cannot remove a node, as it has ApiBoundaryNodeRecord")]
- fn should_panic_if_node_is_api_boundary_node() {
+ #[should_panic(expected = "Cannot remove this node, as it is an active API boundary node")]
+ fn should_panic_if_node_is_api_boundary_node_and_no_replacement() {
let mut registry = invariant_compliant_registry(0);
// Add node to registry
let (mutate_request, node_ids_and_dkg_pks) =
@@ -261,6 +270,119 @@ mod tests {
registry.do_remove_node(payload, node_operator_id);
}
+ #[test]
+ fn should_succeed_to_replace_api_boundary_node() {
+ let mut registry = invariant_compliant_registry(0);
+ // Add node to registry
+ let (mutate_request, node_ids_and_dkg_pks) =
+ prepare_registry_with_nodes(1 /* mutation id */, 2 /* node count */);
+ registry.maybe_apply_mutation_internal(mutate_request.mutations);
+ let mut node_ids = node_ids_and_dkg_pks.keys();
+ let old_node_id = node_ids
+ .next()
+ .expect("should contain at least one node ID")
+ .to_owned();
+ let new_node_id = node_ids
+ .next()
+ .expect("should contain at least two node IDs")
+ .to_owned();
+
+ let node_operator_id = registry_add_node_operator_for_node(&mut registry, old_node_id, 0);
+
+ // turn first node into an API BN by adding the record to the registry
+ let api_bn = ApiBoundaryNodeRecord {
+ version: ReplicaVersion::default().to_string(),
+ };
+ registry.maybe_apply_mutation_internal(vec![insert(
+ make_api_boundary_node_record_key(old_node_id),
+ api_bn.encode_to_vec(),
+ )]);
+ let payload = RemoveNodeDirectlyPayload {
+ node_id: old_node_id,
+ };
+
+ registry.do_replace_node_with_another(payload, node_operator_id, new_node_id);
+
+ // Verify the removed node's API boundary node record has been removed
+ assert!(registry
+ .get(
+ make_api_boundary_node_record_key(old_node_id).as_bytes(),
+ registry.latest_version()
+ )
+ .is_none());
+
+ // Verify the replacement node has been turned into an API boundary node
+ assert!(registry
+ .get(
+ make_api_boundary_node_record_key(new_node_id).as_bytes(),
+ registry.latest_version()
+ )
+ .is_some());
+
+ // Verify the old node is removed from the registry
+ assert!(registry
+ .get(
+ make_node_record_key(old_node_id).as_bytes(),
+ registry.latest_version()
+ )
+ .is_none());
+
+ // Verify the new node is present in the registry
+ assert!(registry.get_node(new_node_id).is_some());
+
+ // Verify node operator allowance set to 1
+ let updated_operator = get_node_operator_record(®istry, node_operator_id).unwrap();
+ assert_eq!(updated_operator.node_allowance, 1);
+ }
+
+ #[test]
+ #[should_panic(
+ expected = "invariant check failed with message: domain field of the NodeRecord"
+ )]
+ fn should_panic_to_replace_api_boundary_node_if_new_node_has_no_domain() {
+ let mut registry = invariant_compliant_registry(0);
+ // Add node to registry
+ let (mutate_request, node_ids_and_dkg_pks) =
+ prepare_registry_with_nodes(1 /* mutation id */, 2 /* node count */);
+ registry.maybe_apply_mutation_internal(mutate_request.mutations);
+ let mut node_ids = node_ids_and_dkg_pks.keys();
+ let old_node_id = node_ids
+ .next()
+ .expect("should contain at least one node ID")
+ .to_owned();
+ let new_node_id = node_ids
+ .next()
+ .expect("should contain at least two node IDs")
+ .to_owned();
+
+ let node_operator_id = registry_add_node_operator_for_node(&mut registry, old_node_id, 0);
+
+ // turn first node into an API BN by adding the record to the registry
+ let api_bn = ApiBoundaryNodeRecord {
+ version: ReplicaVersion::default().to_string(),
+ };
+ registry.maybe_apply_mutation_internal(vec![insert(
+ make_api_boundary_node_record_key(old_node_id),
+ api_bn.encode_to_vec(),
+ )]);
+
+ // remove the default domain name from the replacement node such that the API boundary node invariant check fails
+ let mut node_record = registry.get_node_or_panic(new_node_id);
+ node_record.domain = None;
+ let update_node_record = update(
+ make_node_record_key(new_node_id).as_bytes(),
+ node_record.encode_to_vec(),
+ );
+ let mutations = vec![update_node_record];
+ registry.maybe_apply_mutation_internal(mutations);
+
+ let payload = RemoveNodeDirectlyPayload {
+ node_id: old_node_id,
+ };
+
+ registry.do_replace_node_with_another(payload, node_operator_id, new_node_id);
+ }
+
#[test]
fn should_succeed() {
let mut registry = invariant_compliant_registry(0);
@@ -441,52 +563,6 @@ mod tests {
// Should fail because the DC of operator1 and operator2 does not match
registry.do_remove_node(payload, operator2_id);
}
- #[test]
- fn should_replace_node_in_subnet() {
- let mut registry = invariant_compliant_registry(0);
-
- // Add nodes to the registry
- let (mutate_request, node_ids_and_dkg_pks) = prepare_registry_with_nodes(1, 2);
- registry.maybe_apply_mutation_internal(mutate_request.mutations);
- let node_ids = node_ids_and_dkg_pks.keys().cloned().collect::<Vec<_>>();
- let node_operator_id = registry_add_node_operator_for_node(&mut registry, node_ids[0], 0);
-
- // Create a subnet with the first node
- let subnet_id =
- registry_create_subnet_with_nodes(&mut registry, &node_ids_and_dkg_pks, &[0]);
-
- // Replace the node_ids[0] with node_ids[1], while node_ids[0] is in a subnet
- let payload = RemoveNodeDirectlyPayload {
- node_id: node_ids[0],
- };
-
- registry.do_replace_node_with_another(payload, node_operator_id, node_ids[1]);
-
- // Verify the subnet record is updated with the new node
- let expected_membership: Vec<NodeId> = vec![node_ids[1]];
- let actual_membership: Vec<NodeId> = registry
- .get_subnet_or_panic(subnet_id)
- .membership
- .iter()
- .map(|bytes| NodeId::from(PrincipalId::try_from(bytes).unwrap()))
- .collect();
- assert_eq!(actual_membership, expected_membership);
-
- // Verify the old node is removed from the registry
- assert!(registry
- .get(
- make_node_record_key(node_ids[0]).as_bytes(),
- registry.latest_version()
- )
- .is_none());
-
- // Verify the new node is present in the registry
- assert!(registry.get_node(node_ids[1]).is_some());
-
- // Verify node operator allowance increased by 1
- let updated_operator = get_node_operator_record(®istry, node_operator_id).unwrap();
- assert_eq!(updated_operator.node_allowance, 1);
- }
#[test]
#[should_panic(expected = "Cannot remove a node that is a member of a subnet")]
From a19550fae7853b2ee79e206e4b1a4957f6ce9efa Mon Sep 17 00:00:00 2001
From: Ruediger Birkner <ruediger.birkner@dfinity.org>
Date: Fri, 31 Jan 2025 13:06:50 +0000
Subject: [PATCH 2/3] updated changelog
---
rs/nns/governance/unreleased_changelog.md | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rs/nns/governance/unreleased_changelog.md b/rs/nns/governance/unreleased_changelog.md
index aebd9775eb2..99b6a240520 100644
--- a/rs/nns/governance/unreleased_changelog.md
+++ b/rs/nns/governance/unreleased_changelog.md
@@ -13,7 +13,7 @@ on the process that this file is part of, see
Two new fields are added to the request, and one to the response.
-The request now supports `page_size` and `page_number`. If `page_size` is greater than
+The request now supports `page_size` and `page_number`. If `page_size` is greater than
`MAX_LIST_NEURONS_RESULTS` (currently 500), the API will treat it as `MAX_LIST_NEURONS_RESULTS`, and
continue procesisng the request. If `page_number` is None, the API will treat it as Some(0)
@@ -22,7 +22,7 @@ additional requests need to be made.
This will only affect neuron holders with more than 500 neurons, which is a small minority.
-This allows neuron holders with many neurons to list all of their neurons, whereas before,
+This allows neuron holders with many neurons to list all of their neurons, whereas before,
responses could be too large to be sent by the protocol.
### Migrating Active Neurons to Stable Memory
From 97360a3b8fe60d103dd37609ea8a4d171aa6e559 Mon Sep 17 00:00:00 2001
From: Ruediger Birkner <ruediger.birkner@dfinity.org>
Date: Tue, 4 Feb 2025 09:06:08 +0000
Subject: [PATCH 3/3] addressing comments
---
.../do_remove_node_directly.rs | 29 ++++++++++---------
rs/registry/canister/unreleased_changelog.md | 6 ++++
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs b/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs
index a8db24d16c0..cdf11fd36ef 100644
--- a/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs
+++ b/rs/registry/canister/src/mutations/node_management/do_remove_node_directly.rs
@@ -51,8 +51,9 @@ impl Registry {
}
// Prepare mutations for removing or replacing a node in the registry.
- // If new_node_id is Some, the old node is in-place replaced with the new node, even if the old node is in active use (i.e., assigned to a subnet or acts as an API boundary node).
- // If new_node_id is None, the old node is only removed from the registry and is not allowed to be in active use.
+ // * If new_node_id is Some, the old node is in-place replaced with the new node, even if the old node is
+ // in active use (i.e., assigned to a subnet or acts as an API boundary node).
+ // * If new_node_id is None, the old node is only removed from the registry and is not allowed to be in active use.
pub fn make_remove_or_replace_node_mutations(
&mut self,
payload: RemoveNodeDirectlyPayload,
@@ -123,23 +124,25 @@ impl Registry {
let mut mutations = vec![];
- // 3. Check if the node is an API Boundary Node. If there is a replacement node, remove the existing node and try to assign the new one to act as API boundary node. This will only .
+ // 3. Check if the node is an API Boundary Node. If there is a replacement node, remove the existing node
+ // and try to assign the new one to act as API boundary node. This will only work if the new node meets all
+ // the requirements of an API boundary node (e.g., it is configured with a domain name).
if let Some(api_bn_record) = self.get_api_boundary_node_record(payload.node_id) {
- if let Some(node_id) = new_node_id {
- // remove the existing API boundary node record
- let old_key = make_api_boundary_node_record_key(payload.node_id);
- mutations.push(delete(old_key));
-
- // create the new API boundary node record by just cloning the old one and inserting it with the new key
- let new_key = make_api_boundary_node_record_key(node_id);
- mutations.push(insert(new_key, api_bn_record.clone().encode_to_vec()));
- } else {
+ let Some(replacement_node_id) = new_node_id else {
panic!(
"{}do_remove_node_directly: Cannot remove this node, as it is an active API boundary node: {}",
LOG_PREFIX,
make_api_boundary_node_record_key(payload.node_id)
);
- }
+ };
+
+ // remove the existing API boundary node record
+ let old_key = make_api_boundary_node_record_key(payload.node_id);
+ mutations.push(delete(old_key));
+
+ // create the new API boundary node record by just cloning the old one and inserting it with the new key
+ let new_key = make_api_boundary_node_record_key(replacement_node_id);
+ mutations.push(insert(new_key, api_bn_record.clone().encode_to_vec()));
}
// 4. Check if node is in a subnet, and if so, replace it in the subnet by updating the membership in the subnet record.
diff --git a/rs/registry/canister/unreleased_changelog.md b/rs/registry/canister/unreleased_changelog.md
index 4e2637ad439..52f7e85fd0f 100644
--- a/rs/registry/canister/unreleased_changelog.md
+++ b/rs/registry/canister/unreleased_changelog.md
@@ -16,6 +16,12 @@ on the process that this file is part of, see
This update migrates registry from using dfn_core to using virtual memory regions provided by ic_stable_structures
MemoryManager. This allows in the future to migrate the Registry records into stable memory.
+### Automatically replace the nodes when an active API boundary node is replaced
+
+`add_node` will now also automatically replace a node if it is being redeployed and has
+been active as an API boundary node before. It will fail if the redeployed node does not
+meet the requirements for an API boundary node (i.e., is configured with a domain name).
+
## Deprecated
## Removed