From e42d31ddcfe9ae2eafc9ded37fe4711c21e00fa4 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 30 Oct 2024 15:48:04 +0200 Subject: [PATCH 01/23] cli: Revert warning Signed-off-by: Alexandru Vasile --- substrate/client/cli/src/commands/run_cmd.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/substrate/client/cli/src/commands/run_cmd.rs b/substrate/client/cli/src/commands/run_cmd.rs index f91d18aca749..f79e5b558e37 100644 --- a/substrate/client/cli/src/commands/run_cmd.rs +++ b/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> { From fd680625af88db87f358d929d2c91d6e89ee43b9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 30 Oct 2024 16:55:49 +0200 Subject: [PATCH 02/23] authority-discovery: Obtain global addresses from listen addresses Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 6f4fbac77e05..1182cca890a9 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -73,6 +73,9 @@ 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; +/// Maximum number of global listen addresses published by the node. +const MAX_GLOBAL_LISTEN_ADDRESSES: usize = 4; + /// Maximum number of in-flight DHT lookups at any given point in time. const MAX_IN_FLIGHT_LOOKUPS: usize = 8; @@ -376,10 +379,59 @@ where 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) if !IpNetwork::from(ip).is_global() => false, + multiaddr::Protocol::Ip6(ip) if !IpNetwork::from(ip).is_global() => false, + _ => true, + }) + }; + + // Removes the `/p2p/..` from the address if it is present. + let address_without_p2p = |mut address: 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}'.", + ); + debug_assert!(false); + } + address.pop(); + } + address + }; + + // 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)) + } else { + None + } + }) + .take(MAX_GLOBAL_LISTEN_ADDRESSES); + let addresses = self .public_addresses .clone() .into_iter() + .chain(global_listen_addresses) .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() { From 36a4a1e2fac6fe2afde88cad54839692dbaaf04b Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 30 Oct 2024 17:22:52 +0200 Subject: [PATCH 03/23] authority-discovery: Chain an deduplicate addresses Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 52 +++++++------------ 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 1182cca890a9..97f31c080925 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -427,45 +427,29 @@ where }) .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); + if publish_non_global_ips { + Some(address) + } else if address_is_global(&address) { + Some(address) + } else { + None + } + }); + + let mut seen_addresses = HashSet::new(); + let addresses = self .public_addresses .clone() .into_iter() .chain(global_listen_addresses) - .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(external_addresses) + // Deduplicate addresses. + .filter(|address| seen_addresses.insert(address.clone())) .collect::>(); if !addresses.is_empty() { From f1c7ed699806fd5eeb67b3d4db9dd82345ee7dab Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 30 Oct 2024 17:29:01 +0200 Subject: [PATCH 04/23] authority-discovery: Refactor `address_without_p2p` into a dedicated fn Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 50 +++++++------------ 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 97f31c080925..d166f803a89e 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -274,20 +274,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() }; @@ -391,22 +378,6 @@ where }) }; - // Removes the `/p2p/..` from the address if it is present. - let address_without_p2p = |mut address: 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}'.", - ); - debug_assert!(false); - } - address.pop(); - } - address - }; - // These are the addresses the node is listening for incoming connections, // as reported by installed protocols (tcp / websocket etc). // @@ -420,7 +391,7 @@ where .into_iter() .filter_map(|address| { if address_is_global(&address) { - Some(address_without_p2p(address)) + Some(address_without_p2p(address, local_peer_id)) } else { None } @@ -430,7 +401,7 @@ where // 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); + let address = address_without_p2p(address, local_peer_id); if publish_non_global_ips { Some(address) } else if address_is_global(&address) { @@ -982,6 +953,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`]. From 5ca1f8be014bcf313b15d815cce60920a5c72000 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 30 Oct 2024 17:30:31 +0200 Subject: [PATCH 05/23] authority-discovery: Cap max addresses to publish Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index d166f803a89e..16a5c058464a 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -76,6 +76,9 @@ const MAX_ADDRESSES_PER_AUTHORITY: usize = 10; /// 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; @@ -421,6 +424,7 @@ where .chain(external_addresses) // Deduplicate addresses. .filter(|address| seen_addresses.insert(address.clone())) + .take(MAX_ADDRESSES_TO_PUBLISH) .collect::>(); if !addresses.is_empty() { From 0b091ef2ba01599f031fc1b50dfaefe382e5a0dc Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 30 Oct 2024 17:56:50 +0200 Subject: [PATCH 06/23] authority-discovery: Bump max cached addresses to 16 Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 16a5c058464a..9db12c9ca254 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -71,7 +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 = 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; From 6bbfce0fa996654cbec68ef925b1710bc631bfbf Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 30 Oct 2024 18:14:29 +0200 Subject: [PATCH 07/23] Add prdoc Signed-off-by: Alexandru Vasile --- prdoc/pr_6298.prdoc | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 prdoc/pr_6298.prdoc diff --git a/prdoc/pr_6298.prdoc b/prdoc/pr_6298.prdoc new file mode 100644 index 000000000000..dbc88b437b25 --- /dev/null +++ b/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 From 58a84326fd3bfc736e4d93786473ebc9ce0b81d9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 1 Nov 2024 13:56:14 +0200 Subject: [PATCH 08/23] authority-discovery: Warn in authority discovery about non public addreses Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 49 ++++++++++++++++--- 1 file changed, 41 insertions(+), 8 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 9db12c9ca254..a6f31818e5f9 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -37,7 +37,7 @@ use ip_network::IpNetwork; use libp2p::kad::{PeerRecord, Record}; use linked_hash_set::LinkedHashSet; -use log::{debug, error, trace}; +use log::{debug, error, trace, warn}; use prometheus_endpoint::{register, Counter, CounterVec, Gauge, Opts, U64}; use prost::Message; use rand::{seq::SliceRandom, thread_rng}; @@ -180,6 +180,8 @@ pub struct Worker { metrics: Option, + warn_public_addresses: bool, + role: Role, phantom: PhantomData, @@ -302,6 +304,7 @@ where addr_cache, role, metrics, + warn_public_addresses: false, phantom: PhantomData, last_known_records: HashMap::new(), } @@ -366,7 +369,7 @@ where } } - fn addresses_to_publish(&self) -> impl Iterator { + fn addresses_to_publish(&mut self) -> impl Iterator { let local_peer_id = self.network.local_peer_id(); let publish_non_global_ips = self.publish_non_global_ips; @@ -388,7 +391,7 @@ where // 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 + let mut global_listen_addresses = self .network .listen_addresses() .into_iter() @@ -399,11 +402,15 @@ where None } }) - .take(MAX_GLOBAL_LISTEN_ADDRESSES); + .take(MAX_GLOBAL_LISTEN_ADDRESSES) + .peekable(); // 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 mut 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 { Some(address) @@ -412,7 +419,17 @@ where } else { None } - }); + }) + .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(); @@ -432,6 +449,21 @@ 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; + + warn!( + 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 validators." + ); + } } // The address must include the local peer id. @@ -448,7 +480,8 @@ 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() { From 9c0d40dfe72c5bedd96d52b580eb87363c9fec02 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 1 Nov 2024 14:20:19 +0200 Subject: [PATCH 09/23] tests: Adress testing Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker/tests.rs b/substrate/client/authority-discovery/src/worker/tests.rs index d71a85db8b81..8018b5ea492d 100644 --- a/substrate/client/authority-discovery/src/worker/tests.rs +++ b/substrate/client/authority-discovery/src/worker/tests.rs @@ -1018,7 +1018,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(), @@ -1056,7 +1056,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(), From 0bb45267d391892572cfc199212281f5b1bbadd8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 1 Nov 2024 14:25:25 +0200 Subject: [PATCH 10/23] authority-disovery: Replace validator with authority Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index a6f31818e5f9..21dc96a1d042 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -461,7 +461,7 @@ where "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 validators." + This will become a hard requirement in future versions for authorities." ); } } From 9b34c651343009335d858b9673176905cb842f13 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 1 Nov 2024 14:27:06 +0200 Subject: [PATCH 11/23] prdoc: Add more details Signed-off-by: Alexandru Vasile --- prdoc/pr_6298.prdoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_6298.prdoc b/prdoc/pr_6298.prdoc index dbc88b437b25..0a31d9843e71 100644 --- a/prdoc/pr_6298.prdoc +++ b/prdoc/pr_6298.prdoc @@ -15,8 +15,10 @@ doc: - 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: + 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-cli From 1ce39e8b293008fc31dbb5954335fded5ff771bd Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Fri, 1 Nov 2024 17:51:05 +0200 Subject: [PATCH 12/23] Update substrate/client/authority-discovery/src/worker.rs Co-authored-by: Dmitry Markin --- substrate/client/authority-discovery/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 21dc96a1d042..ec3e0cd8aa91 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -456,7 +456,7 @@ where { self.warn_public_addresses = true; - warn!( + error!( target: LOG_TARGET, "No public addresses configured and no global listen addresses found. \ Authority DHT record may contain unreachable addresses. \ From e6f329d5d6e4d7c284a2b42cb015acef053d9fa3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Fri, 1 Nov 2024 18:09:01 +0200 Subject: [PATCH 13/23] Fix clippy Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index ec3e0cd8aa91..91f0be1c1258 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -37,7 +37,7 @@ use ip_network::IpNetwork; use libp2p::kad::{PeerRecord, Record}; use linked_hash_set::LinkedHashSet; -use log::{debug, error, trace, warn}; +use log::{debug, error, trace}; use prometheus_endpoint::{register, Counter, CounterVec, Gauge, Opts, U64}; use prost::Message; use rand::{seq::SliceRandom, thread_rng}; From 7fca9eee5ae6cd54d29d0c77143dd0e24913ce72 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Mon, 4 Nov 2024 15:28:25 +0200 Subject: [PATCH 14/23] Update substrate/client/authority-discovery/src/worker.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/client/authority-discovery/src/worker.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 91f0be1c1258..82a13b623752 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -396,8 +396,7 @@ where .listen_addresses() .into_iter() .filter_map(|address| { - if address_is_global(&address) { - Some(address_without_p2p(address, local_peer_id)) + address_is_global(&address).then(|| address_without_p2p(address, local_peer_id)) } else { None } From cedf8fa36843792f805ca6430c465e30842d51e2 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Mon, 4 Nov 2024 15:28:34 +0200 Subject: [PATCH 15/23] Update substrate/client/authority-discovery/src/worker.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/client/authority-discovery/src/worker.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 82a13b623752..0ec107ee52e9 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -378,8 +378,8 @@ where 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, + multiaddr::Protocol::Ip4(ip) => IpNetwork::from(ip).is_global(), + multiaddr::Protocol::Ip6(ip) => IpNetwork::from(ip).is_global(), _ => true, }) }; From 7f6efce122d5144c5ec813f4bd07d05a1638ac19 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> Date: Mon, 4 Nov 2024 15:28:40 +0200 Subject: [PATCH 16/23] Update substrate/client/authority-discovery/src/worker.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/client/authority-discovery/src/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 0ec107ee52e9..d1c469e5d730 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -411,7 +411,7 @@ where .into_iter() .filter_map(|address| { let address = address_without_p2p(address, local_peer_id); - if publish_non_global_ips { + (publish_non_global_ips || address_is_global(&address)).then(|| address) Some(address) } else if address_is_global(&address) { Some(address) From 094b5f5239cf5426fc23adb2455ef3b90d8833de Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 15:34:02 +0200 Subject: [PATCH 17/23] authority-discovery: Fix build Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index d1c469e5d730..8e01d7325d5c 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -396,10 +396,7 @@ where .listen_addresses() .into_iter() .filter_map(|address| { - address_is_global(&address).then(|| address_without_p2p(address, local_peer_id)) - } else { - None - } + address_is_global(&address).then(|| address_without_p2p(address, local_peer_id)) }) .take(MAX_GLOBAL_LISTEN_ADDRESSES) .peekable(); @@ -412,12 +409,6 @@ where .filter_map(|address| { let address = address_without_p2p(address, local_peer_id); (publish_non_global_ips || address_is_global(&address)).then(|| address) - Some(address) - } else if address_is_global(&address) { - Some(address) - } else { - None - } }) .peekable(); From 53a07d4a80f879bd7ae5d90d3c3b9c9be38b9fd7 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 15:34:48 +0200 Subject: [PATCH 18/23] authority-discovery: Add docs for warn_public_addresses Signed-off-by: Alexandru Vasile --- substrate/client/authority-discovery/src/worker.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 8e01d7325d5c..3c3091a5b8c8 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -180,6 +180,7 @@ pub struct Worker { metrics: Option, + /// Flag to ensure the warning about missing public addresses is only printed once. warn_public_addresses: bool, role: Role, From 9fb02e07bd1d4485b431dd05dfa2f6bac3899c06 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 16:37:41 +0200 Subject: [PATCH 19/23] authority-discovery: Add AddressType wrapper for better logs Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 54 ++++++++++++++----- 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 3c3091a5b8c8..8f2db1ab5c98 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -280,7 +280,7 @@ where config .public_addresses .into_iter() - .map(|address| address_without_p2p(address, local_peer_id)) + .map(|address| AddressType::PublicAddress(address).without_p2p(local_peer_id)) .collect() }; @@ -397,7 +397,8 @@ where .listen_addresses() .into_iter() .filter_map(|address| { - address_is_global(&address).then(|| address_without_p2p(address, local_peer_id)) + address_is_global(&address) + .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id)) }) .take(MAX_GLOBAL_LISTEN_ADDRESSES) .peekable(); @@ -408,8 +409,8 @@ where .external_addresses() .into_iter() .filter_map(|address| { - let address = address_without_p2p(address, local_peer_id); - (publish_non_global_ips || address_is_global(&address)).then(|| address) + (publish_non_global_ips || address_is_global(&address)) + .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) }) .peekable(); @@ -982,18 +983,43 @@ 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}'.", - ); +#[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, Some("public address")), + AddressType::GlobalListenAddress(address) => (address, Some("global listen address")), + AddressType::ExternalAddress(address) => (address, None), + }; + + if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { + if peer_id != *local_peer_id.as_ref() { + if let Some(source) = source { + error!( + target: LOG_TARGET, + "Network returned '{source}' '{address}' with peer id \ + not matching the local peer id '{local_peer_id}'.", + ); + } + } + address.pop(); } - address.pop(); + address } - address } /// NetworkProvider provides [`Worker`] with all necessary hooks into the From 9fed62ea4703c2a77e06df801b9820a353b77972 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 16:38:31 +0200 Subject: [PATCH 20/23] authority-discovery: Log error for all types on mismatched peerId Signed-off-by: Alexandru Vasile --- .../client/authority-discovery/src/worker.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 8f2db1ab5c98..9319fbe6321e 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -1001,20 +1001,18 @@ impl AddressType { 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, Some("public address")), - AddressType::GlobalListenAddress(address) => (address, Some("global listen address")), - AddressType::ExternalAddress(address) => (address, None), + 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() { - if let Some(source) = source { - error!( - target: LOG_TARGET, - "Network returned '{source}' '{address}' with peer id \ - not matching the local peer id '{local_peer_id}'.", - ); - } + error!( + target: LOG_TARGET, + "Network returned '{source}' '{address}' with peer id \ + not matching the local peer id '{local_peer_id}'.", + ); } address.pop(); } From eab484c241c5ed52fcd926ce78575487b36d79d3 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 17:03:38 +0200 Subject: [PATCH 21/23] prdoc: Try to remove sc-cli for semvercheck Signed-off-by: Alexandru Vasile --- prdoc/pr_6298.prdoc | 2 -- 1 file changed, 2 deletions(-) diff --git a/prdoc/pr_6298.prdoc b/prdoc/pr_6298.prdoc index 0a31d9843e71..fa8d73b11943 100644 --- a/prdoc/pr_6298.prdoc +++ b/prdoc/pr_6298.prdoc @@ -21,7 +21,5 @@ doc: mechanism itself: `No public addresses configured and no global listen addresses found`. crates: - - name: sc-cli - bump: patch - name: sc-authority-discovery bump: patch From 49f6d2eeaea9df889139de80c81fc6039fbefa44 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 18:45:58 +0200 Subject: [PATCH 22/23] primitives: Revert panicHook import to fix CI for now Signed-off-by: Alexandru Vasile --- substrate/primitives/panic-handler/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/primitives/panic-handler/src/lib.rs b/substrate/primitives/panic-handler/src/lib.rs index 81ccaaee828e..c4a7eb8dc67c 100644 --- a/substrate/primitives/panic-handler/src/lib.rs +++ b/substrate/primitives/panic-handler/src/lib.rs @@ -30,7 +30,7 @@ use std::{ cell::Cell, io::{self, Write}, marker::PhantomData, - panic::{self, PanicHookInfo}, + panic::{self, PanicInfo}, sync::LazyLock, thread, }; @@ -149,7 +149,7 @@ fn strip_control_codes(input: &str) -> std::borrow::Cow { } /// Function being called when a panic happens. -fn panic_hook(info: &PanicHookInfo, report_url: &str, version: &str) { +fn panic_hook(info: &PanicInfo, report_url: &str, version: &str) { let location = info.location(); let file = location.as_ref().map(|l| l.file()).unwrap_or(""); let line = location.as_ref().map(|l| l.line()).unwrap_or(0); From 31b3595a919cfac1e22bdd7011bcc20eab6f07a0 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 4 Nov 2024 19:10:04 +0200 Subject: [PATCH 23/23] Revert "primitives: Revert panicHook import to fix CI for now" This reverts commit 49f6d2eeaea9df889139de80c81fc6039fbefa44. --- substrate/primitives/panic-handler/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/primitives/panic-handler/src/lib.rs b/substrate/primitives/panic-handler/src/lib.rs index c4a7eb8dc67c..81ccaaee828e 100644 --- a/substrate/primitives/panic-handler/src/lib.rs +++ b/substrate/primitives/panic-handler/src/lib.rs @@ -30,7 +30,7 @@ use std::{ cell::Cell, io::{self, Write}, marker::PhantomData, - panic::{self, PanicInfo}, + panic::{self, PanicHookInfo}, sync::LazyLock, thread, }; @@ -149,7 +149,7 @@ fn strip_control_codes(input: &str) -> std::borrow::Cow { } /// Function being called when a panic happens. -fn panic_hook(info: &PanicInfo, report_url: &str, version: &str) { +fn panic_hook(info: &PanicHookInfo, report_url: &str, version: &str) { let location = info.location(); let file = location.as_ref().map(|l| l.file()).unwrap_or(""); let line = location.as_ref().map(|l| l.line()).unwrap_or(0);