authority-discovery: Populate DHT records with public listen addresses

prdoc/pr_6298.prdoc

@@ -0,0 +1,25 @@
+title: Populate authority DHT records with public listen addresses
+
+doc:
+  - audience: [ Node Dev, Node Operator ]
+    description: |
+      This PR populates the authority DHT records with public listen addresses if any.
+      The change effectively ensures that addresses are added to the DHT record in the
+      following order:
+        1. Public addresses provided by CLI `--public-addresses`
+        2. Maximum of 4 public (global) listen addresses (if any)
+        3. Any external addresses discovered from the network (ie from `/identify` protocol)
+
+      While at it, this PR adds the following constraints on the number of addresses:
+       - Total number of addresses cached is bounded at 16 (increased from 10).
+       - A maximum number of 32 addresses are published to DHT records (previously unbounded).
+       - A maximum of 4 global listen addresses are utilized.
+
+      This PR also removes the following warning:
+      `WARNING: No public address specified, validator node may not be reachable.`
+
+crates:
+  - name: sc-cli
+    bump: patch
+  - name: sc-authority-discovery
+    bump: patch

substrate/client/authority-discovery/src/worker.rs

@@ -71,7 +71,13 @@ pub mod tests;
 const LOG_TARGET: &str = "sub-authority-discovery";
 
 /// Maximum number of addresses cached per authority. Additional addresses are discarded.
-const MAX_ADDRESSES_PER_AUTHORITY: usize = 10;
+const MAX_ADDRESSES_PER_AUTHORITY: usize = 16;
+
+/// Maximum number of global listen addresses published by the node.
+const MAX_GLOBAL_LISTEN_ADDRESSES: usize = 4;
+
+/// Maximum number of addresses to publish in a single record.
+const MAX_ADDRESSES_TO_PUBLISH: usize = 32;
 
 /// Maximum number of in-flight DHT lookups at any given point in time.
 const MAX_IN_FLIGHT_LOOKUPS: usize = 8;
@@ -271,20 +277,7 @@ where
 			config
 				.public_addresses
 				.into_iter()
-				.map(|mut address| {
-					if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
-						if peer_id != *local_peer_id.as_ref() {
-							error!(
-								target: LOG_TARGET,
-								"Discarding invalid local peer ID in public address {address}.",
-							);
-						}
-						// Always discard `/p2p/...` protocol for proper address comparison (local
-						// peer id will be added before publishing).
-						address.pop();
-					}
-					address
-				})
+				.map(|address| address_without_p2p(address, local_peer_id))
 				.collect()
 		};
 
@@ -376,44 +369,62 @@ where
 	fn addresses_to_publish(&self) -> impl Iterator<Item = Multiaddr> {
 		let local_peer_id = self.network.local_peer_id();
 		let publish_non_global_ips = self.publish_non_global_ips;
-		let addresses = self
-			.public_addresses
-			.clone()
-			.into_iter()
-			.chain(self.network.external_addresses().into_iter().filter_map(|mut address| {
-				// Make sure the reported external address does not contain `/p2p/...` protocol.
-				if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
-					if peer_id != *local_peer_id.as_ref() {
-						error!(
-							target: LOG_TARGET,
-							"Network returned external address '{address}' with peer id \
-							 not matching the local peer id '{local_peer_id}'.",
-						);
-						debug_assert!(false);
-					}
-					address.pop();
-				}
 
-				if self.public_addresses.contains(&address) {
-					// Already added above.
-					None
+		// Checks that the address is global.
+		let address_is_global = |address: &Multiaddr| {
+			address.iter().all(|protocol| match protocol {
+				// The `ip_network` library is used because its `is_global()` method is stable,
+				// while `is_global()` in the standard library currently isn't.
+				multiaddr::Protocol::Ip4(ip) if !IpNetwork::from(ip).is_global() => false,
+				multiaddr::Protocol::Ip6(ip) if !IpNetwork::from(ip).is_global() => false,
+				_ => true,
+			})
+		};
+
+		// These are the addresses the node is listening for incoming connections,
+		// as reported by installed protocols (tcp / websocket etc).
+		//
+		// We double check the address is global. In other words, we double check the node
+		// is not running behind a NAT.
+		// Note: we do this regardless of the `publish_non_global_ips` setting, since the
+		// node discovers many external addresses via the identify protocol.
+		let global_listen_addresses = self
+			.network
+			.listen_addresses()
+			.into_iter()
+			.filter_map(|address| {
+				if address_is_global(&address) {
+					Some(address_without_p2p(address, local_peer_id))
 				} else {
-					Some(address)
+					None
 				}
-			}))
-			.filter(move |address| {
+			})
+			.take(MAX_GLOBAL_LISTEN_ADDRESSES);
+
+		// Similar to listen addresses that takes into consideration `publish_non_global_ips`.
+		let external_addresses =
+			self.network.external_addresses().into_iter().filter_map(|address| {
+				let address = address_without_p2p(address, local_peer_id);
 				if publish_non_global_ips {
-					return true
+					Some(address)
+				} else if address_is_global(&address) {
+					Some(address)
+				} else {
+					None
 				}
+			});
 
-				address.iter().all(|protocol| match protocol {
-					// The `ip_network` library is used because its `is_global()` method is stable,
-					// while `is_global()` in the standard library currently isn't.
-					multiaddr::Protocol::Ip4(ip) if !IpNetwork::from(ip).is_global() => false,
-					multiaddr::Protocol::Ip6(ip) if !IpNetwork::from(ip).is_global() => false,
-					_ => true,
-				})
-			})
+		let mut seen_addresses = HashSet::new();
+
+		let addresses = self
+			.public_addresses
+			.clone()
+			.into_iter()
+			.chain(global_listen_addresses)
+			.chain(external_addresses)
+			// Deduplicate addresses.
+			.filter(|address| seen_addresses.insert(address.clone()))
+			.take(MAX_ADDRESSES_TO_PUBLISH)
 			.collect::<Vec<_>>();
 
 		if !addresses.is_empty() {
@@ -946,6 +957,21 @@ where
 	}
 }
 
+/// Removes the `/p2p/..` from the address if it is present.
+fn address_without_p2p(mut address: Multiaddr, local_peer_id: PeerId) -> Multiaddr {
+	if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() {
+		if peer_id != *local_peer_id.as_ref() {
+			error!(
+				target: LOG_TARGET,
+				"Network returned external address '{address}' with peer id \
+				 not matching the local peer id '{local_peer_id}'.",
+			);
+		}
+		address.pop();
+	}
+	address
+}
+
 /// NetworkProvider provides [`Worker`] with all necessary hooks into the
 /// underlying Substrate networking. Using this trait abstraction instead of
 /// `sc_network::NetworkService` directly is necessary to unit test [`Worker`].

substrate/client/cli/src/commands/run_cmd.rs

@@ -201,17 +201,7 @@ impl CliConfiguration for RunCmd {
 	}
 
 	fn network_params(&self) -> Option<&NetworkParams> {
-		let network_params = &self.network_params;
-		let is_authority = self.role(self.is_dev().ok()?).ok()?.is_authority();
-		if is_authority && network_params.public_addr.is_empty() {
-			eprintln!(
-				"WARNING: No public address specified, validator node may not be reachable.
-				Consider setting `--public-addr` to the public IP address of this node.
-				This will become a hard requirement in future versions."
-			);
-		}
-
-		Some(network_params)
+		Some(&self.network_params)
 	}
 
 	fn keystore_params(&self) -> Option<&KeystoreParams> {