client/network: Allow configuring Kademlia's disjoint query paths (#7356)

The Rust libp2p-kad implementation can require iterative queries to use
disjoint paths for increased resiliency in the presence of potentially
adversarial nodes.

Allow Substrate users to enable this feature via the
`--kademlia-disjoint-query-paths` flag.

substrate/client/cli/src/params/network_params.rs

@@ -97,6 +97,14 @@ pub struct NetworkParams {
 	/// By default this option is true for `--dev` and false otherwise.
 	#[structopt(long)]
 	pub discover_local: bool,
+
+	/// Require iterative Kademlia DHT queries to use disjoint paths for increased resiliency in the
+	/// presence of potentially adversarial nodes.
+	///
+	/// See the S/Kademlia paper for more information on the high level design as well as its
+	/// security improvements.
+	#[structopt(long)]
+	pub kademlia_disjoint_query_paths: bool,
 }
 
 impl NetworkParams {
@@ -156,6 +164,7 @@ impl NetworkParams {
 			},
 			max_parallel_downloads: self.max_parallel_downloads,
 			allow_non_globals_in_dht: self.discover_local || is_dev,
+			kademlia_disjoint_query_paths: self.kademlia_disjoint_query_paths,
 		}
 	}
 }

substrate/client/network/src/config.rs

@@ -423,6 +423,9 @@ pub struct NetworkConfiguration {
 	pub max_parallel_downloads: u32,
 	/// Should we insert non-global addresses into the DHT?
 	pub allow_non_globals_in_dht: bool,
+	/// Require iterative Kademlia DHT queries to use disjoint paths for increased resiliency in the
+	/// presence of potentially adversarial nodes.
+	pub kademlia_disjoint_query_paths: bool,
 }
 
 impl NetworkConfiguration {
@@ -454,6 +457,7 @@ impl NetworkConfiguration {
 			},
 			max_parallel_downloads: 5,
 			allow_non_globals_in_dht: false,
+			kademlia_disjoint_query_paths: false,
 		}
 	}
 

substrate/client/network/src/discovery.rs

@@ -84,7 +84,8 @@ pub struct DiscoveryConfig {
 	allow_non_globals_in_dht: bool,
 	discovery_only_if_under_num: u64,
 	enable_mdns: bool,
-	kademlias: HashMap<ProtocolId, Kademlia<MemoryStore>>
+	kademlia_disjoint_query_paths: bool,
+	protocol_ids: HashSet<ProtocolId>,
 }
 
 impl DiscoveryConfig {
@@ -97,7 +98,8 @@ impl DiscoveryConfig {
 			allow_non_globals_in_dht: false,
 			discovery_only_if_under_num: std::u64::MAX,
 			enable_mdns: false,
-			kademlias: HashMap::new()
+			kademlia_disjoint_query_paths: false,
+			protocol_ids: HashSet::new()
 		}
 	}
 
