From 49f44ff357f7b93bc3f4400c49d207220029faa3 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Mon, 25 Feb 2019 20:49:15 +0400 Subject: [PATCH 01/10] Choose pivot node at random --- tests/net_dynamic_hb.rs | 92 +++++++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 40 deletions(-) diff --git a/tests/net_dynamic_hb.rs b/tests/net_dynamic_hb.rs index a569863f..ac06c8c4 100644 --- a/tests/net_dynamic_hb.rs +++ b/tests/net_dynamic_hb.rs @@ -42,6 +42,18 @@ where queue[0..n].choose_multiple(rng, k).cloned().collect() } +/// Chooses one correct validator at random and returns its id. +fn choose_rnd_pivot_node<'a, R: rand::Rng>( + rng: &mut R, + correct_nodes: impl Iterator>, +) -> usize { + correct_nodes + .collect::>() + .choose(rng) + .map(|n| *n.id()) + .expect("expected at least one correct node") +} + /// Test configuration for dynamic honey badger tests. #[derive(Debug)] struct TestConfig { @@ -72,15 +84,13 @@ prop_compose! { } } -/// Proptest wrapper for `do_drop_and_readd`. +/// Proptest wrapper for `do_drop_and_re_add`. proptest! { - #![proptest_config(ProptestConfig { - cases: 1, .. ProptestConfig::default() - })] + #![proptest_config(ProptestConfig::with_cases(1))] #[test] #[allow(clippy::unnecessary_operation)] - fn drop_and_readd(cfg in arb_config()) { - do_drop_and_readd(cfg) + fn drop_and_re_add(cfg in arb_config()) { + do_drop_and_re_add(cfg) } } @@ -88,7 +98,7 @@ proptest! { /// running a regular honey badger network. // TODO: Add an observer node to the test network. #[allow(clippy::needless_pass_by_value, clippy::cyclomatic_complexity)] -fn do_drop_and_readd(cfg: TestConfig) { +fn do_drop_and_re_add(cfg: TestConfig) { let mut rng: TestRng = TestRng::from_seed(cfg.seed); // First, we create a new test network with Honey Badger instances. @@ -119,15 +129,9 @@ fn do_drop_and_readd(cfg: TestConfig) { let mut state = TestState::new(net); - // We will use the first correct node as the node we will remove from and re-add to the network. - // Note: This should be randomized using proptest. - let pivot_node_id: usize = *(state - .net - .correct_nodes() - .nth(0) - .expect("expected at least one correct node") - .id()); - println!("Will remove and readd node #{}", pivot_node_id); + // We will use a random correct node as the node we will remove from and re-add to the network. + let pivot_node_id = choose_rnd_pivot_node(&mut rng, state.net.correct_nodes()); + println!("Will remove and re-add node #{}", pivot_node_id); // We generate a list of transaction we want to propose, for each node. All nodes will propose // a number between 0..total_txs, chosen randomly. @@ -156,8 +160,8 @@ fn do_drop_and_readd(cfg: TestConfig) { .expect("pivot node missing") .algorithm() .algo() - .netinfo() - .clone(); + .netinfo(); + let pub_keys_add = netinfo.public_key_map().clone(); let mut pub_keys_rm = pub_keys_add.clone(); pub_keys_rm.remove(&pivot_node_id); @@ -186,11 +190,6 @@ fn do_drop_and_readd(cfg: TestConfig) { .map(|n| (*n.id(), (0..10).collect())) .collect(); let mut received_batches: BTreeMap = BTreeMap::new(); - // Whether node 0 was rejoined as a validator. - let mut rejoined_pivot_node = false; - // The removed pivot node which is to be restarted as soon as all remaining validators agree to - // add it back. - let mut saved_node: Option> = None; // Run the network: loop { @@ -249,7 +248,7 @@ fn do_drop_and_readd(cfg: TestConfig) { ChangeState::InProgress(Change::NodeChange(ref pub_keys)) if *pub_keys == pub_keys_add => { - println!("Node {} is progressing with readding.", node_id); + println!("Node {} is progressing with re-adding.", node_id); awaiting_addition_in_progress.remove(&node_id); } @@ -258,12 +257,13 @@ fn do_drop_and_readd(cfg: TestConfig) { { println!("Node {} done adding.", node_id); // Node added, ensure it has been removed first. - if awaiting_removal.contains(&node_id) { - panic!( - "Node {} reported a success `Add({}, _)` before `Remove({})`", - node_id, pivot_node_id, pivot_node_id - ); - } + assert!( + !awaiting_removal.contains(&node_id), + "Node {} reported a success `Add({}, _)` before `Remove({})`", + node_id, + pivot_node_id, + pivot_node_id + ); awaiting_addition.remove(&node_id); } _ => { @@ -289,7 +289,7 @@ fn do_drop_and_readd(cfg: TestConfig) { .expect("failed to send `Add` input"); assert!(step.output.is_empty()); awaiting_addition_input.remove(&node_id); - println!("Node {} started readding.", node_id); + println!("Node {} started re-adding.", node_id); } // Record whether or not we received some output. @@ -334,7 +334,7 @@ fn do_drop_and_readd(cfg: TestConfig) { // participant. if node_id != pivot_node_id && awaiting_removal.is_empty() - && !rejoined_pivot_node + && !state.rejoined_pivot_node { expected_outputs .get_mut(&pivot_node_id) @@ -345,7 +345,9 @@ fn do_drop_and_readd(cfg: TestConfig) { } // If this is the first batch from a correct node with a vote to add node 0 back, take // the join plan of the batch and use it to restart node 0. - if !rejoined_pivot_node && !state.net[node_id].is_faulty() && state.join_plan.is_none() + if !state.rejoined_pivot_node + && !state.net[node_id].is_faulty() + && state.join_plan.is_none() { if let ChangeState::InProgress(Change::NodeChange(pub_keys)) = batch.change() { if *pub_keys == pub_keys_add { @@ -358,29 +360,32 @@ fn do_drop_and_readd(cfg: TestConfig) { } } // Restart the pivot node having checked that it can be correctly restarted. - if !rejoined_pivot_node && awaiting_addition_in_progress.is_empty() { + if !state.rejoined_pivot_node && awaiting_addition_in_progress.is_empty() { if let Some(join_plan) = state.join_plan.take() { - let node = saved_node.take().expect("the pivot node wasn't saved"); + let node = state + .saved_node + .take() + .expect("the pivot node wasn't saved"); let step = restart_node_for_add(&mut state.net, node, join_plan, &mut rng); state .net .process_step(pivot_node_id, &step) .expect("processing a step failed"); - rejoined_pivot_node = true; + state.rejoined_pivot_node = true; } } } // Decide - from the point of view of the pivot node - whether it is ready to go offline. - if !rejoined_pivot_node - && saved_node.is_none() + if !state.rejoined_pivot_node + && state.saved_node.is_none() && state.net[pivot_node_id].algorithm().is_removed() { println!( "Removing the pivot node {} from the test network.", pivot_node_id ); - saved_node = state.net.remove_node(&pivot_node_id); + state.saved_node = state.net.remove_node(&pivot_node_id); if node_id == pivot_node_id { // Further operations on the cranked node are not possible. Continue with // processing other nodes. @@ -455,10 +460,15 @@ where { /// The test network. net: VirtualNet, - /// The join plan for readding the pivot node. + /// The join plan for re-adding the pivot node. join_plan: Option>, /// The epoch in which the pivot node should go offline. shutdown_epoch: Option, + /// The removed pivot node which is to be restarted as soon as all remaining validators agree to + /// add it back. + saved_node: Option>, + /// Whether pivot node was rejoined as a validator. + rejoined_pivot_node: bool, } impl TestState @@ -471,6 +481,8 @@ where net, join_plan: None, shutdown_epoch: None, + saved_node: None, + rejoined_pivot_node: false, } } } From 3be837d83ba7ebf2e9e86444deed4c9db0928a08 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Tue, 26 Feb 2019 15:15:07 +0400 Subject: [PATCH 02/10] Choose random number of nodes for removing in net_dynamic_hb test --- tests/net/mod.rs | 10 ++ tests/net_dynamic_hb.rs | 324 ++++++++++++++++++++++++---------------- 2 files changed, 204 insertions(+), 130 deletions(-) diff --git a/tests/net/mod.rs b/tests/net/mod.rs index 2a3978a9..9d4894f6 100644 --- a/tests/net/mod.rs +++ b/tests/net/mod.rs @@ -657,6 +657,16 @@ where self.nodes.remove(id) } + /// Removes a set of nodes with the given IDs from the network and all messages addressed to + /// them. Returns the removed nodes if there were nodes with this IDs at the time of removal. + #[inline] + pub fn remove_nodes(&mut self, ids: &BTreeSet) -> Vec> { + ids.iter() + .map(|id| self.remove_node(id)) + .filter_map(|x| x) + .collect() + } + /// Retrieve a node by ID. /// /// Returns `None` if the node ID is not part of the network. diff --git a/tests/net_dynamic_hb.rs b/tests/net_dynamic_hb.rs index ac06c8c4..8bf0f100 100644 --- a/tests/net_dynamic_hb.rs +++ b/tests/net_dynamic_hb.rs @@ -12,6 +12,7 @@ use rand::{seq::SliceRandom, SeedableRng}; use crate::net::adversary::{Adversary, ReorderingAdversary}; use crate::net::proptest::{gen_seed, NetworkDimension, TestRng, TestRngSeed}; use crate::net::{NetBuilder, NewNodeInfo, Node, VirtualNet}; +use threshold_crypto::PublicKey; type DHB = SenderQueue, usize>>; @@ -42,18 +43,6 @@ where queue[0..n].choose_multiple(rng, k).cloned().collect() } -/// Chooses one correct validator at random and returns its id. -fn choose_rnd_pivot_node<'a, R: rand::Rng>( - rng: &mut R, - correct_nodes: impl Iterator>, -) -> usize { - correct_nodes - .collect::>() - .choose(rng) - .map(|n| *n.id()) - .expect("expected at least one correct node") -} - /// Test configuration for dynamic honey badger tests. #[derive(Debug)] struct TestConfig { @@ -129,9 +118,8 @@ fn do_drop_and_re_add(cfg: TestConfig) { let mut state = TestState::new(net); - // We will use a random correct node as the node we will remove from and re-add to the network. - let pivot_node_id = choose_rnd_pivot_node(&mut rng, state.net.correct_nodes()); - println!("Will remove and re-add node #{}", pivot_node_id); + let nodes_for_remove = state.subset_for_remove(&mut rng); + println!("Will remove and re-add nodes {:?}", nodes_for_remove); // We generate a list of transaction we want to propose, for each node. All nodes will propose // a number between 0..total_txs, chosen randomly. @@ -153,41 +141,32 @@ fn do_drop_and_re_add(cfg: TestConfig) { .expect("could not send initial transaction"); } - // Afterwards, remove a specific node from the dynamic honey badger network. - let netinfo = state - .net - .get(pivot_node_id) - .expect("pivot node missing") - .algorithm() - .algo() - .netinfo(); - - let pub_keys_add = netinfo.public_key_map().clone(); - let mut pub_keys_rm = pub_keys_add.clone(); - pub_keys_rm.remove(&pivot_node_id); + // Afterwards, remove a specific nodes from the dynamic honey badger network. + let old_pub_keys = state.get_pub_keys(); + let new_pub_keys: BTreeMap = old_pub_keys + .clone() + .into_iter() + .filter(|(id, _)| !nodes_for_remove.contains(id)) + .collect(); state .net .broadcast_input( - &Input::Change(Change::NodeChange(pub_keys_rm.clone())), + &Input::Change(Change::NodeChange(new_pub_keys.clone())), &mut rng, ) .expect("broadcasting failed"); // We are tracking (correct) nodes' state through the process by ticking them off individually. - let non_pivot_nodes: BTreeSet<_> = state - .net - .correct_nodes() - .map(|n| *n.id()) - .filter(|id| *id != pivot_node_id) - .collect(); - let mut awaiting_removal: BTreeSet<_> = state.net.correct_nodes().map(|n| *n.id()).collect(); - let mut awaiting_addition_input: BTreeSet<_> = non_pivot_nodes.clone(); - let mut awaiting_addition_in_progress: BTreeSet<_> = non_pivot_nodes.clone(); - let mut awaiting_addition: BTreeSet<_> = awaiting_removal.clone(); - let mut expected_outputs: BTreeMap<_, BTreeSet<_>> = state - .net - .correct_nodes() - .map(|n| (*n.id(), (0..10).collect())) + let correct_nodes: BTreeSet<_> = state.net.correct_nodes().map(|n| *n.id()).collect(); + let non_rm_nodes = &correct_nodes - &nodes_for_remove; + + let mut awaiting_apply_new_subset: BTreeSet<_> = correct_nodes.clone(); + let mut awaiting_apply_old_subset: BTreeSet<_> = correct_nodes.clone(); + let mut awaiting_apply_old_subset_input: BTreeSet<_> = non_rm_nodes.clone(); + let mut awaiting_apply_old_subset_in_progress: BTreeSet<_> = non_rm_nodes.clone(); + let mut expected_outputs: BTreeMap<_, BTreeSet<_>> = correct_nodes + .iter() + .map(|id| (id, (0..10).collect())) .collect(); let mut received_batches: BTreeMap = BTreeMap::new(); @@ -205,15 +184,16 @@ fn do_drop_and_re_add(cfg: TestConfig) { batch.epoch() ); } - let expected_participants: Vec<_> = if awaiting_removal.contains(&node_id) { - // The node hasn't removed the pivot node yet. - pub_keys_add.keys() - } else if awaiting_addition.contains(&node_id) { - // The node has removed the pivot node but hasn't added it back yet. - pub_keys_rm.keys() + let expected_participants: Vec<_> = if awaiting_apply_new_subset.contains(&node_id) + { + // The node hasn't applied a new subset of nodes yet. + old_pub_keys.keys() + } else if awaiting_apply_old_subset.contains(&node_id) { + // The node has applied a new subset of nodes. + new_pub_keys.keys() } else { - // The node has added the pivot node back. - pub_keys_add.keys() + // The node has applied the original (previous) subset of nodes back. + old_pub_keys.keys() } .collect(); assert!( @@ -238,33 +218,34 @@ fn do_drop_and_re_add(cfg: TestConfig) { for change in step.output.iter().map(|output| output.change()) { match change { ChangeState::Complete(Change::NodeChange(ref pub_keys)) - if *pub_keys == pub_keys_rm => + if *pub_keys == new_pub_keys => { - println!("Node {} done removing.", node_id); - // Removal complete, tally: - awaiting_removal.remove(&node_id); + println!("Node {} done applying the new subset.", node_id); + // Applying a new subset complete, tally: + awaiting_apply_new_subset.remove(&node_id); } ChangeState::InProgress(Change::NodeChange(ref pub_keys)) - if *pub_keys == pub_keys_add => + if *pub_keys == old_pub_keys => { - println!("Node {} is progressing with re-adding.", node_id); - awaiting_addition_in_progress.remove(&node_id); + println!( + "Node {} is progressing for applying the old subset.", + node_id + ); + awaiting_apply_old_subset_in_progress.remove(&node_id); } ChangeState::Complete(Change::NodeChange(ref pub_keys)) - if *pub_keys == pub_keys_add => + if *pub_keys == old_pub_keys => { - println!("Node {} done adding.", node_id); - // Node added, ensure it has been removed first. + println!("Node {} done applying the old subset back.", node_id); + // Node has applied the old subset, ensure it has applied the new subset previously. assert!( - !awaiting_removal.contains(&node_id), - "Node {} reported a success `Add({}, _)` before `Remove({})`", + !awaiting_apply_new_subset.contains(&node_id), + "Node {} reported a success applying the old subset before applying the new subset.", node_id, - pivot_node_id, - pivot_node_id ); - awaiting_addition.remove(&node_id); + awaiting_apply_old_subset.remove(&node_id); } _ => { println!("Unhandled change: {:?}", change); @@ -273,23 +254,23 @@ fn do_drop_and_re_add(cfg: TestConfig) { } let (era, hb_epoch) = state.net[node_id].algorithm().algo().epoch(); - if node_id != pivot_node_id - && awaiting_addition_input.contains(&node_id) + if !nodes_for_remove.contains(&node_id) + && awaiting_apply_old_subset_input.contains(&node_id) && state.shutdown_epoch.is_some() && era + hb_epoch >= state.shutdown_epoch.unwrap() { - // Now we can add the node again. Public keys will be reused. + // Now we apply old subset of node back. Public keys will be reused. let step = state .net .send_input( node_id, - Input::Change(Change::NodeChange(pub_keys_add.clone())), + Input::Change(Change::NodeChange(old_pub_keys.clone())), &mut rng, ) - .expect("failed to send `Add` input"); + .expect("failed to send `apply old subset` input"); assert!(step.output.is_empty()); - awaiting_addition_input.remove(&node_id); - println!("Node {} started re-adding.", node_id); + awaiting_apply_old_subset_input.remove(&node_id); + println!("Node {} started to apply old subset.", node_id); } // Record whether or not we received some output. @@ -308,10 +289,10 @@ fn do_drop_and_re_add(cfg: TestConfig) { node_id, ); - // If this is a batch removing the pivot node, record the epoch in which the pivot node - // will shut down. + // If this is a batch applying the new subset of nodes, record the epoch + // in which 'nodes_for_remove' will shut down. if let ChangeState::Complete(Change::NodeChange(ref pub_keys)) = batch.change() { - if *pub_keys == pub_keys_rm { + if *pub_keys == new_pub_keys { state.shutdown_epoch = Some(batch.epoch() + 1); } } @@ -329,28 +310,30 @@ fn do_drop_and_re_add(cfg: TestConfig) { .get_mut(&node_id) .expect("output set disappeared") .remove(tx); - // Also delete expected output from the pivot node if that node is currently - // removed. It does not output any values in epochs in which it is not a - // participant. - if node_id != pivot_node_id - && awaiting_removal.is_empty() - && !state.rejoined_pivot_node + // Also, delete expected output from the 'nodes_for_remove' if those nodes are + // currently removed. They do not output any values in epochs in which theirs are + // not a participant. + if !nodes_for_remove.contains(&node_id) + && awaiting_apply_new_subset.is_empty() + && !state.old_subset_applied { - expected_outputs - .get_mut(&pivot_node_id) - .expect("pivot node output set disappeared") - .remove(tx); + nodes_for_remove.iter().for_each(|id| { + expected_outputs + .get_mut(&id) + .expect(&format!("output set disappeared for node {}", id)) + .remove(tx); + }); } } } - // If this is the first batch from a correct node with a vote to add node 0 back, take - // the join plan of the batch and use it to restart node 0. - if !state.rejoined_pivot_node + // If this is the first batch from a correct node with a vote to apply the old subset + // back, take the join plan of the batch and use it to restart removed nodes. + if !state.old_subset_applied && !state.net[node_id].is_faulty() && state.join_plan.is_none() { if let ChangeState::InProgress(Change::NodeChange(pub_keys)) = batch.change() { - if *pub_keys == pub_keys_add { + if *pub_keys == old_pub_keys { state.join_plan = Some( batch .join_plan() @@ -359,34 +342,43 @@ fn do_drop_and_re_add(cfg: TestConfig) { } } } - // Restart the pivot node having checked that it can be correctly restarted. - if !state.rejoined_pivot_node && awaiting_addition_in_progress.is_empty() { + // Restart removed nodes having checked that theirs can be correctly restarted. + if !state.old_subset_applied && awaiting_apply_old_subset_in_progress.is_empty() { if let Some(join_plan) = state.join_plan.take() { - let node = state - .saved_node - .take() - .expect("the pivot node wasn't saved"); - let step = restart_node_for_add(&mut state.net, node, join_plan, &mut rng); - state - .net - .process_step(pivot_node_id, &step) - .expect("processing a step failed"); - state.rejoined_pivot_node = true; + let saved_nodes: Vec<_> = state.saved_nodes.drain(..).collect(); + + assert!(!saved_nodes.is_empty(), "removed nodes wasn't saved"); + + saved_nodes.into_iter().for_each(|node| { + let node_id = *node.id(); + let step = + restart_node_for_add(&mut state.net, node, join_plan.clone(), &mut rng); + state + .net + .process_step(node_id, &step) + .expect("processing a step failed"); + }); + state.old_subset_applied = true; } } } - // Decide - from the point of view of the pivot node - whether it is ready to go offline. - if !state.rejoined_pivot_node - && state.saved_node.is_none() - && state.net[pivot_node_id].algorithm().is_removed() + let all_removed = |nodes: &BTreeSet| { + nodes + .iter() + .all(|id| state.net[*id].algorithm().is_removed()) + }; + // Decide - from the point of view of removed nodes - whether their are ready to go offline. + if !state.old_subset_applied + && state.saved_nodes.is_empty() + && all_removed(&nodes_for_remove) { println!( - "Removing the pivot node {} from the test network.", - pivot_node_id + "Removing nodes {:?} from the test network.", + nodes_for_remove ); - state.saved_node = state.net.remove_node(&pivot_node_id); - if node_id == pivot_node_id { + state.saved_nodes = state.net.remove_nodes(&nodes_for_remove); + if nodes_for_remove.contains(&node_id) { // Further operations on the cranked node are not possible. Continue with // processing other nodes. continue; @@ -395,10 +387,11 @@ fn do_drop_and_re_add(cfg: TestConfig) { // Check if we are done. if expected_outputs.values().all(|s| s.is_empty()) - && awaiting_addition.is_empty() - && awaiting_removal.is_empty() + && awaiting_apply_old_subset.is_empty() + && awaiting_apply_new_subset.is_empty() { - // All outputs are empty and all nodes have removed and added the single pivot node. + // All outputs are empty, the old subset was applied back after that + // new subset was applied. break; } @@ -415,18 +408,24 @@ fn do_drop_and_re_add(cfg: TestConfig) { } } - // As a final step, we verify that all nodes have arrived at the same conclusion. The pivot node - // can miss some batches while it was removed. - let full_node = state + // As a final step, we verify that all nodes have arrived at the same conclusion. + // Removed nodes can miss some batches while there were removed. + let result: Vec<_> = state .net .correct_nodes() - .find(|node| *node.id() != pivot_node_id) - .expect("Could not find a full node"); - state.net.verify_batches(&full_node); - println!("End result: {:?}", full_node.outputs()); + .filter(|node| !nodes_for_remove.contains(node.id())) + .map(|node| { + state.net.verify_batches(&node); + node.outputs() + }) + .collect(); + + assert!(!result.is_empty(), "Could not find a full node"); + + println!("End result: {:?}", result); } -/// Restarts node 0 on the test network for adding it back as a validator. +/// Restarts specified node on the test network for adding it back as a validator. fn restart_node_for_add( net: &mut VirtualNet, mut node: Node, @@ -448,7 +447,7 @@ where let step = node .algorithm_mut() .restart(join_plan, peer_ids.into_iter(), rng) - .expect("failed to restart pivot node"); + .expect("failed to restart the node"); net.insert_node(node); step } @@ -460,15 +459,15 @@ where { /// The test network. net: VirtualNet, - /// The join plan for re-adding the pivot node. + /// The join plan for adding nodes. join_plan: Option>, - /// The epoch in which the pivot node should go offline. + /// The epoch in which the removed nodes should go offline. shutdown_epoch: Option, - /// The removed pivot node which is to be restarted as soon as all remaining validators agree to - /// add it back. - saved_node: Option>, - /// Whether pivot node was rejoined as a validator. - rejoined_pivot_node: bool, + /// The removed nodes which are to be restarted as soon as all remaining + /// validators agree to add their back. + saved_nodes: Vec>, + /// Whether the old subset of validators was applied back to the network. + old_subset_applied: bool, } impl TestState @@ -481,8 +480,73 @@ where net, join_plan: None, shutdown_epoch: None, - saved_node: None, - rejoined_pivot_node: false, + saved_nodes: Vec::new(), + old_subset_applied: false, } } + + /// Selects random subset of validators which can be safely removed from the network + /// + /// The cluster always remain correct after removing this subset from the custer. + /// This method may select as correct nodes as malicious. + fn subset_for_remove(&self, rng: &mut R) -> BTreeSet + where + R: rand::Rng, + { + let net = &self.net; + let (faulty, correct): (Vec<_>, Vec<_>) = net.nodes().partition(|n| n.is_faulty()); + + let f = faulty.len(); + let n = correct.len() + f; + + assert!( + n >= f * 3 + 1, + "the network is already captured by the faulty nodes" + ); + + let max_number_correct_nodes_for_remove = n - (f * 3 + 1); + let remove_from_correct = rng.gen_range(0, max_number_correct_nodes_for_remove + 1); + let remove_from_faulty = if remove_from_correct == 0 && f > 0 { + // if we can't remove any correct node, because network will become in an + // incorrect state we have to remove at least 1 faulty node + rng.gen_range(1, f + 1) + } else { + rng.gen_range(0, f + 1) + }; + + let result: BTreeSet = correct + .choose_multiple(rng, remove_from_correct) + .map(|n| *n.id()) + .chain( + faulty + .choose_multiple(rng, remove_from_faulty) + .map(|n| *n.id()), + ) + .collect(); + + assert!( + !result.is_empty(), + "subset for remove should have at least one node" + ); + + println!( + "Max number of nodes for removing is {}, was chosen {} nodes", + max_number_correct_nodes_for_remove + f, + result.len() + ); + + result + } + + /// Returns clone of all public keys for this network. + fn get_pub_keys(&self) -> BTreeMap { + self.net + .get(0) + .expect("network should have at least one node") + .algorithm() + .algo() + .netinfo() + .public_key_map() + .clone() + } } From 8e71da6528667eff2c9888d1a4dc6e126fa584e9 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Tue, 26 Feb 2019 17:26:55 +0400 Subject: [PATCH 03/10] Docs and code small fixes --- tests/net/mod.rs | 3 +-- tests/net_dynamic_hb.rs | 7 +++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/net/mod.rs b/tests/net/mod.rs index 9d4894f6..883fb576 100644 --- a/tests/net/mod.rs +++ b/tests/net/mod.rs @@ -662,8 +662,7 @@ where #[inline] pub fn remove_nodes(&mut self, ids: &BTreeSet) -> Vec> { ids.iter() - .map(|id| self.remove_node(id)) - .filter_map(|x| x) + .filter_map(|id| self.remove_node(id)) .collect() } diff --git a/tests/net_dynamic_hb.rs b/tests/net_dynamic_hb.rs index 8bf0f100..8cd39bd3 100644 --- a/tests/net_dynamic_hb.rs +++ b/tests/net_dynamic_hb.rs @@ -192,7 +192,7 @@ fn do_drop_and_re_add(cfg: TestConfig) { // The node has applied a new subset of nodes. new_pub_keys.keys() } else { - // The node has applied the original (previous) subset of nodes back. + // The node has applied the old (previous) subset of nodes back. old_pub_keys.keys() } .collect(); @@ -220,7 +220,7 @@ fn do_drop_and_re_add(cfg: TestConfig) { ChangeState::Complete(Change::NodeChange(ref pub_keys)) if *pub_keys == new_pub_keys => { - println!("Node {} done applying the new subset.", node_id); + println!("Node {} done applying a new subset.", node_id); // Applying a new subset complete, tally: awaiting_apply_new_subset.remove(&node_id); } @@ -320,8 +320,7 @@ fn do_drop_and_re_add(cfg: TestConfig) { nodes_for_remove.iter().for_each(|id| { expected_outputs .get_mut(&id) - .expect(&format!("output set disappeared for node {}", id)) - .remove(tx); + .map(|output| output.remove(tx)); }); } } From d58368efaba9f8f0cbd7c291ffdafcbeaf520292 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Tue, 26 Feb 2019 18:15:06 +0400 Subject: [PATCH 04/10] clippy fix --- tests/net_dynamic_hb.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/net_dynamic_hb.rs b/tests/net_dynamic_hb.rs index 8cd39bd3..ae437b73 100644 --- a/tests/net_dynamic_hb.rs +++ b/tests/net_dynamic_hb.rs @@ -499,7 +499,7 @@ where let n = correct.len() + f; assert!( - n >= f * 3 + 1, + n > f * 3, "the network is already captured by the faulty nodes" ); From d082f699733b828b6e6cd34433ac1757b207b42a Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Tue, 26 Feb 2019 18:33:35 +0400 Subject: [PATCH 05/10] Cargo fmt for stable toolchain and add rust-toolchain file as well --- rust-toolchain | 1 + tests/net/mod.rs | 4 +--- 2 files changed, 2 insertions(+), 3 deletions(-) create mode 100644 rust-toolchain diff --git a/rust-toolchain b/rust-toolchain new file mode 100644 index 00000000..2bf5ad04 --- /dev/null +++ b/rust-toolchain @@ -0,0 +1 @@ +stable diff --git a/tests/net/mod.rs b/tests/net/mod.rs index 883fb576..c0960e09 100644 --- a/tests/net/mod.rs +++ b/tests/net/mod.rs @@ -661,9 +661,7 @@ where /// them. Returns the removed nodes if there were nodes with this IDs at the time of removal. #[inline] pub fn remove_nodes(&mut self, ids: &BTreeSet) -> Vec> { - ids.iter() - .filter_map(|id| self.remove_node(id)) - .collect() + ids.iter().filter_map(|id| self.remove_node(id)).collect() } /// Retrieve a node by ID. From 884fd20be575409e80dd05d35d64df313e01aaf7 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Tue, 26 Feb 2019 18:43:22 +0400 Subject: [PATCH 06/10] Remove rust-toolchain file --- rust-toolchain | 1 - 1 file changed, 1 deletion(-) delete mode 100644 rust-toolchain diff --git a/rust-toolchain b/rust-toolchain deleted file mode 100644 index 2bf5ad04..00000000 --- a/rust-toolchain +++ /dev/null @@ -1 +0,0 @@ -stable From 03439c835d613d7d4174c5c5545019093c1145a7 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Tue, 26 Feb 2019 20:56:13 +0400 Subject: [PATCH 07/10] Fix grammar and improve selecting nodes for removing --- tests/net/mod.rs | 2 +- tests/net_dynamic_hb.rs | 39 ++++++++++++++++++++++++--------------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/tests/net/mod.rs b/tests/net/mod.rs index c0960e09..8a00a14d 100644 --- a/tests/net/mod.rs +++ b/tests/net/mod.rs @@ -658,7 +658,7 @@ where } /// Removes a set of nodes with the given IDs from the network and all messages addressed to - /// them. Returns the removed nodes if there were nodes with this IDs at the time of removal. + /// them. Returns the removed nodes if there were nodes with these IDs at the time of removal. #[inline] pub fn remove_nodes(&mut self, ids: &BTreeSet) -> Vec> { ids.iter().filter_map(|id| self.remove_node(id)).collect() diff --git a/tests/net_dynamic_hb.rs b/tests/net_dynamic_hb.rs index ae437b73..d8d6acf0 100644 --- a/tests/net_dynamic_hb.rs +++ b/tests/net_dynamic_hb.rs @@ -141,7 +141,7 @@ fn do_drop_and_re_add(cfg: TestConfig) { .expect("could not send initial transaction"); } - // Afterwards, remove a specific nodes from the dynamic honey badger network. + // Afterwards, remove specific nodes from the dynamic honey badger network. let old_pub_keys = state.get_pub_keys(); let new_pub_keys: BTreeMap = old_pub_keys .clone() @@ -341,7 +341,7 @@ fn do_drop_and_re_add(cfg: TestConfig) { } } } - // Restart removed nodes having checked that theirs can be correctly restarted. + // Restart removed nodes having checked that they can be correctly restarted. if !state.old_subset_applied && awaiting_apply_old_subset_in_progress.is_empty() { if let Some(join_plan) = state.join_plan.take() { let saved_nodes: Vec<_> = state.saved_nodes.drain(..).collect(); @@ -367,7 +367,7 @@ fn do_drop_and_re_add(cfg: TestConfig) { .iter() .all(|id| state.net[*id].algorithm().is_removed()) }; - // Decide - from the point of view of removed nodes - whether their are ready to go offline. + // Decide - from the point of view of removed nodes - whether they are ready to go offline. if !state.old_subset_applied && state.saved_nodes.is_empty() && all_removed(&nodes_for_remove) @@ -408,7 +408,7 @@ fn do_drop_and_re_add(cfg: TestConfig) { } // As a final step, we verify that all nodes have arrived at the same conclusion. - // Removed nodes can miss some batches while there were removed. + // Removed nodes can miss some batches while they were removed. let result: Vec<_> = state .net .correct_nodes() @@ -463,7 +463,7 @@ where /// The epoch in which the removed nodes should go offline. shutdown_epoch: Option, /// The removed nodes which are to be restarted as soon as all remaining - /// validators agree to add their back. + /// validators agree to add them back. saved_nodes: Vec>, /// Whether the old subset of validators was applied back to the network. old_subset_applied: bool, @@ -484,10 +484,10 @@ where } } - /// Selects random subset of validators which can be safely removed from the network + /// Selects random subset of validators which can be safely removed from the network. /// - /// The cluster always remain correct after removing this subset from the custer. - /// This method may select as correct nodes as malicious. + /// The cluster always remain correct after removing this subset from the cluster. + /// This method may select correct nodes as well as malicious ones. fn subset_for_remove(&self, rng: &mut R) -> BTreeSet where R: rand::Rng, @@ -503,16 +503,22 @@ where "the network is already captured by the faulty nodes" ); - let max_number_correct_nodes_for_remove = n - (f * 3 + 1); - let remove_from_correct = rng.gen_range(0, max_number_correct_nodes_for_remove + 1); - let remove_from_faulty = if remove_from_correct == 0 && f > 0 { - // if we can't remove any correct node, because network will become in an - // incorrect state we have to remove at least 1 faulty node + let max_number_correct_nodes_for_remove = |n: usize, f: usize| n - (f * 3 + 1); + + let remove_from_faulty = if max_number_correct_nodes_for_remove(n, f) == 0 && f > 0 { + // we can't remove any correct node now, because network will become in an incorrect state + // but we will be able to remove correct nodes after removing several malicious nodes, + // so at least one malicious node should be guaranteed removed rng.gen_range(1, f + 1) } else { rng.gen_range(0, f + 1) }; + let new_f = f - remove_from_faulty; + let new_n = n - remove_from_faulty; + let remove_from_correct = + rng.gen_range(0, max_number_correct_nodes_for_remove(new_n, new_f) + 1); + let result: BTreeSet = correct .choose_multiple(rng, remove_from_correct) .map(|n| *n.id()) @@ -527,10 +533,13 @@ where !result.is_empty(), "subset for remove should have at least one node" ); - + assert!( + n - result.len() > 0, + "can't remove all nodes from the network, at least one node remain" + ); println!( "Max number of nodes for removing is {}, was chosen {} nodes", - max_number_correct_nodes_for_remove + f, + max_number_correct_nodes_for_remove(new_n, new_f) + new_f, result.len() ); From 5ec29452f6905d4b067d530b1d5ef3fd4d3eb44e Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Wed, 27 Feb 2019 14:47:00 +0400 Subject: [PATCH 08/10] Simplify selecting nodes for remove --- tests/net_dynamic_hb.rs | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/tests/net_dynamic_hb.rs b/tests/net_dynamic_hb.rs index d8d6acf0..18ee3465 100644 --- a/tests/net_dynamic_hb.rs +++ b/tests/net_dynamic_hb.rs @@ -12,6 +12,7 @@ use rand::{seq::SliceRandom, SeedableRng}; use crate::net::adversary::{Adversary, ReorderingAdversary}; use crate::net::proptest::{gen_seed, NetworkDimension, TestRng, TestRngSeed}; use crate::net::{NetBuilder, NewNodeInfo, Node, VirtualNet}; +use hbbft::util; use threshold_crypto::PublicKey; type DHB = SenderQueue, usize>>; @@ -503,21 +504,13 @@ where "the network is already captured by the faulty nodes" ); - let max_number_correct_nodes_for_remove = |n: usize, f: usize| n - (f * 3 + 1); + assert_ne!(n, 1, "Only one node left; nothing to remove"); - let remove_from_faulty = if max_number_correct_nodes_for_remove(n, f) == 0 && f > 0 { - // we can't remove any correct node now, because network will become in an incorrect state - // but we will be able to remove correct nodes after removing several malicious nodes, - // so at least one malicious node should be guaranteed removed - rng.gen_range(1, f + 1) - } else { - rng.gen_range(0, f + 1) - }; + let new_n = rng.gen_range(1, n); // new_n is between 1 and n-1 + let new_f = rng.gen_range(0, f.min(util::max_faulty(new_n)) + 1); - let new_f = f - remove_from_faulty; - let new_n = n - remove_from_faulty; - let remove_from_correct = - rng.gen_range(0, max_number_correct_nodes_for_remove(new_n, new_f) + 1); + let remove_from_faulty = f - new_f; + let remove_from_correct = n - new_n - remove_from_faulty; let result: BTreeSet = correct .choose_multiple(rng, remove_from_correct) @@ -533,15 +526,7 @@ where !result.is_empty(), "subset for remove should have at least one node" ); - assert!( - n - result.len() > 0, - "can't remove all nodes from the network, at least one node remain" - ); - println!( - "Max number of nodes for removing is {}, was chosen {} nodes", - max_number_correct_nodes_for_remove(new_n, new_f) + new_f, - result.len() - ); + println!("Was chosen {} nodes for removing.", result.len()); result } From 2124b71bb1209462d3a34a5445d2d37e4e9599e1 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Wed, 27 Feb 2019 19:29:21 +0400 Subject: [PATCH 09/10] Fix tests --- tests/net_dynamic_hb.proptest-regressions | 7 +++++++ tests/net_dynamic_hb.rs | 6 ++---- 2 files changed, 9 insertions(+), 4 deletions(-) create mode 100644 tests/net_dynamic_hb.proptest-regressions diff --git a/tests/net_dynamic_hb.proptest-regressions b/tests/net_dynamic_hb.proptest-regressions new file mode 100644 index 00000000..64ca3332 --- /dev/null +++ b/tests/net_dynamic_hb.proptest-regressions @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +xs 2003596573 965950084 3635999772 1934211882 diff --git a/tests/net_dynamic_hb.rs b/tests/net_dynamic_hb.rs index 18ee3465..018b15ef 100644 --- a/tests/net_dynamic_hb.rs +++ b/tests/net_dynamic_hb.rs @@ -504,9 +504,7 @@ where "the network is already captured by the faulty nodes" ); - assert_ne!(n, 1, "Only one node left; nothing to remove"); - - let new_n = rng.gen_range(1, n); // new_n is between 1 and n-1 + let new_n = rng.gen_range(2, n); // new_n is between 2 and n-1 let new_f = rng.gen_range(0, f.min(util::max_faulty(new_n)) + 1); let remove_from_faulty = f - new_f; @@ -526,7 +524,7 @@ where !result.is_empty(), "subset for remove should have at least one node" ); - println!("Was chosen {} nodes for removing.", result.len()); + println!("{} nodes were chosen for removing", result.len()); result } From dc972a63845cb7adcae6b13de7389b6a1a3a7ca1 Mon Sep 17 00:00:00 2001 From: "C.Solovev" Date: Wed, 27 Feb 2019 20:22:54 +0400 Subject: [PATCH 10/10] Remove net_dynamic_hb.proptest-regressions file --- tests/net_dynamic_hb.proptest-regressions | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 tests/net_dynamic_hb.proptest-regressions diff --git a/tests/net_dynamic_hb.proptest-regressions b/tests/net_dynamic_hb.proptest-regressions deleted file mode 100644 index 64ca3332..00000000 --- a/tests/net_dynamic_hb.proptest-regressions +++ /dev/null @@ -1,7 +0,0 @@ -# Seeds for failure cases proptest has generated in the past. It is -# automatically read and these particular cases re-run before any -# novel cases are generated. -# -# It is recommended to check this file in to source control so that -# everyone who runs the test benefits from these saved cases. -xs 2003596573 965950084 3635999772 1934211882