chore: allow automatically replacing a node even if it is active as an API BN

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.
 
 ### Periodic Confirmation
@@ -78,6 +78,10 @@ No neurons are actually migrated yet.
   was default to true before, and now it's default to true. More details can be found at:
   https://forum.dfinity.org/t/listneurons-api-change-empty-neurons/40311
 
+* `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

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(&registry, 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() {