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.

client/cli/src/params/network_params.rs

@@ -88,6 +88,12 @@ pub struct NetworkParams {
 	/// Experimental feature flag.
 	#[structopt(long = "use-yamux-flow-control")]
 	pub use_yamux_flow_control: bool,
+
+	/// Allow non-global IP addresses and domain names to be added to the DHT.
+	///
+	/// By default this option is true for `--dev` and false otherwise.
+	#[structopt(long)]
+	pub allow_non_global_addresses_in_dht: bool,
 }
 
 impl NetworkParams {
@@ -140,6 +146,7 @@ impl NetworkParams {
 				use_yamux_flow_control: self.use_yamux_flow_control,
 			},
 			max_parallel_downloads: self.max_parallel_downloads,
+			allow_non_globals_in_dht: self.allow_non_global_addresses_in_dht || is_dev
 		}
 	}
 }

client/network/Cargo.toml

@@ -27,39 +27,40 @@ erased-serde = "0.3.9"
 fnv = "1.0.6"
 fork-tree = { version = "2.0.0-dev", path = "../../utils/fork-tree" }
 futures = "0.3.4"
-futures_codec = "0.3.3"
 futures-timer = "3.0.1"
-wasm-timer = "0.2"
+futures_codec = "0.3.3"
+hex = "0.4.0"
+ip_network = "0.3.4"
 linked-hash-map = "0.5.2"
 linked_hash_set = "0.1.3"
 log = "0.4.8"
 lru = "0.4.0"
 nohash-hasher = "0.2.0"
 parking_lot = "0.10.0"
+pin-project = "0.4.6"
+prometheus-endpoint = { package = "substrate-prometheus-endpoint", version = "0.8.0-dev", path = "../../utils/prometheus" }
 prost = "0.6.1"
 rand = "0.7.2"
-hex = "0.4.0"
 sc-block-builder = { version = "0.8.0-dev", path = "../block-builder" }
 sc-client = { version = "0.8.0-dev", path = "../" }
 sc-client-api = { version = "2.0.0-dev", path = "../api" }
 sc-peerset = { version = "2.0.0-dev", path = "../peerset" }
-pin-project = "0.4.6"
 serde = { version = "1.0.101", features = ["derive"] }
 serde_json = "1.0.41"
 slog = { version = "2.5.2", features = ["nested-values"] }
 slog_derive = "0.2.0"
 smallvec = "0.6.10"
 sp-arithmetic = { version = "2.0.0-dev", path = "../../primitives/arithmetic" }
-sp-utils = { version = "2.0.0-dev", path = "../../primitives/utils" }
 sp-blockchain = { version = "2.0.0-dev", path = "../../primitives/blockchain" }
 sp-consensus = { version = "0.8.0-dev", path = "../../primitives/consensus/common" }
 sp-consensus-babe = { version = "0.8.0-dev", path = "../../primitives/consensus/babe" }
 sp-core = { version = "2.0.0-dev", path = "../../primitives/core" }
 sp-runtime = { version = "2.0.0-dev", path = "../../primitives/runtime" }
-prometheus-endpoint = { package = "substrate-prometheus-endpoint", version = "0.8.0-dev", path = "../../utils/prometheus" }
+sp-utils = { version = "2.0.0-dev", path = "../../primitives/utils" }
 thiserror = "1"
 unsigned-varint = { version = "0.3.1", features = ["futures", "futures-codec"] }
 void = "1.0.2"
+wasm-timer = "0.2"
 zeroize = "1.0.0"
 
 [dependencies.libp2p]

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
 	}
 }

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`.

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 => {