From d8a2bad3a18f9ba1ffad0537ea0f9095bbf6e075 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 26 Feb 2020 15:04:00 +0100 Subject: [PATCH 01/63] protocols/kad: Implement S-Kademlia's lookup over disjoint paths The extension paper S-Kademlia includes a proposal for lookups over disjoint paths. Within vanilla Kademlia, queries keep track of the closest nodes in a single bucket. Any adversary along the path can thus influence all future paths, in case they can come up with the next-closest (not overall closest) hops. S-Kademlia tries to solve the attack above by querying over disjoint paths using multiple buckets. To adjust the libp2p Kademlia implementation accordingly this change-set introduces an additional peers iterator: `DisjointClosestPeersIter`. This new iterator wraps around a set of `ClosestPeersIter` giving each of them a share of the allowed parallelism, amount of striven for results and set of initial nodes to contact. `DisjointClosestPeersIter` enforces that each of the `ClosestPeersIter` explore disjoint paths by having each peer instantly fail that was queried by a different iterator. --- protocols/kad/src/query.rs | 24 +- protocols/kad/src/query/peers.rs | 2 +- .../kad/src/query/peers/disjoint_closest.rs | 325 ++++++++++++++++++ 3 files changed, 349 insertions(+), 2 deletions(-) create mode 100644 protocols/kad/src/query/peers/disjoint_closest.rs diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index f00879a1ec6..183a0946cc7 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -22,6 +22,7 @@ mod peers; use peers::PeersIterState; use peers::closest::{ClosestPeersIter, ClosestPeersIterConfig}; +use peers::disjoint_closest::{DisjointClosestPeersIter, DisjointClosestPeersIterConfig}; use peers::fixed::FixedPeersIter; use crate::K_VALUE; @@ -111,6 +112,21 @@ impl QueryPool { self.add(peer_iter, inner) } + /// Adds a query to the pool that iterates towards the closest peers to the target on disjoint + /// paths. + pub fn add_iter_disjoint_closest(&mut self, target: T, peers: I, inner: TInner) -> QueryId + where + T: Into + Clone, + I: IntoIterator> + { + let cfg = DisjointClosestPeersIterConfig { + num_results: self.config.replication_factor.get(), + .. DisjointClosestPeersIterConfig::default() + }; + let peer_iter = QueryPeerIter::DisjointClosest(DisjointClosestPeersIter::with_config(cfg, target, peers)); + self.add(peer_iter, inner) + } + fn add(&mut self, peer_iter: QueryPeerIter, inner: TInner) -> QueryId { let id = QueryId(self.next_id); self.next_id = self.next_id.wrapping_add(1); @@ -216,6 +232,7 @@ pub struct Query { /// The peer selection strategies that can be used by queries. enum QueryPeerIter { Closest(ClosestPeersIter), + DisjointClosest(DisjointClosestPeersIter), Fixed(FixedPeersIter) } @@ -234,6 +251,7 @@ impl Query { pub fn on_failure(&mut self, peer: &PeerId) { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.on_failure(peer), + QueryPeerIter::DisjointClosest(iter) => iter.on_failure(peer), QueryPeerIter::Fixed(iter) => iter.on_failure(peer) } } @@ -247,6 +265,7 @@ impl Query { { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.on_success(peer, new_peers), + QueryPeerIter::DisjointClosest(iter) => iter.on_success(peer, new_peers), QueryPeerIter::Fixed(iter) => iter.on_success(peer) } } @@ -255,6 +274,7 @@ impl Query { pub fn is_waiting(&self, peer: &PeerId) -> bool { match &self.peer_iter { QueryPeerIter::Closest(iter) => iter.is_waiting(peer), + QueryPeerIter::DisjointClosest(iter) => iter.is_waiting(peer), QueryPeerIter::Fixed(iter) => iter.is_waiting(peer) } } @@ -263,6 +283,7 @@ impl Query { fn next(&mut self, now: Instant) -> PeersIterState { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.next(now), + QueryPeerIter::DisjointClosest(iter) => iter.next(now), QueryPeerIter::Fixed(iter) => iter.next() } } @@ -274,6 +295,7 @@ impl Query { pub fn finish(&mut self) { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.finish(), + QueryPeerIter::DisjointClosest(iter) => iter.finish(), QueryPeerIter::Fixed(iter) => iter.finish() } } @@ -282,6 +304,7 @@ impl Query { pub fn into_result(self) -> QueryResult> { let peers = match self.peer_iter { QueryPeerIter::Closest(iter) => Either::Left(iter.into_result()), + QueryPeerIter::DisjointClosest(iter) => unimplemented!(), QueryPeerIter::Fixed(iter) => Either::Right(iter.into_result()) }; QueryResult { inner: self.inner, peers } @@ -295,4 +318,3 @@ pub struct QueryResult { /// The successfully contacted peers. pub peers: TPeers } - diff --git a/protocols/kad/src/query/peers.rs b/protocols/kad/src/query/peers.rs index 97ad016635c..31fdc3b9594 100644 --- a/protocols/kad/src/query/peers.rs +++ b/protocols/kad/src/query/peers.rs @@ -39,6 +39,7 @@ //! [`Finished`]: peers::PeersIterState::Finished pub mod closest; +pub mod disjoint_closest; pub mod fixed; use libp2p_core::PeerId; @@ -65,4 +66,3 @@ pub enum PeersIterState<'a> { /// The iterator finished. Finished } - diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/disjoint_closest.rs new file mode 100644 index 00000000000..ca2f9cd8d19 --- /dev/null +++ b/protocols/kad/src/query/peers/disjoint_closest.rs @@ -0,0 +1,325 @@ +use super::*; +use crate::kbucket::{Key, KeyBytes}; +use crate::query::peers::closest::{ClosestPeersIter, ClosestPeersIterConfig}; +use crate::{ALPHA_VALUE, K_VALUE}; +use libp2p_core::PeerId; +use std::{collections::HashMap, time::Duration}; +use wasm_timer::Instant; + +// TODO: This duplicates a lot of documentation (See ClosestPeersIterConfig). +#[derive(Debug, Clone)] +pub struct DisjointClosestPeersIterConfig { + /// Allowed level of parallelism. + /// + /// The `α` parameter in the Kademlia paper. The maximum number of peers that + /// the iterator is allowed to wait for in parallel while iterating towards the closest + /// nodes to a target. Defaults to `ALPHA_VALUE`. + pub parallelism: usize, + + // TODO: Document that it needs to be equal or larger than parallelism? + pub disjoint_paths: usize, + + /// Number of results (closest peers) to search for. + /// + /// The number of closest peers for which the iterator must obtain successful results + /// in order to finish successfully. Defaults to `K_VALUE`. + pub num_results: usize, + + /// The timeout for a single peer. + /// + /// If a successful result is not reported for a peer within this timeout + /// window, the iterator considers the peer unresponsive and will not wait for + /// the peer when evaluating the termination conditions, until and unless a + /// result is delivered. Defaults to `10` seconds. + pub peer_timeout: Duration, +} + +impl Default for DisjointClosestPeersIterConfig { + fn default() -> Self { + DisjointClosestPeersIterConfig { + parallelism: ALPHA_VALUE.get(), + disjoint_paths: 3, + num_results: K_VALUE.get(), + peer_timeout: Duration::from_secs(10), + } + } +} + +/// Wraps around a set of `ClosestPeersIter`, enforcing the amount of configured disjoint paths +/// according to the S/Kademlia paper. +pub struct DisjointClosestPeersIter { + iters: Vec, + /// Mapping of yielded peers to iterator that yielded them. + /// + /// More specifically index into the `DisjointClosestPeersIter::iters` vector. On the one hand + /// this is used to link responses from remote peers back to the corresponding iterator, on the + /// other hand it is used to track which peers have been contacted in the past. + yielded_peers: HashMap, +} + +impl DisjointClosestPeersIter { + /// Creates a new iterator with a default configuration. + pub fn new(target: KeyBytes, known_closest_peers: I) -> Self + where + I: IntoIterator>, + { + Self::with_config( + DisjointClosestPeersIterConfig::default(), + target, + known_closest_peers, + ) + } + + /// Creates a new iterator with the given configuration. + pub fn with_config( + config: DisjointClosestPeersIterConfig, + target: T, + known_closest_peers: I, + ) -> Self + where + I: IntoIterator>, + T: Into + Clone, + { + let iters = split_inputs_per_disjoint_path(&config, known_closest_peers) + .into_iter() + .map(|(config, peers)| ClosestPeersIter::with_config(config, target.clone(), peers)) + .collect(); + + DisjointClosestPeersIter { + iters, + yielded_peers: HashMap::new(), + } + } + + pub fn on_failure(&mut self, peer: &PeerId) { + self.iters[self.yielded_peers[peer]].on_failure(peer); + } + + pub fn on_success(&mut self, peer: &PeerId, closer_peers: I) + where + I: IntoIterator, + { + self.iters[self.yielded_peers[peer]].on_success(peer, closer_peers); + } + + pub fn is_waiting(&self, peer: &PeerId) -> bool { + self.iters[self.yielded_peers[peer]].is_waiting(peer) + } + + pub fn next(&mut self, now: Instant) -> PeersIterState { + let mut state = PeersIterState::Finished; + + // TODO: Iterating through all iterators in the same order might be unfair. + for (i, iter) in &mut self.iters.iter_mut().enumerate() { + loop { + match iter.next(now) { + PeersIterState::Waiting(None) => { + match state { + PeersIterState::Waiting(None) => {} + PeersIterState::WaitingAtCapacity => { + state = PeersIterState::Waiting(None) + } + PeersIterState::Finished => state = PeersIterState::Waiting(None), + // TODO: Document. + _ => unreachable!(), + }; + + break; + } + PeersIterState::Waiting(Some(peer)) => { + // TODO: Hack to get around the borrow checker. Can we do better? + let peer = peer.clone().into_owned(); + + if self.yielded_peers.contains_key(&peer) { + // Another iterator already returned this peer. S/Kademlia requires each + // peer to be only used on one path. Marking it as failed for this + // iterator, asking it to return another peer in the next loop + // iteration. + iter.on_failure(&peer); + } else { + self.yielded_peers.insert(peer.clone(), i); + return PeersIterState::Waiting(Some(Cow::Owned(peer))); + } + } + PeersIterState::WaitingAtCapacity => { + match state { + PeersIterState::Waiting(None) => {} + PeersIterState::WaitingAtCapacity => {} + PeersIterState::Finished => state = PeersIterState::WaitingAtCapacity, + // TODO: Document. + _ => unreachable!(), + }; + + break; + } + PeersIterState::Finished => break, + } + } + } + + state + } + + pub fn finish(&mut self) { + for iter in &mut self.iters { + iter.finish() + } + } + + // TODO: Collects all Iterators into a Vec and again returns an Iterator. Can we do better? + pub fn into_result(self) -> impl Iterator { + self.iters + .into_iter() + .fold(vec![], |mut acc, iter| { + acc.extend(iter.into_result()); + acc + }) + .into_iter() + } +} + +/// Takes as input a set of resources (allowed parallelism, striven for number of results, set of +/// peers) and splits them equally (best-effort) into buckets, one for each disjoint path. +/// +/// 'best-effort' as in no more than one apart. E.g. with 10 peers and 4 disjoint paths it would +/// return [3, 3, 2, 2]. +// +// TODO: What if parallelism is smaller than disjoint_paths? In that case we would not explore as +// many disjoint paths as we could. +fn split_inputs_per_disjoint_path( + config: &DisjointClosestPeersIterConfig, + peers: I, +) -> Vec<(ClosestPeersIterConfig, Vec>)> +where + I: IntoIterator>, +{ + let parallelism_per_iter = config.parallelism / config.disjoint_paths; + let remaining_parallelism = config.parallelism % config.disjoint_paths; + + let num_results_per_iter = config.num_results / config.disjoint_paths; + let remaining_num_results = config.num_results % config.disjoint_paths; + + let mut peers = peers.into_iter().collect::>>(); + let peers_per_iter = peers.len() / config.disjoint_paths; + let remaining_peers = peers.len() % config.disjoint_paths; + + (0..config.disjoint_paths).map(|i| { + let parallelism = if i < remaining_parallelism { + parallelism_per_iter + 1 + } else { + parallelism_per_iter + }; + + let num_results = if i < remaining_num_results { + num_results_per_iter + 1 + } else { + num_results_per_iter + }; + + // TODO: Should we shuffle the peers beforehand? They might be sorted which will lead to + // unfairness between iterators. + let peers = if i < remaining_peers { + peers.split_off(peers.len() - (peers_per_iter + 1)) + } else { + peers.split_off(peers.len() - peers_per_iter) + }; + + ( + ClosestPeersIterConfig { + parallelism, + num_results, + peer_timeout: config.peer_timeout, + }, + peers, + ) + }).collect() +} + +#[cfg(test)] +mod tests { + use super::*; + use quickcheck::*; + use rand::Rng; + + impl Arbitrary for DisjointClosestPeersIterConfig { + fn arbitrary(g: &mut G) -> Self { + DisjointClosestPeersIterConfig { + parallelism: g.gen::() as usize, + num_results: g.gen::() as usize, + disjoint_paths: g.gen::() as usize, + peer_timeout: Duration::from_secs(1), + } + } + } + + #[derive(Debug, Clone)] + struct PeerVec(pub Vec>); + + impl Arbitrary for PeerVec { + fn arbitrary(g: &mut G) -> Self { + PeerVec( + (0..g.gen_range(1, 60)) + .map(|_| PeerId::random()) + .map(Key::from) + .collect(), + ) + } + } + + #[test] + fn split_inputs_per_disjoint_path_quickcheck() { + fn prop(config: DisjointClosestPeersIterConfig, peers: PeerVec) -> TestResult { + if config.parallelism == 0 + || config.num_results == 0 + || config.disjoint_paths == 0 + || peers.0.len() == 0 + { + return TestResult::discard(); + } + + let mut iters = split_inputs_per_disjoint_path(&config, peers.0.clone()); + + // Ensure the sum of each resource of each disjoint path equals the allowed input. + + assert_eq!( + config.parallelism, + iters + .iter() + .fold(0, |acc, (config, _)| { acc + config.parallelism }), + ); + + assert_eq!( + config.num_results, + iters + .iter() + .fold(0, |acc, (config, _)| { acc + config.num_results }), + ); + + assert_eq!( + peers.0.len(), + iters + .iter() + .fold(0, |acc, (_, peers)| { acc + peers.len() }), + ); + + // Ensure 'best-effort' fairness, the highest and lowest are newer more than 1 apart. + + iters.sort_by(|(a_config, _), (b_config, _)| { + a_config.parallelism.cmp(&b_config.parallelism) + }); + assert!(iters[iters.len() - 1].0.parallelism - iters[0].0.parallelism <= 1); + + iters.sort_by(|(a_config, _), (b_config, _)| { + a_config.num_results.cmp(&b_config.num_results) + }); + assert!(iters[iters.len() - 1].0.num_results - iters[0].0.num_results <= 1); + + iters.sort_by(|(_, a_peers), (_, b_peers)| a_peers.len().cmp(&b_peers.len())); + assert!(iters[iters.len() - 1].1.len() - iters[0].1.len() <= 1); + + TestResult::passed() + } + + quickcheck(prop as fn(_, _) -> _) + } +} From 1fd2d357cba6e1ed745830be63b249de4bd382c4 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 26 Feb 2020 17:55:41 +0100 Subject: [PATCH 02/63] protocols/kad: Add basic test for disjoint path peer iterator --- .../kad/src/query/peers/disjoint_closest.rs | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/disjoint_closest.rs index ca2f9cd8d19..2ab1532fa42 100644 --- a/protocols/kad/src/query/peers/disjoint_closest.rs +++ b/protocols/kad/src/query/peers/disjoint_closest.rs @@ -322,4 +322,106 @@ mod tests { quickcheck(prop as fn(_, _) -> _) } + + #[test] + fn s_kademlia_disjoint_paths() { + let now = Instant::now(); + let target: KeyBytes = Key::from(PeerId::random()).into(); + + let mut pool = [0; 12].into_iter() + .map(|_| Key::from(PeerId::random())) + .collect::>(); + + pool.sort_unstable_by(|a, b| { + target.distance(a).cmp(&target.distance(b)) + }); + + let known_closest_peers = pool.split_off(pool.len() - 3); + + let config = DisjointClosestPeersIterConfig { + parallelism: 3, + disjoint_paths: 3, + num_results: 3, + ..DisjointClosestPeersIterConfig::default() + }; + + let mut peers_iter = DisjointClosestPeersIter::with_config( + config, + target, + known_closest_peers.clone(), + ); + + ////////////////////////////////////////////////////////////////////////////// + // First round. + + for _ in 0..3 { + if let PeersIterState::Waiting(Some(Cow::Owned(peer))) = peers_iter.next(now) { + assert!(known_closest_peers.contains(&Key::from(peer))); + } else { + panic!("Expected iterator to return peer to query."); + } + } + + assert_eq!( + PeersIterState::WaitingAtCapacity, + peers_iter.next(now), + ); + + let response_2 = pool.split_off(pool.len() - 3); + let response_3 = pool.split_off(pool.len() - 3); + // Keys are closer than any of the previous two responses from honest node 1 and 2. + let malicious_response_1 = pool.split_off(pool.len() - 3); + + // Response from malicious peer 1. + peers_iter.on_success( + known_closest_peers[0].preimage(), + malicious_response_1.clone().into_iter().map(|k| k.preimage().clone()), + ); + + // Response from peer 2. + peers_iter.on_success( + known_closest_peers[1].preimage(), + response_2.clone().into_iter().map(|k| k.preimage().clone()), + ); + + // Response from peer 3. + peers_iter.on_success( + known_closest_peers[2].preimage(), + response_3.clone().into_iter().map(|k| k.preimage().clone()), + ); + + ////////////////////////////////////////////////////////////////////////////// + // Second round. + + let mut next_to_query = vec![]; + for _ in 0..3 { + if let PeersIterState::Waiting(Some(Cow::Owned(peer))) = peers_iter.next(now) { + next_to_query.push(peer) + } else { + panic!("Expected iterator to return peer to query."); + } + }; + + // Expect a peer from each disjoint path. + assert!(next_to_query.contains(malicious_response_1[0].preimage())); + assert!(next_to_query.contains(response_2[0].preimage())); + assert!(next_to_query.contains(response_3[0].preimage())); + + for peer in next_to_query { + peers_iter.on_success(&peer, vec![]); + } + + assert_eq!( + PeersIterState::Finished, + peers_iter.next(now), + ); + + let final_peers: Vec<_> = peers_iter.into_result().collect(); + + // Expect final result to contain peer from each disjoint path, even though not all are + // among the best ones. + assert!(final_peers.contains(malicious_response_1[0].preimage())); + assert!(final_peers.contains(response_2[0].preimage())); + assert!(final_peers.contains(response_3[0].preimage())); + } } From 0eabcebd2656fc75d5352b8f59468bbcd5337097 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 26 Feb 2020 18:05:42 +0100 Subject: [PATCH 03/63] protocols/kad/src/query/peers/closest.rs: Fix wrong comment `into_result` does not return the target, but only the closest peers. --- protocols/kad/src/query/peers/closest.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index f4e37e3d12e..558302bc650 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -347,7 +347,7 @@ impl ClosestPeersIter { self.state == State::Finished } - /// Consumes the iterator, returning the target and the closest peers. + /// Consumes the iterator, returning the closest peers. pub fn into_result(self) -> impl Iterator { self.closest_peers .into_iter() From ba975897f7860c04924cdd50c96ceefbc71d5b3a Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 26 Feb 2020 18:07:49 +0100 Subject: [PATCH 04/63] protocols/kad/src/query.rs: Replace closest with disjoint of 1 Replace the ClosestPeersIter with a DisjointClosestPeersIter with a disjoint_path config of 1, resulting in the same behavior. --- protocols/kad/src/query.rs | 29 ++++------------------------- 1 file changed, 4 insertions(+), 25 deletions(-) diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 183a0946cc7..c31e8fadbb6 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -21,7 +21,6 @@ mod peers; use peers::PeersIterState; -use peers::closest::{ClosestPeersIter, ClosestPeersIterConfig}; use peers::disjoint_closest::{DisjointClosestPeersIter, DisjointClosestPeersIterConfig}; use peers::fixed::FixedPeersIter; @@ -98,32 +97,19 @@ impl QueryPool { self.add(peer_iter, inner) } - /// Adds a query to the pool that iterates towards the closest peers to the target. - pub fn add_iter_closest(&mut self, target: T, peers: I, inner: TInner) -> QueryId - where - T: Into, - I: IntoIterator> - { - let cfg = ClosestPeersIterConfig { - num_results: self.config.replication_factor.get(), - .. ClosestPeersIterConfig::default() - }; - let peer_iter = QueryPeerIter::Closest(ClosestPeersIter::with_config(cfg, target, peers)); - self.add(peer_iter, inner) - } - /// Adds a query to the pool that iterates towards the closest peers to the target on disjoint /// paths. - pub fn add_iter_disjoint_closest(&mut self, target: T, peers: I, inner: TInner) -> QueryId + pub fn add_iter_closest(&mut self, target: T, peers: I, inner: TInner) -> QueryId where T: Into + Clone, I: IntoIterator> { let cfg = DisjointClosestPeersIterConfig { num_results: self.config.replication_factor.get(), + disjoint_paths: 1, .. DisjointClosestPeersIterConfig::default() }; - let peer_iter = QueryPeerIter::DisjointClosest(DisjointClosestPeersIter::with_config(cfg, target, peers)); + let peer_iter = QueryPeerIter::Closest(DisjointClosestPeersIter::with_config(cfg, target, peers)); self.add(peer_iter, inner) } @@ -231,8 +217,7 @@ pub struct Query { /// The peer selection strategies that can be used by queries. enum QueryPeerIter { - Closest(ClosestPeersIter), - DisjointClosest(DisjointClosestPeersIter), + Closest(DisjointClosestPeersIter), Fixed(FixedPeersIter) } @@ -251,7 +236,6 @@ impl Query { pub fn on_failure(&mut self, peer: &PeerId) { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.on_failure(peer), - QueryPeerIter::DisjointClosest(iter) => iter.on_failure(peer), QueryPeerIter::Fixed(iter) => iter.on_failure(peer) } } @@ -265,7 +249,6 @@ impl Query { { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.on_success(peer, new_peers), - QueryPeerIter::DisjointClosest(iter) => iter.on_success(peer, new_peers), QueryPeerIter::Fixed(iter) => iter.on_success(peer) } } @@ -274,7 +257,6 @@ impl Query { pub fn is_waiting(&self, peer: &PeerId) -> bool { match &self.peer_iter { QueryPeerIter::Closest(iter) => iter.is_waiting(peer), - QueryPeerIter::DisjointClosest(iter) => iter.is_waiting(peer), QueryPeerIter::Fixed(iter) => iter.is_waiting(peer) } } @@ -283,7 +265,6 @@ impl Query { fn next(&mut self, now: Instant) -> PeersIterState { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.next(now), - QueryPeerIter::DisjointClosest(iter) => iter.next(now), QueryPeerIter::Fixed(iter) => iter.next() } } @@ -295,7 +276,6 @@ impl Query { pub fn finish(&mut self) { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.finish(), - QueryPeerIter::DisjointClosest(iter) => iter.finish(), QueryPeerIter::Fixed(iter) => iter.finish() } } @@ -304,7 +284,6 @@ impl Query { pub fn into_result(self) -> QueryResult> { let peers = match self.peer_iter { QueryPeerIter::Closest(iter) => Either::Left(iter.into_result()), - QueryPeerIter::DisjointClosest(iter) => unimplemented!(), QueryPeerIter::Fixed(iter) => Either::Right(iter.into_result()) }; QueryResult { inner: self.inner, peers } From 1d963e5594f4740f4f3a088e73829211d7d527bf Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 1 Apr 2020 20:11:41 +0200 Subject: [PATCH 05/63] src/query/peers/disjoint_closest: Only split up needed peers Previous implementation would split up all provided peers only to have the `ClosestDisjointIter` choose a subset of it. This patch only splits up the needed subsets. --- protocols/kad/src/query/peers/disjoint_closest.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/disjoint_closest.rs index 2ab1532fa42..482aa32c01c 100644 --- a/protocols/kad/src/query/peers/disjoint_closest.rs +++ b/protocols/kad/src/query/peers/disjoint_closest.rs @@ -199,7 +199,11 @@ where let num_results_per_iter = config.num_results / config.disjoint_paths; let remaining_num_results = config.num_results % config.disjoint_paths; - let mut peers = peers.into_iter().collect::>>(); + let mut peers = peers.into_iter() + // Each `[ClosestPeersIterConfig]` should be configured with ALPHA_VALUE + // peers at initialization. + .take(ALPHA_VALUE.get() * config.disjoint_paths) + .collect::>>(); let peers_per_iter = peers.len() / config.disjoint_paths; let remaining_peers = peers.len() % config.disjoint_paths; From 9a2a7057056a391cde4f81c9b272acb155253bb8 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 2 Apr 2020 15:50:46 +0200 Subject: [PATCH 06/63] protocols/kad/query/disjoint: Add license header --- .../kad/src/query/peers/disjoint_closest.rs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/disjoint_closest.rs index 482aa32c01c..629feab26d0 100644 --- a/protocols/kad/src/query/peers/disjoint_closest.rs +++ b/protocols/kad/src/query/peers/disjoint_closest.rs @@ -1,3 +1,23 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the "Software"), +// to deal in the Software without restriction, including without limitation +// the rights to use, copy, modify, merge, publish, distribute, sublicense, +// and/or sell copies of the Software, and to permit persons to whom the +// Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + use super::*; use crate::kbucket::{Key, KeyBytes}; use crate::query::peers::closest::{ClosestPeersIter, ClosestPeersIterConfig}; From be0b9bfc43fc1ac77c52ebf7d2cee7449c41c7e4 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 2 Apr 2020 16:14:56 +0200 Subject: [PATCH 07/63] protocols/kad: Expose configuration of disjoint path lookups --- protocols/kad/src/behaviour.rs | 13 ++++++++++++- protocols/kad/src/query.rs | 22 ++++++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 588bdd8aca2..c2c9e2a1e62 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -142,6 +142,18 @@ impl KademliaConfig { self } + /// Enable queries to use disjoint paths when iteratively looking for the + /// closest node to a target. + /// + /// See the S/Kademlia paper for details. + // + // TODO: Document that when enabled the number of disjoint paths is equal to the number of + // parallelism. + pub fn enable_disjoint_path_queries(&mut self) -> &mut Self { + self.query_config.disjoint_paths = true; + self + } + /// Sets the TTL for stored records. /// /// The TTL should be significantly longer than the (re-)publication @@ -1895,4 +1907,3 @@ impl QueryInfo { } } } - diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index c31e8fadbb6..e8534fc95ad 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -104,11 +104,16 @@ impl QueryPool { T: Into + Clone, I: IntoIterator> { - let cfg = DisjointClosestPeersIterConfig { + let mut cfg = DisjointClosestPeersIterConfig { num_results: self.config.replication_factor.get(), disjoint_paths: 1, .. DisjointClosestPeersIterConfig::default() }; + + if self.config.disjoint_paths { + cfg.disjoint_paths = cfg.parallelism; + } + let peer_iter = QueryPeerIter::Closest(DisjointClosestPeersIter::with_config(cfg, target, peers)); self.add(peer_iter, inner) } @@ -189,15 +194,28 @@ pub struct QueryId(usize); /// The configuration for queries in a `QueryPool`. #[derive(Debug, Clone)] pub struct QueryConfig { + /// Timeout of a single query. + /// + /// See [`crate::behaviour::KademliaConfig::set_query_timeout`] for details. pub timeout: Duration, + + /// The replication factor to use. + /// + /// See [`crate::behaviour::KademliaConfig::set_replication_factor`] for details. pub replication_factor: NonZeroUsize, + + /// Whether to use disjoint paths on iterative lookups. + /// + /// See [`crate::behaviour::KademliaConfig::enable_disjoint_path_queries`] for details. + pub disjoint_paths: bool, } impl Default for QueryConfig { fn default() -> Self { QueryConfig { timeout: Duration::from_secs(60), - replication_factor: NonZeroUsize::new(K_VALUE.get()).expect("K_VALUE > 0") + replication_factor: NonZeroUsize::new(K_VALUE.get()).expect("K_VALUE > 0"), + disjoint_paths: false, } } } From ca089ba4d676fd8e99512a59f57ea885080edd5a Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 2 Apr 2020 18:26:04 +0200 Subject: [PATCH 08/63] protocols/kad: Make disjoint_paths binary Have a user either disable or enable the disjoint paths feature. When enabled always set the amount of disjoint paths to use to the amount of parallelism. --- protocols/kad/src/behaviour.rs | 2 +- protocols/kad/src/query.rs | 12 ++-- .../kad/src/query/peers/disjoint_closest.rs | 63 +++++++++---------- 3 files changed, 34 insertions(+), 43 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index c2c9e2a1e62..b8d22ccfc46 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -150,7 +150,7 @@ impl KademliaConfig { // TODO: Document that when enabled the number of disjoint paths is equal to the number of // parallelism. pub fn enable_disjoint_path_queries(&mut self) -> &mut Self { - self.query_config.disjoint_paths = true; + self.query_config.use_disjoint_paths = true; self } diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index e8534fc95ad..b2226ff3284 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -104,16 +104,12 @@ impl QueryPool { T: Into + Clone, I: IntoIterator> { - let mut cfg = DisjointClosestPeersIterConfig { + let cfg = DisjointClosestPeersIterConfig { num_results: self.config.replication_factor.get(), - disjoint_paths: 1, + use_disjoint_paths: self.config.use_disjoint_paths, .. DisjointClosestPeersIterConfig::default() }; - if self.config.disjoint_paths { - cfg.disjoint_paths = cfg.parallelism; - } - let peer_iter = QueryPeerIter::Closest(DisjointClosestPeersIter::with_config(cfg, target, peers)); self.add(peer_iter, inner) } @@ -207,7 +203,7 @@ pub struct QueryConfig { /// Whether to use disjoint paths on iterative lookups. /// /// See [`crate::behaviour::KademliaConfig::enable_disjoint_path_queries`] for details. - pub disjoint_paths: bool, + pub use_disjoint_paths: bool, } impl Default for QueryConfig { @@ -215,7 +211,7 @@ impl Default for QueryConfig { QueryConfig { timeout: Duration::from_secs(60), replication_factor: NonZeroUsize::new(K_VALUE.get()).expect("K_VALUE > 0"), - disjoint_paths: false, + use_disjoint_paths: false, } } } diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/disjoint_closest.rs index 629feab26d0..25439eb7db1 100644 --- a/protocols/kad/src/query/peers/disjoint_closest.rs +++ b/protocols/kad/src/query/peers/disjoint_closest.rs @@ -36,8 +36,8 @@ pub struct DisjointClosestPeersIterConfig { /// nodes to a target. Defaults to `ALPHA_VALUE`. pub parallelism: usize, - // TODO: Document that it needs to be equal or larger than parallelism? - pub disjoint_paths: usize, + // TODO: Document that the number of disjoint paths is equal to the amount of parallelism. + pub use_disjoint_paths: bool, /// Number of results (closest peers) to search for. /// @@ -58,7 +58,7 @@ impl Default for DisjointClosestPeersIterConfig { fn default() -> Self { DisjointClosestPeersIterConfig { parallelism: ALPHA_VALUE.get(), - disjoint_paths: 3, + use_disjoint_paths: false, num_results: K_VALUE.get(), peer_timeout: Duration::from_secs(10), } @@ -100,6 +100,22 @@ impl DisjointClosestPeersIter { I: IntoIterator>, T: Into + Clone, { + // TODO: Document that this basically makes the disjoint path iterator a no-op shallow wrapper. + if !config.use_disjoint_paths { + return DisjointClosestPeersIter { + iters: vec![ClosestPeersIter::with_config( + ClosestPeersIterConfig { + parallelism: config.parallelism, + num_results: config.num_results, + peer_timeout: config.peer_timeout, + }, + target, + known_closest_peers, + )], + yielded_peers: HashMap::new(), + } + } + let iters = split_inputs_per_disjoint_path(&config, known_closest_peers) .into_iter() .map(|(config, peers)| ClosestPeersIter::with_config(config, target.clone(), peers)) @@ -213,27 +229,19 @@ fn split_inputs_per_disjoint_path( where I: IntoIterator>, { - let parallelism_per_iter = config.parallelism / config.disjoint_paths; - let remaining_parallelism = config.parallelism % config.disjoint_paths; - - let num_results_per_iter = config.num_results / config.disjoint_paths; - let remaining_num_results = config.num_results % config.disjoint_paths; + // TODO: Make it more obvious that the number of parallelism is the number of disjoint paths. + let num_results_per_iter = config.num_results / config.parallelism; + let remaining_num_results = config.num_results % config.parallelism; let mut peers = peers.into_iter() // Each `[ClosestPeersIterConfig]` should be configured with ALPHA_VALUE // peers at initialization. - .take(ALPHA_VALUE.get() * config.disjoint_paths) + .take(ALPHA_VALUE.get() * config.parallelism) .collect::>>(); - let peers_per_iter = peers.len() / config.disjoint_paths; - let remaining_peers = peers.len() % config.disjoint_paths; - - (0..config.disjoint_paths).map(|i| { - let parallelism = if i < remaining_parallelism { - parallelism_per_iter + 1 - } else { - parallelism_per_iter - }; + let peers_per_iter = peers.len() / config.parallelism; + let remaining_peers = peers.len() % config.parallelism; + (0..config.parallelism).map(|i| { let num_results = if i < remaining_num_results { num_results_per_iter + 1 } else { @@ -250,7 +258,7 @@ where ( ClosestPeersIterConfig { - parallelism, + parallelism: 1, num_results, peer_timeout: config.peer_timeout, }, @@ -270,7 +278,7 @@ mod tests { DisjointClosestPeersIterConfig { parallelism: g.gen::() as usize, num_results: g.gen::() as usize, - disjoint_paths: g.gen::() as usize, + use_disjoint_paths: g.gen::(), peer_timeout: Duration::from_secs(1), } } @@ -295,7 +303,6 @@ mod tests { fn prop(config: DisjointClosestPeersIterConfig, peers: PeerVec) -> TestResult { if config.parallelism == 0 || config.num_results == 0 - || config.disjoint_paths == 0 || peers.0.len() == 0 { return TestResult::discard(); @@ -305,13 +312,6 @@ mod tests { // Ensure the sum of each resource of each disjoint path equals the allowed input. - assert_eq!( - config.parallelism, - iters - .iter() - .fold(0, |acc, (config, _)| { acc + config.parallelism }), - ); - assert_eq!( config.num_results, iters @@ -328,11 +328,6 @@ mod tests { // Ensure 'best-effort' fairness, the highest and lowest are newer more than 1 apart. - iters.sort_by(|(a_config, _), (b_config, _)| { - a_config.parallelism.cmp(&b_config.parallelism) - }); - assert!(iters[iters.len() - 1].0.parallelism - iters[0].0.parallelism <= 1); - iters.sort_by(|(a_config, _), (b_config, _)| { a_config.num_results.cmp(&b_config.num_results) }); @@ -364,7 +359,7 @@ mod tests { let config = DisjointClosestPeersIterConfig { parallelism: 3, - disjoint_paths: 3, + use_disjoint_paths: true, num_results: 3, ..DisjointClosestPeersIterConfig::default() }; From 17b9a686a6a2efde8e0d70069012c9ede09f1afb Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 3 Apr 2020 18:15:44 +0200 Subject: [PATCH 09/63] protocols/kad/query/peers/disjoint: Add property test Add test ensuring DisjointClosestPeersIter derives same peer set result as ClosestPeersIter on perfect network conditions. --- .../kad/src/query/peers/disjoint_closest.rs | 142 +++++++++++++++++- 1 file changed, 141 insertions(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/disjoint_closest.rs index 25439eb7db1..bf9920a52b7 100644 --- a/protocols/kad/src/query/peers/disjoint_closest.rs +++ b/protocols/kad/src/query/peers/disjoint_closest.rs @@ -271,7 +271,7 @@ where mod tests { use super::*; use quickcheck::*; - use rand::Rng; + use rand::{Rng, seq::SliceRandom}; impl Arbitrary for DisjointClosestPeersIterConfig { fn arbitrary(g: &mut G) -> Self { @@ -443,4 +443,144 @@ mod tests { assert!(final_peers.contains(response_2[0].preimage())); assert!(final_peers.contains(response_3[0].preimage())); } + + fn random_peers(n: usize) -> impl Iterator + Clone { + (0 .. n).map(|_| PeerId::random()) + } + + #[derive(Debug, Clone)] + struct Graph(HashMap); + + impl Arbitrary for Graph { + fn arbitrary(g: &mut G) -> Self { + let mut peers = HashMap::new(); + let mut peer_ids = random_peers(g.gen_range(K_VALUE.get(), 100)) + .collect::>(); + + for peer_id in peer_ids.clone() { + peer_ids.shuffle(g); + + peers.insert(peer_id, Peer{ + known_peers: peer_ids[0..g.gen_range(1, K_VALUE.get())].to_vec(), + }); + } + + Graph(peers) + } + } + + #[derive(Debug, Clone)] + struct Peer { + known_peers: Vec, + } + + #[derive(Debug, Clone)] + struct Parallelism(usize); + + impl Arbitrary for Parallelism{ + fn arbitrary(g: &mut G) -> Self { + Parallelism(g.gen_range(1, 10)) + } + } + + #[derive(Debug, Clone)] + struct NumResults(usize); + + impl Arbitrary for NumResults{ + fn arbitrary(g: &mut G) -> Self { + NumResults(g.gen_range(1, K_VALUE.get())) + } + } + + enum PeerIterator { + DisjointClosest(DisjointClosestPeersIter), + Closest(ClosestPeersIter), + DisjointClosestDisjointDisabled(DisjointClosestPeersIter), + } + + impl PeerIterator { + fn next(&mut self, now: Instant) -> PeersIterState { + match self { + PeerIterator::DisjointClosest(iter) => iter.next(now), + PeerIterator::Closest(iter) => iter.next(now), + PeerIterator::DisjointClosestDisjointDisabled(iter) => iter.next(now), + } + } + + fn on_success(&mut self, peer: &PeerId, closer_peers: Vec) { + match self { + PeerIterator::DisjointClosest(iter) => iter.on_success(peer, closer_peers), + PeerIterator::Closest(iter) => iter.on_success(peer, closer_peers), + PeerIterator::DisjointClosestDisjointDisabled(iter) => iter.on_success(peer, closer_peers), + } + } + + fn into_result(self) -> Vec { + match self { + PeerIterator::DisjointClosest(iter) => iter.into_result().collect(), + PeerIterator::Closest(iter) => iter.into_result().collect(), + PeerIterator::DisjointClosestDisjointDisabled(iter) => iter.into_result().collect(), + } + } + } + + #[test] + fn closest_and_disjoint_closest_yield_same_result() { + fn prop(graph: Graph, parallelism: Parallelism, num_results: NumResults) { + let now = Instant::now(); + let target: KeyBytes = Key::from(PeerId::random()).into(); + + let mut known_closest_peers = graph.0.iter().take(parallelism.0 * ALPHA_VALUE.get()).map(|(key, _peers)| Key::new(key.clone())).collect::>(); + known_closest_peers.sort_unstable_by(|a, b| { + target.distance(a).cmp(&target.distance(b)) + }); + + let iters = vec![ + PeerIterator::DisjointClosest(DisjointClosestPeersIter::with_config(DisjointClosestPeersIterConfig{ + parallelism: parallelism.0, + use_disjoint_paths: true, + num_results: num_results.0, + ..DisjointClosestPeersIterConfig::default() + }, target.clone(), known_closest_peers.clone())), + PeerIterator::Closest(ClosestPeersIter::with_config(ClosestPeersIterConfig{ + parallelism: parallelism.0, + num_results: num_results.0, + ..ClosestPeersIterConfig::default() + }, target.clone(), known_closest_peers.clone())), + PeerIterator::DisjointClosestDisjointDisabled(DisjointClosestPeersIter::with_config(DisjointClosestPeersIterConfig{ + parallelism: parallelism.0, + use_disjoint_paths: false, + num_results: num_results.0, + ..DisjointClosestPeersIterConfig::default() + }, target.clone(), known_closest_peers.clone())), + ]; + + iters.into_iter().map(|mut iter| { + loop { + match iter.next(now) { + PeersIterState::Waiting(Some(peer_id)) => { + let peer_id = peer_id.clone().into_owned(); + iter.on_success(&peer_id, graph.0.get(&peer_id).unwrap().known_peers.clone()) + } , + PeersIterState::WaitingAtCapacity | PeersIterState::Waiting(None) => panic!("There are never more than one requests in flight."), + PeersIterState::Finished => break, + } + } + + let mut result = iter.into_result().into_iter().map(Key::new).collect::>(); + result.sort_unstable_by(|a, b| { + target.distance(a).cmp(&target.distance(b)) + }); + result + }).fold(None, |acc, res| { + match acc { + None => Some(res), + Some(prev_res) if prev_res.len() != res.len() => panic!("unequal amount of peers"), + res @ _ => res, + } + }); + } + + QuickCheck::new().quickcheck(prop as fn(_, _, _)) + } } From 3e8097b7359d3508c69679ab8ab166a8f4d3f616 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 21 Apr 2020 18:35:36 +0200 Subject: [PATCH 10/63] src/query/peers/disjoint: Have all closest iter share peers at init All [`ClosestPeersIter`] share the same set of peers at initialization. The [`DisjointClosestPeersIter`] ensures a peer is only ever queried by a single [`ClosestPeersIter`]. --- .../kad/src/query/peers/disjoint_closest.rs | 199 +++++++++--------- 1 file changed, 94 insertions(+), 105 deletions(-) diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/disjoint_closest.rs index bf9920a52b7..7b6411f52c0 100644 --- a/protocols/kad/src/query/peers/disjoint_closest.rs +++ b/protocols/kad/src/query/peers/disjoint_closest.rs @@ -116,9 +116,14 @@ impl DisjointClosestPeersIter { } } - let iters = split_inputs_per_disjoint_path(&config, known_closest_peers) + // TODO: Don't collect them all (all k-buckets), but only parts. + let peers = known_closest_peers.into_iter().collect::>(); + let iters = split_num_results_per_disjoint_path(&config) .into_iter() - .map(|(config, peers)| ClosestPeersIter::with_config(config, target.clone(), peers)) + // NOTE: All [`ClosestPeersIter`] share the same set of peers at initialization. The + // [`DisjointClosestPeersIter`] ensures a peer is only ever queried by a single + // [`ClosestPeersIter`]. + .map(|config| ClosestPeersIter::with_config(config, target.clone(), peers.clone())) .collect(); DisjointClosestPeersIter { @@ -214,33 +219,19 @@ impl DisjointClosestPeersIter { } } -/// Takes as input a set of resources (allowed parallelism, striven for number of results, set of -/// peers) and splits them equally (best-effort) into buckets, one for each disjoint path. +/// Takes as input a [`DisjointClosestPeersIterConfig`] splits `num_results` for each disjoint path +/// (`== parallelism`) equally (best-effort) into buckets, one for each disjoint path returning a +/// `ClosestPeersIterConfig` for each disjoint path. /// -/// 'best-effort' as in no more than one apart. E.g. with 10 peers and 4 disjoint paths it would -/// return [3, 3, 2, 2]. -// -// TODO: What if parallelism is smaller than disjoint_paths? In that case we would not explore as -// many disjoint paths as we could. -fn split_inputs_per_disjoint_path( +/// 'best-effort' as in no more than one apart. E.g. with 10 overall num_result and 4 disjoint paths +/// it would return [3, 3, 2, 2]. +fn split_num_results_per_disjoint_path( config: &DisjointClosestPeersIterConfig, - peers: I, -) -> Vec<(ClosestPeersIterConfig, Vec>)> -where - I: IntoIterator>, -{ - // TODO: Make it more obvious that the number of parallelism is the number of disjoint paths. +) -> Vec { + // Note: The number of parallelism is equal to the number of disjoint paths. let num_results_per_iter = config.num_results / config.parallelism; let remaining_num_results = config.num_results % config.parallelism; - let mut peers = peers.into_iter() - // Each `[ClosestPeersIterConfig]` should be configured with ALPHA_VALUE - // peers at initialization. - .take(ALPHA_VALUE.get() * config.parallelism) - .collect::>>(); - let peers_per_iter = peers.len() / config.parallelism; - let remaining_peers = peers.len() % config.parallelism; - (0..config.parallelism).map(|i| { let num_results = if i < remaining_num_results { num_results_per_iter + 1 @@ -248,22 +239,11 @@ where num_results_per_iter }; - // TODO: Should we shuffle the peers beforehand? They might be sorted which will lead to - // unfairness between iterators. - let peers = if i < remaining_peers { - peers.split_off(peers.len() - (peers_per_iter + 1)) - } else { - peers.split_off(peers.len() - peers_per_iter) - }; - - ( ClosestPeersIterConfig { parallelism: 1, num_results, peer_timeout: config.peer_timeout, - }, - peers, - ) + } }).collect() } @@ -272,6 +252,7 @@ mod tests { use super::*; use quickcheck::*; use rand::{Rng, seq::SliceRandom}; + use std::collections::HashSet; impl Arbitrary for DisjointClosestPeersIterConfig { fn arbitrary(g: &mut G) -> Self { @@ -299,47 +280,35 @@ mod tests { } #[test] - fn split_inputs_per_disjoint_path_quickcheck() { - fn prop(config: DisjointClosestPeersIterConfig, peers: PeerVec) -> TestResult { - if config.parallelism == 0 - || config.num_results == 0 - || peers.0.len() == 0 + fn split_num_results_per_disjoint_path_quickcheck() { + fn prop(config: DisjointClosestPeersIterConfig) -> TestResult { + if config.parallelism == 0 || config.num_results == 0 { return TestResult::discard(); } - let mut iters = split_inputs_per_disjoint_path(&config, peers.0.clone()); + let mut iters = split_num_results_per_disjoint_path(&config); - // Ensure the sum of each resource of each disjoint path equals the allowed input. + // Ensure the sum of all disjoint paths equals the allowed input. assert_eq!( config.num_results, iters .iter() - .fold(0, |acc, (config, _)| { acc + config.num_results }), - ); - - assert_eq!( - peers.0.len(), - iters - .iter() - .fold(0, |acc, (_, peers)| { acc + peers.len() }), + .fold(0, |acc, config| { acc + config.num_results }), ); // Ensure 'best-effort' fairness, the highest and lowest are newer more than 1 apart. - iters.sort_by(|(a_config, _), (b_config, _)| { + iters.sort_by(|a_config, b_config| { a_config.num_results.cmp(&b_config.num_results) }); - assert!(iters[iters.len() - 1].0.num_results - iters[0].0.num_results <= 1); - - iters.sort_by(|(_, a_peers), (_, b_peers)| a_peers.len().cmp(&b_peers.len())); - assert!(iters[iters.len() - 1].1.len() - iters[0].1.len() <= 1); + assert!(iters[iters.len() - 1].num_results - iters[0].num_results <= 1); TestResult::passed() } - quickcheck(prop as fn(_, _) -> _) + quickcheck(prop as fn(_) -> _) } #[test] @@ -347,7 +316,7 @@ mod tests { let now = Instant::now(); let target: KeyBytes = Key::from(PeerId::random()).into(); - let mut pool = [0; 12].into_iter() + let mut pool = [0; 12].iter() .map(|_| Key::from(PeerId::random())) .collect::>(); @@ -454,14 +423,14 @@ mod tests { impl Arbitrary for Graph { fn arbitrary(g: &mut G) -> Self { let mut peers = HashMap::new(); - let mut peer_ids = random_peers(g.gen_range(K_VALUE.get(), 100)) + let mut peer_ids = random_peers(g.gen_range(K_VALUE.get(), 500)) .collect::>(); for peer_id in peer_ids.clone() { peer_ids.shuffle(g); peers.insert(peer_id, Peer{ - known_peers: peer_ids[0..g.gen_range(1, K_VALUE.get())].to_vec(), + known_peers: peer_ids[0..g.gen_range(K_VALUE.get(), peer_ids.len())].to_vec(), }); } @@ -493,41 +462,44 @@ mod tests { } enum PeerIterator { - DisjointClosest(DisjointClosestPeersIter), + Disjoint(DisjointClosestPeersIter), Closest(ClosestPeersIter), - DisjointClosestDisjointDisabled(DisjointClosestPeersIter), } impl PeerIterator { fn next(&mut self, now: Instant) -> PeersIterState { match self { - PeerIterator::DisjointClosest(iter) => iter.next(now), + PeerIterator::Disjoint(iter) => iter.next(now), PeerIterator::Closest(iter) => iter.next(now), - PeerIterator::DisjointClosestDisjointDisabled(iter) => iter.next(now), } } fn on_success(&mut self, peer: &PeerId, closer_peers: Vec) { match self { - PeerIterator::DisjointClosest(iter) => iter.on_success(peer, closer_peers), + PeerIterator::Disjoint(iter) => iter.on_success(peer, closer_peers), PeerIterator::Closest(iter) => iter.on_success(peer, closer_peers), - PeerIterator::DisjointClosestDisjointDisabled(iter) => iter.on_success(peer, closer_peers), } } fn into_result(self) -> Vec { match self { - PeerIterator::DisjointClosest(iter) => iter.into_result().collect(), + PeerIterator::Disjoint(iter) => iter.into_result().collect(), PeerIterator::Closest(iter) => iter.into_result().collect(), - PeerIterator::DisjointClosestDisjointDisabled(iter) => iter.into_result().collect(), } } } + /// Ensure [`ClosestPeersIter`] and [`DisjointClosestPeersIter`] yield similar closest peers. + /// + // NOTE: One can not ensure both iterators yield the *same* result. While [`ClosestPeersIter`] + // always yields the closest peers, [`DisjointClosestPeersIter`] might not. Imagine a node on + // path x yielding the 22 absolute closest peers, not returned by any other node. In addition + // imagine only 10 results are allowed per path. Path x will choose the 10 closest out of those + // 22 and drop the remaining 12, thus the overall [`DisjointClosestPeersIter`] will not yield + // the absolute closest peers combining all paths. #[test] - fn closest_and_disjoint_closest_yield_same_result() { + fn closest_and_disjoint_closest_yield_similar_result() { fn prop(graph: Graph, parallelism: Parallelism, num_results: NumResults) { - let now = Instant::now(); let target: KeyBytes = Key::from(PeerId::random()).into(); let mut known_closest_peers = graph.0.iter().take(parallelism.0 * ALPHA_VALUE.get()).map(|(key, _peers)| Key::new(key.clone())).collect::>(); @@ -535,52 +507,69 @@ mod tests { target.distance(a).cmp(&target.distance(b)) }); - let iters = vec![ - PeerIterator::DisjointClosest(DisjointClosestPeersIter::with_config(DisjointClosestPeersIterConfig{ - parallelism: parallelism.0, - use_disjoint_paths: true, - num_results: num_results.0, - ..DisjointClosestPeersIterConfig::default() - }, target.clone(), known_closest_peers.clone())), - PeerIterator::Closest(ClosestPeersIter::with_config(ClosestPeersIterConfig{ + let closest = drive_to_finish(PeerIterator::Closest(ClosestPeersIter::with_config( + ClosestPeersIterConfig{ parallelism: parallelism.0, num_results: num_results.0, ..ClosestPeersIterConfig::default() - }, target.clone(), known_closest_peers.clone())), - PeerIterator::DisjointClosestDisjointDisabled(DisjointClosestPeersIter::with_config(DisjointClosestPeersIterConfig{ + }, + target.clone(), + known_closest_peers.clone(), + )), &graph, &target); + + let disjoint = drive_to_finish(PeerIterator::Disjoint(DisjointClosestPeersIter::with_config( + DisjointClosestPeersIterConfig{ parallelism: parallelism.0, - use_disjoint_paths: false, + use_disjoint_paths: true, num_results: num_results.0, ..DisjointClosestPeersIterConfig::default() - }, target.clone(), known_closest_peers.clone())), - ]; - - iters.into_iter().map(|mut iter| { - loop { - match iter.next(now) { - PeersIterState::Waiting(Some(peer_id)) => { - let peer_id = peer_id.clone().into_owned(); - iter.on_success(&peer_id, graph.0.get(&peer_id).unwrap().known_peers.clone()) - } , - PeersIterState::WaitingAtCapacity | PeersIterState::Waiting(None) => panic!("There are never more than one requests in flight."), - PeersIterState::Finished => break, - } + }, + target.clone(), + known_closest_peers.clone(), + )),&graph, &target); + + assert_eq!( + closest.len(), disjoint.len() + ); + + if closest != disjoint { + let closest_only = closest.difference(&disjoint).collect::>(); + let disjoint_only = disjoint.difference(&closest).collect::>(); + if closest_only.len() > num_results.0 / 2 { + panic!( + "Expected both iterators to derive same peer set or be no more than \ + `(num_results / 2)` apart, but only `ClosestPeersIter` got {:?} and only \ + `DisjointClosestPeersIter` got {:?}.", + closest_only, disjoint_only, + ); } + }; + } - let mut result = iter.into_result().into_iter().map(Key::new).collect::>(); - result.sort_unstable_by(|a, b| { - target.distance(a).cmp(&target.distance(b)) - }); - result - }).fold(None, |acc, res| { - match acc { - None => Some(res), - Some(prev_res) if prev_res.len() != res.len() => panic!("unequal amount of peers"), - res @ _ => res, + fn drive_to_finish( + mut iter: PeerIterator, + graph: &Graph, + target: &KeyBytes, + ) -> HashSet { + let now = Instant::now(); + loop { + match iter.next(now) { + PeersIterState::Waiting(Some(peer_id)) => { + let peer_id = peer_id.clone().into_owned(); + iter.on_success(&peer_id, graph.0.get(&peer_id).unwrap().known_peers.clone()) + } , + PeersIterState::WaitingAtCapacity | PeersIterState::Waiting(None) => panic!("There are never more than one requests in flight."), + PeersIterState::Finished => break, } + } + + let mut result = iter.into_result().into_iter().map(Key::new).collect::>(); + result.sort_unstable_by(|a, b| { + target.distance(a).cmp(&target.distance(b)) }); + result.into_iter().map(|k| k.into_preimage()).collect() } - QuickCheck::new().quickcheck(prop as fn(_, _, _)) + QuickCheck::new().tests(10).quickcheck(prop as fn(_, _, _)) } } From 438075853c34efc5297b8e74a5a9c426b2a3fa70 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 21 Apr 2020 18:58:08 +0200 Subject: [PATCH 11/63] protocols/kad/query: Move closest disjoint peers iter --- protocols/kad/src/query.rs | 27 ++- protocols/kad/src/query/peers.rs | 1 - protocols/kad/src/query/peers/closest.rs | 2 + .../disjoint.rs} | 156 ++++++------------ 4 files changed, 76 insertions(+), 110 deletions(-) rename protocols/kad/src/query/peers/{disjoint_closest.rs => closest/disjoint.rs} (78%) diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index b2226ff3284..e775bc428f2 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -21,7 +21,7 @@ mod peers; use peers::PeersIterState; -use peers::disjoint_closest::{DisjointClosestPeersIter, DisjointClosestPeersIterConfig}; +use peers::closest::{ClosestPeersIterConfig, ClosestPeersIter, disjoint::ClosestDisjointPeersIter}; use peers::fixed::FixedPeersIter; use crate::K_VALUE; @@ -104,13 +104,19 @@ impl QueryPool { T: Into + Clone, I: IntoIterator> { - let cfg = DisjointClosestPeersIterConfig { + let cfg = ClosestPeersIterConfig { num_results: self.config.replication_factor.get(), - use_disjoint_paths: self.config.use_disjoint_paths, - .. DisjointClosestPeersIterConfig::default() + .. ClosestPeersIterConfig::default() + }; + + let peer_iter = if self.config.use_disjoint_paths { + QueryPeerIter::ClosestDisjoint( + ClosestDisjointPeersIter::with_config(cfg, target, peers), + ) + } else { + QueryPeerIter::Closest(ClosestPeersIter::with_config(cfg, target, peers)) }; - let peer_iter = QueryPeerIter::Closest(DisjointClosestPeersIter::with_config(cfg, target, peers)); self.add(peer_iter, inner) } @@ -231,7 +237,8 @@ pub struct Query { /// The peer selection strategies that can be used by queries. enum QueryPeerIter { - Closest(DisjointClosestPeersIter), + Closest(ClosestPeersIter), + ClosestDisjoint(ClosestDisjointPeersIter), Fixed(FixedPeersIter) } @@ -250,6 +257,7 @@ impl Query { pub fn on_failure(&mut self, peer: &PeerId) { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.on_failure(peer), + QueryPeerIter::ClosestDisjoint(iter) => iter.on_failure(peer), QueryPeerIter::Fixed(iter) => iter.on_failure(peer) } } @@ -263,6 +271,7 @@ impl Query { { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.on_success(peer, new_peers), + QueryPeerIter::ClosestDisjoint(iter) => iter.on_success(peer, new_peers), QueryPeerIter::Fixed(iter) => iter.on_success(peer) } } @@ -271,6 +280,7 @@ impl Query { pub fn is_waiting(&self, peer: &PeerId) -> bool { match &self.peer_iter { QueryPeerIter::Closest(iter) => iter.is_waiting(peer), + QueryPeerIter::ClosestDisjoint(iter) => iter.is_waiting(peer), QueryPeerIter::Fixed(iter) => iter.is_waiting(peer) } } @@ -279,6 +289,7 @@ impl Query { fn next(&mut self, now: Instant) -> PeersIterState { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.next(now), + QueryPeerIter::ClosestDisjoint(iter) => iter.next(now), QueryPeerIter::Fixed(iter) => iter.next() } } @@ -290,6 +301,7 @@ impl Query { pub fn finish(&mut self) { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.finish(), + QueryPeerIter::ClosestDisjoint(iter) => iter.finish(), QueryPeerIter::Fixed(iter) => iter.finish() } } @@ -297,7 +309,8 @@ impl Query { /// Consumes the query, producing the final `QueryResult`. pub fn into_result(self) -> QueryResult> { let peers = match self.peer_iter { - QueryPeerIter::Closest(iter) => Either::Left(iter.into_result()), + QueryPeerIter::Closest(iter) => Either::Left(Either::Left(iter.into_result())), + QueryPeerIter::ClosestDisjoint(iter) => Either::Left(Either::Right(iter.into_result())), QueryPeerIter::Fixed(iter) => Either::Right(iter.into_result()) }; QueryResult { inner: self.inner, peers } diff --git a/protocols/kad/src/query/peers.rs b/protocols/kad/src/query/peers.rs index 757ffa3e827..964068aa25a 100644 --- a/protocols/kad/src/query/peers.rs +++ b/protocols/kad/src/query/peers.rs @@ -39,7 +39,6 @@ //! [`Finished`]: PeersIterState::Finished pub mod closest; -pub mod disjoint_closest; pub mod fixed; use libp2p_core::PeerId; diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index 9c9de9eee3c..58095d64d87 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -27,6 +27,8 @@ use std::{time::Duration, iter::FromIterator}; use std::collections::btree_map::{BTreeMap, Entry}; use wasm_timer::Instant; +pub mod disjoint; + /// A peer iterator for a dynamically changing list of peers, sorted by increasing /// distance to a chosen target. #[derive(Debug, Clone)] diff --git a/protocols/kad/src/query/peers/disjoint_closest.rs b/protocols/kad/src/query/peers/closest/disjoint.rs similarity index 78% rename from protocols/kad/src/query/peers/disjoint_closest.rs rename to protocols/kad/src/query/peers/closest/disjoint.rs index 7b6411f52c0..b60be18e57f 100644 --- a/protocols/kad/src/query/peers/disjoint_closest.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -21,70 +21,30 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; use crate::query::peers::closest::{ClosestPeersIter, ClosestPeersIterConfig}; -use crate::{ALPHA_VALUE, K_VALUE}; use libp2p_core::PeerId; -use std::{collections::HashMap, time::Duration}; +use std::collections::HashMap; use wasm_timer::Instant; -// TODO: This duplicates a lot of documentation (See ClosestPeersIterConfig). -#[derive(Debug, Clone)] -pub struct DisjointClosestPeersIterConfig { - /// Allowed level of parallelism. - /// - /// The `α` parameter in the Kademlia paper. The maximum number of peers that - /// the iterator is allowed to wait for in parallel while iterating towards the closest - /// nodes to a target. Defaults to `ALPHA_VALUE`. - pub parallelism: usize, - - // TODO: Document that the number of disjoint paths is equal to the amount of parallelism. - pub use_disjoint_paths: bool, - - /// Number of results (closest peers) to search for. - /// - /// The number of closest peers for which the iterator must obtain successful results - /// in order to finish successfully. Defaults to `K_VALUE`. - pub num_results: usize, - - /// The timeout for a single peer. - /// - /// If a successful result is not reported for a peer within this timeout - /// window, the iterator considers the peer unresponsive and will not wait for - /// the peer when evaluating the termination conditions, until and unless a - /// result is delivered. Defaults to `10` seconds. - pub peer_timeout: Duration, -} - -impl Default for DisjointClosestPeersIterConfig { - fn default() -> Self { - DisjointClosestPeersIterConfig { - parallelism: ALPHA_VALUE.get(), - use_disjoint_paths: false, - num_results: K_VALUE.get(), - peer_timeout: Duration::from_secs(10), - } - } -} - -/// Wraps around a set of `ClosestPeersIter`, enforcing the amount of configured disjoint paths -/// according to the S/Kademlia paper. -pub struct DisjointClosestPeersIter { +/// Wraps around a set of `ClosestPeersIter`, enforcing a disjoint discovery path per configured +/// parallelism according to the S/Kademlia paper. +pub struct ClosestDisjointPeersIter { iters: Vec, /// Mapping of yielded peers to iterator that yielded them. /// - /// More specifically index into the `DisjointClosestPeersIter::iters` vector. On the one hand + /// More specifically index into the `ClosestDisjointPeersIter::iters` vector. On the one hand /// this is used to link responses from remote peers back to the corresponding iterator, on the /// other hand it is used to track which peers have been contacted in the past. yielded_peers: HashMap, } -impl DisjointClosestPeersIter { +impl ClosestDisjointPeersIter { /// Creates a new iterator with a default configuration. pub fn new(target: KeyBytes, known_closest_peers: I) -> Self where I: IntoIterator>, { Self::with_config( - DisjointClosestPeersIterConfig::default(), + ClosestPeersIterConfig::default(), target, known_closest_peers, ) @@ -92,7 +52,7 @@ impl DisjointClosestPeersIter { /// Creates a new iterator with the given configuration. pub fn with_config( - config: DisjointClosestPeersIterConfig, + config: ClosestPeersIterConfig, target: T, known_closest_peers: I, ) -> Self @@ -100,33 +60,17 @@ impl DisjointClosestPeersIter { I: IntoIterator>, T: Into + Clone, { - // TODO: Document that this basically makes the disjoint path iterator a no-op shallow wrapper. - if !config.use_disjoint_paths { - return DisjointClosestPeersIter { - iters: vec![ClosestPeersIter::with_config( - ClosestPeersIterConfig { - parallelism: config.parallelism, - num_results: config.num_results, - peer_timeout: config.peer_timeout, - }, - target, - known_closest_peers, - )], - yielded_peers: HashMap::new(), - } - } - // TODO: Don't collect them all (all k-buckets), but only parts. let peers = known_closest_peers.into_iter().collect::>(); let iters = split_num_results_per_disjoint_path(&config) .into_iter() // NOTE: All [`ClosestPeersIter`] share the same set of peers at initialization. The - // [`DisjointClosestPeersIter`] ensures a peer is only ever queried by a single + // [`ClosestDisjointPeersIter`] ensures a peer is only ever queried by a single // [`ClosestPeersIter`]. .map(|config| ClosestPeersIter::with_config(config, target.clone(), peers.clone())) .collect(); - DisjointClosestPeersIter { + ClosestDisjointPeersIter { iters, yielded_peers: HashMap::new(), } @@ -219,14 +163,14 @@ impl DisjointClosestPeersIter { } } -/// Takes as input a [`DisjointClosestPeersIterConfig`] splits `num_results` for each disjoint path +/// Takes as input a [`ClosestPeersIterConfig`] splits `num_results` for each disjoint path /// (`== parallelism`) equally (best-effort) into buckets, one for each disjoint path returning a /// `ClosestPeersIterConfig` for each disjoint path. /// /// 'best-effort' as in no more than one apart. E.g. with 10 overall num_result and 4 disjoint paths /// it would return [3, 3, 2, 2]. fn split_num_results_per_disjoint_path( - config: &DisjointClosestPeersIterConfig, + config: &ClosestPeersIterConfig, ) -> Vec { // Note: The number of parallelism is equal to the number of disjoint paths. let num_results_per_iter = config.num_results / config.parallelism; @@ -250,16 +194,17 @@ fn split_num_results_per_disjoint_path( #[cfg(test)] mod tests { use super::*; + + use crate::{ALPHA_VALUE, K_VALUE}; use quickcheck::*; use rand::{Rng, seq::SliceRandom}; use std::collections::HashSet; - impl Arbitrary for DisjointClosestPeersIterConfig { + impl Arbitrary for ClosestPeersIterConfig { fn arbitrary(g: &mut G) -> Self { - DisjointClosestPeersIterConfig { + ClosestPeersIterConfig { parallelism: g.gen::() as usize, num_results: g.gen::() as usize, - use_disjoint_paths: g.gen::(), peer_timeout: Duration::from_secs(1), } } @@ -281,7 +226,7 @@ mod tests { #[test] fn split_num_results_per_disjoint_path_quickcheck() { - fn prop(config: DisjointClosestPeersIterConfig) -> TestResult { + fn prop(config: ClosestPeersIterConfig) -> TestResult { if config.parallelism == 0 || config.num_results == 0 { return TestResult::discard(); @@ -326,14 +271,13 @@ mod tests { let known_closest_peers = pool.split_off(pool.len() - 3); - let config = DisjointClosestPeersIterConfig { + let config = ClosestPeersIterConfig { parallelism: 3, - use_disjoint_paths: true, num_results: 3, - ..DisjointClosestPeersIterConfig::default() + ..ClosestPeersIterConfig::default() }; - let mut peers_iter = DisjointClosestPeersIter::with_config( + let mut peers_iter = ClosestDisjointPeersIter::with_config( config, target, known_closest_peers.clone(), @@ -462,7 +406,7 @@ mod tests { } enum PeerIterator { - Disjoint(DisjointClosestPeersIter), + Disjoint(ClosestDisjointPeersIter), Closest(ClosestPeersIter), } @@ -489,44 +433,52 @@ mod tests { } } - /// Ensure [`ClosestPeersIter`] and [`DisjointClosestPeersIter`] yield similar closest peers. + /// Ensure [`ClosestPeersIter`] and [`ClosestDisjointPeersIter`] yield similar closest peers. /// // NOTE: One can not ensure both iterators yield the *same* result. While [`ClosestPeersIter`] - // always yields the closest peers, [`DisjointClosestPeersIter`] might not. Imagine a node on + // always yields the closest peers, [`ClosestDisjointPeersIter`] might not. Imagine a node on // path x yielding the 22 absolute closest peers, not returned by any other node. In addition // imagine only 10 results are allowed per path. Path x will choose the 10 closest out of those - // 22 and drop the remaining 12, thus the overall [`DisjointClosestPeersIter`] will not yield + // 22 and drop the remaining 12, thus the overall [`ClosestDisjointPeersIter`] will not yield // the absolute closest peers combining all paths. #[test] fn closest_and_disjoint_closest_yield_similar_result() { fn prop(graph: Graph, parallelism: Parallelism, num_results: NumResults) { let target: KeyBytes = Key::from(PeerId::random()).into(); - let mut known_closest_peers = graph.0.iter().take(parallelism.0 * ALPHA_VALUE.get()).map(|(key, _peers)| Key::new(key.clone())).collect::>(); + let mut known_closest_peers = graph.0.iter() + .take(parallelism.0 * ALPHA_VALUE.get()) + .map(|(key, _peers)| Key::new(key.clone())) + .collect::>(); known_closest_peers.sort_unstable_by(|a, b| { target.distance(a).cmp(&target.distance(b)) }); - let closest = drive_to_finish(PeerIterator::Closest(ClosestPeersIter::with_config( - ClosestPeersIterConfig{ - parallelism: parallelism.0, - num_results: num_results.0, - ..ClosestPeersIterConfig::default() - }, - target.clone(), - known_closest_peers.clone(), - )), &graph, &target); - - let disjoint = drive_to_finish(PeerIterator::Disjoint(DisjointClosestPeersIter::with_config( - DisjointClosestPeersIterConfig{ - parallelism: parallelism.0, - use_disjoint_paths: true, - num_results: num_results.0, - ..DisjointClosestPeersIterConfig::default() - }, - target.clone(), - known_closest_peers.clone(), - )),&graph, &target); + let cfg = ClosestPeersIterConfig{ + parallelism: parallelism.0, + num_results: num_results.0, + ..ClosestPeersIterConfig::default() + }; + + let closest = drive_to_finish( + PeerIterator::Closest(ClosestPeersIter::with_config( + cfg.clone(), + target.clone(), + known_closest_peers.clone(), + )), + &graph, + &target, + ); + + let disjoint = drive_to_finish( + PeerIterator::Disjoint(ClosestDisjointPeersIter::with_config( + cfg, + target.clone(), + known_closest_peers.clone(), + )), + &graph, + &target, + ); assert_eq!( closest.len(), disjoint.len() @@ -539,7 +491,7 @@ mod tests { panic!( "Expected both iterators to derive same peer set or be no more than \ `(num_results / 2)` apart, but only `ClosestPeersIter` got {:?} and only \ - `DisjointClosestPeersIter` got {:?}.", + `ClosestDisjointPeersIter` got {:?}.", closest_only, disjoint_only, ); } From 189c3301f349cd5fef17971cc017ec0e95027585 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 21 Apr 2020 19:02:46 +0200 Subject: [PATCH 12/63] protocols/kad/query/disjoint: Don't initialize will all peers --- protocols/kad/src/behaviour.rs | 11 ++++++----- protocols/kad/src/behaviour/test.rs | 2 ++ protocols/kad/src/query/peers/closest.rs | 1 + protocols/kad/src/query/peers/closest/disjoint.rs | 4 +--- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 5eb109a0763..19627627ac7 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -153,11 +153,12 @@ impl KademliaConfig { /// Enable queries to use disjoint paths when iteratively looking for the /// closest node to a target. /// - /// See the S/Kademlia paper for details. - // - // TODO: Document that when enabled the number of disjoint paths is equal to the number of - // parallelism. - pub fn enable_disjoint_path_queries(&mut self) -> &mut Self { + /// When enabled the amount of disjoint paths used equals the configured + /// parallelism. + /// + /// See the S/Kademlia paper for more information on the high level design + /// as well as its security improvements. + pub fn use_disjoint_path_queries(&mut self) -> &mut Self { self.query_config.use_disjoint_paths = true; self } diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index afeda26a98f..801923a1d30 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -49,6 +49,8 @@ use multihash::{wrap, Code, Multihash}; type TestSwarm = Swarm>; +// TODO: Have these tests use disjoint paths as well. + fn build_node() -> (Multiaddr, TestSwarm) { build_node_with_config(Default::default()) } diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index 58095d64d87..d8cb8982f57 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -57,6 +57,7 @@ pub struct ClosestPeersIterConfig { /// The `α` parameter in the Kademlia paper. The maximum number of peers that /// the iterator is allowed to wait for in parallel while iterating towards the closest /// nodes to a target. Defaults to `ALPHA_VALUE`. + // TODO: Should this be configurable? pub parallelism: usize, /// Number of results (closest peers) to search for. diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index b60be18e57f..8cb1ec06a7e 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -20,7 +20,6 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; -use crate::query::peers::closest::{ClosestPeersIter, ClosestPeersIterConfig}; use libp2p_core::PeerId; use std::collections::HashMap; use wasm_timer::Instant; @@ -60,8 +59,7 @@ impl ClosestDisjointPeersIter { I: IntoIterator>, T: Into + Clone, { - // TODO: Don't collect them all (all k-buckets), but only parts. - let peers = known_closest_peers.into_iter().collect::>(); + let peers = known_closest_peers.into_iter().take(K_VALUE.get()).collect::>(); let iters = split_num_results_per_disjoint_path(&config) .into_iter() // NOTE: All [`ClosestPeersIter`] share the same set of peers at initialization. The From 069d48ad2c1ac02d37a98bc52614a6bdcca08f50 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 29 Apr 2020 16:55:06 +0200 Subject: [PATCH 13/63] protocols/kad/query/disjoint: Query closest iters fairly Don't start with closest iter 0 each time, but instead start off with iter least querried so far. --- .../kad/src/query/peers/closest/disjoint.rs | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 8cb1ec06a7e..2d3d03bf697 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -28,12 +28,14 @@ use wasm_timer::Instant; /// parallelism according to the S/Kademlia paper. pub struct ClosestDisjointPeersIter { iters: Vec, - /// Mapping of yielded peers to iterator that yielded them. + /// Mapping of yielded peers to index of iterator that yielded them. /// /// More specifically index into the `ClosestDisjointPeersIter::iters` vector. On the one hand /// this is used to link responses from remote peers back to the corresponding iterator, on the /// other hand it is used to track which peers have been contacted in the past. yielded_peers: HashMap, + /// Index of the iterator last queried. + last_quiered: usize, } impl ClosestDisjointPeersIter { @@ -66,11 +68,15 @@ impl ClosestDisjointPeersIter { // [`ClosestDisjointPeersIter`] ensures a peer is only ever queried by a single // [`ClosestPeersIter`]. .map(|config| ClosestPeersIter::with_config(config, target.clone(), peers.clone())) - .collect(); + .collect::>(); + + let iters_len = iters.len(); ClosestDisjointPeersIter { iters, yielded_peers: HashMap::new(), + // Wraps around, thus iterator 0 will be queried first. + last_quiered: iters_len - 1, } } @@ -92,8 +98,19 @@ impl ClosestDisjointPeersIter { pub fn next(&mut self, now: Instant) -> PeersIterState { let mut state = PeersIterState::Finished; - // TODO: Iterating through all iterators in the same order might be unfair. - for (i, iter) in &mut self.iters.iter_mut().enumerate() { + // Order in which to query the iterators to ensure fairness. Make sure + // to query the last queried iterator after all others. + let iter_order = { + let mut all = (0..self.iters.len()).collect::>(); + let mut next_up = all.split_off(self.last_quiered + 1); + next_up.append(&mut all); + next_up + }; + + for i in &mut iter_order.into_iter() { + self.last_quiered = i; + let iter = &mut self.iters[i]; + loop { match iter.next(now) { PeersIterState::Waiting(None) => { @@ -478,9 +495,7 @@ mod tests { &target, ); - assert_eq!( - closest.len(), disjoint.len() - ); + assert_eq!(closest.len(), disjoint.len()); if closest != disjoint { let closest_only = closest.difference(&disjoint).collect::>(); From 5ba56165c112ce8c53884a716824b80e472da810 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 29 Apr 2020 17:54:50 +0200 Subject: [PATCH 14/63] protocols/kad/query/disjoint: Address minor TODOs --- .../kad/src/query/peers/closest/disjoint.rs | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 2d3d03bf697..fc64a4d18e0 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -28,7 +28,7 @@ use wasm_timer::Instant; /// parallelism according to the S/Kademlia paper. pub struct ClosestDisjointPeersIter { iters: Vec, - /// Mapping of yielded peers to index of iterator that yielded them. + /// Mapping of yielded peers to iterator that yielded them. /// /// More specifically index into the `ClosestDisjointPeersIter::iters` vector. On the one hand /// this is used to link responses from remote peers back to the corresponding iterator, on the @@ -64,8 +64,9 @@ impl ClosestDisjointPeersIter { let peers = known_closest_peers.into_iter().take(K_VALUE.get()).collect::>(); let iters = split_num_results_per_disjoint_path(&config) .into_iter() - // NOTE: All [`ClosestPeersIter`] share the same set of peers at initialization. The - // [`ClosestDisjointPeersIter`] ensures a peer is only ever queried by a single + // NOTE: All [`ClosestPeersIter`] share the same set of peers at + // initialization. The [`ClosestDisjointPeersIter.yielded_peers`] + // ensures a peer is only ever queried by a single // [`ClosestPeersIter`]. .map(|config| ClosestPeersIter::with_config(config, target.clone(), peers.clone())) .collect::>(); @@ -99,7 +100,7 @@ impl ClosestDisjointPeersIter { let mut state = PeersIterState::Finished; // Order in which to query the iterators to ensure fairness. Make sure - // to query the last queried iterator after all others. + // to query the previously queried iterator last. let iter_order = { let mut all = (0..self.iters.len()).collect::>(); let mut next_up = all.split_off(self.last_quiered + 1); @@ -115,39 +116,45 @@ impl ClosestDisjointPeersIter { match iter.next(now) { PeersIterState::Waiting(None) => { match state { + PeersIterState::Waiting(Some(_)) => { + // [`ClosestDisjointPeersIter::next`] returns immediately once a + // [`ClosestPeersIter`] yielded a peer. Thus this state is + // unreachable. + unreachable!(); + }, PeersIterState::Waiting(None) => {} PeersIterState::WaitingAtCapacity => { state = PeersIterState::Waiting(None) } PeersIterState::Finished => state = PeersIterState::Waiting(None), - // TODO: Document. - _ => unreachable!(), }; break; } PeersIterState::Waiting(Some(peer)) => { - // TODO: Hack to get around the borrow checker. Can we do better? - let peer = peer.clone().into_owned(); - - if self.yielded_peers.contains_key(&peer) { + if self.yielded_peers.contains_key(&*peer) { // Another iterator already returned this peer. S/Kademlia requires each // peer to be only used on one path. Marking it as failed for this // iterator, asking it to return another peer in the next loop // iteration. + let peer = peer.into_owned(); iter.on_failure(&peer); } else { - self.yielded_peers.insert(peer.clone(), i); - return PeersIterState::Waiting(Some(Cow::Owned(peer))); + self.yielded_peers.insert(peer.clone().into_owned(), i); + return PeersIterState::Waiting(Some(Cow::Owned(peer.into_owned()))); } } PeersIterState::WaitingAtCapacity => { match state { + PeersIterState::Waiting(Some(_)) => { + // [`ClosestDisjointPeersIter::next`] returns immediately once a + // [`ClosestPeersIter`] yielded a peer. Thus this state is + // unreachable. + unreachable!(); + }, PeersIterState::Waiting(None) => {} PeersIterState::WaitingAtCapacity => {} PeersIterState::Finished => state = PeersIterState::WaitingAtCapacity, - // TODO: Document. - _ => unreachable!(), }; break; @@ -166,15 +173,8 @@ impl ClosestDisjointPeersIter { } } - // TODO: Collects all Iterators into a Vec and again returns an Iterator. Can we do better? pub fn into_result(self) -> impl Iterator { - self.iters - .into_iter() - .fold(vec![], |mut acc, iter| { - acc.extend(iter.into_result()); - acc - }) - .into_iter() + self.iters.into_iter().flat_map(|i| i.into_result()) } } From 9d94642adee2f519a0938beaa3ca2e597e31a760 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 29 Apr 2020 20:41:27 +0200 Subject: [PATCH 15/63] protocols/kad: Fix intra doc link --- protocols/kad/src/query.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index e775bc428f2..da6e04a0255 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -208,7 +208,7 @@ pub struct QueryConfig { /// Whether to use disjoint paths on iterative lookups. /// - /// See [`crate::behaviour::KademliaConfig::enable_disjoint_path_queries`] for details. + /// See [`crate::behaviour::KademliaConfig::use_disjoint_path_queries`] for details. pub use_disjoint_paths: bool, } From 2660a751b2cf853b0374cecedecb4ca16cfe2f08 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 29 Apr 2020 22:08:38 +0200 Subject: [PATCH 16/63] protocols/kad/query/disjoint: Ignore failures from other iterators --- protocols/kad/src/query/peers/closest/disjoint.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index fc64a4d18e0..66e6fcaa94e 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -82,7 +82,12 @@ impl ClosestDisjointPeersIter { } pub fn on_failure(&mut self, peer: &PeerId) { - self.iters[self.yielded_peers[peer]].on_failure(peer); + // All peer failures are reported to all queries and thus to all peer + // iterators. If this iterator never started a request to the given peer + // ignore the failure. + if let Some(index) = self.yielded_peers.get(peer) { + self.iters[*index].on_failure(peer) + } } pub fn on_success(&mut self, peer: &PeerId, closer_peers: I) From 9e9224c6e29f6a40427505b7ae204d534fc199d1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 14:36:26 +0200 Subject: [PATCH 17/63] protocols/kad/query/disjoint: Shorten comment --- protocols/kad/src/query/peers/closest/disjoint.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 66e6fcaa94e..320a53287e9 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -24,15 +24,16 @@ use libp2p_core::PeerId; use std::collections::HashMap; use wasm_timer::Instant; -/// Wraps around a set of `ClosestPeersIter`, enforcing a disjoint discovery path per configured -/// parallelism according to the S/Kademlia paper. +/// Wraps around a set of `ClosestPeersIter`, enforcing a disjoint discovery +/// path per configured parallelism according to the S/Kademlia paper. pub struct ClosestDisjointPeersIter { iters: Vec, /// Mapping of yielded peers to iterator that yielded them. /// - /// More specifically index into the `ClosestDisjointPeersIter::iters` vector. On the one hand - /// this is used to link responses from remote peers back to the corresponding iterator, on the - /// other hand it is used to track which peers have been contacted in the past. + /// More specifically index into `iters`. On the one hand this is used to + /// link responses from remote peers back to the corresponding iterator, on + /// the other hand it is used to track which peers have been contacted by + /// which iterator. yielded_peers: HashMap, /// Index of the iterator last queried. last_quiered: usize, From 0d7ba7450531f1823af1b7a738ed12299f86fba9 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 14:39:38 +0200 Subject: [PATCH 18/63] protocols/kad: Fix typo --- protocols/kad/src/query/peers/closest/disjoint.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 320a53287e9..f0cb9d6bc49 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -36,7 +36,7 @@ pub struct ClosestDisjointPeersIter { /// which iterator. yielded_peers: HashMap, /// Index of the iterator last queried. - last_quiered: usize, + last_queried: usize, } impl ClosestDisjointPeersIter { @@ -78,7 +78,7 @@ impl ClosestDisjointPeersIter { iters, yielded_peers: HashMap::new(), // Wraps around, thus iterator 0 will be queried first. - last_quiered: iters_len - 1, + last_queried: iters_len - 1, } } @@ -109,13 +109,13 @@ impl ClosestDisjointPeersIter { // to query the previously queried iterator last. let iter_order = { let mut all = (0..self.iters.len()).collect::>(); - let mut next_up = all.split_off(self.last_quiered + 1); + let mut next_up = all.split_off(self.last_queried + 1); next_up.append(&mut all); next_up }; for i in &mut iter_order.into_iter() { - self.last_quiered = i; + self.last_queried = i; let iter = &mut self.iters[i]; loop { From ff358aa2708e13599f5e1ead396ad99b1f336fc1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 14:47:20 +0200 Subject: [PATCH 19/63] protocols/kad/query/disjoint: Prevent index panics Make sure `on_success` and `is_waiting` do not panic when called with an unknown peer id. --- protocols/kad/src/query/peers/closest/disjoint.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index f0cb9d6bc49..a6da2b2c48d 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -95,11 +95,17 @@ impl ClosestDisjointPeersIter { where I: IntoIterator, { - self.iters[self.yielded_peers[peer]].on_success(peer, closer_peers); + if let Some(index) = self.yielded_peers.get(peer) { + self.iters[*index].on_success(peer, closer_peers); + } } pub fn is_waiting(&self, peer: &PeerId) -> bool { - self.iters[self.yielded_peers[peer]].is_waiting(peer) + if let Some(index) = self.yielded_peers.get(peer) { + self.iters[*index].is_waiting(peer) + } else { + false + } } pub fn next(&mut self, now: Instant) -> PeersIterState { From dd714a663840d8b1c7a5c4ba69c0708eaf78362c Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 15:12:07 +0200 Subject: [PATCH 20/63] protocols/kad/query/disjoint: Track state with Option --- .../kad/src/query/peers/closest/disjoint.rs | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index a6da2b2c48d..81e7e723649 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -109,7 +109,7 @@ impl ClosestDisjointPeersIter { } pub fn next(&mut self, now: Instant) -> PeersIterState { - let mut state = PeersIterState::Finished; + let mut state = None; // Order in which to query the iterators to ensure fairness. Make sure // to query the previously queried iterator last. @@ -128,17 +128,22 @@ impl ClosestDisjointPeersIter { match iter.next(now) { PeersIterState::Waiting(None) => { match state { - PeersIterState::Waiting(Some(_)) => { + Some(PeersIterState::Waiting(Some(_))) => { // [`ClosestDisjointPeersIter::next`] returns immediately once a // [`ClosestPeersIter`] yielded a peer. Thus this state is // unreachable. unreachable!(); }, - PeersIterState::Waiting(None) => {} - PeersIterState::WaitingAtCapacity => { - state = PeersIterState::Waiting(None) + Some(PeersIterState::Waiting(None)) => {} + Some(PeersIterState::WaitingAtCapacity) => { + state = Some(PeersIterState::Waiting(None)) } - PeersIterState::Finished => state = PeersIterState::Waiting(None), + Some(PeersIterState::Finished) => { + // `state` is never set to `Finished`. + unreachable!(); + } + None => state = Some(PeersIterState::Waiting(None)), + }; break; @@ -158,15 +163,19 @@ impl ClosestDisjointPeersIter { } PeersIterState::WaitingAtCapacity => { match state { - PeersIterState::Waiting(Some(_)) => { + Some(PeersIterState::Waiting(Some(_))) => { // [`ClosestDisjointPeersIter::next`] returns immediately once a // [`ClosestPeersIter`] yielded a peer. Thus this state is // unreachable. unreachable!(); }, - PeersIterState::Waiting(None) => {} - PeersIterState::WaitingAtCapacity => {} - PeersIterState::Finished => state = PeersIterState::WaitingAtCapacity, + Some(PeersIterState::Waiting(None)) => {} + Some(PeersIterState::WaitingAtCapacity) => {} + Some(PeersIterState::Finished) => { + // `state` is never set to `Finished`. + unreachable!(); + }, + None => state = Some(PeersIterState::WaitingAtCapacity), }; break; @@ -176,7 +185,7 @@ impl ClosestDisjointPeersIter { } } - state + state.unwrap_or(PeersIterState::Finished) } pub fn finish(&mut self) { From 20650ee8153f24ee6da4cd1dd1876e4dcef87468 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 15:33:27 +0200 Subject: [PATCH 21/63] protocols/kad/query/disjoint: Add comment --- protocols/kad/src/query/peers/closest/disjoint.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 81e7e723649..b7957274454 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -136,6 +136,8 @@ impl ClosestDisjointPeersIter { }, Some(PeersIterState::Waiting(None)) => {} Some(PeersIterState::WaitingAtCapacity) => { + // At least one ClosestPeersIter is no longer at capacity, thus the + // composite ClosestDisjointPeersIter is no longer at capacity. state = Some(PeersIterState::Waiting(None)) } Some(PeersIterState::Finished) => { From 164c753961cd8699e0791e2069ace11eb5584a0c Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 15:33:58 +0200 Subject: [PATCH 22/63] protocols/kad/query/disjoint: Reuse Arbitrary implementations --- .../kad/src/query/peers/closest/disjoint.rs | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index b7957274454..c31057686be 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -238,11 +238,29 @@ mod tests { use rand::{Rng, seq::SliceRandom}; use std::collections::HashSet; + #[derive(Debug, Clone)] + struct Parallelism(usize); + + impl Arbitrary for Parallelism{ + fn arbitrary(g: &mut G) -> Self { + Parallelism(g.gen_range(1, 10)) + } + } + + #[derive(Debug, Clone)] + struct NumResults(usize); + + impl Arbitrary for NumResults{ + fn arbitrary(g: &mut G) -> Self { + NumResults(g.gen_range(1, K_VALUE.get())) + } + } + impl Arbitrary for ClosestPeersIterConfig { fn arbitrary(g: &mut G) -> Self { ClosestPeersIterConfig { - parallelism: g.gen::() as usize, - num_results: g.gen::() as usize, + parallelism: Parallelism::arbitrary(g).0, + num_results: NumResults::arbitrary(g).0, peer_timeout: Duration::from_secs(1), } } @@ -425,24 +443,6 @@ mod tests { known_peers: Vec, } - #[derive(Debug, Clone)] - struct Parallelism(usize); - - impl Arbitrary for Parallelism{ - fn arbitrary(g: &mut G) -> Self { - Parallelism(g.gen_range(1, 10)) - } - } - - #[derive(Debug, Clone)] - struct NumResults(usize); - - impl Arbitrary for NumResults{ - fn arbitrary(g: &mut G) -> Self { - NumResults(g.gen_range(1, K_VALUE.get())) - } - } - enum PeerIterator { Disjoint(ClosestDisjointPeersIter), Closest(ClosestPeersIter), From 244aaa4a9577b0092a0ea5ec5d8e5c14b649efdd Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 15:34:20 +0200 Subject: [PATCH 23/63] protocols/kad/query/disjoint: Rename test --- protocols/kad/src/query/peers/closest/disjoint.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index c31057686be..86ee5277920 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -281,7 +281,7 @@ mod tests { } #[test] - fn split_num_results_per_disjoint_path_quickcheck() { + fn num_results_distribution() { fn prop(config: ClosestPeersIterConfig) -> TestResult { if config.parallelism == 0 || config.num_results == 0 { From 8ea3c1bc8bb166a8b25308914f971546ba6d71d9 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 6 May 2020 14:19:33 +0200 Subject: [PATCH 24/63] protocols/kad/query/disjoint: Allow peer on multiple paths Given a path that yields a peer already querried by another path, it is safe to have the path include the peer in its closest peers set. It is not safe to return the list of peers returned by the querried path, as otherwise the paths would merge. This commit does the former without the latter. --- protocols/kad/src/query/peers/closest.rs | 1 - .../kad/src/query/peers/closest/disjoint.rs | 352 ++++++++++++------ 2 files changed, 232 insertions(+), 121 deletions(-) diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index d8cb8982f57..58095d64d87 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -57,7 +57,6 @@ pub struct ClosestPeersIterConfig { /// The `α` parameter in the Kademlia paper. The maximum number of peers that /// the iterator is allowed to wait for in parallel while iterating towards the closest /// nodes to a target. Defaults to `ALPHA_VALUE`. - // TODO: Should this be configurable? pub parallelism: usize, /// Number of results (closest peers) to search for. diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 86ee5277920..c52f1d01d94 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -21,12 +21,13 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; use libp2p_core::PeerId; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use wasm_timer::Instant; /// Wraps around a set of `ClosestPeersIter`, enforcing a disjoint discovery /// path per configured parallelism according to the S/Kademlia paper. pub struct ClosestDisjointPeersIter { + config: ClosestPeersIterConfig, iters: Vec, /// Mapping of yielded peers to iterator that yielded them. /// @@ -34,11 +35,35 @@ pub struct ClosestDisjointPeersIter { /// link responses from remote peers back to the corresponding iterator, on /// the other hand it is used to track which peers have been contacted by /// which iterator. - yielded_peers: HashMap, + contacted_peers: HashMap, /// Index of the iterator last queried. last_queried: usize, } +struct IteratorIndex(usize); + +struct PeerState { + initiated_by: IteratorIndex, + additionally_awaited_by: Vec, + response: ResponseState, +} + +impl PeerState { + fn new(initiated_by: IteratorIndex) -> Self { + PeerState { + initiated_by, + additionally_awaited_by: vec![], + response: ResponseState::Waiting, + } + } +} + +enum ResponseState { + Waiting, + Succeeded, + Failed, +} + impl ClosestDisjointPeersIter { /// Creates a new iterator with a default configuration. pub fn new(target: KeyBytes, known_closest_peers: I) -> Self @@ -63,20 +88,20 @@ impl ClosestDisjointPeersIter { T: Into + Clone, { let peers = known_closest_peers.into_iter().take(K_VALUE.get()).collect::>(); - let iters = split_num_results_per_disjoint_path(&config) - .into_iter() + let iters = (0..config.parallelism) // NOTE: All [`ClosestPeersIter`] share the same set of peers at - // initialization. The [`ClosestDisjointPeersIter.yielded_peers`] + // initialization. The [`ClosestDisjointPeersIter.contacted_peers`] // ensures a peer is only ever queried by a single // [`ClosestPeersIter`]. - .map(|config| ClosestPeersIter::with_config(config, target.clone(), peers.clone())) + .map(|_| ClosestPeersIter::with_config(config.clone(), target.clone(), peers.clone())) .collect::>(); let iters_len = iters.len(); ClosestDisjointPeersIter { + config, iters, - yielded_peers: HashMap::new(), + contacted_peers: HashMap::new(), // Wraps around, thus iterator 0 will be queried first. last_queried: iters_len - 1, } @@ -86,8 +111,14 @@ impl ClosestDisjointPeersIter { // All peer failures are reported to all queries and thus to all peer // iterators. If this iterator never started a request to the given peer // ignore the failure. - if let Some(index) = self.yielded_peers.get(peer) { - self.iters[*index].on_failure(peer) + if let Some(PeerState{ initiated_by, additionally_awaited_by, response }) = self.contacted_peers.get_mut(peer) { + *response = ResponseState::Failed; + + self.iters[initiated_by.0].on_failure(peer); + + for i in additionally_awaited_by { + self.iters[i.0].on_failure(peer); + } } } @@ -95,17 +126,28 @@ impl ClosestDisjointPeersIter { where I: IntoIterator, { - if let Some(index) = self.yielded_peers.get(peer) { - self.iters[*index].on_success(peer, closer_peers); + if let Some(PeerState{ initiated_by, additionally_awaited_by, response }) = self.contacted_peers.get_mut(peer) { + *response = ResponseState::Succeeded; + + self.iters[initiated_by.0].on_success(peer, closer_peers); + + for i in additionally_awaited_by { + // TODO: Document why empty. + self.iters[i.0].on_success(peer, std::iter::empty()); + } } } pub fn is_waiting(&self, peer: &PeerId) -> bool { - if let Some(index) = self.yielded_peers.get(peer) { - self.iters[*index].is_waiting(peer) - } else { - false + if let Some(PeerState{ initiated_by, additionally_awaited_by, .. }) = self.contacted_peers.get(peer) { + for i in std::iter::once(initiated_by).chain(additionally_awaited_by.iter()) { + if self.iters[i.0].is_waiting(peer) { + return true; + } + } } + + false } pub fn next(&mut self, now: Instant) -> PeersIterState { @@ -151,16 +193,36 @@ impl ClosestDisjointPeersIter { break; } PeersIterState::Waiting(Some(peer)) => { - if self.yielded_peers.contains_key(&*peer) { - // Another iterator already returned this peer. S/Kademlia requires each - // peer to be only used on one path. Marking it as failed for this - // iterator, asking it to return another peer in the next loop - // iteration. - let peer = peer.into_owned(); - iter.on_failure(&peer); - } else { - self.yielded_peers.insert(peer.clone().into_owned(), i); - return PeersIterState::Waiting(Some(Cow::Owned(peer.into_owned()))); + match self.contacted_peers.get_mut(&*peer) { + Some(PeerState{ additionally_awaited_by, response, .. }) => { + // TODO: Update + // Another iterator already returned this peer. S/Kademlia requires each + // peer to be only used on one path. Marking it as failed for this + // iterator, asking it to return another peer in the next loop + // iteration. + let peer = peer.into_owned(); + + additionally_awaited_by.push(IteratorIndex(i)); + + match response { + // TODO document do nothing for now. + ResponseState::Waiting => {}, + ResponseState::Succeeded => { + // TODO: document why not return any new peers. + iter.on_success(&peer, std::iter::empty()); + }, + ResponseState::Failed => { + iter.on_failure(&peer); + }, + } + }, + None => { + self.contacted_peers.insert( + peer.clone().into_owned(), + PeerState::new(IteratorIndex(i)), + ); + return PeersIterState::Waiting(Some(Cow::Owned(peer.into_owned()))); + }, } } PeersIterState::WaitingAtCapacity => { @@ -197,43 +259,36 @@ impl ClosestDisjointPeersIter { } pub fn into_result(self) -> impl Iterator { - self.iters.into_iter().flat_map(|i| i.into_result()) - } -} + let mut result = HashSet::new(); -/// Takes as input a [`ClosestPeersIterConfig`] splits `num_results` for each disjoint path -/// (`== parallelism`) equally (best-effort) into buckets, one for each disjoint path returning a -/// `ClosestPeersIterConfig` for each disjoint path. -/// -/// 'best-effort' as in no more than one apart. E.g. with 10 overall num_result and 4 disjoint paths -/// it would return [3, 3, 2, 2]. -fn split_num_results_per_disjoint_path( - config: &ClosestPeersIterConfig, -) -> Vec { - // Note: The number of parallelism is equal to the number of disjoint paths. - let num_results_per_iter = config.num_results / config.parallelism; - let remaining_num_results = config.num_results % config.parallelism; - - (0..config.parallelism).map(|i| { - let num_results = if i < remaining_num_results { - num_results_per_iter + 1 - } else { - num_results_per_iter - }; + let mut iters = self.iters.into_iter().map(ClosestPeersIter::into_result).collect::>(); - ClosestPeersIterConfig { - parallelism: 1, - num_results, - peer_timeout: config.peer_timeout, + 'outer: loop { + let mut progress = false; + + for iter in iters.iter_mut() { + if let Some(peer) = iter.next() { + progress = true; + result.insert(peer); + if result.len() == self.config.num_results { + break 'outer; + } + } } - }).collect() -} + if !progress { + break; + } + } + + result.into_iter() + } +} #[cfg(test)] mod tests { use super::*; - use crate::{ALPHA_VALUE, K_VALUE}; + use crate::K_VALUE; use quickcheck::*; use rand::{Rng, seq::SliceRandom}; use std::collections::HashSet; @@ -280,38 +335,6 @@ mod tests { } } - #[test] - fn num_results_distribution() { - fn prop(config: ClosestPeersIterConfig) -> TestResult { - if config.parallelism == 0 || config.num_results == 0 - { - return TestResult::discard(); - } - - let mut iters = split_num_results_per_disjoint_path(&config); - - // Ensure the sum of all disjoint paths equals the allowed input. - - assert_eq!( - config.num_results, - iters - .iter() - .fold(0, |acc, config| { acc + config.num_results }), - ); - - // Ensure 'best-effort' fairness, the highest and lowest are newer more than 1 apart. - - iters.sort_by(|a_config, b_config| { - a_config.num_results.cmp(&b_config.num_results) - }); - assert!(iters[iters.len() - 1].num_results - iters[0].num_results <= 1); - - TestResult::passed() - } - - quickcheck(prop as fn(_) -> _) - } - #[test] fn s_kademlia_disjoint_paths() { let now = Instant::now(); @@ -334,7 +357,7 @@ mod tests { }; let mut peers_iter = ClosestDisjointPeersIter::with_config( - config, + config.clone(), target, known_closest_peers.clone(), ); @@ -399,6 +422,15 @@ mod tests { peers_iter.on_success(&peer, vec![]); } + // Mark all remaining peers as succeeded. + for _ in 0..6 { + if let PeersIterState::Waiting(Some(Cow::Owned(peer))) = peers_iter.next(now) { + peers_iter.on_success(&peer, vec![]); + } else { + panic!("Expected iterator to return peer to query."); + } + } + assert_eq!( PeersIterState::Finished, peers_iter.next(now), @@ -406,6 +438,8 @@ mod tests { let final_peers: Vec<_> = peers_iter.into_result().collect(); + assert_eq!(config.num_results, final_peers.len()); + // Expect final result to contain peer from each disjoint path, even though not all are // among the best ones. assert!(final_peers.contains(malicious_response_1[0].preimage())); @@ -417,30 +451,96 @@ mod tests { (0 .. n).map(|_| PeerId::random()) } - #[derive(Debug, Clone)] + #[derive(Clone)] struct Graph(HashMap); + impl std::fmt::Debug for Graph { + fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { + fmt.debug_list().entries(self.0.iter().map(|(id, _)| id)).finish() + } + } + impl Arbitrary for Graph { fn arbitrary(g: &mut G) -> Self { - let mut peers = HashMap::new(); - let mut peer_ids = random_peers(g.gen_range(K_VALUE.get(), 500)) + let mut peer_ids = random_peers(g.gen_range(K_VALUE.get(), 200)) + .map(|peer_id| (peer_id.clone(), Key::from(peer_id))) .collect::>(); - for peer_id in peer_ids.clone() { + // Make each peer aware of its direct neighborhood. + let mut peers = peer_ids.clone().into_iter() + .map(|(peer_id, key)| { + peer_ids.sort_unstable_by(|(_, a), (_, b)| { + key.distance(a).cmp(&key.distance(b)) + }); + + assert_eq!(peer_id, peer_ids[0].0); + + let known_peers = peer_ids.iter() + // Skip itself. + .skip(1) + .take(K_VALUE.get()) + .cloned() + .collect::>(); + + (peer_id, Peer{ known_peers }) + }) + .collect::>(); + + // Make each peer aware of a random set of other peers within the graph. + for (peer_id, peer) in peers.iter_mut() { peer_ids.shuffle(g); - peers.insert(peer_id, Peer{ - known_peers: peer_ids[0..g.gen_range(K_VALUE.get(), peer_ids.len())].to_vec(), - }); + let num_peers = g.gen_range(K_VALUE.get(), peer_ids.len() + 1); + // let num_peers = peer_ids.len(); + let mut random_peer_ids = peer_ids.choose_multiple(g, num_peers) + // Make sure not to include itself. + .filter(|(id, _)| peer_id != id) + .cloned() + .collect::>(); + + peer.known_peers.append(&mut random_peer_ids); + peer.known_peers = std::mem::replace(&mut peer.known_peers, vec![]) + // Deduplicate peer ids. + .into_iter().collect::>().into_iter().collect(); } Graph(peers) } } + impl Graph { + fn get_closest_peer(&self, target: &KeyBytes) -> PeerId { + self.0.iter() + .map(|(peer_id, _)| (target.distance(&Key::new(peer_id.clone())), peer_id)) + .fold(None, |acc, (distance_b, peer_id_b)| { + match acc { + None => Some((distance_b, peer_id_b)), + Some((distance_a, peer_id_a)) => if distance_a < distance_b { + Some((distance_a, peer_id_a)) + } else { + Some((distance_b, peer_id_b)) + } + } + + }) + .expect("Graph to have at least one peer.") + .1.clone() + } + } + #[derive(Debug, Clone)] struct Peer { - known_peers: Vec, + known_peers: Vec<(PeerId, Key)>, + } + + impl Peer { + fn get_closest_peers(&mut self, target: &KeyBytes) -> Vec { + self.known_peers.sort_unstable_by(|(_, a), (_, b)| { + target.distance(a).cmp(&target.distance(b)) + }); + + self.known_peers.iter().take(K_VALUE.get()).map(|(id, _)| id).cloned().collect() + } } enum PeerIterator { @@ -471,21 +571,20 @@ mod tests { } } - /// Ensure [`ClosestPeersIter`] and [`ClosestDisjointPeersIter`] yield similar closest peers. - /// - // NOTE: One can not ensure both iterators yield the *same* result. While [`ClosestPeersIter`] - // always yields the closest peers, [`ClosestDisjointPeersIter`] might not. Imagine a node on - // path x yielding the 22 absolute closest peers, not returned by any other node. In addition - // imagine only 10 results are allowed per path. Path x will choose the 10 closest out of those - // 22 and drop the remaining 12, thus the overall [`ClosestDisjointPeersIter`] will not yield - // the absolute closest peers combining all paths. + /// Ensure [`ClosestPeersIter`] and [`ClosestDisjointPeersIter`] yield same closest peers. #[test] - fn closest_and_disjoint_closest_yield_similar_result() { - fn prop(graph: Graph, parallelism: Parallelism, num_results: NumResults) { + fn closest_and_disjoint_closest_yield_same_result() { + fn prop(graph: Graph, parallelism: Parallelism, num_results: NumResults) -> TestResult { + // TODO: Don't enforce this in a test but in the implementation itself as well. + if parallelism.0 > num_results.0 { + return TestResult::discard(); + } + let target: KeyBytes = Key::from(PeerId::random()).into(); + let closest_peer = graph.get_closest_peer(&target); let mut known_closest_peers = graph.0.iter() - .take(parallelism.0 * ALPHA_VALUE.get()) + .take(K_VALUE.get()) .map(|(key, _peers)| Key::new(key.clone())) .collect::>(); known_closest_peers.sort_unstable_by(|a, b| { @@ -504,7 +603,7 @@ mod tests { target.clone(), known_closest_peers.clone(), )), - &graph, + graph.clone(), &target, ); @@ -514,29 +613,38 @@ mod tests { target.clone(), known_closest_peers.clone(), )), - &graph, + graph.clone(), &target, ); + assert!( + closest.contains(&closest_peer), + "Expected ClosestPeersIter to find closest peer.", + ); + assert!( + disjoint.contains(&closest_peer), + "Expected ClosestDisjointPeersIter to find closest peer.", + ); + assert_eq!(closest.len(), disjoint.len()); if closest != disjoint { let closest_only = closest.difference(&disjoint).collect::>(); let disjoint_only = disjoint.difference(&closest).collect::>(); - if closest_only.len() > num_results.0 / 2 { - panic!( - "Expected both iterators to derive same peer set or be no more than \ - `(num_results / 2)` apart, but only `ClosestPeersIter` got {:?} and only \ - `ClosestDisjointPeersIter` got {:?}.", - closest_only, disjoint_only, - ); - } + + panic!( + "Expected both iterators to derive same peer set, but only `ClosestPeersIter` \ + got {:?} and only `ClosestDisjointPeersIter` got {:?}.", + closest_only, disjoint_only, + ); }; + + TestResult::passed() } fn drive_to_finish( mut iter: PeerIterator, - graph: &Graph, + mut graph: Graph, target: &KeyBytes, ) -> HashSet { let now = Instant::now(); @@ -544,9 +652,13 @@ mod tests { match iter.next(now) { PeersIterState::Waiting(Some(peer_id)) => { let peer_id = peer_id.clone().into_owned(); - iter.on_success(&peer_id, graph.0.get(&peer_id).unwrap().known_peers.clone()) + let closest_peers = graph.0.get_mut(&peer_id) + .unwrap() + .get_closest_peers(&target); + iter.on_success(&peer_id, closest_peers); } , - PeersIterState::WaitingAtCapacity | PeersIterState::Waiting(None) => panic!("There are never more than one requests in flight."), + PeersIterState::WaitingAtCapacity | PeersIterState::Waiting(None) => + panic!("There are never more than one requests in flight."), PeersIterState::Finished => break, } } @@ -558,6 +670,6 @@ mod tests { result.into_iter().map(|k| k.into_preimage()).collect() } - QuickCheck::new().tests(10).quickcheck(prop as fn(_, _, _)) + QuickCheck::new().tests(10).quickcheck(prop as fn(_, _, _) -> _) } } From 79105d5a736fb6eccbf2780258314cdebb020022 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 11 May 2020 09:36:14 +0200 Subject: [PATCH 25/63] protocols/kad/query/disjoint: Rework IteratorIndex --- .../kad/src/query/peers/closest/disjoint.rs | 164 ++++++++++++------ 1 file changed, 111 insertions(+), 53 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index c52f1d01d94..68447737a99 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -21,47 +21,27 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; use libp2p_core::PeerId; -use std::collections::{HashMap, HashSet}; +use std::{collections::{HashMap, HashSet}, ops::{Add, Index, IndexMut}}; use wasm_timer::Instant; -/// Wraps around a set of `ClosestPeersIter`, enforcing a disjoint discovery +/// Wraps around a set of [`ClosestPeersIter`], enforcing a disjoint discovery /// path per configured parallelism according to the S/Kademlia paper. pub struct ClosestDisjointPeersIter { config: ClosestPeersIterConfig, + + /// The set of wrapped [`ClosestPeersIter`]. iters: Vec, - /// Mapping of yielded peers to iterator that yielded them. + + /// Mapping of contacted peers by their [`PeerId`] to [`PeerState`] + /// containing the corresponding iterator indices as well as the response + /// state. /// - /// More specifically index into `iters`. On the one hand this is used to - /// link responses from remote peers back to the corresponding iterator, on - /// the other hand it is used to track which peers have been contacted by - /// which iterator. + /// Used to track which iterator contacted which peer. See [`PeerState`] + /// for details. contacted_peers: HashMap, - /// Index of the iterator last queried. - last_queried: usize, -} - -struct IteratorIndex(usize); - -struct PeerState { - initiated_by: IteratorIndex, - additionally_awaited_by: Vec, - response: ResponseState, -} - -impl PeerState { - fn new(initiated_by: IteratorIndex) -> Self { - PeerState { - initiated_by, - additionally_awaited_by: vec![], - response: ResponseState::Waiting, - } - } -} -enum ResponseState { - Waiting, - Succeeded, - Failed, + /// Index of the iterator last queried. + last_queried: IteratorIndex, } impl ClosestDisjointPeersIter { @@ -91,8 +71,9 @@ impl ClosestDisjointPeersIter { let iters = (0..config.parallelism) // NOTE: All [`ClosestPeersIter`] share the same set of peers at // initialization. The [`ClosestDisjointPeersIter.contacted_peers`] - // ensures a peer is only ever queried by a single - // [`ClosestPeersIter`]. + // mapping ensures that a successful response from a peer is only + // ever passed to a single [`ClosestPeersIter`]. See + // [`ClosestDisjointPeersIter::on_success`] for details. .map(|_| ClosestPeersIter::with_config(config.clone(), target.clone(), peers.clone())) .collect::>(); @@ -103,7 +84,7 @@ impl ClosestDisjointPeersIter { iters, contacted_peers: HashMap::new(), // Wraps around, thus iterator 0 will be queried first. - last_queried: iters_len - 1, + last_queried: (iters_len - 1).into(), } } @@ -114,10 +95,10 @@ impl ClosestDisjointPeersIter { if let Some(PeerState{ initiated_by, additionally_awaited_by, response }) = self.contacted_peers.get_mut(peer) { *response = ResponseState::Failed; - self.iters[initiated_by.0].on_failure(peer); + self.iters[*initiated_by].on_failure(peer); for i in additionally_awaited_by { - self.iters[i.0].on_failure(peer); + self.iters[*i].on_failure(peer); } } } @@ -127,13 +108,20 @@ impl ClosestDisjointPeersIter { I: IntoIterator, { if let Some(PeerState{ initiated_by, additionally_awaited_by, response }) = self.contacted_peers.get_mut(peer) { + // Mark the response as succeeded for future iterators querying this + // peer. There is no need to keep the `closer_peers` around, given + // that they are only passed to the first iterator. *response = ResponseState::Succeeded; - self.iters[initiated_by.0].on_success(peer, closer_peers); + // Pass the new `closer_peers` to the iterator that first contacted + // the peer. + self.iters[*initiated_by].on_success(peer, closer_peers); for i in additionally_awaited_by { - // TODO: Document why empty. - self.iters[i.0].on_success(peer, std::iter::empty()); + // Only report the success to all remaining not-first iterators. + // Do not pass the `closer_peers` in order to uphold the + // S/Kademlia disjoint paths guarantee. + self.iters[*i].on_success(peer, std::iter::empty()); } } } @@ -141,7 +129,7 @@ impl ClosestDisjointPeersIter { pub fn is_waiting(&self, peer: &PeerId) -> bool { if let Some(PeerState{ initiated_by, additionally_awaited_by, .. }) = self.contacted_peers.get(peer) { for i in std::iter::once(initiated_by).chain(additionally_awaited_by.iter()) { - if self.iters[i.0].is_waiting(peer) { + if self.iters[*i].is_waiting(peer) { return true; } } @@ -156,8 +144,8 @@ impl ClosestDisjointPeersIter { // Order in which to query the iterators to ensure fairness. Make sure // to query the previously queried iterator last. let iter_order = { - let mut all = (0..self.iters.len()).collect::>(); - let mut next_up = all.split_off(self.last_queried + 1); + let mut all = (0..self.iters.len()).map(Into::into).collect::>(); + let mut next_up = all.split_off((self.last_queried + 1).into()); next_up.append(&mut all); next_up }; @@ -195,20 +183,21 @@ impl ClosestDisjointPeersIter { PeersIterState::Waiting(Some(peer)) => { match self.contacted_peers.get_mut(&*peer) { Some(PeerState{ additionally_awaited_by, response, .. }) => { - // TODO: Update - // Another iterator already returned this peer. S/Kademlia requires each - // peer to be only used on one path. Marking it as failed for this - // iterator, asking it to return another peer in the next loop - // iteration. - let peer = peer.into_owned(); + // Another iterator already contacted this peer. - additionally_awaited_by.push(IteratorIndex(i)); + let peer = peer.into_owned(); + additionally_awaited_by.push(i); match response { - // TODO document do nothing for now. + // The iterator will be notified later whether the given node + // was successfully contacted or not. See + // [`ClosestDisjointPeersIter::on_success`] for details. ResponseState::Waiting => {}, ResponseState::Succeeded => { - // TODO: document why not return any new peers. + // Given that iterator was not the first to contact the peer + // it will not be made aware of the closer peers discovered + // to uphold the S/Kademlia disjoint paths guarantee. See + // [`ClosestDisjointPeersIter::on_success`] for details. iter.on_success(&peer, std::iter::empty()); }, ResponseState::Failed => { @@ -217,9 +206,10 @@ impl ClosestDisjointPeersIter { } }, None => { + // The iterator is the first to contact this peer. self.contacted_peers.insert( peer.clone().into_owned(), - PeerState::new(IteratorIndex(i)), + PeerState::new(i), ); return PeersIterState::Waiting(Some(Cow::Owned(peer.into_owned()))); }, @@ -284,6 +274,74 @@ impl ClosestDisjointPeersIter { result.into_iter() } } + +/// Index into the [`ClosestDisjointPeersIter`] `iters` vector. +#[derive(Copy, Clone)] +struct IteratorIndex(usize); + +impl From for IteratorIndex { + fn from(i: usize) -> Self { + IteratorIndex(i) + } +} + +impl From for usize { + fn from(i: IteratorIndex) -> Self { + i.0 + } +} + +impl Add for IteratorIndex { + type Output = Self; + + fn add(self, rhs: usize) -> Self::Output { + (self.0 + rhs).into() + } +} + +impl Index for Vec { + type Output = ClosestPeersIter; + + fn index(&self, index: IteratorIndex) -> &Self::Output { + &self[index.0] + } +} + +impl IndexMut for Vec { + fn index_mut(&mut self, index: IteratorIndex) -> &mut Self::Output { + &mut self[index.0] + } +} + +/// State tracking the iterators that yielded (i.e. tried to contact) a peer. See +/// [`ClosestDisjointPeersIter::on_success`] for details. +struct PeerState { + /// First iterator to yield the peer. Will be notified both of the outcome + /// (success/failure) as well as the closer peers. + initiated_by: IteratorIndex, + /// Additional iterators only notified of the outcome (success/failure), not + /// the closer peers, in order to uphold the S/Kademlia disjoint paths + /// guarantee. + additionally_awaited_by: Vec, + response: ResponseState, +} + +impl PeerState { + fn new(initiated_by: IteratorIndex) -> Self { + PeerState { + initiated_by, + additionally_awaited_by: vec![], + response: ResponseState::Waiting, + } + } +} + +enum ResponseState { + Waiting, + Succeeded, + Failed, +} + #[cfg(test)] mod tests { use super::*; From fc727e58e4c705071734eddd7898b5ba626e966d Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 11 May 2020 15:49:43 +0200 Subject: [PATCH 26/63] protocols/kad/*: Make query parallelism configurable --- protocols/kad/src/behaviour.rs | 13 ++++++ protocols/kad/src/query.rs | 14 ++++-- protocols/kad/src/query/peers/closest.rs | 43 ++++++++++--------- .../kad/src/query/peers/closest/disjoint.rs | 24 +++++++---- protocols/kad/src/query/peers/fixed.rs | 9 ++-- 5 files changed, 66 insertions(+), 37 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 19627627ac7..f032e4e69fc 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -150,6 +150,19 @@ impl KademliaConfig { self } + /// Sets the allowed level of parallelism. + /// + /// The `α` parameter in the Kademlia paper. The maximum number of peers that + /// the iterator is allowed to wait for in parallel while iterating towards the closest + /// nodes to a target. Defaults to `ALPHA_VALUE`. + /// + /// When used with [`KademliaConfig::use_disjoint_path_queries`] it equals the amount of + /// disjoint paths used. + pub fn set_parallelism(&mut self, parallelism: NonZeroUsize) -> &mut Self { + self.query_config.parallelism = parallelism; + self + } + /// Enable queries to use disjoint paths when iteratively looking for the /// closest node to a target. /// diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index da6e04a0255..5a6dfb9849c 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -24,7 +24,7 @@ use peers::PeersIterState; use peers::closest::{ClosestPeersIterConfig, ClosestPeersIter, disjoint::ClosestDisjointPeersIter}; use peers::fixed::FixedPeersIter; -use crate::K_VALUE; +use crate::{ALPHA_VALUE, K_VALUE}; use crate::kbucket::{Key, KeyBytes}; use either::Either; use fnv::FnvHashMap; @@ -92,7 +92,8 @@ impl QueryPool { I: IntoIterator> { let peers = peers.into_iter().map(|k| k.into_preimage()).collect::>(); - let parallelism = self.config.replication_factor.get(); + // TODO: Which parallelism should this use? + let parallelism = self.config.replication_factor; let peer_iter = QueryPeerIter::Fixed(FixedPeersIter::new(peers, parallelism)); self.add(peer_iter, inner) } @@ -105,7 +106,8 @@ impl QueryPool { I: IntoIterator> { let cfg = ClosestPeersIterConfig { - num_results: self.config.replication_factor.get(), + num_results: self.config.replication_factor, + parallelism: self.config.parallelism, .. ClosestPeersIterConfig::default() }; @@ -206,6 +208,11 @@ pub struct QueryConfig { /// See [`crate::behaviour::KademliaConfig::set_replication_factor`] for details. pub replication_factor: NonZeroUsize, + /// Allowed level of parallelism. + /// + /// See [`crate::behaviour::KademliaConfig::set_parallelism`] for details. + pub parallelism: NonZeroUsize, + /// Whether to use disjoint paths on iterative lookups. /// /// See [`crate::behaviour::KademliaConfig::use_disjoint_path_queries`] for details. @@ -217,6 +224,7 @@ impl Default for QueryConfig { QueryConfig { timeout: Duration::from_secs(60), replication_factor: NonZeroUsize::new(K_VALUE.get()).expect("K_VALUE > 0"), + parallelism: ALPHA_VALUE, use_disjoint_paths: false, } } diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index 58095d64d87..af93d81ed2e 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -23,7 +23,7 @@ use super::*; use crate::{K_VALUE, ALPHA_VALUE}; use crate::kbucket::{Key, KeyBytes, Distance}; use libp2p_core::PeerId; -use std::{time::Duration, iter::FromIterator}; +use std::{time::Duration, iter::FromIterator, num::NonZeroUsize}; use std::collections::btree_map::{BTreeMap, Entry}; use wasm_timer::Instant; @@ -57,13 +57,13 @@ pub struct ClosestPeersIterConfig { /// The `α` parameter in the Kademlia paper. The maximum number of peers that /// the iterator is allowed to wait for in parallel while iterating towards the closest /// nodes to a target. Defaults to `ALPHA_VALUE`. - pub parallelism: usize, + pub parallelism: NonZeroUsize, /// Number of results (closest peers) to search for. /// /// The number of closest peers for which the iterator must obtain successful results /// in order to finish successfully. Defaults to `K_VALUE`. - pub num_results: usize, + pub num_results: NonZeroUsize, /// The timeout for a single peer. /// @@ -77,8 +77,8 @@ pub struct ClosestPeersIterConfig { impl Default for ClosestPeersIterConfig { fn default() -> Self { ClosestPeersIterConfig { - parallelism: ALPHA_VALUE.get(), - num_results: K_VALUE.get(), + parallelism: ALPHA_VALUE, + num_results: K_VALUE, peer_timeout: Duration::from_secs(10), } } @@ -181,14 +181,14 @@ impl ClosestPeersIter { // than any peer seen so far (i.e. is the first entry), or the iterator did // not yet accumulate enough closest peers. progress = self.closest_peers.keys().next() == Some(&distance) - || num_closest < self.config.num_results; + || num_closest < self.config.num_results.get(); } // Update the iterator state. self.state = match self.state { State::Iterating { no_progress } => { let no_progress = if progress { 0 } else { no_progress + 1 }; - if no_progress >= self.config.parallelism { + if no_progress >= self.config.parallelism.get() { State::Stalled } else { State::Iterating { no_progress } @@ -304,7 +304,7 @@ impl ClosestPeersIter { *cnt += 1; // If `num_results` successful results have been delivered for the // closest peers, the iterator is done. - if *cnt >= self.config.num_results { + if *cnt >= self.config.num_results.get() { self.state = State::Finished; return PeersIterState::Finished } @@ -360,7 +360,7 @@ impl ClosestPeersIter { None } }) - .take(self.config.num_results) + .take(self.config.num_results.get()) } /// Checks if the iterator is at capacity w.r.t. the permitted parallelism. @@ -372,9 +372,9 @@ impl ClosestPeersIter { fn at_capacity(&self) -> bool { match self.state { State::Stalled => self.num_waiting >= usize::max( - self.config.num_results, self.config.parallelism + self.config.num_results.get(), self.config.parallelism.get() ), - State::Iterating { .. } => self.num_waiting >= self.config.parallelism, + State::Iterating { .. } => self.num_waiting >= self.config.parallelism.get(), State::Finished => true } } @@ -472,8 +472,8 @@ mod tests { let known_closest_peers = random_peers(g.gen_range(1, 60)).map(Key::from); let target = Key::from(Into::::into(PeerId::random())); let config = ClosestPeersIterConfig { - parallelism: g.gen_range(1, 10), - num_results: g.gen_range(1, 25), + parallelism: NonZeroUsize::new(g.gen_range(1, 10)).unwrap(), + num_results: NonZeroUsize::new(g.gen_range(1, 25)).unwrap(), peer_timeout: Duration::from_secs(g.gen_range(10, 30)), }; ClosestPeersIter::with_config(config, target, known_closest_peers) @@ -527,7 +527,7 @@ mod tests { .map(|e| e.key.clone()) .collect::>(); let num_known = expected.len(); - let max_parallelism = usize::min(iter.config.parallelism, num_known); + let max_parallelism = usize::min(iter.config.parallelism.get(), num_known); let target = iter.target.clone(); let mut remaining; @@ -566,7 +566,7 @@ mod tests { // peers or an error, thus finishing the "in-flight requests". for (i, k) in expected.iter().enumerate() { if rng.gen_bool(0.75) { - let num_closer = rng.gen_range(0, iter.config.num_results + 1); + let num_closer = rng.gen_range(0, iter.config.num_results.get() + 1); let closer_peers = random_peers(num_closer).collect::>(); remaining.extend(closer_peers.iter().cloned().map(Key::from)); iter.on_success(k.preimage(), closer_peers); @@ -602,16 +602,16 @@ mod tests { assert!(sorted(&target, &closest)); - if closest.len() < num_results { + if closest.len() < num_results.get() { // The iterator returned fewer results than requested. Therefore // either the initial number of known peers must have been // less than the desired number of results, or there must // have been failures. - assert!(num_known < num_results || num_failures > 0); + assert!(num_known < num_results.get() || num_failures > 0); // All peers must have been contacted. assert!(all_contacted, "Not all peers have been contacted."); } else { - assert_eq!(num_results, closest.len(), "Too many results."); + assert_eq!(num_results.get(), closest.len(), "Too many results."); } } @@ -721,7 +721,7 @@ mod tests { fn prop(mut iter: ClosestPeersIter) { iter.state = State::Stalled; - for i in 0..usize::max(iter.config.parallelism, iter.config.num_results) { + for i in 0..usize::max(iter.config.parallelism.get(), iter.config.num_results.get()) { iter.num_waiting = i; assert!( !iter.at_capacity(), @@ -730,7 +730,10 @@ mod tests { ) } - iter.num_waiting = usize::max(iter.config.parallelism, iter.config.num_results); + iter.num_waiting = usize::max( + iter.config.parallelism.get(), + iter.config.num_results.get(), + ); assert!( iter.at_capacity(), "Iterator should be at capacity if `max(parallelism, num_results)` requests are \ diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 68447737a99..f262d64bc69 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -67,8 +67,14 @@ impl ClosestDisjointPeersIter { I: IntoIterator>, T: Into + Clone, { + assert!( + config.parallelism <= config.num_results, + "In order to uphold S/Kademlia's disjoint paths guarantee one \ + needs at least one result per disjoint path (parallelism).", + ); + let peers = known_closest_peers.into_iter().take(K_VALUE.get()).collect::>(); - let iters = (0..config.parallelism) + let iters = (0..config.parallelism.get()) // NOTE: All [`ClosestPeersIter`] share the same set of peers at // initialization. The [`ClosestDisjointPeersIter.contacted_peers`] // mapping ensures that a successful response from a peer is only @@ -260,7 +266,7 @@ impl ClosestDisjointPeersIter { if let Some(peer) = iter.next() { progress = true; result.insert(peer); - if result.len() == self.config.num_results { + if result.len() == self.config.num_results.get() { break 'outer; } } @@ -352,20 +358,20 @@ mod tests { use std::collections::HashSet; #[derive(Debug, Clone)] - struct Parallelism(usize); + struct Parallelism(NonZeroUsize); impl Arbitrary for Parallelism{ fn arbitrary(g: &mut G) -> Self { - Parallelism(g.gen_range(1, 10)) + Parallelism(NonZeroUsize::new(g.gen_range(1, 10)).unwrap()) } } #[derive(Debug, Clone)] - struct NumResults(usize); + struct NumResults(NonZeroUsize); impl Arbitrary for NumResults{ fn arbitrary(g: &mut G) -> Self { - NumResults(g.gen_range(1, K_VALUE.get())) + NumResults(NonZeroUsize::new(g.gen_range(1, K_VALUE.get())).unwrap()) } } @@ -409,8 +415,8 @@ mod tests { let known_closest_peers = pool.split_off(pool.len() - 3); let config = ClosestPeersIterConfig { - parallelism: 3, - num_results: 3, + parallelism: NonZeroUsize::new(3).unwrap(), + num_results: NonZeroUsize::new(3).unwrap(), ..ClosestPeersIterConfig::default() }; @@ -496,7 +502,7 @@ mod tests { let final_peers: Vec<_> = peers_iter.into_result().collect(); - assert_eq!(config.num_results, final_peers.len()); + assert_eq!(config.num_results.get(), final_peers.len()); // Expect final result to contain peer from each disjoint path, even though not all are // among the best ones. diff --git a/protocols/kad/src/query/peers/fixed.rs b/protocols/kad/src/query/peers/fixed.rs index 407d5548026..a292d746982 100644 --- a/protocols/kad/src/query/peers/fixed.rs +++ b/protocols/kad/src/query/peers/fixed.rs @@ -22,12 +22,12 @@ use super::*; use fnv::FnvHashMap; use libp2p_core::PeerId; -use std::{vec, collections::hash_map::Entry}; +use std::{vec, collections::hash_map::Entry, num::NonZeroUsize}; /// A peer iterator for a fixed set of peers. pub struct FixedPeersIter { /// Ther permitted parallelism, i.e. number of pending results. - parallelism: usize, + parallelism: NonZeroUsize, /// The state of peers emitted by the iterator. peers: FnvHashMap, @@ -57,7 +57,7 @@ enum PeerState { } impl FixedPeersIter { - pub fn new(peers: Vec, parallelism: usize) -> Self { + pub fn new(peers: Vec, parallelism: NonZeroUsize) -> Self { Self { parallelism, peers: FnvHashMap::default(), @@ -97,7 +97,7 @@ impl FixedPeersIter { match &mut self.state { State::Finished => return PeersIterState::Finished, State::Waiting { num_waiting } => { - if *num_waiting >= self.parallelism { + if *num_waiting >= self.parallelism.get() { return PeersIterState::WaitingAtCapacity } loop { @@ -132,4 +132,3 @@ impl FixedPeersIter { }) } } - From c63922dd0c2f36ba20da7fdeb33514258361464b Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 12 May 2020 11:37:09 +0200 Subject: [PATCH 27/63] protocols/kad: Clarify parallelism for FixedPeersIter --- protocols/kad/src/behaviour.rs | 36 +++++++++++++++++++++------------- protocols/kad/src/query.rs | 4 +--- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index f032e4e69fc..9878296820a 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -123,8 +123,9 @@ impl Default for KademliaConfig { impl KademliaConfig { /// Sets a custom protocol name. /// - /// Kademlia nodes only communicate with other nodes using the same protocol name. Using a - /// custom name therefore allows to segregate the DHT from others, if that is desired. + /// Kademlia nodes only communicate with other nodes using the same protocol + /// name. Using a custom name therefore allows to segregate the DHT from + /// others, if that is desired. pub fn set_protocol_name(&mut self, name: impl Into>) -> &mut Self { self.protocol_config.set_protocol_name(name); self @@ -152,12 +153,16 @@ impl KademliaConfig { /// Sets the allowed level of parallelism. /// - /// The `α` parameter in the Kademlia paper. The maximum number of peers that - /// the iterator is allowed to wait for in parallel while iterating towards the closest - /// nodes to a target. Defaults to `ALPHA_VALUE`. + /// The `α` parameter in the Kademlia paper. The maximum number of peers + /// that an iterative query is allowed to wait for in parallel while + /// iterating towards the closest nodes to a target.Defaults to + /// `ALPHA_VALUE`. /// - /// When used with [`KademliaConfig::use_disjoint_path_queries`] it equals the amount of - /// disjoint paths used. + /// This only controls the level of parallelism of an iterative query, not + /// the level of parallelism of a query to a fixed set of peers. + /// + /// When used with [`KademliaConfig::use_disjoint_path_queries`] it equals + /// the amount of disjoint paths used. pub fn set_parallelism(&mut self, parallelism: NonZeroUsize) -> &mut Self { self.query_config.parallelism = parallelism; self @@ -179,7 +184,8 @@ impl KademliaConfig { /// Sets the TTL for stored records. /// /// The TTL should be significantly longer than the (re-)publication - /// interval, to avoid premature expiration of records. The default is 36 hours. + /// interval, to avoid premature expiration of records. The default is 36 + /// hours. /// /// `None` means records never expire. /// @@ -213,10 +219,10 @@ impl KademliaConfig { /// Sets the (re-)publication interval of stored records. /// - /// Records persist in the DHT until they expire. By default, published records - /// are re-published in regular intervals for as long as the record exists - /// in the local storage of the original publisher, thereby extending the - /// records lifetime. + /// Records persist in the DHT until they expire. By default, published + /// records are re-published in regular intervals for as long as the record + /// exists in the local storage of the original publisher, thereby extending + /// the records lifetime. /// /// This interval should be significantly shorter than the record TTL, to /// ensure records do not expire prematurely. The default is 24 hours. @@ -242,7 +248,8 @@ impl KademliaConfig { /// Sets the interval at which provider records for keys provided /// by the local node are re-published. /// - /// `None` means that stored provider records are never automatically re-published. + /// `None` means that stored provider records are never automatically + /// re-published. /// /// Must be significantly less than the provider record TTL. pub fn set_provider_publication_interval(&mut self, interval: Option) -> &mut Self { @@ -258,7 +265,8 @@ impl KademliaConfig { /// Modifies the maximum allowed size of individual Kademlia packets. /// - /// It might be necessary to increase this value if trying to put large records. + /// It might be necessary to increase this value if trying to put large + /// records. pub fn set_max_packet_size(&mut self, size: usize) -> &mut Self { self.protocol_config.set_max_packet_size(size); self diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 5a6dfb9849c..7d793e4b633 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -92,14 +92,12 @@ impl QueryPool { I: IntoIterator> { let peers = peers.into_iter().map(|k| k.into_preimage()).collect::>(); - // TODO: Which parallelism should this use? let parallelism = self.config.replication_factor; let peer_iter = QueryPeerIter::Fixed(FixedPeersIter::new(peers, parallelism)); self.add(peer_iter, inner) } - /// Adds a query to the pool that iterates towards the closest peers to the target on disjoint - /// paths. + /// Adds a query to the pool that iterates towards the closest peers to the target. pub fn add_iter_closest(&mut self, target: T, peers: I, inner: TInner) -> QueryId where T: Into + Clone, From c2e168d5589353fc79448538ea40e8e6eeda2ee6 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 13 May 2020 15:25:16 +0200 Subject: [PATCH 28/63] protocols/kad: Improve comments --- protocols/kad/src/behaviour.rs | 10 +++++----- protocols/kad/src/query.rs | 2 +- protocols/kad/src/query/peers/closest/disjoint.rs | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 9878296820a..3437df18cce 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -151,11 +151,11 @@ impl KademliaConfig { self } - /// Sets the allowed level of parallelism. + /// Sets the allowed level of parallelism for iterative queries. /// /// The `α` parameter in the Kademlia paper. The maximum number of peers /// that an iterative query is allowed to wait for in parallel while - /// iterating towards the closest nodes to a target.Defaults to + /// iterating towards the closest nodes to a target. Defaults to /// `ALPHA_VALUE`. /// /// This only controls the level of parallelism of an iterative query, not @@ -168,10 +168,10 @@ impl KademliaConfig { self } - /// Enable queries to use disjoint paths when iteratively looking for the - /// closest node to a target. + /// Require iterative queries to use disjoint paths for increased resiliency + /// in the presence of potentially adversarial nodes. /// - /// When enabled the amount of disjoint paths used equals the configured + /// When enabled the number of disjoint paths used equals the configured /// parallelism. /// /// See the S/Kademlia paper for more information on the high level design diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 7d793e4b633..9317bfaeb8a 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -206,7 +206,7 @@ pub struct QueryConfig { /// See [`crate::behaviour::KademliaConfig::set_replication_factor`] for details. pub replication_factor: NonZeroUsize, - /// Allowed level of parallelism. + /// Allowed level of parallelism for iterative queries. /// /// See [`crate::behaviour::KademliaConfig::set_parallelism`] for details. pub parallelism: NonZeroUsize, diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index f262d64bc69..0df5879dc38 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -722,7 +722,7 @@ mod tests { iter.on_success(&peer_id, closest_peers); } , PeersIterState::WaitingAtCapacity | PeersIterState::Waiting(None) => - panic!("There are never more than one requests in flight."), + panic!("There is never more than one request in flight."), PeersIterState::Finished => break, } } From 58c94471927eb4180d6d5d3c50d29abc2a432798 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 13 May 2020 15:26:45 +0200 Subject: [PATCH 29/63] protocols/kad/query/disjoint: Remove parallelism <= num_results --- protocols/kad/src/query/peers/closest/disjoint.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 0df5879dc38..938e6560ffe 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -67,12 +67,6 @@ impl ClosestDisjointPeersIter { I: IntoIterator>, T: Into + Clone, { - assert!( - config.parallelism <= config.num_results, - "In order to uphold S/Kademlia's disjoint paths guarantee one \ - needs at least one result per disjoint path (parallelism).", - ); - let peers = known_closest_peers.into_iter().take(K_VALUE.get()).collect::>(); let iters = (0..config.parallelism.get()) // NOTE: All [`ClosestPeersIter`] share the same set of peers at @@ -639,7 +633,6 @@ mod tests { #[test] fn closest_and_disjoint_closest_yield_same_result() { fn prop(graph: Graph, parallelism: Parallelism, num_results: NumResults) -> TestResult { - // TODO: Don't enforce this in a test but in the implementation itself as well. if parallelism.0 > num_results.0 { return TestResult::discard(); } From 16fa97f1b6d0983ce128ceab216cf0357362f3f3 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 13 May 2020 15:42:46 +0200 Subject: [PATCH 30/63] protocols/kad/query/disjoint: Remove additionally_awaited_by --- .../kad/src/query/peers/closest/disjoint.rs | 39 +++++++------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 938e6560ffe..e9f4d3b44cb 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -92,13 +92,11 @@ impl ClosestDisjointPeersIter { // All peer failures are reported to all queries and thus to all peer // iterators. If this iterator never started a request to the given peer // ignore the failure. - if let Some(PeerState{ initiated_by, additionally_awaited_by, response }) = self.contacted_peers.get_mut(peer) { + if let Some(PeerState{ response, .. }) = self.contacted_peers.get_mut(peer) { *response = ResponseState::Failed; - self.iters[*initiated_by].on_failure(peer); - - for i in additionally_awaited_by { - self.iters[*i].on_failure(peer); + for iter in &mut self.iters { + iter.on_failure(peer); } } } @@ -107,31 +105,29 @@ impl ClosestDisjointPeersIter { where I: IntoIterator, { - if let Some(PeerState{ initiated_by, additionally_awaited_by, response }) = self.contacted_peers.get_mut(peer) { - // Mark the response as succeeded for future iterators querying this + if let Some(PeerState{ initiated_by, response }) = self.contacted_peers.get_mut(peer) { + // Mark the response as succeeded for future iterators yielding this // peer. There is no need to keep the `closer_peers` around, given // that they are only passed to the first iterator. *response = ResponseState::Succeeded; - // Pass the new `closer_peers` to the iterator that first contacted + // Pass the new `closer_peers` to the iterator that first yielded // the peer. self.iters[*initiated_by].on_success(peer, closer_peers); - for i in additionally_awaited_by { + for iter in &mut self.iters { // Only report the success to all remaining not-first iterators. // Do not pass the `closer_peers` in order to uphold the // S/Kademlia disjoint paths guarantee. - self.iters[*i].on_success(peer, std::iter::empty()); + iter.on_success(peer, std::iter::empty()); } } } pub fn is_waiting(&self, peer: &PeerId) -> bool { - if let Some(PeerState{ initiated_by, additionally_awaited_by, .. }) = self.contacted_peers.get(peer) { - for i in std::iter::once(initiated_by).chain(additionally_awaited_by.iter()) { - if self.iters[*i].is_waiting(peer) { - return true; - } + for iter in &self.iters { + if iter.is_waiting(peer) { + return true; } } @@ -182,11 +178,9 @@ impl ClosestDisjointPeersIter { } PeersIterState::Waiting(Some(peer)) => { match self.contacted_peers.get_mut(&*peer) { - Some(PeerState{ additionally_awaited_by, response, .. }) => { + Some(PeerState{ response, .. }) => { // Another iterator already contacted this peer. - let peer = peer.into_owned(); - additionally_awaited_by.push(i); match response { // The iterator will be notified later whether the given node @@ -313,16 +307,14 @@ impl IndexMut for Vec { } } -/// State tracking the iterators that yielded (i.e. tried to contact) a peer. See +/// State tracking the iterator that yielded (i.e. tried to contact) a peer. See /// [`ClosestDisjointPeersIter::on_success`] for details. struct PeerState { /// First iterator to yield the peer. Will be notified both of the outcome /// (success/failure) as well as the closer peers. initiated_by: IteratorIndex, - /// Additional iterators only notified of the outcome (success/failure), not - /// the closer peers, in order to uphold the S/Kademlia disjoint paths - /// guarantee. - additionally_awaited_by: Vec, + /// Keeping track of the response state. In case other iterators later on + /// yield the same peer, they can be notified of the response outcome. response: ResponseState, } @@ -330,7 +322,6 @@ impl PeerState { fn new(initiated_by: IteratorIndex) -> Self { PeerState { initiated_by, - additionally_awaited_by: vec![], response: ResponseState::Waiting, } } From 2175b16f0be0acdb27abbd5defcd3723d8e48c88 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 15 May 2020 08:05:53 +0200 Subject: [PATCH 31/63] protocols/kad/query/disjoint: Use Iterator::cycle for iterator order --- .../kad/src/query/peers/closest/disjoint.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index e9f4d3b44cb..1047f24ce82 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -137,14 +137,12 @@ impl ClosestDisjointPeersIter { pub fn next(&mut self, now: Instant) -> PeersIterState { let mut state = None; - // Order in which to query the iterators to ensure fairness. Make sure - // to query the previously queried iterator last. - let iter_order = { - let mut all = (0..self.iters.len()).map(Into::into).collect::>(); - let mut next_up = all.split_off((self.last_queried + 1).into()); - next_up.append(&mut all); - next_up - }; + // Order in which to query the iterators to ensure fairness. + let iter_order = (0..self.iters.len()).map(Into::into).cycle() + // Make sure to query the previously queried iterator last. + .skip(Into::::into(self.last_queried) + 1) + // Query each iterator at most once. + .take(self.iters.len()); for i in &mut iter_order.into_iter() { self.last_queried = i; @@ -270,7 +268,7 @@ impl ClosestDisjointPeersIter { } /// Index into the [`ClosestDisjointPeersIter`] `iters` vector. -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] struct IteratorIndex(usize); impl From for IteratorIndex { From 8b12f81f0e82830024396fb0f8412c1d1015944f Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 15 May 2020 12:28:53 +0200 Subject: [PATCH 32/63] protocols/kad/query/disjoint: Introduce ResultIter to returning all peers In the case of no adversarial peers or connectivity issues along any path, all paths return the same result, deduplicated through the `ResultIter`, thus overall `into_result` returns `num_results`. In the case of adversarial peers or connectivity issues `ClosestDisjointPeersIter` tries to return the `num_results` closest benign peers, but as it can not differentiate benign from faulty paths it as well returns faulty peers and thus overall returns more than `num_results` peers. --- .../kad/src/query/peers/closest/disjoint.rs | 264 +++++++++++++++--- 1 file changed, 220 insertions(+), 44 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 1047f24ce82..4299e97740e 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -21,13 +21,14 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; use libp2p_core::PeerId; -use std::{collections::{HashMap, HashSet}, ops::{Add, Index, IndexMut}}; +use std::{collections::{HashMap, HashSet}, iter::{Peekable}, ops::{Add, Index, IndexMut}}; use wasm_timer::Instant; /// Wraps around a set of [`ClosestPeersIter`], enforcing a disjoint discovery /// path per configured parallelism according to the S/Kademlia paper. pub struct ClosestDisjointPeersIter { config: ClosestPeersIterConfig, + target: KeyBytes, /// The set of wrapped [`ClosestPeersIter`]. iters: Vec, @@ -81,6 +82,7 @@ impl ClosestDisjointPeersIter { ClosestDisjointPeersIter { config, + target: target.into(), iters, contacted_peers: HashMap::new(), // Wraps around, thus iterator 0 will be queried first. @@ -240,30 +242,19 @@ impl ClosestDisjointPeersIter { } } + /// Note: In the case of no adversarial peers or connectivity issues along + /// any path, all paths return the same result, deduplicated through + /// the `ResultIter`, thus overall `into_result` returns + /// `num_results`. In the case of adversarial peers or connectivity + /// issues `ClosestDisjointPeersIter` tries to return the + /// `num_results` closest benign peers, but as it can not + /// differentiate benign from faulty paths it as well returns faulty + /// peers and thus overall returns more than `num_results` peers. pub fn into_result(self) -> impl Iterator { - let mut result = HashSet::new(); + let result_per_path= self.iters.into_iter() + .map(|iter| iter.into_result().map(Key::from)); - let mut iters = self.iters.into_iter().map(ClosestPeersIter::into_result).collect::>(); - - 'outer: loop { - let mut progress = false; - - for iter in iters.iter_mut() { - if let Some(peer) = iter.next() { - progress = true; - result.insert(peer); - if result.len() == self.config.num_results.get() { - break 'outer; - } - } - } - - if !progress { - break; - } - } - - result.into_iter() + ResultIter::new(self.target, result_per_path).map(Key::into_preimage) } } @@ -331,6 +322,68 @@ enum ResponseState { Failed, } +/// Iterator combining the result of multiple [`ClosestPeersIter`] into a single +/// deduplicated ordered iterator. +// +// Note: This operates under the assumption that `I` is ordered. +#[derive(Clone, Debug)] +struct ResultIter where + I: Iterator>, +{ + target: KeyBytes, + iters: Vec>, +} + +impl>> ResultIter { + fn new(target: KeyBytes, iters: impl Iterator) -> Self { + ResultIter{ + target, + iters: iters.map(Iterator::peekable).collect(), + } + } +} + +impl>> Iterator for ResultIter { + type Item = I::Item; + + fn next(&mut self) -> Option { + let target = &self.target; + + self.iters.iter_mut() + // Find the iterator with the next closest peer. + .fold( + Option::<&mut Peekable<_>>::None, + |iter_a, iter_b| { + let iter_a = match iter_a { + Some(iter_a) => iter_a, + None => return Some(iter_b), + }; + + match (iter_a.peek(), iter_b.peek()) { + (Some(next_a), Some(next_b)) => { + if next_a == next_b { + // Remove from one for deduplication. + iter_b.next(); + return Some(iter_a) + } + + if target.distance(next_a) < target.distance(next_b) { + Some(iter_a) + } else { + Some(iter_b) + } + }, + (Some(_), None) => Some(iter_a), + (None, Some(_)) => Some(iter_b), + (None, None) => None, + } + }, + ) + // Pop off the next closest peer from that iterator. + .and_then(Iterator::next) + } +} + #[cfg(test)] mod tests { use super::*; @@ -340,6 +393,125 @@ mod tests { use rand::{Rng, seq::SliceRandom}; use std::collections::HashSet; + impl Arbitrary for ResultIter>> { + fn arbitrary(g: &mut G) -> Self { + let target = Target::arbitrary(g).0; + let num_closest_iters = g.gen_range(0, 20 + 1); + let peers = random_peers( + g.gen_range(0, 20 * num_closest_iters + 1), + g, + ); + + let iters: Vec<_> = (0..num_closest_iters) + .map(|_| { + let num_peers = g.gen_range(0, 20 + 1); + let mut peers = peers.choose_multiple(g, num_peers) + .cloned() + .map(Key::from) + .collect::>(); + + peers.sort_unstable_by(|a, b| { + target.distance(a).cmp(&target.distance(b)) + }); + + peers.into_iter() + }) + .collect(); + + ResultIter::new(target, iters.into_iter()) + } + + fn shrink(&self) -> Box> { + let peers = self.iters + .clone() + .into_iter() + .flatten() + .collect::>() + .into_iter() + .collect::>(); + + let iters = self.iters.clone() + .into_iter() + .map(|iter| iter.collect::>()) + .collect(); + + Box::new(ResultIterShrinker { + target: self.target.clone(), + peers, + iters, + }) + } + } + + struct ResultIterShrinker { + target: KeyBytes, + peers: Vec>, + iters: Vec>>, + } + + impl Iterator for ResultIterShrinker { + type Item = ResultIter>>; + + /// Return an iterator of [`ResultIter`]s with each of them missing a + /// different peer from the original set. + fn next(&mut self) -> Option { + // The peer that should not be included. + let peer = self.peers.pop()?; + + let iters = self.iters.clone().into_iter() + .filter_map(|mut iter| { + iter.retain(|p| p != &peer); + if iter.is_empty() { + return None; + } + Some(iter.into_iter()) + }).collect::>(); + + Some(ResultIter::new(self.target.clone(), iters.into_iter())) + } + } + + #[derive(Clone, Debug)] + struct Target(KeyBytes); + + impl Arbitrary for Target { + fn arbitrary(g: &mut G) -> Self { + Target(Key::from(random_peers(1, g).pop().unwrap()).into()) + } + } + + fn random_peers(n: usize, g: &mut R) -> Vec { + (0 .. n).map(|_| PeerId::from_multihash( + multihash::wrap(multihash::Code::Sha2_256, &g.gen::<[u8; 32]>()) + ).unwrap()).collect() + } + + #[test] + fn result_iter_returns_deduplicated_ordered_peer_id_stream() { + fn prop(result_iter: ResultIter>>) { + let expected = { + let mut deduplicated = result_iter.clone() + .iters + .into_iter() + .flatten() + .collect::>() + .into_iter() + .map(Key::from) + .collect::>(); + + deduplicated.sort_unstable_by(|a, b| { + result_iter.target.distance(a).cmp(&result_iter.target.distance(b)) + }); + + deduplicated + }; + + assert_eq!(expected, result_iter.collect::>()); + } + + QuickCheck::new().quickcheck(prop as fn(_)) + } + #[derive(Debug, Clone)] struct Parallelism(NonZeroUsize); @@ -409,7 +581,7 @@ mod tests { known_closest_peers.clone(), ); - ////////////////////////////////////////////////////////////////////////////// + //////////////////////////////////////////////////////////////////////// // First round. for _ in 0..3 { @@ -427,7 +599,8 @@ mod tests { let response_2 = pool.split_off(pool.len() - 3); let response_3 = pool.split_off(pool.len() - 3); - // Keys are closer than any of the previous two responses from honest node 1 and 2. + // Keys are closer than any of the previous two responses from honest + // node 1 and 2. let malicious_response_1 = pool.split_off(pool.len() - 3); // Response from malicious peer 1. @@ -448,7 +621,7 @@ mod tests { response_3.clone().into_iter().map(|k| k.preimage().clone()), ); - ////////////////////////////////////////////////////////////////////////////// + //////////////////////////////////////////////////////////////////////// // Second round. let mut next_to_query = vec![]; @@ -485,19 +658,13 @@ mod tests { let final_peers: Vec<_> = peers_iter.into_result().collect(); - assert_eq!(config.num_results.get(), final_peers.len()); - - // Expect final result to contain peer from each disjoint path, even though not all are - // among the best ones. + // Expect final result to contain peer from each disjoint path, even + // though not all are among the best ones. assert!(final_peers.contains(malicious_response_1[0].preimage())); assert!(final_peers.contains(response_2[0].preimage())); assert!(final_peers.contains(response_3[0].preimage())); } - fn random_peers(n: usize) -> impl Iterator + Clone { - (0 .. n).map(|_| PeerId::random()) - } - #[derive(Clone)] struct Graph(HashMap); @@ -509,7 +676,8 @@ mod tests { impl Arbitrary for Graph { fn arbitrary(g: &mut G) -> Self { - let mut peer_ids = random_peers(g.gen_range(K_VALUE.get(), 200)) + let mut peer_ids = random_peers(g.gen_range(K_VALUE.get(), 200), g) + .into_iter() .map(|peer_id| (peer_id.clone(), Key::from(peer_id))) .collect::>(); @@ -538,7 +706,6 @@ mod tests { peer_ids.shuffle(g); let num_peers = g.gen_range(K_VALUE.get(), peer_ids.len() + 1); - // let num_peers = peer_ids.len(); let mut random_peer_ids = peer_ids.choose_multiple(g, num_peers) // Make sure not to include itself. .filter(|(id, _)| peer_id != id) @@ -626,6 +793,7 @@ mod tests { return TestResult::discard(); } + // TODO: Pass target via prop(). let target: KeyBytes = Key::from(PeerId::random()).into(); let closest_peer = graph.get_closest_peer(&target); @@ -665,23 +833,31 @@ mod tests { assert!( closest.contains(&closest_peer), - "Expected ClosestPeersIter to find closest peer.", + "Expected `ClosestPeersIter` to find closest peer.", ); assert!( disjoint.contains(&closest_peer), - "Expected ClosestDisjointPeersIter to find closest peer.", + "Expected `ClosestDisjointPeersIter` to find closest peer.", ); - assert_eq!(closest.len(), disjoint.len()); + assert!( + closest.len() == num_results.0.get(), + "Expected `ClosestPeersIter` to find `num_results` closest \ + peers." + ); + assert!( + disjoint.len() >= num_results.0.get(), + "Expected `ClosestDisjointPeersIter` to find at least \ + `num_results` closest peers." + ); - if closest != disjoint { + if closest.len() > disjoint.len() { let closest_only = closest.difference(&disjoint).collect::>(); - let disjoint_only = disjoint.difference(&closest).collect::>(); panic!( - "Expected both iterators to derive same peer set, but only `ClosestPeersIter` \ - got {:?} and only `ClosestDisjointPeersIter` got {:?}.", - closest_only, disjoint_only, + "Expected `ClosestDisjointPeersIter` to find all peers \ + found by `ClosestPeersIter`, but it did not find {:?}.", + closest_only, ); }; From fe8941f61b3e45e00210a124935625d35725580c Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 16 May 2020 12:11:47 +0200 Subject: [PATCH 33/63] protocols/kad: Don't finish query early unless all paths are queried --- protocols/kad/Cargo.toml | 1 + protocols/kad/src/behaviour.rs | 61 ++++++++---- protocols/kad/src/behaviour/test.rs | 97 ++++++++++++++++++- protocols/kad/src/query.rs | 20 +++- .../kad/src/query/peers/closest/disjoint.rs | 14 ++- 5 files changed, 162 insertions(+), 31 deletions(-) diff --git a/protocols/kad/Cargo.toml b/protocols/kad/Cargo.toml index b9734d182b6..f79fae2253c 100644 --- a/protocols/kad/Cargo.toml +++ b/protocols/kad/Cargo.toml @@ -30,6 +30,7 @@ unsigned-varint = { version = "0.3", features = ["futures-codec"] } void = "1.0" [dev-dependencies] +futures-timer = "3.0" libp2p-secio = { version = "0.18.0", path = "../secio" } libp2p-yamux = { version = "0.18.0", path = "../../muxers/yamux" } quickcheck = "0.9.0" diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 6e0ee22a66d..c08e0f2f7cd 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -277,7 +277,7 @@ impl Kademlia where for<'a> TStore: RecordStore<'a> { - /// Creates a new `Kademlia` network behaviour with the given configuration. + /// Creates a new `Kademlia` network behaviour with a default configuration. pub fn new(id: PeerId, store: TStore) -> Self { Self::with_config(id, store, Default::default()) } @@ -418,13 +418,16 @@ where if record.is_expired(Instant::now()) { self.store.remove(key) } else { - records.push(record.into_owned()); if quorum.get() == 1 { self.queued_events.push_back(NetworkBehaviourAction::GenerateEvent( - KademliaEvent::GetRecordResult(Ok(GetRecordOk { records })) + KademliaEvent::GetRecordResult(Ok( + GetRecordOk { records: vec![record.into_owned()] }, + )) )); return; } + + records.push((None, record.into_owned())); } } @@ -823,6 +826,7 @@ where } QueryInfo::GetRecord { key, records, quorum, cache_at } => { + let records = records.into_iter().map(|r| r.1).collect::>(); let result = if records.len() >= quorum.get() { // [not empty] if let Some(cache_key) = cache_at { // Cache the record at the closest node to the key that @@ -830,7 +834,7 @@ where let record = records.first().expect("[not empty]").clone(); let quorum = NonZeroUsize::new(1).expect("1 > 0"); let context = PutRecordContext::Cache; - let info = QueryInfo::PutRecord { record, quorum, context, num_results: 0 }; + let info = QueryInfo::PutRecord { record: record, quorum, context, successfull_peers: vec![] }; let inner = QueryInner::new(info); self.queries.add_fixed(iter::once(cache_key.into_preimage()), inner); } @@ -847,18 +851,18 @@ where } QueryInfo::PreparePutRecord { record, quorum, context } => { - let info = QueryInfo::PutRecord { record, quorum, context, num_results: 0 }; + let info = QueryInfo::PutRecord { record, quorum, context, successfull_peers: vec![] }; let inner = QueryInner::new(info); self.queries.add_fixed(result.peers, inner); None } - QueryInfo::PutRecord { record, quorum, num_results, context } => { + QueryInfo::PutRecord { record, quorum, successfull_peers, context } => { let result = |key: record::Key| { - if num_results >= quorum.get() { + if successfull_peers.len() >= quorum.get() { Ok(PutRecordOk { key }) } else { - Err(PutRecordError::QuorumFailed { key, quorum, num_results }) + Err(PutRecordError::QuorumFailed { key, quorum, num_results: successfull_peers.len() }) } }; match context { @@ -939,10 +943,10 @@ where } } - QueryInfo::PutRecord { record, quorum, num_results, context } => { + QueryInfo::PutRecord { record, quorum, successfull_peers, context } => { let err = Err(PutRecordError::Timeout { key: record.key, - num_results, + num_results: successfull_peers.len(), quorum }); match context { @@ -963,7 +967,12 @@ where QueryInfo::GetRecord { key, records, quorum, .. } => Some(KademliaEvent::GetRecordResult(Err( - GetRecordError::Timeout { key, records, quorum }))), + GetRecordError::Timeout { + key, + records: records.into_iter().map(|r| r.1).collect(), + quorum, + } + ))), QueryInfo::GetProviders { key, providers } => Some(KademliaEvent::GetProvidersResult(Err( @@ -1333,9 +1342,16 @@ where key, records, quorum, cache_at } = &mut query.inner.info { if let Some(record) = record { - records.push(record); - if records.len() == quorum.get() { - query.finish() + records.push((Some(source.clone()), record)); + + if records.len() >= quorum.get() { + // Desired quorum reached. The query may finish. See + // [`Query::may_finish`] for details. + let peers = records.iter() + .filter_map(|(peer, _record)| peer.as_ref()) + .cloned() + .collect::>(); + query.may_finish(peers) } } else if quorum.get() == 1 { // It is a "standard" Kademlia query, for which the @@ -1371,11 +1387,12 @@ where if let Some(query) = self.queries.get_mut(&user_data) { query.on_success(&source, vec![]); if let QueryInfo::PutRecord { - num_results, quorum, .. + successfull_peers, quorum, .. } = &mut query.inner.info { - *num_results += 1; - if *num_results == quorum.get() { - query.finish() + successfull_peers.push(source); + if successfull_peers.len() >= quorum.get() { + let peers = successfull_peers.clone(); + query.may_finish(peers) } } } @@ -1905,7 +1922,8 @@ enum QueryInfo { PutRecord { record: Record, quorum: NonZeroUsize, - num_results: usize, + /// A list of peers the given record has been replicated to. + successfull_peers: Vec, context: PutRecordContext, }, @@ -1913,8 +1931,9 @@ enum QueryInfo { GetRecord { /// The key to look for. key: record::Key, - /// The records found. - records: Vec, + /// The records with the id of the peer that returned them. `None` when + /// the record was found in the local store. + records: Vec<(Option, Record)>, /// The number of records to look for. quorum: NonZeroUsize, /// The closest peer to `key` that did not return a record. diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 801923a1d30..c93a11f1549 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -24,12 +24,13 @@ use super::*; use crate::{ALPHA_VALUE, K_VALUE}; use crate::kbucket::Distance; -use crate::record::store::MemoryStore; +use crate::record::{Key, store::MemoryStore}; use futures::{ prelude::*, executor::block_on, future::poll_fn, }; +use futures_timer::Delay; use libp2p_core::{ PeerId, Transport, @@ -44,7 +45,7 @@ use libp2p_swarm::Swarm; use libp2p_yamux as yamux; use quickcheck::*; use rand::{Rng, random, thread_rng}; -use std::{collections::{HashSet, HashMap}, io, num::NonZeroUsize, u64}; +use std::{collections::{HashSet, HashMap}, time::Duration, io, num::NonZeroUsize, u64}; use multihash::{wrap, Code, Multihash}; type TestSwarm = Swarm>; @@ -810,3 +811,95 @@ fn exp_decr_expiration_overflow() { quickcheck(prop_no_panic as fn(_, _)) } + +#[test] +fn disjoint_query_does_not_finish_before_all_paths_did() { + let mut config = KademliaConfig::default(); + config.use_disjoint_path_queries(); + // I.e. setting the amount disjoint paths to be explored to 2. + config.set_parallelism(NonZeroUsize::new(2).unwrap()); + + + let mut alice = build_node_with_config(config); + let mut trudy = build_node(); // Trudy the intrudor, an adversary. + let mut bob = build_node(); + + let key = Key::new(&multihash::Sha2_256::digest(&vec![1, 2, 3, 4])); + let record_bob = Record::new(key.clone(), b"bob".to_vec()); + let record_trudy = Record::new(key.clone(), b"trudy".to_vec()); + + // Make `trudy` and `bob` aware of their version of the record searched by + // `alice`. + trudy.1.store.put(record_bob.clone()).unwrap(); + bob.1.store.put(record_trudy.clone()).unwrap(); + + // Make `trudy` and `bob` known to `alice`. + alice.1.add_address(Swarm::local_peer_id(&trudy.1), trudy.0.clone()); + alice.1.add_address(Swarm::local_peer_id(&bob.1), bob.0.clone()); + + // Have `alice` query the Dht for `key` with a quorum of 1. + alice.1.get_record(&key, Quorum::One); + + // The default peer timeout is 10 seconds. Choosing 1 seconds here should + // give enough head room to prevent connections to `bob` to time out. + let mut before_timeout = Delay::new(Duration::from_secs(1)); + + // Poll only `alice` and `trudy` expecting `alice` not yet to return a query + // result as it is not able to connect to `bob` just yet. + block_on( + poll_fn(|ctx| { + for (_, swarm) in &mut [&mut alice, &mut trudy] { + loop { + match swarm.poll_next_unpin(ctx) { + Poll::Ready(Some(KademliaEvent::GetRecordResult(result))) => { + match result { + Ok(_) => panic!( + "Expected query not to finish until all \ + disjoint paths have been queried.", + ), + Err(e) => unreachable!("{:?}", e), + } + } + // Ignore any other event. + Poll::Ready(Some(_)) => (), + Poll::Ready(None) => panic!("Expected Kademlia behaviour not to finish."), + Poll::Pending => break, + } + } + } + + // Make sure not to wait until connections to `bob` time out. + before_timeout.poll_unpin(ctx) + }) + ); + + // Poll `alice` and `bob` expecting `alice` to return a successful query + // result as it is now able to explore the second disjoint path. + let records = block_on( + poll_fn(|ctx| { + for (_, swarm) in &mut [&mut alice, &mut bob] { + loop { + match swarm.poll_next_unpin(ctx) { + Poll::Ready(Some(KademliaEvent::GetRecordResult(result))) => { + match result { + Ok(ok) => return Poll::Ready(ok.records), + Err(e) => unreachable!("{:?}", e), + } + } + // Ignore any other event. + Poll::Ready(Some(_)) => (), + Poll::Ready(None) => panic!( + "Expected Kademlia behaviour not to finish.", + ), + Poll::Pending => break, + } + } + } + + Poll::Pending + }) + ); + + assert!(records.contains(&record_bob)); + assert!(records.contains(&record_trudy)); +} diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 21c2134da2c..0f8ef273b9c 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -299,14 +299,24 @@ impl Query { } } - /// Finishes the query prematurely. + /// Offer to finish the query prematurely, providing the peers that + /// returned either a record or a set of providers as an argument. /// - /// A finished query immediately stops yielding new peers to contact and will be - /// reported by [`QueryPool::poll`] via [`QueryPoolState::Finished`]. - pub fn finish(&mut self) { + /// It is up to the `QueryPeerIter` to decide whether to finish or continue + /// querying other peers. E.g. in the case of using disjoint paths a + /// `QueryPeerIter` may decide not yet to finish in case not every path + /// has yet yielded a result. + /// + /// A finished query immediately stops yielding new peers to contact and + /// will be reported by [`QueryPool::poll`] via + /// [`QueryPoolState::Finished`]. + pub fn may_finish(&mut self, peers: I) + where + I: IntoIterator + { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.finish(), - QueryPeerIter::ClosestDisjoint(iter) => iter.finish(), + QueryPeerIter::ClosestDisjoint(iter) => iter.may_finish(peers), QueryPeerIter::Fixed(iter) => iter.finish() } } diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 5a2d75a57b0..0dddb815bf2 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -236,9 +236,15 @@ impl ClosestDisjointPeersIter { state.unwrap_or(PeersIterState::Finished) } - pub fn finish(&mut self) { - for iter in &mut self.iters { - iter.finish() + /// See [`Query::may_finish`] for documentation. + pub fn may_finish(&mut self, peers: I) + where + I: IntoIterator + { + for peer in peers { + if let Some(PeerState{ initiated_by, .. }) = self.contacted_peers.get_mut(&peer) { + self.iters[*initiated_by].finish(); + } } } @@ -298,6 +304,7 @@ impl IndexMut for Vec { /// State tracking the iterator that yielded (i.e. tried to contact) a peer. See /// [`ClosestDisjointPeersIter::on_success`] for details. +#[derive(Debug)] struct PeerState { /// First iterator to yield the peer. Will be notified both of the outcome /// (success/failure) as well as the closer peers. @@ -316,6 +323,7 @@ impl PeerState { } } +#[derive(Debug)] enum ResponseState { Waiting, Succeeded, From 4e76145e8f12edffad6870bb580c1fc840b342a2 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 16 May 2020 16:41:34 +0200 Subject: [PATCH 34/63] protocols/kad/query/disjoint: Have quickcheck generate target --- protocols/kad/src/query/peers/closest/disjoint.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 13ef316e471..0853b0c7c92 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -855,13 +855,17 @@ mod tests { /// Ensure [`ClosestPeersIter`] and [`ClosestDisjointPeersIter`] yield same closest peers. #[test] fn closest_and_disjoint_closest_yield_same_result() { - fn prop(graph: Graph, parallelism: Parallelism, num_results: NumResults) -> TestResult { + fn prop( + target: Target, + graph: Graph, + parallelism: Parallelism, + num_results: NumResults, + ) -> TestResult { if parallelism.0 > num_results.0 { return TestResult::discard(); } - // TODO: Pass target via prop(). - let target: KeyBytes = Key::from(PeerId::random()).into(); + let target: KeyBytes = target.0; let closest_peer = graph.get_closest_peer(&target); let mut known_closest_peers = graph.0.iter() @@ -959,6 +963,6 @@ mod tests { result.into_iter().map(|k| k.into_preimage()).collect() } - QuickCheck::new().tests(10).quickcheck(prop as fn(_, _, _) -> _) + QuickCheck::new().tests(10).quickcheck(prop as fn(_, _, _, _) -> _) } } From 2336299c1589682934323a0e79b298e90370bd70 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 16 May 2020 17:13:48 +0200 Subject: [PATCH 35/63] protocols/kad/behaviour/test: Have bootstrap test use disjoint paths --- protocols/kad/src/behaviour/test.rs | 55 +++++++++++++++++++---------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 2230010b4f4..3fee25c8242 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -22,7 +22,7 @@ use super::*; -use crate::{ALPHA_VALUE, K_VALUE}; +use crate::K_VALUE; use crate::kbucket::Distance; use crate::record::{Key, store::MemoryStore}; use futures::{ @@ -44,14 +44,12 @@ use libp2p_secio::SecioConfig; use libp2p_swarm::Swarm; use libp2p_yamux as yamux; use quickcheck::*; -use rand::{Rng, random, thread_rng}; +use rand::{Rng, random, thread_rng, rngs::StdRng, SeedableRng}; use std::{collections::{HashSet, HashMap}, time::Duration, io, num::NonZeroUsize, u64}; use multihash::{wrap, Code, Multihash}; type TestSwarm = Swarm>; -// TODO: Have these tests use disjoint paths as well. - fn build_node() -> (Multiaddr, TestSwarm) { build_node_with_config(Default::default()) } @@ -135,21 +133,45 @@ fn random_multihash() -> Multihash { wrap(Code::Sha2_256, &thread_rng().gen::<[u8; 32]>()) } +#[derive(Clone, Debug)] +struct Seed([u8; 32]); + +impl Arbitrary for Seed { + fn arbitrary(g: &mut G) -> Seed { + Seed(g.gen()) + } +} + #[test] fn bootstrap() { - fn run(rng: &mut impl Rng) { + fn prop(seed: Seed) { + let mut rng = StdRng::from_seed(seed.0); + let num_total = rng.gen_range(2, 20); - // When looking for the closest node to a key, Kademlia considers ALPHA_VALUE nodes to query - // at initialization. If `num_groups` is larger than ALPHA_VALUE the remaining locally known - // nodes will not be considered. Given that no other node is aware of them, they would be - // lost entirely. To prevent the above restrict `num_groups` to be equal or smaller than - // ALPHA_VALUE. - let num_group = rng.gen_range(1, (num_total % ALPHA_VALUE.get()) + 2); - - let mut swarms = build_connected_nodes(num_total, num_group).into_iter() + // When looking for the closest node to a key, Kademlia considers + // K_VALUE nodes to query at initialization. If `num_groups` is larger + // than K_VALUE the remaining locally known nodes will not be + // considered. Given that no other node is aware of them, they would be + // lost entirely. To prevent the above restrict `num_groups` to be equal + // or smaller than K_VALUE. + let num_group = rng.gen_range(1, (num_total % K_VALUE.get()) + 2); + + let mut cfg = KademliaConfig::default(); + if rng.gen() { + cfg.use_disjoint_path_queries(); + } + + let mut swarms = build_connected_nodes_with_config( + num_total, + num_group, + cfg, + ).into_iter() .map(|(_a, s)| s) .collect::>(); - let swarm_ids: Vec<_> = swarms.iter().map(Swarm::local_peer_id).cloned().collect(); + let swarm_ids: Vec<_> = swarms.iter() + .map(Swarm::local_peer_id) + .cloned() + .collect(); let qid = swarms[0].bootstrap().unwrap(); @@ -193,10 +215,7 @@ fn bootstrap() { ) } - let mut rng = thread_rng(); - for _ in 0 .. 10 { - run(&mut rng) - } + QuickCheck::new().tests(10).quickcheck(prop as fn(_) -> _) } #[test] From 2a6303ded9fb3479c73c9562a4fd67a31c101996 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 16 May 2020 17:30:40 +0200 Subject: [PATCH 36/63] protocols/kad/behaviour/test: Make put_record&add_provider use disjoint --- protocols/kad/src/behaviour/test.rs | 49 +++++++++++++++++++---------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 3fee25c8242..78e29f52143 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -437,16 +437,21 @@ fn get_record_not_found() { ) } -/// A node joining a fully connected network via a single bootnode should be able to put a record to -/// the X closest nodes of the network where X is equal to the configured replication factor. +/// A node joining a fully connected network via three (ALPHA_VALUE) bootnodes +/// should be able to put a record to the X closest nodes of the network where X +/// is equal to the configured replication factor. #[test] fn put_record() { - fn prop(replication_factor: usize, records: Vec) { - let replication_factor = NonZeroUsize::new(replication_factor % (K_VALUE.get() / 2) + 1).unwrap(); + fn prop(records: Vec, seed: Seed) { + let mut rng = StdRng::from_seed(seed.0); + let replication_factor = NonZeroUsize::new(rng.gen_range(1, K_VALUE.get() + 1)).unwrap(); let num_total = replication_factor.get() * 2; let mut config = KademliaConfig::default(); config.set_replication_factor(replication_factor); + if rng.gen() { + config.use_disjoint_path_queries(); + } let mut swarms = { let mut fully_connected_swarms = build_fully_connected_nodes_with_config( @@ -455,10 +460,13 @@ fn put_record() { ); let mut single_swarm = build_node_with_config(config); - single_swarm.1.add_address( - Swarm::local_peer_id(&fully_connected_swarms[0].1), - fully_connected_swarms[0].0.clone(), - ); + // Connect `single_swarm` to three bootnodes. + for i in 0..3 { + single_swarm.1.add_address( + Swarm::local_peer_id(&fully_connected_swarms[i].1), + fully_connected_swarms[i].0.clone(), + ); + } let mut swarms = vec![single_swarm]; swarms.append(&mut fully_connected_swarms); @@ -703,17 +711,21 @@ fn get_record_many() { ) } -/// A node joining a fully connected network via a single bootnode should be able to add itself as a -/// provider to the X closest nodes of the network where X is equal to the configured replication -/// factor. +/// A node joining a fully connected network via three (ALPHA_VALUE) bootnodes +/// should be able to add itself as a provider to the X closest nodes of the +/// network where X is equal to the configured replication factor. #[test] fn add_provider() { - fn prop(replication_factor: usize, keys: Vec) { - let replication_factor = NonZeroUsize::new(replication_factor % (K_VALUE.get() / 2) + 1).unwrap(); + fn prop(keys: Vec, seed: Seed) { + let mut rng = StdRng::from_seed(seed.0); + let replication_factor = NonZeroUsize::new(rng.gen_range(1, K_VALUE.get() + 1)).unwrap(); let num_total = replication_factor.get() * 2; let mut config = KademliaConfig::default(); config.set_replication_factor(replication_factor); + if rng.gen() { + config.use_disjoint_path_queries(); + } let mut swarms = { let mut fully_connected_swarms = build_fully_connected_nodes_with_config( @@ -722,10 +734,13 @@ fn add_provider() { ); let mut single_swarm = build_node_with_config(config); - single_swarm.1.add_address( - Swarm::local_peer_id(&fully_connected_swarms[0].1), - fully_connected_swarms[0].0.clone(), - ); + // Connect `single_swarm` to three bootnodes. + for i in 0..3 { + single_swarm.1.add_address( + Swarm::local_peer_id(&fully_connected_swarms[i].1), + fully_connected_swarms[i].0.clone(), + ); + } let mut swarms = vec![single_swarm]; swarms.append(&mut fully_connected_swarms); From 0fce409f8602335c03f85ce968c0c1f191e959d7 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 16 May 2020 20:15:42 +0200 Subject: [PATCH 37/63] protocols/kad/query/disjoint: Fix intra doc link --- protocols/kad/src/query/peers/closest/disjoint.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 0853b0c7c92..53eea322e92 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -277,7 +277,7 @@ impl ClosestDisjointPeersIter { /// /// `peers` representing the peers that either returned or put a record. /// - /// See [`Query::may_finish`] for details. + /// See [`crate::query::Query::may_finish`] for details. pub fn may_finish(&mut self, peers: I) where I: IntoIterator From 4c8fe6e6adf933200ade85b878dcde9b1c5c5df1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 17 May 2020 21:30:44 +0200 Subject: [PATCH 38/63] protocols/kad/query/disjoint: Do not initialize order for each next() --- .../kad/src/query/peers/closest/disjoint.rs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 53eea322e92..d074a120eb0 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -21,7 +21,11 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; use libp2p_core::PeerId; -use std::{collections::HashMap, iter::{Peekable}, ops::{Add, Index, IndexMut}}; +use std::{ + collections::HashMap, + iter::{Cycle, Map, Peekable}, + ops::{Add, Index, IndexMut, Range}, +}; use wasm_timer::Instant; /// Wraps around a set of [`ClosestPeersIter`], enforcing a disjoint discovery @@ -32,6 +36,9 @@ pub struct ClosestDisjointPeersIter { /// The set of wrapped [`ClosestPeersIter`]. iters: Vec, + /// Order in which to query the iterators ensuring fairness across + /// [`ClosestPeersIterConfig::next`] calls. + iter_order: Cycle, fn(usize) -> IteratorIndex>>, /// Mapping of contacted peers by their [`PeerId`] to [`PeerState`] /// containing the corresponding iterator indices as well as the response @@ -40,9 +47,6 @@ pub struct ClosestDisjointPeersIter { /// Used to track which iterator contacted which peer. See [`PeerState`] /// for details. contacted_peers: HashMap, - - /// Index of the iterator last queried. - last_queried: IteratorIndex, } impl ClosestDisjointPeersIter { @@ -84,9 +88,8 @@ impl ClosestDisjointPeersIter { config, target: target.into(), iters, + iter_order: (0..iters_len).map(Into::into as fn(usize) -> IteratorIndex).cycle(), contacted_peers: HashMap::new(), - // Wraps around, thus iterator 0 will be queried first. - last_queried: (iters_len - 1).into(), } } @@ -173,17 +176,16 @@ impl ClosestDisjointPeersIter { } pub fn next(&mut self, now: Instant) -> PeersIterState { + let mut num_iters_querrried = 0; let mut state = None; - // Order in which to query the iterators to ensure fairness. - let iter_order = (0..self.iters.len()).map(Into::into).cycle() - // Make sure to query the previously queried iterator last. - .skip(Into::::into(self.last_queried) + 1) - // Query each iterator at most once. - .take(self.iters.len()); + for i in &mut self.iter_order { + // Ensure querying each iterator at most once. + num_iters_querrried += 1; + if num_iters_querrried > self.iters.len() { + break; + } - for i in &mut iter_order.into_iter() { - self.last_queried = i; let iter = &mut self.iters[i]; loop { From 4be526e42d5c6202d4420aced7d782f6caf27681 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 17 May 2020 21:44:35 +0200 Subject: [PATCH 39/63] protocols/kad/behaviour/test: Init put_record with at least 4 nodes Require at least 1 node under test and 3 bootnodes. --- protocols/kad/src/behaviour/test.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 78e29f52143..f15e8318764 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -444,7 +444,7 @@ fn get_record_not_found() { fn put_record() { fn prop(records: Vec, seed: Seed) { let mut rng = StdRng::from_seed(seed.0); - let replication_factor = NonZeroUsize::new(rng.gen_range(1, K_VALUE.get() + 1)).unwrap(); + let replication_factor = NonZeroUsize::new(rng.gen_range(4, K_VALUE.get() + 1)).unwrap(); let num_total = replication_factor.get() * 2; let mut config = KademliaConfig::default(); @@ -621,6 +621,12 @@ fn put_record() { ) } + // Past failures. + prop(vec![], Seed([ + 28, 84, 29, 227, 26, 164, 229, 32, 162, 130, 83, 64, 58, 253, 14, 211, 3, + 165, 16, 84, 161, 114, 216, 118, 214, 163, 213, 97, 145, 40, 218, 155, + ])); + QuickCheck::new().tests(3).quickcheck(prop as fn(_,_) -> _) } From 14a8e2f74ec8960251f5d93b95441361c9653e3c Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 18 May 2020 08:14:49 +0200 Subject: [PATCH 40/63] protocols/kad/query/disjoint: Remove num_iters_query counter --- protocols/kad/src/query/peers/closest/disjoint.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index d074a120eb0..9e4e7846c33 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -88,7 +88,7 @@ impl ClosestDisjointPeersIter { config, target: target.into(), iters, - iter_order: (0..iters_len).map(Into::into as fn(usize) -> IteratorIndex).cycle(), + iter_order: (0..iters_len).map(IteratorIndex as fn(usize) -> IteratorIndex).cycle(), contacted_peers: HashMap::new(), } } @@ -176,16 +176,11 @@ impl ClosestDisjointPeersIter { } pub fn next(&mut self, now: Instant) -> PeersIterState { - let mut num_iters_querrried = 0; let mut state = None; - for i in &mut self.iter_order { - // Ensure querying each iterator at most once. - num_iters_querrried += 1; - if num_iters_querrried > self.iters.len() { - break; - } - + // Ensure querying each iterator at most once. + for _ in 0 .. self.iters.len() { + let i = self.iter_order.next().expect("Cycle never ends."); let iter = &mut self.iters[i]; loop { From ea7f3ede68aab24a27032d44050c1c8ec806b89f Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 18 May 2020 08:26:02 +0200 Subject: [PATCH 41/63] protocols/kad/query/disjoint: Fix intra doc comment link --- protocols/kad/src/query/peers/closest/disjoint.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 9e4e7846c33..b77e77d00b2 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -37,7 +37,7 @@ pub struct ClosestDisjointPeersIter { /// The set of wrapped [`ClosestPeersIter`]. iters: Vec, /// Order in which to query the iterators ensuring fairness across - /// [`ClosestPeersIterConfig::next`] calls. + /// [`ClosestPeersIter::next`] calls. iter_order: Cycle, fn(usize) -> IteratorIndex>>, /// Mapping of contacted peers by their [`PeerId`] to [`PeerState`] From 5b04baa88c8e3818238642144babdab38a3b670e Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 21 May 2020 12:39:27 +0200 Subject: [PATCH 42/63] protocols/kad/behaviour: Rename successfull_peers to success --- protocols/kad/src/behaviour.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 1e696f9cbd6..c1934f9a3e7 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -935,7 +935,7 @@ where record, quorum, phase: PutRecordPhase::PutRecord { - successfull_peers: vec![], + success: vec![], get_closest_peers_stats: QueryStats::empty() } }; @@ -969,7 +969,7 @@ where record, quorum, phase: PutRecordPhase::PutRecord { - successfull_peers: vec![], + success: vec![], get_closest_peers_stats: result.stats } }; @@ -982,13 +982,13 @@ where context, record, quorum, - phase: PutRecordPhase::PutRecord { successfull_peers, get_closest_peers_stats } + phase: PutRecordPhase::PutRecord { success, get_closest_peers_stats } } => { let mk_result = |key: record::Key| { - if successfull_peers.len() >= quorum.get() { + if success.len() >= quorum.get() { Ok(PutRecordOk { key }) } else { - Err(PutRecordError::QuorumFailed { key, quorum, num_results: successfull_peers.len() }) + Err(PutRecordError::QuorumFailed { key, quorum, num_results: success.len() }) } }; match context { @@ -1087,7 +1087,7 @@ where quorum, num_results: match phase { PutRecordPhase::GetClosestPeers => 0, - PutRecordPhase::PutRecord { ref successfull_peers, .. } => successfull_peers.len(), + PutRecordPhase::PutRecord { ref success, .. } => success.len(), } }); match context { @@ -1559,11 +1559,11 @@ where if let Some(query) = self.queries.get_mut(&user_data) { query.on_success(&source, vec![]); if let QueryInfo::PutRecord { - phase: PutRecordPhase::PutRecord { successfull_peers, .. }, quorum, .. + phase: PutRecordPhase::PutRecord { success, .. }, quorum, .. } = &mut query.inner.info { - successfull_peers.push(source); - if successfull_peers.len() >= quorum.get() { - let peers = successfull_peers.clone(); + success.push(source); + if success.len() >= quorum.get() { + let peers = success.clone(); query.may_finish(peers) } } @@ -2199,7 +2199,7 @@ pub enum PutRecordPhase { /// The query is replicating the record to the closest nodes to the key. PutRecord { /// A list of peers the given record has been successfully replicated to. - successfull_peers: Vec, + success: Vec, /// Query statistics from the finished `GetClosestPeers` phase. get_closest_peers_stats: QueryStats, }, From f5d917c4510c9021bf1183f1a0423e69d414cf51 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 21 May 2020 13:48:02 +0200 Subject: [PATCH 43/63] protocols/kad/behaviour/test: Adjust network size setup --- protocols/kad/src/behaviour/test.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index f15e8318764..51e5a7e7d1e 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -444,7 +444,7 @@ fn get_record_not_found() { fn put_record() { fn prop(records: Vec, seed: Seed) { let mut rng = StdRng::from_seed(seed.0); - let replication_factor = NonZeroUsize::new(rng.gen_range(4, K_VALUE.get() + 1)).unwrap(); + let replication_factor = NonZeroUsize::new(rng.gen_range(2, (K_VALUE.get() / 2) + 1)).unwrap(); let num_total = replication_factor.get() * 2; let mut config = KademliaConfig::default(); @@ -724,7 +724,7 @@ fn get_record_many() { fn add_provider() { fn prop(keys: Vec, seed: Seed) { let mut rng = StdRng::from_seed(seed.0); - let replication_factor = NonZeroUsize::new(rng.gen_range(1, K_VALUE.get() + 1)).unwrap(); + let replication_factor = NonZeroUsize::new(rng.gen_range(2, (K_VALUE.get() / 2) + 1)).unwrap(); let num_total = replication_factor.get() * 2; let mut config = KademliaConfig::default(); From 0500056838218fdd081dd5c26ad3dd6f18d47c2b Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 25 May 2020 12:47:46 +0200 Subject: [PATCH 44/63] protocols/kad/behaviour/test: Remove newline --- protocols/kad/src/behaviour/test.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 51e5a7e7d1e..5bf07ca7080 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -928,7 +928,6 @@ fn disjoint_query_does_not_finish_before_all_paths_did() { // I.e. setting the amount disjoint paths to be explored to 2. config.set_parallelism(NonZeroUsize::new(2).unwrap()); - let mut alice = build_node_with_config(config); let mut trudy = build_node(); // Trudy the intrudor, an adversary. let mut bob = build_node(); From a39388c8abb20f5595068e883264cad136226071 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 10 Jun 2020 15:48:22 +0200 Subject: [PATCH 45/63] protocols/kad/behaviour: Rename disjoint paths config option --- protocols/kad/src/behaviour.rs | 6 +++--- protocols/kad/src/behaviour/test.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index c1934f9a3e7..1da9ed9bcd3 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -165,7 +165,7 @@ impl KademliaConfig { /// This only controls the level of parallelism of an iterative query, not /// the level of parallelism of a query to a fixed set of peers. /// - /// When used with [`KademliaConfig::use_disjoint_path_queries`] it equals + /// When used with [`KademliaConfig::disjoint_query_paths`] it equals /// the amount of disjoint paths used. pub fn set_parallelism(&mut self, parallelism: NonZeroUsize) -> &mut Self { self.query_config.parallelism = parallelism; @@ -180,8 +180,8 @@ impl KademliaConfig { /// /// See the S/Kademlia paper for more information on the high level design /// as well as its security improvements. - pub fn use_disjoint_path_queries(&mut self) -> &mut Self { - self.query_config.use_disjoint_paths = true; + pub fn disjoint_query_paths(&mut self, enabled: bool) -> &mut Self { + self.query_config.use_disjoint_paths = enabled; self } diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 5bf07ca7080..056d617b225 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -730,7 +730,7 @@ fn add_provider() { let mut config = KademliaConfig::default(); config.set_replication_factor(replication_factor); if rng.gen() { - config.use_disjoint_path_queries(); + config.disjoint_query_paths(true); } let mut swarms = { @@ -924,7 +924,7 @@ fn exp_decr_expiration_overflow() { #[test] fn disjoint_query_does_not_finish_before_all_paths_did() { let mut config = KademliaConfig::default(); - config.use_disjoint_path_queries(); + config.disjoint_query_paths(true); // I.e. setting the amount disjoint paths to be explored to 2. config.set_parallelism(NonZeroUsize::new(2).unwrap()); From 1eb24366ebe22d7934a3b70425895b0a571d6f3f Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 10 Jun 2020 15:49:33 +0200 Subject: [PATCH 46/63] protocols/kad/behaviour.rs: Introduce PeerRecord struct --- protocols/kad/src/behaviour.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 1da9ed9bcd3..72feeb14007 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -464,7 +464,7 @@ where if record.is_expired(Instant::now()) { self.store.remove(key) } else { - records.push((None, record.into_owned())); + records.push(PeerRecord{ peer: None, record: record.into_owned()}); } } @@ -922,7 +922,7 @@ where } QueryInfo::GetRecord { key, records, quorum, cache_at } => { - let records = records.into_iter().map(|r| r.1).collect::>(); + let records = records.into_iter().map(|r| r.record).collect::>(); let results = if records.len() >= quorum.get() { // [not empty] if let Some(cache_key) = cache_at { // Cache the record at the closest node to the key that @@ -1135,7 +1135,7 @@ where result: QueryResult::GetRecord(Err( GetRecordError::Timeout { key, - records: records.into_iter().map(|r| r.1).collect(), + records: records.into_iter().map(|r| r.record).collect(), quorum, } )) @@ -1514,13 +1514,13 @@ where key, records, quorum, cache_at } = &mut query.inner.info { if let Some(record) = record { - records.push((Some(source.clone()), record)); + records.push(PeerRecord{ peer: Some(source.clone()), record }); if records.len() >= quorum.get() { // Desired quorum reached. The query may finish. See // [`Query::may_finish`] for details. let peers = records.iter() - .filter_map(|(peer, _record)| peer.as_ref()) + .filter_map(|PeerRecord{ peer, .. }| peer.as_ref()) .cloned() .collect::>(); query.may_finish(peers) @@ -1706,6 +1706,12 @@ impl Quorum { } } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PeerRecord { + pub peer: Option, + pub record: Record, +} + ////////////////////////////////////////////////////////////////////////////// // Events @@ -2110,7 +2116,7 @@ pub enum QueryInfo { key: record::Key, /// The records with the id of the peer that returned them. `None` when /// the record was found in the local store. - records: Vec<(Option, Record)>, + records: Vec, /// The number of records to look for. quorum: NonZeroUsize, /// The closest peer to `key` that did not return a record. From 7e64f2c47b1230a251287cbe9202ddd4a7f2ac99 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 10 Jun 2020 16:08:53 +0200 Subject: [PATCH 47/63] protocols/kad/behaviour: Expose PeerRecord on the public API --- protocols/kad/src/behaviour.rs | 17 +++++++---------- protocols/kad/src/behaviour/test.rs | 27 +++++++++++++++++++-------- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 72feeb14007..e2929dacbf2 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -922,12 +922,11 @@ where } QueryInfo::GetRecord { key, records, quorum, cache_at } => { - let records = records.into_iter().map(|r| r.record).collect::>(); let results = if records.len() >= quorum.get() { // [not empty] if let Some(cache_key) = cache_at { // Cache the record at the closest node to the key that // did not return the record. - let record = records.first().expect("[not empty]").clone(); + let record = records.first().expect("[not empty]").record.clone(); let quorum = NonZeroUsize::new(1).expect("1 > 0"); let context = PutRecordContext::Cache; let info = QueryInfo::PutRecord { @@ -1133,11 +1132,7 @@ where id: query_id, stats: result.stats, result: QueryResult::GetRecord(Err( - GetRecordError::Timeout { - key, - records: records.into_iter().map(|r| r.record).collect(), - quorum, - } + GetRecordError::Timeout { key, records, quorum }, )) }), @@ -1706,6 +1701,8 @@ impl Quorum { } } +/// A record either received by the given peer or retrieved from the local +/// record store. #[derive(Debug, Clone, PartialEq, Eq)] pub struct PeerRecord { pub peer: Option, @@ -1795,7 +1792,7 @@ pub type GetRecordResult = Result; /// The successful result of [`Kademlia::get_record`]. #[derive(Debug, Clone)] pub struct GetRecordOk { - pub records: Vec + pub records: Vec } /// The error result of [`Kademlia::get_record`]. @@ -1807,12 +1804,12 @@ pub enum GetRecordError { }, QuorumFailed { key: record::Key, - records: Vec, + records: Vec, quorum: NonZeroUsize }, Timeout { key: record::Key, - records: Vec, + records: Vec, quorum: NonZeroUsize } } diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 056d617b225..bddfb7b3dd8 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -654,11 +654,13 @@ fn get_record() { loop { match swarm.poll_next_unpin(ctx) { Poll::Ready(Some(KademliaEvent::QueryResult { - id, result: QueryResult::GetRecord(Ok(ok)), .. + id, + result: QueryResult::GetRecord(Ok(GetRecordOk { records })), + .. })) => { assert_eq!(id, qid); - assert_eq!(ok.records.len(), 1); - assert_eq!(ok.records.first(), Some(&record)); + assert_eq!(records.len(), 1); + assert_eq!(records.first().unwrap().record, record); return Poll::Ready(()); } // Ignore any other event. @@ -698,11 +700,13 @@ fn get_record_many() { loop { match swarm.poll_next_unpin(ctx) { Poll::Ready(Some(KademliaEvent::QueryResult { - id, result: QueryResult::GetRecord(Ok(ok)), .. + id, + result: QueryResult::GetRecord(Ok(GetRecordOk { records })), + .. })) => { assert_eq!(id, qid); - assert_eq!(ok.records.len(), num_results); - assert_eq!(ok.records.first(), Some(&record)); + assert_eq!(records.len(), num_results); + assert_eq!(records.first().unwrap().record, record); return Poll::Ready(()); } // Ignore any other event. @@ -1014,6 +1018,13 @@ fn disjoint_query_does_not_finish_before_all_paths_did() { }) ); - assert!(records.contains(&record_bob)); - assert!(records.contains(&record_trudy)); + assert_eq!(2, records.len()); + assert!(records.contains(&PeerRecord { + peer: Some(Swarm::local_peer_id(&bob).clone()), + record: record_bob, + })); + assert!(records.contains(&PeerRecord { + peer: Some(Swarm::local_peer_id(&trudy).clone()), + record: record_trudy, + })); } From 981c7aaafe948bbbb35f968f4581c52035b309a6 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 10 Jun 2020 16:09:34 +0200 Subject: [PATCH 48/63] protocols/kad/behaviour/test.rs: Apply feedback --- protocols/kad/src/behaviour/test.rs | 68 +++++++++++++++++++---------- 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index bddfb7b3dd8..50be4bb749a 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -149,16 +149,16 @@ fn bootstrap() { let num_total = rng.gen_range(2, 20); // When looking for the closest node to a key, Kademlia considers - // K_VALUE nodes to query at initialization. If `num_groups` is larger + // K_VALUE nodes to query at initialization. If `num_group` is larger // than K_VALUE the remaining locally known nodes will not be // considered. Given that no other node is aware of them, they would be - // lost entirely. To prevent the above restrict `num_groups` to be equal + // lost entirely. To prevent the above restrict `num_group` to be equal // or smaller than K_VALUE. let num_group = rng.gen_range(1, (num_total % K_VALUE.get()) + 2); let mut cfg = KademliaConfig::default(); if rng.gen() { - cfg.use_disjoint_path_queries(); + cfg.disjoint_query_paths(true); } let mut swarms = build_connected_nodes_with_config( @@ -444,13 +444,14 @@ fn get_record_not_found() { fn put_record() { fn prop(records: Vec, seed: Seed) { let mut rng = StdRng::from_seed(seed.0); - let replication_factor = NonZeroUsize::new(rng.gen_range(2, (K_VALUE.get() / 2) + 1)).unwrap(); - let num_total = replication_factor.get() * 2; + let replication_factor = NonZeroUsize::new(rng.gen_range(1, (K_VALUE.get() / 2) + 1)).unwrap(); + // At least 4 nodes, 1 under test + 3 bootnodes. + let num_total = usize::max(4, replication_factor.get() * 2); let mut config = KademliaConfig::default(); config.set_replication_factor(replication_factor); if rng.gen() { - config.use_disjoint_path_queries(); + config.disjoint_query_paths(true); } let mut swarms = { @@ -621,12 +622,6 @@ fn put_record() { ) } - // Past failures. - prop(vec![], Seed([ - 28, 84, 29, 227, 26, 164, 229, 32, 162, 130, 83, 64, 58, 253, 14, 211, 3, - 165, 16, 84, 161, 114, 216, 118, 214, 163, 213, 97, 145, 40, 218, 155, - ])); - QuickCheck::new().tests(3).quickcheck(prop as fn(_,_) -> _) } @@ -729,7 +724,8 @@ fn add_provider() { fn prop(keys: Vec, seed: Seed) { let mut rng = StdRng::from_seed(seed.0); let replication_factor = NonZeroUsize::new(rng.gen_range(2, (K_VALUE.get() / 2) + 1)).unwrap(); - let num_total = replication_factor.get() * 2; + // At least 4 nodes, 1 under test + 3 bootnodes. + let num_total = usize::max(4, replication_factor.get() * 2); let mut config = KademliaConfig::default(); config.set_replication_factor(replication_factor); @@ -936,21 +932,24 @@ fn disjoint_query_does_not_finish_before_all_paths_did() { let mut trudy = build_node(); // Trudy the intrudor, an adversary. let mut bob = build_node(); - let key = Key::new(&multihash::Sha2_256::digest(&vec![1, 2, 3, 4])); + let key = Key::new(&multihash::Sha2_256::digest(&thread_rng().gen::<[u8; 32]>())); let record_bob = Record::new(key.clone(), b"bob".to_vec()); let record_trudy = Record::new(key.clone(), b"trudy".to_vec()); - // Make `trudy` and `bob` aware of their version of the record searched by + // Make `bob` and `trudy` aware of their version of the record searched by // `alice`. - trudy.1.store.put(record_bob.clone()).unwrap(); - bob.1.store.put(record_trudy.clone()).unwrap(); + bob.1.store.put(record_bob.clone()).unwrap(); + trudy.1.store.put(record_trudy.clone()).unwrap(); // Make `trudy` and `bob` known to `alice`. alice.1.add_address(Swarm::local_peer_id(&trudy.1), trudy.0.clone()); alice.1.add_address(Swarm::local_peer_id(&bob.1), bob.0.clone()); + // Drop the swarm addresses. + let (mut alice, mut bob, mut trudy) = (alice.1, bob.1, trudy.1); + // Have `alice` query the Dht for `key` with a quorum of 1. - alice.1.get_record(&key, Quorum::One); + alice.get_record(&key, Quorum::One); // The default peer timeout is 10 seconds. Choosing 1 seconds here should // give enough head room to prevent connections to `bob` to time out. @@ -960,19 +959,23 @@ fn disjoint_query_does_not_finish_before_all_paths_did() { // result as it is not able to connect to `bob` just yet. block_on( poll_fn(|ctx| { - for (_, swarm) in &mut [&mut alice, &mut trudy] { + for (i, swarm) in [&mut alice, &mut trudy].iter_mut().enumerate() { loop { match swarm.poll_next_unpin(ctx) { Poll::Ready(Some(KademliaEvent::QueryResult{ result: QueryResult::GetRecord(result), .. })) => { + if i != 0 { + panic!("Expected `QueryResult` from Alice.") + } + match result { Ok(_) => panic!( "Expected query not to finish until all \ - disjoint paths have been queried.", + disjoint paths have been explored.", ), - Err(e) => unreachable!("{:?}", e), + Err(e) => panic!("{:?}", e), } } // Ignore any other event. @@ -988,17 +991,38 @@ fn disjoint_query_does_not_finish_before_all_paths_did() { }) ); + // Make sure `alice` has exactly one query with `trudy`'s record only. + assert_eq!(1, alice.queries.iter().count()); + alice.queries.iter().for_each(|q| { + match &q.inner.info { + QueryInfo::GetRecord{ records, .. } => { + assert_eq!( + *records, + vec![PeerRecord { + peer: Some(Swarm::local_peer_id(&trudy).clone()), + record: record_trudy.clone(), + }], + ); + }, + i @ _ => panic!("Unexpected query info: {:?}", i), + } + }); + // Poll `alice` and `bob` expecting `alice` to return a successful query // result as it is now able to explore the second disjoint path. let records = block_on( poll_fn(|ctx| { - for (_, swarm) in &mut [&mut alice, &mut bob] { + for (i, swarm) in [&mut alice, &mut bob].iter_mut().enumerate() { loop { match swarm.poll_next_unpin(ctx) { Poll::Ready(Some(KademliaEvent::QueryResult{ result: QueryResult::GetRecord(result), .. })) => { + if i != 0 { + panic!("Expected `QueryResult` from Alice.") + } + match result { Ok(ok) => return Poll::Ready(ok.records), Err(e) => unreachable!("{:?}", e), From fac63753ce5f63b070adb7af5467a65438789b35 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 10 Jun 2020 18:27:09 +0200 Subject: [PATCH 49/63] protocols/kad/behaviour: Allow replication factor of 1 in add_provider --- protocols/kad/src/behaviour/test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 50be4bb749a..6b5358c33eb 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -723,7 +723,7 @@ fn get_record_many() { fn add_provider() { fn prop(keys: Vec, seed: Seed) { let mut rng = StdRng::from_seed(seed.0); - let replication_factor = NonZeroUsize::new(rng.gen_range(2, (K_VALUE.get() / 2) + 1)).unwrap(); + let replication_factor = NonZeroUsize::new(rng.gen_range(1, (K_VALUE.get() / 2) + 1)).unwrap(); // At least 4 nodes, 1 under test + 3 bootnodes. let num_total = usize::max(4, replication_factor.get() * 2); From a01c96019c1666409f67618a62486df37bc6841f Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 11 Jun 2020 11:47:59 +0200 Subject: [PATCH 50/63] protocols/kadquery.rs: Update may_finish comment --- protocols/kad/src/query.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 9eb6463f2f2..4bc2cae1b18 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -341,8 +341,8 @@ impl Query { state } - /// Offer to finish the query prematurely, providing the peers that - /// returned either a record or a set of providers as an argument. + /// Tries to (gracefully) finish the query prematurely, providing the peers + /// that are no longer of interest for further progress of the query. /// /// It is up to the `QueryPeerIter` to decide whether to finish or continue /// querying other peers. E.g. in the case of using disjoint paths a From 4955330a15b746209ad221b6eb9f1e4796ea8488 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 11 Jun 2020 12:24:35 +0200 Subject: [PATCH 51/63] protocols/kad/query/disjoint: Prevent failure overwriting success A `ClosestDisjointPeersIter` keeps track of the replies from all contacted peers. If a `on_success` follows an `on_failure` the iterator should keep track of the success, not have the `on_failure` overwrite it. --- .../kad/src/query/peers/closest/disjoint.rs | 63 ++++++++++++++----- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index b77e77d00b2..919769214e8 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -110,10 +110,12 @@ impl ClosestDisjointPeersIter { // iterators. If this iterator never started a request to the given peer // ignore the failure. if let Some(PeerState{ initiated_by, response }) = self.contacted_peers.get_mut(peer) { - *response = ResponseState::Failed; - updated = self.iters[*initiated_by].on_failure(peer); + if updated { + *response = ResponseState::Failed; + } + for iter in &mut self.iters { iter.on_failure(peer); } @@ -124,11 +126,12 @@ impl ClosestDisjointPeersIter { /// Callback for delivering the result of a successful request to a peer. /// - /// Delivering results of requests back to the iterator allows the iterator to make - /// progress. The iterator is said to make progress either when the given - /// `closer_peers` contain a peer closer to the target than any peer seen so far, - /// or when the iterator did not yet accumulate `num_results` closest peers and - /// `closer_peers` contains a new peer, regardless of its distance to the target. + /// Delivering results of requests back to the iterator allows the iterator + /// to make progress. The iterator is said to make progress either when the + /// given `closer_peers` contain a peer closer to the target than any peer + /// seen so far, or when the iterator did not yet accumulate `num_results` + /// closest peers and `closer_peers` contains a new peer, regardless of its + /// distance to the target. /// /// If the iterator is currently waiting for a result from `peer`, /// the iterator state is updated and `true` is returned. In that @@ -145,15 +148,18 @@ impl ClosestDisjointPeersIter { let mut updated = false; if let Some(PeerState{ initiated_by, response }) = self.contacted_peers.get_mut(peer) { - // Mark the response as succeeded for future iterators yielding this - // peer. There is no need to keep the `closer_peers` around, given - // that they are only passed to the first iterator. - *response = ResponseState::Succeeded; - // Pass the new `closer_peers` to the iterator that first yielded // the peer. updated = self.iters[*initiated_by].on_success(peer, closer_peers); + if updated { + // Mark the response as succeeded for future iterators yielding + // this peer. There is no need to keep the `closer_peers` + // around, given that they are only passed to the first + // iterator. + *response = ResponseState::Succeeded; + } + for iter in &mut self.iters { // Only report the success to all remaining not-first iterators. // Do not pass the `closer_peers` in order to uphold the @@ -321,7 +327,7 @@ impl ClosestDisjointPeersIter { } /// Index into the [`ClosestDisjointPeersIter`] `iters` vector. -#[derive(Copy, Clone, Debug)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] struct IteratorIndex(usize); impl From for IteratorIndex { @@ -360,7 +366,7 @@ impl IndexMut for Vec { /// State tracking the iterator that yielded (i.e. tried to contact) a peer. See /// [`ClosestDisjointPeersIter::on_success`] for details. -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] struct PeerState { /// First iterator to yield the peer. Will be notified both of the outcome /// (success/failure) as well as the closer peers. @@ -379,7 +385,7 @@ impl PeerState { } } -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] enum ResponseState { Waiting, Succeeded, @@ -456,6 +462,7 @@ mod tests { use quickcheck::*; use rand::{Rng, seq::SliceRandom}; use std::collections::HashSet; + use std::iter; impl Arbitrary for ResultIter>> { fn arbitrary(g: &mut G) -> Self { @@ -962,4 +969,30 @@ mod tests { QuickCheck::new().tests(10).quickcheck(prop as fn(_, _, _, _) -> _) } + + #[test] + fn failure_can_not_overwrite_previous_success() { + let now = Instant::now(); + let peer = PeerId::random(); + let mut iter = ClosestDisjointPeersIter::new( + Key::from(PeerId::random()).into(), + iter::once(Key::from(peer.clone())), + ); + + assert!(matches!(iter.next(now), PeersIterState::Waiting(Some(_)))); + + // Expect peer to be marked as succeeded. + assert!(iter.on_success(&peer, iter::empty())); + assert_eq!(iter.contacted_peers.get(&peer), Some(&PeerState { + initiated_by: IteratorIndex(0), + response: ResponseState::Succeeded, + })); + + // Expect peer to stay marked as succeeded. + assert!(!iter.on_failure(&peer)); + assert_eq!(iter.contacted_peers.get(&peer), Some(&PeerState { + initiated_by: IteratorIndex(0), + response: ResponseState::Succeeded, + })); + } } From 9e8e995ced3d2f157a668a497164e8e6539eecfd Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 11 Jun 2020 12:39:03 +0200 Subject: [PATCH 52/63] protocols/kad/query/closest/disjoint: Fix is_finished --- protocols/kad/src/query/peers/closest/disjoint.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 919769214e8..ff68aa70d21 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -301,13 +301,7 @@ impl ClosestDisjointPeersIter { /// Checks whether the iterator has finished. pub fn is_finished(&self) -> bool { - for iter in &self.iters { - if iter.is_finished() { - return true; - } - } - - false + self.iters.iter().all(|i| i.is_finished()) } /// Note: In the case of no adversarial peers or connectivity issues along From 2f10911b7d55881c2c66db4d0577eb4694662d6e Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 11 Jun 2020 13:05:20 +0200 Subject: [PATCH 53/63] protocols/kad: Adjust naming and comments --- protocols/kad/src/behaviour.rs | 6 +++--- protocols/kad/src/query.rs | 14 ++++++++------ .../kad/src/query/peers/closest/disjoint.rs | 17 ++++------------- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index e2929dacbf2..ce763e19d3c 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -1513,12 +1513,12 @@ where if records.len() >= quorum.get() { // Desired quorum reached. The query may finish. See - // [`Query::may_finish`] for details. + // [`Query::try_finish`] for details. let peers = records.iter() .filter_map(|PeerRecord{ peer, .. }| peer.as_ref()) .cloned() .collect::>(); - query.may_finish(peers) + query.try_finish(peers) } } else if quorum.get() == 1 { // It is a "standard" Kademlia query, for which the @@ -1559,7 +1559,7 @@ where success.push(source); if success.len() >= quorum.get() { let peers = success.clone(); - query.may_finish(peers) + query.try_finish(peers) } } } diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 4bc2cae1b18..c7a5e7a8890 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -344,21 +344,23 @@ impl Query { /// Tries to (gracefully) finish the query prematurely, providing the peers /// that are no longer of interest for further progress of the query. /// - /// It is up to the `QueryPeerIter` to decide whether to finish or continue - /// querying other peers. E.g. in the case of using disjoint paths a - /// `QueryPeerIter` may decide not yet to finish in case not every path - /// has yet yielded a result. + /// A query may require that in order to finish gracefully a certain subset + /// of peers must be contacted. E.g. in the case of disjoint query paths a + /// query may only finish gracefully if every path contacted a peer whose + /// response permits termination of the query. The given peers are those for + /// which this is considered to be the case, i.e. for which a termination + /// condition is satisfied. /// /// A finished query immediately stops yielding new peers to contact and /// will be reported by [`QueryPool::poll`] via /// [`QueryPoolState::Finished`]. - pub fn may_finish(&mut self, peers: I) + pub fn try_finish(&mut self, peers: I) where I: IntoIterator { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.finish(), - QueryPeerIter::ClosestDisjoint(iter) => iter.may_finish(peers), + QueryPeerIter::ClosestDisjoint(iter) => iter.finish_paths(peers), QueryPeerIter::Fixed(iter) => iter.finish() } } diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index ff68aa70d21..079b2da3826 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -172,13 +172,7 @@ impl ClosestDisjointPeersIter { } pub fn is_waiting(&self, peer: &PeerId) -> bool { - for iter in &self.iters { - if iter.is_waiting(peer) { - return true; - } - } - - false + self.iters.iter().any(|i| i.is_waiting(peer)) } pub fn next(&mut self, now: Instant) -> PeersIterState { @@ -275,13 +269,10 @@ impl ClosestDisjointPeersIter { state.unwrap_or(PeersIterState::Finished) } - /// Only finish the [`ClosestPeersIter`]s that yielded a peer within - /// `peers`. - /// - /// `peers` representing the peers that either returned or put a record. + /// Finishes all paths containing one of the given peers. /// - /// See [`crate::query::Query::may_finish`] for details. - pub fn may_finish(&mut self, peers: I) + /// See [`crate::query::Query::try_finish`] for details. + pub fn finish_paths(&mut self, peers: I) where I: IntoIterator { From 3448f52b81df2d0345d7d1da7151ebacb275e4f3 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 11 Jun 2020 14:15:52 +0200 Subject: [PATCH 54/63] protocols/kad: Fix use_disjoint_query_paths renaming --- protocols/kad/src/behaviour.rs | 2 +- protocols/kad/src/query.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index ce763e19d3c..4c64e9b7091 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -181,7 +181,7 @@ impl KademliaConfig { /// See the S/Kademlia paper for more information on the high level design /// as well as its security improvements. pub fn disjoint_query_paths(&mut self, enabled: bool) -> &mut Self { - self.query_config.use_disjoint_paths = enabled; + self.query_config.disjoint_query_paths = enabled; self } diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index c7a5e7a8890..0eedfd5e1e6 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -133,7 +133,7 @@ impl QueryPool { .. ClosestPeersIterConfig::default() }; - let peer_iter = if self.config.use_disjoint_paths { + let peer_iter = if self.config.disjoint_query_paths { QueryPeerIter::ClosestDisjoint( ClosestDisjointPeersIter::with_config(cfg, target, peers), ) @@ -238,8 +238,8 @@ pub struct QueryConfig { /// Whether to use disjoint paths on iterative lookups. /// - /// See [`crate::behaviour::KademliaConfig::use_disjoint_path_queries`] for details. - pub use_disjoint_paths: bool, + /// See [`crate::behaviour::KademliaConfig::disjoint_query_paths`] for details. + pub disjoint_query_paths: bool, } impl Default for QueryConfig { @@ -248,7 +248,7 @@ impl Default for QueryConfig { timeout: Duration::from_secs(60), replication_factor: NonZeroUsize::new(K_VALUE.get()).expect("K_VALUE > 0"), parallelism: ALPHA_VALUE, - use_disjoint_paths: false, + disjoint_query_paths: false, } } } From 57acffe54e6d2c365576624d7f5f80b2d9f54285 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 11 Jun 2020 14:46:34 +0200 Subject: [PATCH 55/63] protocols/kad/src/lib: Expose PeerRecord and with /examples --- examples/distributed-key-value-store.rs | 7 ++++--- protocols/kad/src/lib.rs | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/examples/distributed-key-value-store.rs b/examples/distributed-key-value-store.rs index 014418d318b..71773c003f6 100644 --- a/examples/distributed-key-value-store.rs +++ b/examples/distributed-key-value-store.rs @@ -33,13 +33,14 @@ use async_std::{io, task}; use futures::prelude::*; use libp2p::kad::record::store::MemoryStore; use libp2p::kad::{ - record::Key, Kademlia, KademliaEvent, + PeerRecord, PutRecordOk, QueryResult, Quorum, - Record + Record, + record::Key, }; use libp2p::{ NetworkBehaviour, @@ -86,7 +87,7 @@ fn main() -> Result<(), Box> { match message { KademliaEvent::QueryResult { result, .. } => match result { QueryResult::GetRecord(Ok(ok)) => { - for Record { key, value, .. } in ok.records { + for PeerRecord { record: Record { key, value, .. }, ..} in ok.records { println!( "Got record {:?} {:?}", std::str::from_utf8(key.as_ref()).unwrap(), diff --git a/protocols/kad/src/lib.rs b/protocols/kad/src/lib.rs index aa13374f89d..bd36fd3c224 100644 --- a/protocols/kad/src/lib.rs +++ b/protocols/kad/src/lib.rs @@ -45,6 +45,8 @@ pub use behaviour::{ QueryInfo, QueryStats, + PeerRecord, + BootstrapResult, BootstrapOk, BootstrapError, @@ -100,4 +102,3 @@ pub const K_VALUE: NonZeroUsize = unsafe { NonZeroUsize::new_unchecked(20) }; /// /// The current value is `3`. pub const ALPHA_VALUE: NonZeroUsize = unsafe { NonZeroUsize::new_unchecked(3) }; - From 5b76eda47ec4682685579c6959f965296480eede Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 15 Jun 2020 19:08:53 +0200 Subject: [PATCH 56/63] protocols/kad/src/behaviour: Expose success peers on put record error --- protocols/kad/src/behaviour.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 4c64e9b7091..f5dc896684b 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -987,7 +987,7 @@ where if success.len() >= quorum.get() { Ok(PutRecordOk { key }) } else { - Err(PutRecordError::QuorumFailed { key, quorum, num_results: success.len() }) + Err(PutRecordError::QuorumFailed { key, quorum, success }) } }; match context { @@ -1084,9 +1084,9 @@ where let err = Err(PutRecordError::Timeout { key: record.key, quorum, - num_results: match phase { - PutRecordPhase::GetClosestPeers => 0, - PutRecordPhase::PutRecord { ref success, .. } => success.len(), + success: match phase { + PutRecordPhase::GetClosestPeers => vec![], + PutRecordPhase::PutRecord { ref success, .. } => success.clone(), } }); match context { @@ -1849,12 +1849,14 @@ pub struct PutRecordOk { pub enum PutRecordError { QuorumFailed { key: record::Key, - num_results: usize, + /// [`PeerId`]s of the peers the record was successfully stored on. + success: Vec, quorum: NonZeroUsize }, Timeout { key: record::Key, - num_results: usize, + /// [`PeerId`]s of the peers the record was successfully stored on. + success: Vec, quorum: NonZeroUsize }, } From b69f24faf8419768d470c73bdbed24c5a11e43c7 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 15 Jun 2020 19:25:41 +0200 Subject: [PATCH 57/63] protocols/kad/query/disjoint: Log number finished on finish_paths --- protocols/kad/src/query/peers/closest/disjoint.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 079b2da3826..b67b12963ef 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -21,6 +21,7 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; use libp2p_core::PeerId; +use log::debug; use std::{ collections::HashMap, iter::{Cycle, Map, Peekable}, @@ -276,11 +277,19 @@ impl ClosestDisjointPeersIter { where I: IntoIterator { + let mut count = 0; + for peer in peers { + count += 1; if let Some(PeerState{ initiated_by, .. }) = self.contacted_peers.get_mut(&peer) { self.iters[*initiated_by].finish(); } } + + debug!("Finished {} out of {} paths.", count, self.iters.len()); + if self.is_finished() { + debug!("All paths finished."); + } } /// Immediately transitions the iterator to [`PeersIterState::Finished`]. From 8e3dcf3db1f846bbb761b5a1aed5c30bc84363ce Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 15 Jun 2020 19:39:11 +0200 Subject: [PATCH 58/63] protocols/kad/query/disjoint: Don't notify initiator twice --- .../kad/src/query/peers/closest/disjoint.rs | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index b67b12963ef..732e5cbc9c4 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -107,9 +107,6 @@ impl ClosestDisjointPeersIter { pub fn on_failure(&mut self, peer: &PeerId) -> bool { let mut updated = false; - // All peer failures are reported to all queries and thus to all peer - // iterators. If this iterator never started a request to the given peer - // ignore the failure. if let Some(PeerState{ initiated_by, response }) = self.contacted_peers.get_mut(peer) { updated = self.iters[*initiated_by].on_failure(peer); @@ -117,8 +114,12 @@ impl ClosestDisjointPeersIter { *response = ResponseState::Failed; } - for iter in &mut self.iters { - iter.on_failure(peer); + for (i, iter) in &mut self.iters.iter_mut().enumerate() { + if i != (*initiated_by).into() { + // This iterator never triggered an actual request to the + // given peer - thus ignore the returned boolean. + iter.on_failure(peer); + } } } @@ -161,11 +162,16 @@ impl ClosestDisjointPeersIter { *response = ResponseState::Succeeded; } - for iter in &mut self.iters { - // Only report the success to all remaining not-first iterators. - // Do not pass the `closer_peers` in order to uphold the - // S/Kademlia disjoint paths guarantee. - iter.on_success(peer, std::iter::empty()); + for (i, iter) in &mut self.iters.iter_mut().enumerate() { + if i != (*initiated_by).into() { + // Only report the success to all remaining not-first + // iterators. Do not pass the `closer_peers` in order to + // uphold the S/Kademlia disjoint paths guarantee. + // + // This iterator never triggered an actual request to the + // given peer - thus ignore the returned boolean. + iter.on_success(peer, std::iter::empty()); + } } } From 692490a3da331a0903031b55ba9bc63af6d60509 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 15 Jun 2020 20:01:44 +0200 Subject: [PATCH 59/63] protocols/kad/query/disjoint: Have `finish_paths` take reference --- protocols/kad/src/behaviour.rs | 4 ++-- protocols/kad/src/query.rs | 4 ++-- protocols/kad/src/query/peers/closest/disjoint.rs | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index f5dc896684b..1cf18f12efa 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -1518,7 +1518,7 @@ where .filter_map(|PeerRecord{ peer, .. }| peer.as_ref()) .cloned() .collect::>(); - query.try_finish(peers) + query.try_finish(peers.iter()) } } else if quorum.get() == 1 { // It is a "standard" Kademlia query, for which the @@ -1559,7 +1559,7 @@ where success.push(source); if success.len() >= quorum.get() { let peers = success.clone(); - query.try_finish(peers) + query.try_finish(peers.iter()) } } } diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 0eedfd5e1e6..84f3ad0e055 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -354,9 +354,9 @@ impl Query { /// A finished query immediately stops yielding new peers to contact and /// will be reported by [`QueryPool::poll`] via /// [`QueryPoolState::Finished`]. - pub fn try_finish(&mut self, peers: I) + pub fn try_finish<'a, I>(&mut self, peers: I) where - I: IntoIterator + I: IntoIterator { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.finish(), diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 732e5cbc9c4..4a440d87e19 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -279,15 +279,15 @@ impl ClosestDisjointPeersIter { /// Finishes all paths containing one of the given peers. /// /// See [`crate::query::Query::try_finish`] for details. - pub fn finish_paths(&mut self, peers: I) + pub fn finish_paths<'a, I>(&mut self, peers: I) where - I: IntoIterator + I: IntoIterator { let mut count = 0; for peer in peers { count += 1; - if let Some(PeerState{ initiated_by, .. }) = self.contacted_peers.get_mut(&peer) { + if let Some(PeerState{ initiated_by, .. }) = self.contacted_peers.get_mut(peer) { self.iters[*initiated_by].finish(); } } From 799f7f54745d8230d22a02fccbcc9d69e6fccddc Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 15 Jun 2020 20:02:12 +0200 Subject: [PATCH 60/63] protocols/kad/behaviour: Improve `PeerRecord` documentation --- protocols/kad/src/behaviour.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 1cf18f12efa..7059040d2a4 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -1705,6 +1705,8 @@ impl Quorum { /// record store. #[derive(Debug, Clone, PartialEq, Eq)] pub struct PeerRecord { + /// The peer from whom the record was received. `None` if the record was + /// retrieved from local storage. pub peer: Option, pub record: Record, } From e5542e8cd474cd9fb91220a7e0b50c81760148c6 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 16 Jun 2020 19:00:59 +0200 Subject: [PATCH 61/63] Revert "protocols/kad/query/disjoint: Log number finished on finish_paths" This reverts commit b69f24faf8419768d470c73bdbed24c5a11e43c7. --- protocols/kad/src/query/peers/closest/disjoint.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index 4a440d87e19..dfb015e44d8 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -21,7 +21,6 @@ use super::*; use crate::kbucket::{Key, KeyBytes}; use libp2p_core::PeerId; -use log::debug; use std::{ collections::HashMap, iter::{Cycle, Map, Peekable}, @@ -283,19 +282,11 @@ impl ClosestDisjointPeersIter { where I: IntoIterator { - let mut count = 0; - for peer in peers { - count += 1; if let Some(PeerState{ initiated_by, .. }) = self.contacted_peers.get_mut(peer) { self.iters[*initiated_by].finish(); } } - - debug!("Finished {} out of {} paths.", count, self.iters.len()); - if self.is_finished() { - debug!("All paths finished."); - } } /// Immediately transitions the iterator to [`PeersIterState::Finished`]. From d87141c3a92089130bf0af6fe8487935818ae8b5 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 16 Jun 2020 19:02:44 +0200 Subject: [PATCH 62/63] protocols/kad: Add try_finish debug logging --- protocols/kad/src/behaviour.rs | 26 ++++++++++++++++--- protocols/kad/src/query.rs | 8 +++--- .../kad/src/query/peers/closest/disjoint.rs | 4 ++- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 7059040d2a4..73d0fe62504 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -1518,7 +1518,17 @@ where .filter_map(|PeerRecord{ peer, .. }| peer.as_ref()) .cloned() .collect::>(); - query.try_finish(peers.iter()) + let is_finished = query.try_finish(peers.iter()); + + debug!( + "GetRecord query with id {:?} reached the quorum {} finished \ + with recent response from peer {}, counting {} successful \ + responses total.", + user_data, + if is_finished { "and is" } else { "but is not yet" }, + source, + peers.len(), + ); } } else if quorum.get() == 1 { // It is a "standard" Kademlia query, for which the @@ -1556,10 +1566,20 @@ where if let QueryInfo::PutRecord { phase: PutRecordPhase::PutRecord { success, .. }, quorum, .. } = &mut query.inner.info { - success.push(source); + success.push(source.clone()); if success.len() >= quorum.get() { let peers = success.clone(); - query.try_finish(peers.iter()) + let is_finished = query.try_finish(peers.iter()); + + debug!( + "PutRecord query with id {:?} reached the quorum {} finished \ + with recent response from peer {}, counting {} successful \ + responses total.", + user_data, + if is_finished { "and is" } else { "but is not yet" }, + source, + peers.len(), + ); } } } diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 84f3ad0e055..79a674bb070 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -354,15 +354,17 @@ impl Query { /// A finished query immediately stops yielding new peers to contact and /// will be reported by [`QueryPool::poll`] via /// [`QueryPoolState::Finished`]. - pub fn try_finish<'a, I>(&mut self, peers: I) + pub fn try_finish<'a, I>(&mut self, peers: I) -> bool where I: IntoIterator { match &mut self.peer_iter { QueryPeerIter::Closest(iter) => iter.finish(), - QueryPeerIter::ClosestDisjoint(iter) => iter.finish_paths(peers), + QueryPeerIter::ClosestDisjoint(iter) => return iter.finish_paths(peers), QueryPeerIter::Fixed(iter) => iter.finish() - } + }; + + true } /// Finishes the query prematurely. diff --git a/protocols/kad/src/query/peers/closest/disjoint.rs b/protocols/kad/src/query/peers/closest/disjoint.rs index dfb015e44d8..2487d8ccb15 100644 --- a/protocols/kad/src/query/peers/closest/disjoint.rs +++ b/protocols/kad/src/query/peers/closest/disjoint.rs @@ -278,7 +278,7 @@ impl ClosestDisjointPeersIter { /// Finishes all paths containing one of the given peers. /// /// See [`crate::query::Query::try_finish`] for details. - pub fn finish_paths<'a, I>(&mut self, peers: I) + pub fn finish_paths<'a, I>(&mut self, peers: I) -> bool where I: IntoIterator { @@ -287,6 +287,8 @@ impl ClosestDisjointPeersIter { self.iters[*initiated_by].finish(); } } + + self.is_finished() } /// Immediately transitions the iterator to [`PeersIterState::Finished`]. From 1232c55b9c27ca9f6679e67ed2aa4254e10449a6 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 17 Jun 2020 12:26:00 +0200 Subject: [PATCH 63/63] protocols/kad: Rework debug log and doc comments --- protocols/kad/src/behaviour.rs | 45 ++++++++++++++++------------------ protocols/kad/src/query.rs | 14 ++++++----- 2 files changed, 29 insertions(+), 30 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 73d0fe62504..57811434809 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -1511,24 +1511,22 @@ where if let Some(record) = record { records.push(PeerRecord{ peer: Some(source.clone()), record }); - if records.len() >= quorum.get() { + let quorum = quorum.get(); + if records.len() >= quorum { // Desired quorum reached. The query may finish. See // [`Query::try_finish`] for details. let peers = records.iter() .filter_map(|PeerRecord{ peer, .. }| peer.as_ref()) .cloned() .collect::>(); - let is_finished = query.try_finish(peers.iter()); - - debug!( - "GetRecord query with id {:?} reached the quorum {} finished \ - with recent response from peer {}, counting {} successful \ - responses total.", - user_data, - if is_finished { "and is" } else { "but is not yet" }, - source, - peers.len(), - ); + let finished = query.try_finish(peers.iter()); + if !finished { + debug!( + "GetRecord query ({:?}) reached quorum ({}/{}) with \ + response from peer {} but could not yet finish.", + user_data, peers.len(), quorum, source, + ); + } } } else if quorum.get() == 1 { // It is a "standard" Kademlia query, for which the @@ -1567,19 +1565,18 @@ where phase: PutRecordPhase::PutRecord { success, .. }, quorum, .. } = &mut query.inner.info { success.push(source.clone()); - if success.len() >= quorum.get() { + + let quorum = quorum.get(); + if success.len() >= quorum { let peers = success.clone(); - let is_finished = query.try_finish(peers.iter()); - - debug!( - "PutRecord query with id {:?} reached the quorum {} finished \ - with recent response from peer {}, counting {} successful \ - responses total.", - user_data, - if is_finished { "and is" } else { "but is not yet" }, - source, - peers.len(), - ); + let finished = query.try_finish(peers.iter()); + if !finished { + debug!( + "PutRecord query ({:?}) reached quorum ({}/{}) with response \ + from peer {} but could not yet finish.", + user_data, peers.len(), quorum, source, + ); + } } } } diff --git a/protocols/kad/src/query.rs b/protocols/kad/src/query.rs index 79a674bb070..67d9fb5851d 100644 --- a/protocols/kad/src/query.rs +++ b/protocols/kad/src/query.rs @@ -351,6 +351,10 @@ impl Query { /// which this is considered to be the case, i.e. for which a termination /// condition is satisfied. /// + /// Returns `true` if the query did indeed finish, `false` otherwise. In the + /// latter case, a new attempt at finishing the query may be made with new + /// `peers`. + /// /// A finished query immediately stops yielding new peers to contact and /// will be reported by [`QueryPool::poll`] via /// [`QueryPoolState::Finished`]. @@ -359,12 +363,10 @@ impl Query { I: IntoIterator { match &mut self.peer_iter { - QueryPeerIter::Closest(iter) => iter.finish(), - QueryPeerIter::ClosestDisjoint(iter) => return iter.finish_paths(peers), - QueryPeerIter::Fixed(iter) => iter.finish() - }; - - true + QueryPeerIter::Closest(iter) => { iter.finish(); true }, + QueryPeerIter::ClosestDisjoint(iter) => iter.finish_paths(peers), + QueryPeerIter::Fixed(iter) => { iter.finish(); true } + } } /// Finishes the query prematurely.