feat(dre): [DRE-240] Add more info to the subnet membership change proposals

rs/cli/src/runner.rs

@@ -113,7 +113,7 @@ impl Runner {
             .excluding_from_available(request.exclude.clone().unwrap_or_default())
             .including_from_available(request.only.clone().unwrap_or_default())
             .including_from_available(request.include.clone().unwrap_or_default())
-            .resize(request.add, request.remove)?;
+            .resize(request.add, request.remove, 0)?;
 
         let change = SubnetChangeResponse::from(&change);
 
@@ -124,13 +124,13 @@ impl Runner {
         }
         println!("{}", change);
 
-        if change.added.is_empty() && change.removed.is_empty() {
+        if change.added_with_desc.is_empty() && change.removed_with_desc.is_empty() {
             return Ok(());
         }
-        if change.added.len() == change.removed.len() {
+        if change.added_with_desc.len() == change.removed_with_desc.len() {
             self.run_membership_change(change.clone(), replace_proposal_options(&change)?).await
         } else {
-            let action = if change.added.len() < change.removed.len() {
+            let action = if change.added_with_desc.len() < change.removed_with_desc.len() {
                 "Removing nodes from"
             } else {
                 "Adding nodes to"
@@ -189,7 +189,7 @@ impl Runner {
         self.ic_admin
             .propose_run(
                 ProposeCommand::CreateSubnet {
-                    node_ids: subnet_creation_data.added,
+                    node_ids: subnet_creation_data.added_with_desc.iter().map(|a| a.0).collect::<Vec<_>>(),
                     replica_version,
                     other_args,
                 },
@@ -211,7 +211,7 @@ impl Runner {
         }
         println!("{}", change);
 
-        if change.added.is_empty() && change.removed.is_empty() {
+        if change.added_with_desc.is_empty() && change.removed_with_desc.is_empty() {
             return Ok(());
         }
         self.run_membership_change(change.clone(), replace_proposal_options(&change)?).await
@@ -494,8 +494,8 @@ impl Runner {
             let removed_nodes = self.registry.get_decentralized_nodes(&change.get_added_node_ids()).await?;
 
             let subnet_after = subnet_before
-                .with_nodes(added_nodes)
-                .without_nodes(removed_nodes)
+                .with_nodes(added_nodes.into_iter().map(|n| (n, "added".to_string())).collect())
+                .without_nodes(removed_nodes.into_iter().map(|n| (n, "removed".to_string())).collect())
                 .map_err(|e| anyhow::anyhow!(e))?;
 
             let subnet_change = SubnetChange {
@@ -524,7 +524,7 @@ impl Runner {
         let change = SubnetChangeResponse::from(&change_request.rescue()?);
         println!("{}", change);
 
-        if change.added.is_empty() && change.removed.is_empty() {
+        if change.added_with_desc.is_empty() && change.removed_with_desc.is_empty() {
             return Ok(());
         }
 
@@ -615,8 +615,8 @@ impl Runner {
             .propose_run(
                 ProposeCommand::ChangeSubnetMembership {
                     subnet_id,
-                    node_ids_add: change.added.clone(),
-                    node_ids_remove: change.removed.clone(),
+                    node_ids_add: change.added_with_desc.iter().map(|a| a.0).collect::<Vec<_>>(),
+                    node_ids_remove: change.removed_with_desc.iter().map(|a| a.0).collect::<Vec<_>>(),
                 },
                 options,
             )
@@ -629,7 +629,7 @@ impl Runner {
 pub fn replace_proposal_options(change: &SubnetChangeResponse) -> anyhow::Result<ic_admin::ProposeOptions> {
     let subnet_id = change.subnet_id.ok_or_else(|| anyhow::anyhow!("subnet_id is required"))?.to_string();
 
-    let replace_target = if change.added.len() > 1 || change.removed.len() > 1 {
+    let replace_target = if change.added_with_desc.len() > 1 || change.removed_with_desc.len() > 1 {
         "nodes"
     } else {
         "a node"

rs/cli/src/subnet_manager.rs

@@ -1,4 +1,5 @@
 use core::fmt;
+use std::collections::HashSet;
 use std::rc::Rc;
 
 use anyhow::anyhow;
@@ -12,7 +13,6 @@ use ic_management_backend::lazy_registry::LazyRegistry;
 use ic_management_types::MinNakamotoCoefficients;
 use ic_management_types::Network;
 use ic_types::PrincipalId;
-use itertools::Itertools;
 use log::{info, warn};
 
 #[derive(Clone)]
@@ -63,7 +63,7 @@ impl SubnetManager {
             .ok_or_else(|| anyhow!(SubnetManagerError::SubnetTargetNotProvided))
     }
 
-    async fn unhealthy_nodes(&self, subnet: DecentralizedSubnet) -> anyhow::Result<Vec<DecentralizedNode>> {
+    async fn unhealthy_nodes(&self, subnet: DecentralizedSubnet) -> anyhow::Result<Vec<(DecentralizedNode, ic_management_types::Status)>> {
         let health_client = health::HealthClient::new(self.network.clone());
         let subnet_health = health_client.subnet(subnet.id).await?;
 
@@ -76,12 +76,12 @@ impl SubnetManager {
                         None
                     } else {
                         info!("Node {} is {:?}", n.id, health);
-                        Some(n)
+                        Some((n, health.clone()))
                     }
                 }
                 None => {
                     warn!("Node {} has no known health, assuming unhealthy", n.id);
-                    Some(n)
+                    Some((n, ic_management_types::Status::Unknown))
                 }
             })
             .collect::<Vec<_>>();
@@ -119,8 +119,8 @@ impl SubnetManager {
     ) -> anyhow::Result<SubnetChangeResponse> {
         let subnet_query_by = self.get_subnet_query_by(self.target()?).await?;
         let mut motivations: Vec<String> = if let Some(motivation) = motivation { vec![motivation] } else { vec![] };
-        let mut to_be_replaced: Vec<DecentralizedNode> = if let SubnetQueryBy::NodeList(nodes) = &subnet_query_by {
-            nodes.clone()
+        let mut to_be_replaced: Vec<(DecentralizedNode, String)> = if let SubnetQueryBy::NodeList(nodes) = &subnet_query_by {
+            nodes.iter().map(|n| (n.clone(), "as per user request".to_string())).collect()
         } else {
             vec![]
         };
@@ -134,39 +134,36 @@ impl SubnetManager {
             .including_from_available(include.clone().unwrap_or_default())
             .with_min_nakamoto_coefficients(min_nakamoto_coefficients.clone());
 
+        let mut node_ids_unhealthy = HashSet::new();
         if heal {
             let subnet_unhealthy = self.unhealthy_nodes(subnet_change_request.subnet()).await?;
             let subnet_unhealthy_without_included = subnet_unhealthy
                 .into_iter()
-                .filter(|n| !include.as_ref().unwrap_or(&vec![]).contains(&n.id))
+                .filter(|(n, _)| !include.as_ref().unwrap_or(&vec![]).contains(&n.id))
+                .map(|(n, s)| (n, s.to_string().to_lowercase()))
                 .collect::<Vec<_>>();
 
-            to_be_replaced.extend(subnet_unhealthy_without_included);
-
-            let without_specified = to_be_replaced
-                .iter()
-                .filter(|n| match &subnet_query_by {
-                    SubnetQueryBy::NodeList(nodes) => !nodes.contains(n),
-                    _ => true,
-                })
-                .collect_vec();
-
-            if !without_specified.is_empty() {
-                let num_unhealthy = without_specified.len();
-                let replace_target = if num_unhealthy == 1 { "node" } else { "nodes" };
-                motivations.push(format!("replacing {num_unhealthy} unhealthy {replace_target}"));
+            for (n, reason) in subnet_unhealthy_without_included.iter() {
+                motivations.push(format!("replacing {reason} node {}", n.id));
+                node_ids_unhealthy.insert(n.id);
             }
+
+            to_be_replaced.extend(subnet_unhealthy_without_included);
         }
 
         let change = subnet_change_request.optimize(optimize.unwrap_or(0), &to_be_replaced)?;
-        let num_optimized = change.removed().len() - to_be_replaced.len();
 
-        if num_optimized > 0 {
-            let replace_target = if num_optimized == 1 { "node" } else { "nodes" };
-            motivations.push(format!("replacing {num_optimized} {replace_target} to improve subnet decentralization"));
+        for (n, _) in change.removed().iter().filter(|(n, _)| !node_ids_unhealthy.contains(&n.id)) {
+            motivations.push(format!("replacing {} to optimize network topology", n.id));
         }
 
-        let change = SubnetChangeResponse::from(&change).with_motivation(motivations.join("; "));
+        let motivation = format!(
+                "\n{}\n\nNOTE: The information below is provided for your convenience. Please independently verify the decentralization changes rather than relying solely on this summary.\nCode for calculating replacements is at https://github.com/dfinity/dre/blob/79066127f58c852eaf4adda11610e815a426878c/rs/decentralization/src/network.rs#L912\n\n```\n{}\n```\n",
+                motivations.iter().map(|s| format!(" - {}", s)).collect::<Vec<String>>().join("\n"),
+                change
+            );
+
+        let change = SubnetChangeResponse::from(&change).with_motivation(motivation);
 
         Ok(change)
     }

rs/decentralization/src/lib.rs

@@ -2,7 +2,7 @@ pub mod nakamoto;
 pub mod network;
 pub mod subnets;
 use colored::Colorize;
-use itertools::{EitherOrBoth::*, Itertools};
+use itertools::Itertools;
 use std::collections::BTreeMap;
 use std::fmt::{Display, Formatter};
 
@@ -12,8 +12,8 @@ use serde::{self, Deserialize, Serialize};
 
 #[derive(Clone, Debug, Deserialize, Serialize, Default)]
 pub struct SubnetChangeResponse {
-    pub added: Vec<PrincipalId>,
-    pub removed: Vec<PrincipalId>,
+    pub added_with_desc: Vec<(PrincipalId, String)>,
+    pub removed_with_desc: Vec<(PrincipalId, String)>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub subnet_id: Option<PrincipalId>,
     pub score_before: nakamoto::NakamotoScore,
@@ -39,8 +39,8 @@ impl SubnetChangeResponse {
 impl From<&network::SubnetChange> for SubnetChangeResponse {
     fn from(change: &network::SubnetChange) -> Self {
         Self {
-            added: change.added().iter().map(|n| n.id).collect(),
-            removed: change.removed().iter().map(|n| n.id).collect(),
+            added_with_desc: change.added().iter().map(|n| (n.0.id, n.1.clone())).collect(),
+            removed_with_desc: change.removed().iter().map(|n| (n.0.id, n.1.clone())).collect(),
             subnet_id: if change.id == Default::default() { None } else { Some(change.id) },
             score_before: nakamoto::NakamotoScore::new_from_nodes(&change.old_nodes),
             score_after: nakamoto::NakamotoScore::new_from_nodes(&change.new_nodes),
@@ -157,25 +157,14 @@ impl Display for SubnetChangeResponse {
         }
 
         writeln!(f, "{}", table)?;
-        for pair in self.added.iter().zip_longest(self.removed.iter()) {
-            match pair {
-                Both(a, r) => {
-                    writeln!(f, "{}{}", format!("  - {}", r).red(), format!("    + {}", a).green()).expect("write failed");
-                }
-                Left(a) => {
-                    writeln!(
-                        f,
-                        "                                                                   {}",
-                        format!("    + {}", a).green()
-                    )
-                    .expect("write failed");
-                }
-                Right(r) => {
-                    writeln!(f, "{}", format!("  - {}", r).red()).expect("write failed");
-                }
-            }
+        writeln!(f, "    nodes removed:")?;
+        for (id, desc) in &self.removed_with_desc {
+            writeln!(f, "    --> {} [selected based on {}]", id, desc).expect("write failed");
+        }
+        writeln!(f, "\n    nodes added:")?;
+        for (id, desc) in &self.added_with_desc {
+            writeln!(f, "    ++> {} [selected based on {}]", id, desc).expect("write failed");
         }
-        writeln!(f)?;
 
         if let Some(comment) = &self.comment {
             writeln!(f, "{}", format!("*** Note ***\n{}", comment).red())?;