chore(dre): A few more tweaks in the summary generation for node repl…

…acement proposals (#744)

Co-authored-by: sa-github-api <[email protected]>

dashboard/packages/app/src/components/subnets/SubnetChangePage.tsx

@@ -140,7 +140,7 @@ export const SubnetChangePage = ({ network }: { network: string }) => {
           <Grid item xs={12}>
             <Chip label="Vote on the proposal" component="a" href={`https://nns.ic0.app/proposal/?proposal=${change?.proposal_id}`} clickable target='_blank' icon={<HowToVoteIcon style={{ color: purple[500] }} />} />
           </Grid>
-          <Typography variant="h5" style={{ marginLeft: 16 }}>Decentralization scores</Typography>
+          <Typography variant="h5" style={{ marginLeft: 16 }}>Decentralization Nakamoto coefficients</Typography>
           <Grid item xs={12} container direction='column'>
 
             {Object.entries(change?.score_after?.coefficients ?? {}).map(([key, _]) => {
@@ -158,7 +158,7 @@ export const SubnetChangePage = ({ network }: { network: string }) => {
           </Grid>
           {change?.comment && <Grid item>
             <Alert variant="filled" severity="warning">
-              { change?.comment.split("\n").filter(l => l).map((line, index) => { if (index == 0) {return line} else { return <li>{line}</li> } }) }
+              {change?.comment.split("\n").filter(l => l).map((line, index) => { if (index == 0) { return line } else { return <li>{line}</li> } })}
             </Alert>
           </Grid>
           }

rs/cli/src/commands/subnet/create.rs

@@ -22,8 +22,8 @@ pub struct Create {
     #[clap(long, num_args(1..))]
     pub only: Vec<String>,
 
-    #[clap(long, num_args(1..), help = r#"Force t he inclusion of the provided nodes for replacement,
-regardless of the decentralization score"#)]
+    #[clap(long, num_args(1..), help = r#"Force the inclusion of the provided nodes for replacement,
+regardless of the decentralization coefficients"#)]
     pub include: Vec<PrincipalId>,
 
     /// Motivation for replacing custom nodes

rs/cli/src/commands/subnet/replace.rs

@@ -41,7 +41,7 @@ algorithm"#
     pub only: Vec<String>,
 
     /// Force the inclusion of the provided nodes for replacement, regardless
-    /// of the decentralization score
+    /// of the decentralization coefficients
     #[clap(long, num_args(1..))]
     pub include: Vec<PrincipalId>,
 

rs/cli/src/runner.rs

@@ -122,7 +122,6 @@ impl Runner {
                 println!("{}\n", run_log.join("\n"));
             }
         }
-        println!("{}", change);
 
         if change.added_with_desc.is_empty() && change.removed_with_desc.is_empty() {
             return Ok(());
@@ -209,7 +208,6 @@ impl Runner {
                 println!("{}\n", run_log.join("\n"));
             }
         }
-        println!("{}", change);
 
         if change.added_with_desc.is_empty() && change.removed_with_desc.is_empty() {
             return Ok(());

rs/decentralization/src/lib.rs

@@ -74,7 +74,11 @@ impl From<&network::SubnetChange> for SubnetChangeResponse {
 
 impl Display for SubnetChangeResponse {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        writeln!(f, "Decentralization score changes for subnet {}:\n", self.subnet_id.unwrap_or_default())?;
+        writeln!(
+            f,
+            "Decentralization Nakamoto coefficient changes for subnet {}:\n",
+            self.subnet_id.unwrap_or_default()
+        )?;
         let before_individual = self.score_before.scores_individual();
         let after_individual = self.score_after.scores_individual();
         self.score_before
@@ -123,6 +127,15 @@ impl Display for SubnetChangeResponse {
             }
         )?;
 
+        writeln!(f, "Nodes removed:")?;
+        for (id, desc) in &self.removed_with_desc {
+            writeln!(f, " --> {} [selected based on {}]", id, desc).expect("write failed");
+        }
+        writeln!(f, "\nNodes added:")?;
+        for (id, desc) in &self.added_with_desc {
+            writeln!(f, " ++> {} [selected based on {}]", id, desc).expect("write failed");
+        }
+
         let rows = self.feature_diff.values().map(|diff| diff.len()).max().unwrap_or(0);
         let mut table = tabular::Table::new(&self.feature_diff.keys().map(|_| "    {:<}  {:>}").collect::<Vec<_>>().join(""));
         table.add_row(
@@ -156,15 +169,7 @@ impl Display for SubnetChangeResponse {
             }));
         }
 
-        writeln!(f, "{}", table)?;
-        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, "\n\n{}", table)?;
 
         if let Some(comment) = &self.comment {
             writeln!(f, "{}", format!("*** Note ***\n{}", comment).red())?;

rs/decentralization/src/nakamoto/mod.rs

@@ -334,28 +334,36 @@ impl NakamotoScore {
         let mut cmp = self.score_min().partial_cmp(&other.score_min());
 
         if cmp != Some(Ordering::Equal) {
-            return (cmp, "the minimum score across all features".to_string());
+            return (cmp, "the minimum Nakamoto coefficient across all features".to_string());
         }
 
         // Then try to increase the log2 avg
         cmp = self.score_avg_log2().partial_cmp(&other.score_avg_log2());
 
         if cmp != Some(Ordering::Equal) {
-            return (cmp, "the average log2 score across all features".to_string());
+            return (cmp, "the average log2 of Nakamoto Coefficients across all features".to_string());
         }
 
         // Try to pick the candidate that *reduces* the number of nodes
         // controlled by the top actors
         cmp = other.critical_features_num_nodes().partial_cmp(&self.critical_features_num_nodes());
 
         if cmp != Some(Ordering::Equal) {
+            let val_self = self.critical_features_num_nodes();
+            let val_other = other.critical_features_num_nodes();
             return (
                 cmp,
-                format!(
-                    "the number of nodes controlled by dominant actors for critical features (NP, Country) changes from {:?} to {:?}",
-                    other.critical_features_num_nodes(),
-                    self.critical_features_num_nodes()
-                ),
+                if val_self[0] != val_other[0] {
+                    format!(
+                        "the number of nodes controlled by dominant NPs: value changes from {} to {}",
+                        val_other[0], val_self[0]
+                    )
+                } else {
+                    format!(
+                        "the number of nodes controlled by dominant Country actors: value changes from {} to {}",
+                        val_other[1], val_self[1]
+                    )
+                },
             );
         }
 
@@ -366,13 +374,21 @@ impl NakamotoScore {
             .partial_cmp(&other.critical_features_unique_actors());
 
         if cmp != Some(Ordering::Equal) {
+            let val_self = self.critical_features_unique_actors();
+            let val_other = other.critical_features_unique_actors();
             return (
                 cmp,
-                format!(
-                    "the number of different actors for critical features (NP, Country) changes from {:?} to {:?}",
-                    other.critical_features_unique_actors(),
-                    self.critical_features_unique_actors()
-                ),
+                if val_self[0] != val_other[0] {
+                    format!(
+                        "the number of different NP actors: value changes from {} to {}",
+                        val_other[0], val_self[0]
+                    )
+                } else {
+                    format!(
+                        "the number of different Country actors: value changes from {} to {}",
+                        val_other[1], val_self[1]
+                    )
+                },
             );
         }
 
@@ -397,7 +413,7 @@ impl NakamotoScore {
                 cmp = c2.partial_cmp(c1);
 
                 if cmp != Some(Ordering::Equal) {
-                    return (cmp, format!("Nakamoto coefficient for feature {}", feature));
+                    return (cmp, format!("the Nakamoto coefficient value for feature {}", feature));
                 }
             }
         }

rs/decentralization/src/network.rs

@@ -996,17 +996,29 @@ impl SubnetChangeRequest {
             .collect::<Vec<_>>();
 
         info!(
-            "Resizing subnet {} by adding {} nodes and removing {} (+{} unhealthy) nodes. Available {} healthy nodes.",
+            "Resizing subnet {} by removing {} (+{} unhealthy) nodes and adding {} nodes. Available {} healthy nodes.",
             self.subnet.id,
             how_many_nodes_to_add,
             how_many_nodes_to_remove,
             how_many_nodes_unhealthy,
             available_nodes.len()
         );
 
-        let resized_subnet = self
-            .subnet
-            .clone()
+        let resized_subnet = if how_many_nodes_to_remove > 0 {
+            self.subnet
+                .clone()
+                .subnet_with_fewer_nodes(how_many_nodes_to_remove)
+                .map_err(|e| NetworkError::ResizeFailed(e.to_string()))?
+        } else {
+            self.subnet.clone()
+        };
+
+        let available_nodes = available_nodes
+            .iter()
+            .cloned()
+            .chain(resized_subnet.removed_nodes_desc.iter().map(|(n, _)| n.clone()))
+            .collect::<Vec<_>>();
+        let resized_subnet = resized_subnet
             .with_nodes(
                 self.include_nodes
                     .iter()
@@ -1017,14 +1029,6 @@ impl SubnetChangeRequest {
             .subnet_with_more_nodes(how_many_nodes_to_add, &available_nodes)
             .map_err(|e| NetworkError::ResizeFailed(e.to_string()))?;
 
-        let resized_subnet = if how_many_nodes_to_remove > 0 {
-            resized_subnet
-                .subnet_with_fewer_nodes(how_many_nodes_to_remove)
-                .map_err(|e| NetworkError::ResizeFailed(e.to_string()))?
-        } else {
-            resized_subnet
-        };
-
         let subnet_change = SubnetChange {
             id: self.subnet.id,
             old_nodes,
@@ -1259,17 +1263,19 @@ impl NetworkHealRequest {
                 .iter()
                 .find(|change| change.score_after == changes_max_score.score_after)
                 .expect("No suitable changes found");
-            // TODO: consider adding this (or similar) to the proposal summary message
-            // i.e. why are we replacing 2 instead of 1 node?
-            info!(
-                "Selected the change with {} nodes removed and {} nodes added. Select reason: {}",
-                change.removed_with_desc.len(),
-                change.added_with_desc.len(),
-                change
-                    .score_after
-                    .describe_difference_from(&NakamotoScore::new_from_nodes(&subnet.decentralized_subnet.nodes))
-                    .1
-            );
+
+            let num_opt = change.removed_with_desc.len() - unhealthy_nodes_len;
+            let reason_additional_optimizations =
+                format!("\nReplacing additional {} node{} optimizes topology based on {}.
+Note: the heuristic for node replacement relies not only on the Nakamoto coefficient but also on other factors that iteratively optimize network topology.
+Due to this, Nakamoto coefficients may not directly increase in every node replacement proposal.
+Code for comparing decentralization of two candidate subnet topologies is at:
+https://github.com/dfinity/dre/blob/79066127f58c852eaf4adda11610e815a426878c/rs/decentralization/src/nakamoto/mod.rs#L342
+",
+                    num_opt,
+                    if num_opt > 1 { "s" } else { "" },
+                    change.score_after.describe_difference_from(&changes[0].score_after).1
+                );
 
             let mut motivations: Vec<String> = Vec::new();
 
@@ -1293,8 +1299,9 @@ impl NetworkHealRequest {
             available_nodes.retain(|node| !nodes_added.contains(&node.id));
             // TODO: Add instructions for independent verification of the decentralization changes
             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",
+                "\n{}\n{}\nNOTE: The information below is provided for your convenience. Please independently verify the decentralization changes rather than relying solely on this summary.\n\n```\n{}\n```\n",
                 motivations.iter().map(|s| format!(" - {}", s)).collect::<Vec<String>>().join("\n"),
+                reason_additional_optimizations,
                 change
             );
             subnets_changed.push(change.clone().with_motivation(motivation));