From a4720ddccb9296e50167fd7afe6a245e1046f32b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 18 Nov 2024 17:42:14 +0200 Subject: [PATCH] Revert "authority-discovery: Populate DHT records with public listen addresses (#6298)" This reverts commit a519c475ef2ceefe0f44fc05b3f8c1e23efc608f. --- prdoc/pr_6298.prdoc | 25 --- .../client/authority-discovery/src/worker.rs | 176 +++++------------- .../authority-discovery/src/worker/tests.rs | 4 +- substrate/client/cli/src/commands/run_cmd.rs | 12 +- 4 files changed, 64 insertions(+), 153 deletions(-) delete mode 100644 prdoc/pr_6298.prdoc diff --git a/prdoc/pr_6298.prdoc b/prdoc/pr_6298.prdoc deleted file mode 100644 index fa8d73b119432..0000000000000 --- a/prdoc/pr_6298.prdoc +++ /dev/null @@ -1,25 +0,0 @@ -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 diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 9319fbe6321e7..6f4fbac77e059 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -71,13 +71,7 @@ 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 = 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; +const MAX_ADDRESSES_PER_AUTHORITY: usize = 10; /// Maximum number of in-flight DHT lookups at any given point in time. const MAX_IN_FLIGHT_LOOKUPS: usize = 8; @@ -180,9 +174,6 @@ pub struct Worker { metrics: Option, - /// Flag to ensure the warning about missing public addresses is only printed once. - warn_public_addresses: bool, - role: Role, phantom: PhantomData, @@ -280,7 +271,20 @@ where config .public_addresses .into_iter() - .map(|address| AddressType::PublicAddress(address).without_p2p(local_peer_id)) + .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 + }) .collect() }; @@ -305,7 +309,6 @@ where addr_cache, role, metrics, - warn_public_addresses: false, phantom: PhantomData, last_known_records: HashMap::new(), } @@ -370,70 +373,47 @@ where } } - fn addresses_to_publish(&mut self) -> impl Iterator { + fn addresses_to_publish(&self) -> impl Iterator { 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(global_listen_addresses) - .chain(external_addresses) - // Deduplicate addresses. - .filter(|address| seen_addresses.insert(address.clone())) - .take(MAX_ADDRESSES_TO_PUBLISH) + .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, + }) + }) .collect::>(); if !addresses.is_empty() { @@ -441,21 +421,6 @@ where 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." - ); - } } // The address must include the local peer id. @@ -472,8 +437,7 @@ where let key_store = match &self.role { Role::PublishAndDiscover(key_store) => key_store, Role::Discover => return Ok(()), - } - .clone(); + }; let addresses = serialize_addresses(self.addresses_to_publish()); if addresses.is_empty() { @@ -982,44 +946,6 @@ 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`]. diff --git a/substrate/client/authority-discovery/src/worker/tests.rs b/substrate/client/authority-discovery/src/worker/tests.rs index 63b3b334bf1f9..b49615382b8a2 100644 --- a/substrate/client/authority-discovery/src/worker/tests.rs +++ b/substrate/client/authority-discovery/src/worker/tests.rs @@ -1027,7 +1027,7 @@ fn addresses_to_publish_adds_p2p() { )); let (_to_worker, from_service) = mpsc::channel(0); - let mut worker = Worker::new( + let worker = Worker::new( from_service, Arc::new(TestApi { authorities: vec![] }), network.clone(), @@ -1065,7 +1065,7 @@ fn addresses_to_publish_respects_existing_p2p_protocol() { }); let (_to_worker, from_service) = mpsc::channel(0); - let mut worker = Worker::new( + let worker = Worker::new( from_service, Arc::new(TestApi { authorities: vec![] }), network.clone(), diff --git a/substrate/client/cli/src/commands/run_cmd.rs b/substrate/client/cli/src/commands/run_cmd.rs index 7245b46e2f7d0..f47baf2644e7e 100644 --- a/substrate/client/cli/src/commands/run_cmd.rs +++ b/substrate/client/cli/src/commands/run_cmd.rs @@ -345,7 +345,17 @@ impl CliConfiguration for RunCmd { } fn network_params(&self) -> Option<&NetworkParams> { - Some(&self.network_params) + 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) } fn keystore_params(&self) -> Option<&KeystoreParams> {