From 03d10b395cec95e811803c9f9b369f40dcee81ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20Tomi=C4=87?= Date: Mon, 19 Aug 2024 13:30:55 +0200 Subject: [PATCH] chore(dre): A few more tweaks in the summary generation for node replacement proposals (#744) Co-authored-by: sa-github-api <138766536+sa-github-api@users.noreply.github.com> --- .../components/subnets/SubnetChangePage.tsx | 4 +- rs/cli/src/commands/subnet/create.rs | 4 +- rs/cli/src/commands/subnet/replace.rs | 2 +- rs/cli/src/runner.rs | 2 - rs/decentralization/src/lib.rs | 25 +++++---- rs/decentralization/src/nakamoto/mod.rs | 42 +++++++++----- rs/decentralization/src/network.rs | 55 +++++++++++-------- 7 files changed, 80 insertions(+), 54 deletions(-) diff --git a/dashboard/packages/app/src/components/subnets/SubnetChangePage.tsx b/dashboard/packages/app/src/components/subnets/SubnetChangePage.tsx index d93004ca4..8daf80d2a 100644 --- a/dashboard/packages/app/src/components/subnets/SubnetChangePage.tsx +++ b/dashboard/packages/app/src/components/subnets/SubnetChangePage.tsx @@ -140,7 +140,7 @@ export const SubnetChangePage = ({ network }: { network: string }) => { } /> - Decentralization scores + Decentralization Nakamoto coefficients {Object.entries(change?.score_after?.coefficients ?? {}).map(([key, _]) => { @@ -158,7 +158,7 @@ export const SubnetChangePage = ({ network }: { network: string }) => { {change?.comment && - { change?.comment.split("\n").filter(l => l).map((line, index) => { if (index == 0) {return line} else { return
  • {line}
    • } }) } + {change?.comment.split("\n").filter(l => l).map((line, index) => { if (index == 0) { return line } else { return
  • {line}
    • } })}     } diff --git a/rs/cli/src/commands/subnet/create.rs b/rs/cli/src/commands/subnet/create.rs index e3decc12e..f2c99b1e2 100644 --- a/rs/cli/src/commands/subnet/create.rs +++ b/rs/cli/src/commands/subnet/create.rs @@ -22,8 +22,8 @@ pub struct Create { #[clap(long, num_args(1..))] pub only: Vec, - #[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, /// Motivation for replacing custom nodes diff --git a/rs/cli/src/commands/subnet/replace.rs b/rs/cli/src/commands/subnet/replace.rs index 5a21dc893..2bcf99d6c 100644 --- a/rs/cli/src/commands/subnet/replace.rs +++ b/rs/cli/src/commands/subnet/replace.rs @@ -41,7 +41,7 @@ algorithm"# pub only: Vec, /// 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, diff --git a/rs/cli/src/runner.rs b/rs/cli/src/runner.rs index 185cdf919..bc6a6e5bb 100644 --- a/rs/cli/src/runner.rs +++ b/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(()); diff --git a/rs/decentralization/src/lib.rs b/rs/decentralization/src/lib.rs index d2a4b99ba..8fd293ce9 100644 --- a/rs/decentralization/src/lib.rs +++ b/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::>().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())?; diff --git a/rs/decentralization/src/nakamoto/mod.rs b/rs/decentralization/src/nakamoto/mod.rs index a6ebc56fa..aa4baee99 100644 --- a/rs/decentralization/src/nakamoto/mod.rs +++ b/rs/decentralization/src/nakamoto/mod.rs @@ -334,14 +334,14 @@ 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 @@ -349,13 +349,21 @@ impl NakamotoScore { 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)); } } } diff --git a/rs/decentralization/src/network.rs b/rs/decentralization/src/network.rs index 046b614bd..d0015a421 100644 --- a/rs/decentralization/src/network.rs +++ b/rs/decentralization/src/network.rs @@ -996,7 +996,7 @@ impl SubnetChangeRequest { .collect::>(); 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, @@ -1004,9 +1004,21 @@ impl SubnetChangeRequest { 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::>(); + 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 = 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::>().join("\n"), + reason_additional_optimizations, change ); subnets_changed.push(change.clone().with_motivation(motivation));