network: Only insert global addresses into the DHT. (#5735)

* network: Only insert global addresses into the DHT.

Currently every address reported via libp2p-identify is inserted into
the DHT which thus contains a multitude of unreachable addresses such
as from 127.0.0.0/8 or 10.0.0.0/8.

Issue #5099 suggested a dedicated service over UDP to gauge the
reachability of an address, which would however incur extra I/O costs
and be of limited use.

As an alternative and simpler tactic, this PR only allows global IP
addresses to be inserted into the DHT unless an explicit command-line
flag `--allow-non-global-addresses-in-dht` is given or a node is
started with `--dev`. This opt-in behaviour is meant to allow
site-local networks to still make use of a DHT.

* Enable non-global in more test setups.

* Replace command-line option with different name.

* Another test fix.

substrate/client/network/src/config.rs

@@ -398,6 +398,8 @@ pub struct NetworkConfiguration {
 	pub transport: TransportConfig,
 	/// Maximum number of peers to ask the same blocks in parallel.
 	pub max_parallel_downloads: u32,
+	/// Should we insert non-global addresses into the DHT?
+	pub allow_non_globals_in_dht: bool
 }
 
 impl NetworkConfiguration {
@@ -428,6 +430,7 @@ impl NetworkConfiguration {
 				use_yamux_flow_control: false,
 			},
 			max_parallel_downloads: 5,
+			allow_non_globals_in_dht: false
 		}
 	}
 }
@@ -448,6 +451,7 @@ impl NetworkConfiguration {
 				.collect()
 		];
 
+		config.allow_non_globals_in_dht = true;
 		config
 	}
 
@@ -466,6 +470,7 @@ impl NetworkConfiguration {
 				.collect()
 		];
 
+		config.allow_non_globals_in_dht = true;
 		config
 	}
 }

substrate/client/network/src/discovery.rs

@@ -48,6 +48,7 @@
 use crate::config::ProtocolId;
 use futures::prelude::*;
 use futures_timer::Delay;
+use ip_network::IpNetwork;
 use libp2p::core::{connection::{ConnectionId, ListenerId}, ConnectedPoint, Multiaddr, PeerId, PublicKey};
 use libp2p::swarm::{NetworkBehaviour, NetworkBehaviourAction, PollParameters, ProtocolsHandler};
 use libp2p::swarm::protocols_handler::multi::MultiHandler;
@@ -71,6 +72,7 @@ pub struct DiscoveryConfig {
 	local_peer_id: PeerId,
 	user_defined: Vec<(PeerId, Multiaddr)>,
 	allow_private_ipv4: bool,
+	allow_non_globals_in_dht: bool,
 	discovery_only_if_under_num: u64,
 	enable_mdns: bool,
 	kademlias: HashMap<ProtocolId, Kademlia<MemoryStore>>
@@ -83,6 +85,7 @@ impl DiscoveryConfig {
 			local_peer_id: local_public_key.into_peer_id(),
 			user_defined: Vec::new(),
 			allow_private_ipv4: true,
+			allow_non_globals_in_dht: false,
 			discovery_only_if_under_num: std::u64::MAX,
 			enable_mdns: false,
 			kademlias: HashMap::new()
@@ -123,6 +126,12 @@ impl DiscoveryConfig {
 		self
 	}
 
+	/// Should non-global addresses be inserted to the DHT?
+	pub fn allow_non_globals_in_dht(&mut self, value: bool) -> &mut Self {
+		self.allow_non_globals_in_dht = value;
+		self
+	}
+
 	/// Should MDNS discovery be supported?
 	pub fn with_mdns(&mut self, value: bool) -> &mut Self {
 		if value && cfg!(target_os = "unknown") {
@@ -190,6 +199,7 @@ impl DiscoveryConfig {
 			} else {
 				None.into()
 			},
+			allow_non_globals_in_dht: self.allow_non_globals_in_dht
 		}
 	}
 }
@@ -219,6 +229,8 @@ pub struct DiscoveryBehaviour {
 	allow_private_ipv4: bool,
 	/// Number of active connections over which we interrupt the discovery process.
 	discovery_only_if_under_num: u64,
+	/// Should non-global addresses be added to the DHT?
+	allow_non_globals_in_dht: bool
 }
 
 impl DiscoveryBehaviour {
@@ -251,8 +263,12 @@ impl DiscoveryBehaviour {
 	/// **Note**: It is important that you call this method, otherwise the discovery mechanism will
 	/// not properly work.
 	pub fn add_self_reported_address(&mut self, peer_id: &PeerId, addr: Multiaddr) {
-		for k in self.kademlias.values_mut() {
-			k.add_address(peer_id, addr.clone())
+		if self.allow_non_globals_in_dht || self.can_add_to_dht(&addr) {
+			for k in self.kademlias.values_mut() {
+				k.add_address(peer_id, addr.clone())
+			}
+		} else {
+			log::trace!(target: "sub-libp2p", "Ignoring self-reported address {} from {}", addr, peer_id);
 		}
 	}
 
@@ -298,6 +314,23 @@ impl DiscoveryBehaviour {
 			(id, size)
 		})
 	}
+
+	/// Can the given `Multiaddr` be put into the DHT?
+	///
+	/// This test is successful only for global IP addresses and DNS names.
+	//
+	// NB: Currently all DNS names are allowed and no check for TLD suffixes is done
+	// because the set of valid domains is highly dynamic and would require frequent
+	// updates, for example by utilising publicsuffix.org or IANA.
+	pub fn can_add_to_dht(&self, addr: &Multiaddr) -> bool {
+		let ip = match addr.iter().next() {
+			Some(Protocol::Ip4(ip)) => IpNetwork::from(ip),
+			Some(Protocol::Ip6(ip)) => IpNetwork::from(ip),
+			Some(Protocol::Dns4(_)) | Some(Protocol::Dns6(_)) => return true,
+			_ => return false
+		};
+		ip.is_global()
+	}
 }
 
 /// Event generated by the `DiscoveryBehaviour`.
@@ -714,6 +747,7 @@ mod tests {
 				let mut config = DiscoveryConfig::new(keypair.public());
 				config.with_user_defined(user_defined.clone())
 					.allow_private_ipv4(true)
+					.allow_non_globals_in_dht(true)
 					.discovery_limit(50);
 				config.finish()
 			};

substrate/client/network/src/service.rs

@@ -246,6 +246,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
 				config.with_user_defined(known_addresses);
 				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);
 
 				match params.network_config.transport {
 					TransportConfig::MemoryOnly => {