[stable2407] Backport #6298

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 replaces the following warning:
+      `WARNING: No public address specified, validator node may not be reachable.`
+      with a more descriptive one originated from the authority-discovery
+      mechanism itself: `No public addresses configured and no global listen addresses found`.
+
+crates:
+  - name: sc-authority-discovery
+    bump: patch

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

@@ -70,7 +70,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;
@@ -163,6 +169,9 @@ pub struct Worker<Client, Block: BlockT, DhtEventStream> {
 
 	metrics: Option<Metrics>,
 
+	/// Flag to ensure the warning about missing public addresses is only printed once.
+	warn_public_addresses: bool,
+
 	role: Role,
 
 	phantom: PhantomData<Block>,
@@ -249,20 +258,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| AddressType::PublicAddress(address).without_p2p(local_peer_id))
 				.collect()
 		};
 
@@ -286,6 +282,7 @@ where
 			addr_cache,
 			role,
 			metrics,
+			warn_public_addresses: false,
 			phantom: PhantomData,
 		}
 	}
@@ -349,53 +346,100 @@ where
 		}
 	}
 
-	fn addresses_to_publish(&self) -> impl Iterator<Item = Multiaddr> {
+	fn addresses_to_publish(&mut self) -> impl Iterator<Item = Multiaddr> {
 		let local_peer_id = self.network.local_peer_id();
 		let publish_non_global_ips = self.publish_non_global_ips;
+
+		// 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) => IpNetwork::from(ip).is_global(),
+				multiaddr::Protocol::Ip6(ip) => IpNetwork::from(ip).is_global(),
+				_ => 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 mut global_listen_addresses = self
+			.network
+			.listen_addresses()
+			.into_iter()
+			.filter_map(|address| {
+				address_is_global(&address)
+					.then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id))
+			})
+			.take(MAX_GLOBAL_LISTEN_ADDRESSES)
+			.peekable();
+
+		// Similar to listen addresses that takes into consideration `publish_non_global_ips`.
+		let mut external_addresses = self
+			.network
+			.external_addresses()
+			.into_iter()
+			.filter_map(|address| {
+				(publish_non_global_ips || address_is_global(&address))
+					.then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id))
+			})
+			.peekable();
+
+		let has_global_listen_addresses = global_listen_addresses.peek().is_some();
+		trace!(
+			target: LOG_TARGET,
+			"Node has public addresses: {}, global listen addresses: {}, external addresses: {}",
+			!self.public_addresses.is_empty(),
+			has_global_listen_addresses,
+			external_addresses.peek().is_some(),
+		);
+
+		let mut seen_addresses = HashSet::new();
+
 		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
-				} else {
-					Some(address)
-				}
-			}))
-			.filter(move |address| {
-				if publish_non_global_ips {
-					return true
-				}
-
-				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,
-				})
-			})
+			.chain(global_listen_addresses)
+			.chain(external_addresses)
+			// Deduplicate addresses.
+			.filter(|address| seen_addresses.insert(address.clone()))
+			.take(MAX_ADDRESSES_TO_PUBLISH)
 			.collect::<Vec<_>>();
 
+<<<<<<< HEAD
 		debug!(
 			target: LOG_TARGET,
 			"Publishing authority DHT record peer_id='{local_peer_id}' addresses='{addresses:?}'",
 		);
+=======
+		if !addresses.is_empty() {
+			debug!(
+				target: LOG_TARGET,
+				"Publishing authority DHT record peer_id='{local_peer_id}' with addresses='{addresses:?}'",
+			);
+
+			if !self.warn_public_addresses &&
+				self.public_addresses.is_empty() &&
+				!has_global_listen_addresses
+			{
+				self.warn_public_addresses = true;
+
+				error!(
+					target: LOG_TARGET,
+					"No public addresses configured and no global listen addresses found. \
+					Authority DHT record may contain unreachable addresses. \
+					Consider setting `--public-addr` to the public IP address of this node. \
+					This will become a hard requirement in future versions for authorities."
+				);
+			}
+		}
+>>>>>>> 762f824 (authority-discovery: Populate DHT records with public listen addresses (#6298))
 
 		// The address must include the local peer id.
 		addresses
@@ -411,7 +455,8 @@ where
 		let key_store = match &self.role {
 			Role::PublishAndDiscover(key_store) => key_store,
 			Role::Discover => return Ok(()),
-		};
+		}
+		.clone();
 
 		let keys =
 			Worker::<Client, Block, DhtEventStream>::get_own_public_keys_within_authority_set(
@@ -802,6 +847,44 @@ where
 	}
 }
 
+/// Removes the `/p2p/..` from the address if it is present.
+#[derive(Debug, Clone, PartialEq, Eq)]
+enum AddressType {
+	/// The address is specified as a public address via the CLI.
+	PublicAddress(Multiaddr),
+	/// The address is a global listen address.
+	GlobalListenAddress(Multiaddr),
+	/// The address is discovered via the network (ie /identify protocol).
+	ExternalAddress(Multiaddr),
+}
+
+impl AddressType {
+	/// Removes the `/p2p/..` from the address if it is present.
+	///
+	/// In case the peer id in the address does not match the local peer id, an error is logged for
+	/// `ExternalAddress` and `GlobalListenAddress`.
+	fn without_p2p(self, local_peer_id: PeerId) -> Multiaddr {
+		// Get the address and the source str for logging.
+		let (mut address, source) = match self {
+			AddressType::PublicAddress(address) => (address, "public address"),
+			AddressType::GlobalListenAddress(address) => (address, "global listen address"),
+			AddressType::ExternalAddress(address) => (address, "external address"),
+		};
+
+		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 '{source}' '{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/authority-discovery/src/worker/tests.rs

@@ -740,7 +740,7 @@ fn addresses_to_publish_adds_p2p() {
 	));
 
 	let (_to_worker, from_service) = mpsc::channel(0);
-	let worker = Worker::new(
+	let mut worker = Worker::new(
 		from_service,
 		Arc::new(TestApi { authorities: vec![] }),
 		network.clone(),
@@ -778,7 +778,7 @@ fn addresses_to_publish_respects_existing_p2p_protocol() {
 	});
 
 	let (_to_worker, from_service) = mpsc::channel(0);
-	let worker = Worker::new(
+	let mut worker = Worker::new(
 		from_service,
 		Arc::new(TestApi { authorities: vec![] }),
 		network.clone(),