Skip to content

Commit

Permalink
feat(decentralization): more informative messages on penalties before…
Browse files Browse the repository at this point in the history 
… and after the change (#1082)
  • Loading branch information
@sasa-tomic
sasa-tomic authored Nov 14, 2024
1 parent dee4d21 commit 17d9909
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 52 deletions.
28 changes: 17 additions & 11 deletions rs/decentralization/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ pub struct SubnetChangeResponse {
pub health_of_nodes: IndexMap<PrincipalId, HealthStatus>,
pub score_before: nakamoto::NakamotoScore,
pub score_after: nakamoto::NakamotoScore,
pub penalties_before_change: usize,
pub penalties_after_change: usize,
pub business_rules_log: Vec<String>,
pub penalties_before_change: (usize, Vec<String>),
pub penalties_after_change: (usize, Vec<String>),
pub motivation: Option<String>,
pub comment: Option<String>,
pub run_log: Option<Vec<String>>,
Expand All @@ -48,9 +47,8 @@ impl SubnetChangeResponse {
health_of_nodes: node_health.clone(),
score_before: nakamoto::NakamotoScore::new_from_nodes(&change.old_nodes),
score_after: nakamoto::NakamotoScore::new_from_nodes(&change.new_nodes),
penalties_before_change: change.penalties_before_change,
penalties_after_change: change.penalties_after_change,
business_rules_log: change.business_rules_log.clone(),
penalties_before_change: change.penalties_before_change.clone(),
penalties_after_change: change.penalties_after_change.clone(),
motivation,
comment: change.comment.clone(),
run_log: Some(change.run_log.clone()),
Expand Down Expand Up @@ -122,11 +120,11 @@ impl Display for SubnetChangeResponse {
self.score_after.describe_difference_from(&self.score_before).1
)?;

if self.penalties_before_change != self.penalties_after_change || self.penalties_after_change > 0 {
if (self.penalties_before_change.0 != self.penalties_after_change.0) || (self.penalties_after_change.0 > 0) {
writeln!(
f,
"\nImpact on business rules penalties: {} -> {}",
self.penalties_before_change, self.penalties_after_change
self.penalties_before_change.0, self.penalties_after_change.0
)?;
}

Expand Down Expand Up @@ -184,11 +182,19 @@ impl Display for SubnetChangeResponse {

writeln!(f, "\n\n```\n{}```\n", table)?;

if !self.business_rules_log.is_empty() {
if !self.penalties_before_change.1.is_empty() {
writeln!(
f,
"### Business rules check results after the membership change\n\n{}",
self.business_rules_log.join("\n")
"Business rules check results *before* the membership change:\n{}",
self.penalties_before_change.1.iter().map(|l| format!("- {}", l)).join("\n")
)?;
}

if !self.penalties_after_change.1.is_empty() {
writeln!(
f,
"Business rules check results *after* the membership change:\n{}",
self.penalties_after_change.1.iter().map(|l| format!("- {}", l)).join("\n")
)?;
}

Expand Down
50 changes: 21 additions & 29 deletions rs/decentralization/src/network.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1110,12 +1110,10 @@ impl SubnetChangeRequest {
.without_duplicate_added_removed();

let penalties_before_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&self.subnet.id, &old_nodes)
.map_err(|e| NetworkError::ResizeFailed(e.to_string()))?
.0;
.map_err(|e| NetworkError::ResizeFailed(e.to_string()))?;

let business_rules_check_after_change =
DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&self.subnet.id, &resized_subnet.nodes)
.map_err(|e| NetworkError::ResizeFailed(e.to_string()))?;
let penalties_after_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&self.subnet.id, &resized_subnet.nodes)
.map_err(|e| NetworkError::ResizeFailed(e.to_string()))?;

let subnet_change = SubnetChange {
subnet_id: self.subnet.id,
Expand All @@ -1124,8 +1122,7 @@ impl SubnetChangeRequest {
removed_nodes: resized_subnet.removed_nodes,
added_nodes: resized_subnet.added_nodes,
penalties_before_change,
penalties_after_change: business_rules_check_after_change.0,
business_rules_log: business_rules_check_after_change.1,
penalties_after_change,
comment: resized_subnet.comment,
run_log: resized_subnet.run_log,
};
Expand All @@ -1152,9 +1149,8 @@ pub struct SubnetChange {
pub new_nodes: Vec<Node>,
pub removed_nodes: Vec<Node>,
pub added_nodes: Vec<Node>,
pub penalties_before_change: usize,
pub penalties_after_change: usize,
pub business_rules_log: Vec<String>,
pub penalties_before_change: (usize, Vec<String>),
pub penalties_after_change: (usize, Vec<String>),
pub comment: Option<String>,
pub run_log: Vec<String>,
}
Expand All @@ -1163,11 +1159,9 @@ impl SubnetChange {
pub fn with_nodes(self, nodes_to_add: &[Node]) -> Self {
let new_nodes = [self.new_nodes, nodes_to_add.to_vec()].concat();
let penalties_before_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&self.subnet_id, &self.old_nodes)
.expect("Business rules check before should succeed")
.0;
.expect("Business rules check before should succeed");
let penalties_after_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&self.subnet_id, &new_nodes)
.expect("Business rules check after should succeed")
.0;
.expect("Business rules check after should succeed");
Self {
new_nodes,
added_nodes: nodes_to_add.to_vec(),
Expand All @@ -1182,11 +1176,9 @@ impl SubnetChange {
self.removed_nodes.extend(nodes_to_remove.to_vec());
self.new_nodes.retain(|n| !nodes_to_rm.contains(n));
self.penalties_before_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&self.subnet_id, &self.old_nodes)
.expect("Business rules check before should succeed")
.0;
.expect("Business rules check before should succeed");
self.penalties_after_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&self.subnet_id, &self.new_nodes)
.expect("Business rules check after should succeed")
.0;
.expect("Business rules check after should succeed");
self
}

Expand Down Expand Up @@ -1358,7 +1350,7 @@ impl NetworkHealRequest {
"Replacing {} nodes in subnet {} results in subnet with business-rules penalty {} and Nakamoto coefficient: {}\n",
change.node_ids_removed.len(),
subnet.decentralized_subnet.id,
change.penalties_after_change,
change.penalties_after_change.0,
change.score_after
);
}
Expand All @@ -1368,7 +1360,7 @@ impl NetworkHealRequest {
// As a compromise, we will choose the change that has the lowest business-rules penalty,
// or if there is no improvement in the business-rules penalty, we will choose the change
// that replaces the fewest nodes.
let penalty_optimize_min = changes.iter().map(|change| change.penalties_after_change).min().unwrap();
let penalty_optimize_min = changes.iter().map(|change| change.penalties_after_change.0).min().unwrap();
info!("Min business-rules penalty: {}", penalty_optimize_min);

let all_optimizations_desc = changes
Expand All @@ -1377,25 +1369,25 @@ impl NetworkHealRequest {
.skip(1)
.map(|(num_opt, change)| {
format!(
"- {} additional node{}{}: {}",
"- {} additional node{} would result in: {}{}",
num_opt,
if num_opt > 1 { "s" } else { "" },
if change.penalties_after_change > 0 {
format!(" (solution penalty: {})", change.penalties_after_change)
} else {
"".to_string()
},
change
.score_after
.describe_difference_from(&changes[num_opt.saturating_sub(1)].score_after)
.1
.1,
if change.penalties_after_change.0 > 0 {
format!(" (solution penalty: {})", change.penalties_after_change.0)
} else {
"".to_string()
},
)
})
.collect::<Vec<_>>();

let best_changes = changes
.into_iter()
.filter(|change| change.penalties_after_change == penalty_optimize_min)
.filter(|change| change.penalties_after_change.0 == penalty_optimize_min)
.collect::<Vec<_>>();

let changes_max_score = best_changes
Expand All @@ -1404,7 +1396,7 @@ impl NetworkHealRequest {
.expect("Failed to find a replacement with the highest Nakamoto coefficient");
info!("Best Nakamoto coefficient after the change: {}", changes_max_score.score_after);

let change = if penalty_optimize_min > 0 && penalty_optimize_min == best_changes[0].penalties_after_change {
let change = if penalty_optimize_min > 0 && penalty_optimize_min == best_changes[0].penalties_after_change.0 {
info!("No reduction in business-rules penalty, choosing the first change");
&best_changes[0]
} else {
Expand Down
11 changes: 4 additions & 7 deletions rs/ic-management-backend/src/endpoints/query_decentralization.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,9 @@ async fn get_decentralization_analysis(
None => updated_subnet,
};
let penalties_before_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&original_subnet.id, &original_subnet.nodes)
.expect("Business rules check before should succeed")
.0;
let business_rules_check_after_change =
DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&original_subnet.id, &updated_subnet.nodes)
.expect("Business rules check after should succeed");
.expect("Business rules check before should succeed");
let penalties_after_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&original_subnet.id, &updated_subnet.nodes)
.expect("Business rules check after should succeed");

let subnet_change = SubnetChange {
subnet_id: original_subnet.id,
Expand All @@ -112,8 +110,7 @@ async fn get_decentralization_analysis(
removed_nodes: updated_subnet.removed_nodes.clone(),
added_nodes: updated_subnet.added_nodes.clone(),
penalties_before_change,
penalties_after_change: business_rules_check_after_change.0,
business_rules_log: business_rules_check_after_change.1,
penalties_after_change,
comment: updated_subnet.comment.clone(),
run_log: updated_subnet.run_log.clone(),
};
Expand Down
8 changes: 3 additions & 5 deletions rs/ic-management-backend/src/subnets.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@ pub fn get_proposed_subnet_changes(
let proposal: &TopologyChangeProposal = proposal;

let penalties_before_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&subnet.principal, &subnet.nodes)
.expect("Business rules check should succeed")
.0;
let business_rules_check_after_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&subnet.principal, &subnet.nodes)
.expect("Business rules check should succeed");
let penalties_after_change = DecentralizedSubnet::check_business_rules_for_subnet_with_nodes(&subnet.principal, &subnet.nodes)
.expect("Business rules check should succeed");

let change = SubnetChange {
Expand All @@ -27,8 +26,7 @@ pub fn get_proposed_subnet_changes(
removed_nodes: vec![],
added_nodes: vec![],
penalties_before_change,
penalties_after_change: business_rules_check_after_change.0,
business_rules_log: business_rules_check_after_change.1,
penalties_after_change,
comment: None,
run_log: vec![],
}
Expand Down

0 comments on commit 17d9909

Please sign in to comment.