@@ -112,12 +114,7 @@ impl DiscoveryConfig {
 	where
 		I: IntoIterator<Item = (PeerId, Multiaddr)>
 	{
-		for (peer_id, addr) in user_defined {
+		self.user_defined.extend(user_defined);
-			for kad in self.kademlias.values_mut() {
-				kad.add_address(&peer_id, addr.clone());
-			}
-			self.user_defined.push((peer_id, addr))
-		}
 		self
 	}
 
@@ -144,48 +141,71 @@ impl DiscoveryConfig {
 
 	/// Add discovery via Kademlia for the given protocol.
 	pub fn add_protocol(&mut self, id: ProtocolId) -> &mut Self {
-		let name = protocol_name_from_protocol_id(&id);
+		if self.protocol_ids.contains(&id) {
-		self.add_kademlia(id, name);
+			warn!(target: "sub-libp2p", "Discovery already registered for protocol {:?}", id);
+			return self;
+		}
+
+		self.protocol_ids.insert(id);
+
 		self
 	}
 
-	fn add_kademlia(&mut self, id: ProtocolId, proto_name: Vec<u8>) {
+	/// Require iterative Kademlia DHT queries to use disjoint paths for increased resiliency in the
-		if self.kademlias.contains_key(&id) {
+	/// presence of potentially adversarial nodes.
-			warn!(target: "sub-libp2p", "Discovery already registered for protocol {:?}", id);
+	pub fn use_kademlia_disjoint_query_paths(&mut self, value: bool) -> &mut Self {
-			return
+		self.kademlia_disjoint_query_paths = value;
-		}
+		self
-
-		let mut config = KademliaConfig::default();
-		config.set_protocol_name(proto_name);
-		// By default Kademlia attempts to insert all peers into its routing table once a dialing
-		// attempt succeeds. In order to control which peer is added, disable the auto-insertion and
-		// instead add peers manually.
-		config.set_kbucket_inserts(KademliaBucketInserts::Manual);
-
-		let store = MemoryStore::new(self.local_peer_id.clone());
-		let mut kad = Kademlia::with_config(self.local_peer_id.clone(), store, config);
-
-		for (peer_id, addr) in &self.user_defined {
-			kad.add_address(peer_id, addr.clone());
-		}
-
-		self.kademlias.insert(id, kad);
 	}
 
 	/// Create a `DiscoveryBehaviour` from this config.
 	pub fn finish(self) -> DiscoveryBehaviour {
+		let DiscoveryConfig {
+			local_peer_id,
+			user_defined,
+			allow_private_ipv4,
+			allow_non_globals_in_dht,
+			discovery_only_if_under_num,
+			enable_mdns,
+			kademlia_disjoint_query_paths,
+			protocol_ids,
+		} = self;
+
+		let kademlias = protocol_ids.into_iter()
+			.map(|protocol_id| {
+				let proto_name = protocol_name_from_protocol_id(&protocol_id);
+
+				let mut config = KademliaConfig::default();
+				config.set_protocol_name(proto_name);
+				// By default Kademlia attempts to insert all peers into its routing table once a
+				// dialing attempt succeeds. In order to control which peer is added, disable the
+				// auto-insertion and instead add peers manually.
+				config.set_kbucket_inserts(KademliaBucketInserts::Manual);
+				config.disjoint_query_paths(kademlia_disjoint_query_paths);
+
+				let store = MemoryStore::new(local_peer_id.clone());
+				let mut kad = Kademlia::with_config(local_peer_id.clone(), store, config);
+
+				for (peer_id, addr) in &user_defined {
+					kad.add_address(peer_id, addr.clone());
+				}
+
+				(protocol_id, kad)
+			})
+			.collect();
+
 		DiscoveryBehaviour {
-			user_defined: self.user_defined,
+			user_defined,
-			kademlias: self.kademlias,
+			kademlias,
 			next_kad_random_query: Delay::new(Duration::new(0, 0)),
 			duration_to_next_kad: Duration::from_secs(1),
 			pending_events: VecDeque::new(),
-			local_peer_id: self.local_peer_id,
+			local_peer_id,
 			num_connections: 0,
-			allow_private_ipv4: self.allow_private_ipv4,
+			allow_private_ipv4,
-			discovery_only_if_under_num: self.discovery_only_if_under_num,
+			discovery_only_if_under_num,
 			#[cfg(not(target_os = "unknown"))]
-			mdns: if self.enable_mdns {
+			mdns: if enable_mdns {
 				match Mdns::new() {
 					Ok(mdns) => Some(mdns).into(),
 					Err(err) => {
@@ -196,7 +216,7 @@ impl DiscoveryConfig {
 			} else {
 				None.into()
 			},
-			allow_non_globals_in_dht: self.allow_non_globals_in_dht,
+			allow_non_globals_in_dht,
 			known_external_addresses: LruHashSet::new(
 				NonZeroUsize::new(MAX_KNOWN_EXTERNAL_ADDRESSES)
 					.expect("value is a constant; constant is non-zero; qed.")

substrate/client/network/src/service.rs

@@ -292,6 +292,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
 				config.discovery_limit(u64::from(params.network_config.out_peers) + 15);
 				config.add_protocol(params.protocol_id.clone());
 				config.allow_non_globals_in_dht(params.network_config.allow_non_globals_in_dht);
+				config.use_kademlia_disjoint_query_paths(params.network_config.kademlia_disjoint_query_paths);
 
 				match params.network_config.transport {
 					TransportConfig::MemoryOnly => {