From dad4a3adc7aa308ff2f7d3ffed4cc72f45f3fbdb Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 12 Mar 2024 12:38:23 +0200 Subject: [PATCH 01/12] Do not report listen addresses via Identify protocol --- substrate/client/network/src/peer_info.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index 2735bd873db9..e46b2328d80c 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -388,7 +388,7 @@ impl NetworkBehaviour for PeerInfoBehaviour { }, FromSwarm::ExpiredListenAddr(e) => { self.ping.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); - self.identify.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); + // See `FromSwarm::NewListenAddr` for why we do not notify `Identify`. self.external_addresses.remove(e.addr); }, FromSwarm::NewExternalAddr(e) => { @@ -414,7 +414,8 @@ impl NetworkBehaviour for PeerInfoBehaviour { }, FromSwarm::NewListenAddr(e) => { self.ping.on_swarm_event(FromSwarm::NewListenAddr(e)); - self.identify.on_swarm_event(FromSwarm::NewListenAddr(e)); + // Do not report listen addresses to `Identify`, because remote nodes should only + // know about our external addresses. }, } } From 4faed09738b74bb7dc994ffc4dbcfa058f12c832 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 12 Mar 2024 15:32:37 +0200 Subject: [PATCH 02/12] Add `--hide-listen-addr` cli flag --- .../client/cli/src/params/network_params.rs | 11 +++++++++++ substrate/client/network/src/behaviour.rs | 2 ++ substrate/client/network/src/config.rs | 4 ++++ substrate/client/network/src/peer_info.rs | 16 +++++++++++++--- substrate/client/network/src/service.rs | 1 + 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/substrate/client/cli/src/params/network_params.rs b/substrate/client/cli/src/params/network_params.rs index 12f19df2a685..08738efa1478 100644 --- a/substrate/client/cli/src/params/network_params.rs +++ b/substrate/client/cli/src/params/network_params.rs @@ -54,6 +54,10 @@ pub struct NetworkParams { /// Public address that other nodes will use to connect to this node. /// /// This can be used if there's a proxy in front of this node. + /// + /// If you use this flag and the node listens on other global addresses, consider also + /// passing `--hide-listen-addr` so those other addresses are not advertised to remote + /// nodes and not added to DHT. #[arg(long, value_name = "PUBLIC_ADDR", num_args = 1..)] pub public_addr: Vec, @@ -65,6 +69,12 @@ pub struct NetworkParams { #[arg(long, value_name = "LISTEN_ADDR", num_args = 1..)] pub listen_addr: Vec, + /// Do not advertise listen addresses to remote peers, effectively hiding the addresses from + /// DHT. This does not affect addresses added via `--public-addr` and external addresses + /// discovered using Identify protocol. + #[arg(long)] + pub hide_listen_addr: bool, + /// Specify p2p protocol TCP port. #[arg(long, value_name = "PORT", conflicts_with_all = &[ "listen_addr" ])] pub port: Option, @@ -244,6 +254,7 @@ impl NetworkParams { }, default_peers_set_num_full: self.in_peers + self.out_peers, listen_addresses, + hide_listen_addresses: self.hide_listen_addr, public_addresses, node_key, node_name: node_name.to_string(), diff --git a/substrate/client/network/src/behaviour.rs b/substrate/client/network/src/behaviour.rs index 1f234683392f..6f1c4b12b772 100644 --- a/substrate/client/network/src/behaviour.rs +++ b/substrate/client/network/src/behaviour.rs @@ -175,6 +175,7 @@ impl Behaviour { request_response_protocols: Vec, peer_store_handle: PeerStoreHandle, external_addresses: Arc>>, + hide_listen_addresses: bool, ) -> Result { Ok(Self { substrate, @@ -182,6 +183,7 @@ impl Behaviour { user_agent, local_public_key, external_addresses, + hide_listen_addresses, ), discovery: disco_config.finish(), request_responses: request_responses::RequestResponsesBehaviour::new( diff --git a/substrate/client/network/src/config.rs b/substrate/client/network/src/config.rs index 24e96843c32d..334861b119b0 100644 --- a/substrate/client/network/src/config.rs +++ b/substrate/client/network/src/config.rs @@ -578,6 +578,9 @@ pub struct NetworkConfiguration { /// Multiaddresses to listen for incoming connections. pub listen_addresses: Vec, + /// Whether to hide `listen_addresses` from Identify behavior and not report to remote nodes. + pub hide_listen_addresses: bool, + /// Multiaddresses to advertise. Detected automatically if empty. pub public_addresses: Vec, @@ -669,6 +672,7 @@ impl NetworkConfiguration { Self { net_config_path, listen_addresses: Vec::new(), + hide_listen_addresses: false, public_addresses: Vec::new(), boot_nodes: Vec::new(), node_key, diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index e46b2328d80c..3eb7d804a4cf 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -71,6 +71,8 @@ pub struct PeerInfoBehaviour { garbage_collect: Pin + Send>>, /// Record keeping of external addresses. Data is queried by the `NetworkService`. external_addresses: ExternalAddresses, + /// Whether only external addresses should be sent to remote peers in [`Identify`] messages. + hide_listen_addresses: bool, } /// Information about a node we're connected to. @@ -119,6 +121,7 @@ impl PeerInfoBehaviour { user_agent: String, local_public_key: PublicKey, external_addresses: Arc>>, + hide_listen_addresses: bool, ) -> Self { let identify = { let cfg = IdentifyConfig::new("/substrate/1.0".to_string(), local_public_key) @@ -134,6 +137,7 @@ impl PeerInfoBehaviour { nodes_info: FnvHashMap::default(), garbage_collect: Box::pin(interval(GARBAGE_COLLECT_INTERVAL)), external_addresses: ExternalAddresses { addresses: external_addresses }, + hide_listen_addresses, } } @@ -388,7 +392,10 @@ impl NetworkBehaviour for PeerInfoBehaviour { }, FromSwarm::ExpiredListenAddr(e) => { self.ping.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); - // See `FromSwarm::NewListenAddr` for why we do not notify `Identify`. + // See `FromSwarm::NewListenAddr` for why we do not always notify `Identify`. + if !self.hide_listen_addresses { + self.identify.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); + } self.external_addresses.remove(e.addr); }, FromSwarm::NewExternalAddr(e) => { @@ -414,8 +421,11 @@ impl NetworkBehaviour for PeerInfoBehaviour { }, FromSwarm::NewListenAddr(e) => { self.ping.on_swarm_event(FromSwarm::NewListenAddr(e)); - // Do not report listen addresses to `Identify`, because remote nodes should only - // know about our external addresses. + // Only report listen addresses to `Identify` if remote nodes should know about our + // listen addresses. Note that external addresses are always reported. + if !self.hide_listen_addresses { + self.identify.on_swarm_event(FromSwarm::NewListenAddr(e)); + } }, } } diff --git a/substrate/client/network/src/service.rs b/substrate/client/network/src/service.rs index 47e23337633b..1597b50bde0d 100644 --- a/substrate/client/network/src/service.rs +++ b/substrate/client/network/src/service.rs @@ -435,6 +435,7 @@ where request_response_protocols, params.peer_store.clone(), external_addresses.clone(), + network_config.hide_listen_addresses, ); match result { From c52a10e8220bcc1cc20bdb194eccb3714bbbddf3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 12 Mar 2024 15:48:38 +0200 Subject: [PATCH 03/12] Add PRDoc --- prdoc/pr_3657.prdoc | 18 ++++++++++++++++++ .../client/cli/src/params/network_params.rs | 6 +++--- 2 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 prdoc/pr_3657.prdoc diff --git a/prdoc/pr_3657.prdoc b/prdoc/pr_3657.prdoc new file mode 100644 index 000000000000..24621b781509 --- /dev/null +++ b/prdoc/pr_3657.prdoc @@ -0,0 +1,18 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Do not report listen addresses via Identify protocol + +doc: + - audience: Node Operator + description: | + New flag `--hide-listen-addr` is added that makes the node not advertise listen addresses + to remote nodes via Identify protocol, effectively hiding these addresses from DHT discovery. + This does not affect external addresses added with `--public-addr` nor external addresses + discovered with Identify protocol. + Consider passing this flag if you manually specify `--public-addr` and your node listens + on other global addresses, which should not be dialed. + +crates: + - name: sc-network + - name: sc-cli diff --git a/substrate/client/cli/src/params/network_params.rs b/substrate/client/cli/src/params/network_params.rs index 08738efa1478..70fe7d60100b 100644 --- a/substrate/client/cli/src/params/network_params.rs +++ b/substrate/client/cli/src/params/network_params.rs @@ -55,9 +55,9 @@ pub struct NetworkParams { /// /// This can be used if there's a proxy in front of this node. /// - /// If you use this flag and the node listens on other global addresses, consider also - /// passing `--hide-listen-addr` so those other addresses are not advertised to remote - /// nodes and not added to DHT. + /// If you use this flag and the node listens on other global addresses, which should not be + /// dialed, consider also passing `--hide-listen-addr` so those other addresses are not + /// advertised to remote nodes and not added to DHT. #[arg(long, value_name = "PUBLIC_ADDR", num_args = 1..)] pub public_addr: Vec, From 702749f69ab065b9d3b9c6026791abefcb922d0a Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 12 Mar 2024 16:03:25 +0200 Subject: [PATCH 04/12] minor: formatting --- substrate/client/cli/src/params/network_params.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/substrate/client/cli/src/params/network_params.rs b/substrate/client/cli/src/params/network_params.rs index 70fe7d60100b..899f54d47642 100644 --- a/substrate/client/cli/src/params/network_params.rs +++ b/substrate/client/cli/src/params/network_params.rs @@ -70,7 +70,9 @@ pub struct NetworkParams { pub listen_addr: Vec, /// Do not advertise listen addresses to remote peers, effectively hiding the addresses from - /// DHT. This does not affect addresses added via `--public-addr` and external addresses + /// DHT. + /// + /// This does not affect addresses added with `--public-addr` and external addresses /// discovered using Identify protocol. #[arg(long)] pub hide_listen_addr: bool, From 7172f67dbed273978392e93524857e90c0401dbd Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 14 Mar 2024 17:07:25 +0200 Subject: [PATCH 05/12] Rework public addresses advertising via Identify --- .../client/cli/src/params/network_params.rs | 24 +++--- substrate/client/network/src/behaviour.rs | 4 +- substrate/client/network/src/config.rs | 11 ++- substrate/client/network/src/error.rs | 3 + substrate/client/network/src/peer_info.rs | 75 +++++++++++++------ substrate/client/network/src/service.rs | 13 +++- 6 files changed, 88 insertions(+), 42 deletions(-) diff --git a/substrate/client/cli/src/params/network_params.rs b/substrate/client/cli/src/params/network_params.rs index 899f54d47642..721110dbfba0 100644 --- a/substrate/client/cli/src/params/network_params.rs +++ b/substrate/client/cli/src/params/network_params.rs @@ -55,12 +55,20 @@ pub struct NetworkParams { /// /// This can be used if there's a proxy in front of this node. /// - /// If you use this flag and the node listens on other global addresses, which should not be - /// dialed, consider also passing `--hide-listen-addr` so those other addresses are not - /// advertised to remote nodes and not added to DHT. + /// If you use this flag, consider also passing `--public-addr-only` (see below). #[arg(long, value_name = "PUBLIC_ADDR", num_args = 1..)] pub public_addr: Vec, + /// Only include explicitly specified `--public-addr` into the authority discovery DHT record + /// and Identify protocol messages. + /// + /// Do not include automatically discovered external addresses into the authority discovery DHT + /// record and not advertise automatically discovered external addresses and listen addresses + /// to other nodes via Identify protocol, making sure they are not added to the DHT by remote + /// nodes. + #[arg(long, requires = "public_addr")] + pub public_addr_only: bool, + /// Listen on this multiaddress. /// /// By default: @@ -69,14 +77,6 @@ pub struct NetworkParams { #[arg(long, value_name = "LISTEN_ADDR", num_args = 1..)] pub listen_addr: Vec, - /// Do not advertise listen addresses to remote peers, effectively hiding the addresses from - /// DHT. - /// - /// This does not affect addresses added with `--public-addr` and external addresses - /// discovered using Identify protocol. - #[arg(long)] - pub hide_listen_addr: bool, - /// Specify p2p protocol TCP port. #[arg(long, value_name = "PORT", conflicts_with_all = &[ "listen_addr" ])] pub port: Option, @@ -256,8 +256,8 @@ impl NetworkParams { }, default_peers_set_num_full: self.in_peers + self.out_peers, listen_addresses, - hide_listen_addresses: self.hide_listen_addr, public_addresses, + public_addresses_only: self.public_addr_only, node_key, node_name: node_name.to_string(), client_version: client_id.to_string(), diff --git a/substrate/client/network/src/behaviour.rs b/substrate/client/network/src/behaviour.rs index 6f1c4b12b772..85b3d1f1c595 100644 --- a/substrate/client/network/src/behaviour.rs +++ b/substrate/client/network/src/behaviour.rs @@ -175,7 +175,7 @@ impl Behaviour { request_response_protocols: Vec, peer_store_handle: PeerStoreHandle, external_addresses: Arc>>, - hide_listen_addresses: bool, + public_addresses_only: Option>, ) -> Result { Ok(Self { substrate, @@ -183,7 +183,7 @@ impl Behaviour { user_agent, local_public_key, external_addresses, - hide_listen_addresses, + public_addresses_only, ), discovery: disco_config.finish(), request_responses: request_responses::RequestResponsesBehaviour::new( diff --git a/substrate/client/network/src/config.rs b/substrate/client/network/src/config.rs index 334861b119b0..c6d46e9dba4c 100644 --- a/substrate/client/network/src/config.rs +++ b/substrate/client/network/src/config.rs @@ -578,12 +578,15 @@ pub struct NetworkConfiguration { /// Multiaddresses to listen for incoming connections. pub listen_addresses: Vec, - /// Whether to hide `listen_addresses` from Identify behavior and not report to remote nodes. - pub hide_listen_addresses: bool, - /// Multiaddresses to advertise. Detected automatically if empty. pub public_addresses: Vec, + /// Do not advertise automatically discovered address. + /// + /// This applies to external addresses in the authority discovery DHT record and external & + /// listen addresses in Identify protocol messages. + pub public_addresses_only: bool, + /// List of initial node addresses pub boot_nodes: Vec, @@ -672,8 +675,8 @@ impl NetworkConfiguration { Self { net_config_path, listen_addresses: Vec::new(), - hide_listen_addresses: false, public_addresses: Vec::new(), + public_addresses_only: false, boot_nodes: Vec::new(), node_key, default_peers_set_num_full: default_peers_set.in_peers + default_peers_set.out_peers, diff --git a/substrate/client/network/src/error.rs b/substrate/client/network/src/error.rs index 01e8356fb553..2e18687e5260 100644 --- a/substrate/client/network/src/error.rs +++ b/substrate/client/network/src/error.rs @@ -77,6 +77,9 @@ pub enum Error { /// Connection closed. #[error("Connection closed")] ConnectionClosed, + /// Requested to advertise public addresses only, but no public addresses provided. + #[error("Requested to advertise public addresses only, but no public addresses provided.")] + NoPublicAddresses, } // Make `Debug` use the `Display` implementation. diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index 3eb7d804a4cf..52272296c3e8 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -31,11 +31,12 @@ use libp2p::{ Info as IdentifyInfo, }, identity::PublicKey, + multiaddr::Protocol, ping::{Behaviour as Ping, Config as PingConfig, Event as PingEvent, Success as PingSuccess}, swarm::{ behaviour::{ AddressChange, ConnectionClosed, ConnectionEstablished, DialFailure, FromSwarm, - ListenFailure, + ListenFailure, NewExternalAddr, }, ConnectionDenied, ConnectionHandler, ConnectionId, IntoConnectionHandlerSelect, NetworkBehaviour, PollParameters, THandler, THandlerInEvent, THandlerOutEvent, ToSwarm, @@ -71,8 +72,8 @@ pub struct PeerInfoBehaviour { garbage_collect: Pin + Send>>, /// Record keeping of external addresses. Data is queried by the `NetworkService`. external_addresses: ExternalAddresses, - /// Whether only external addresses should be sent to remote peers in [`Identify`] messages. - hide_listen_addresses: bool, + /// Whether only these addresses should be advertised to remote peers in [`Identify`] messages. + public_addresses_only: Option>, } /// Information about a node we're connected to. @@ -121,9 +122,9 @@ impl PeerInfoBehaviour { user_agent: String, local_public_key: PublicKey, external_addresses: Arc>>, - hide_listen_addresses: bool, + public_addresses_only: Option>, ) -> Self { - let identify = { + let mut identify = { let cfg = IdentifyConfig::new("/substrate/1.0".to_string(), local_public_key) .with_agent_version(user_agent) // We don't need any peer information cached. @@ -131,13 +132,34 @@ impl PeerInfoBehaviour { Identify::new(cfg) }; + let mut external_addresses = ExternalAddresses { addresses: external_addresses }; + + public_addresses_only.as_ref().map(|addresses| { + addresses.iter().cloned().for_each(|mut address| { + // Make sure addresses do not contain `/p2p/...` part to be consistent with + // addresses reported by [`libp2p::swarm::Swarm`]. + if matches!(address.iter().last(), Some(Protocol::P2p(_))) { + address.pop(); + } + trace!( + target: "sub-libp2p", + "PeerInfo: adding explicitly set public address {address}.", + ); + // Let `Identify` know about explicit public addresses (ab)using libp2p API. + identify + .on_swarm_event(FromSwarm::NewExternalAddr(NewExternalAddr { addr: &address })); + // Make sure explicit public addresses are added to external addresses. + external_addresses.add(address); + }); + }); + Self { ping: Ping::new(PingConfig::new()), identify, nodes_info: FnvHashMap::default(), garbage_collect: Box::pin(interval(GARBAGE_COLLECT_INTERVAL)), - external_addresses: ExternalAddresses { addresses: external_addresses }, - hide_listen_addresses, + external_addresses, + public_addresses_only, } } @@ -382,26 +404,41 @@ impl NetworkBehaviour for PeerInfoBehaviour { self.ping.on_swarm_event(FromSwarm::ListenerError(e)); self.identify.on_swarm_event(FromSwarm::ListenerError(e)); }, - FromSwarm::ExpiredExternalAddr(e) => { - self.ping.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); - self.identify.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); - }, FromSwarm::NewListener(e) => { self.ping.on_swarm_event(FromSwarm::NewListener(e)); self.identify.on_swarm_event(FromSwarm::NewListener(e)); }, + FromSwarm::NewListenAddr(e) => { + self.ping.on_swarm_event(FromSwarm::NewListenAddr(e)); + // Only notify [`Identify`] if we do not have the explicit list of advertised + // addresses. + if self.public_addresses_only.is_none() { + self.identify.on_swarm_event(FromSwarm::NewListenAddr(e)); + } + }, FromSwarm::ExpiredListenAddr(e) => { self.ping.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); // See `FromSwarm::NewListenAddr` for why we do not always notify `Identify`. - if !self.hide_listen_addresses { + if self.public_addresses_only.is_none() { self.identify.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); } - self.external_addresses.remove(e.addr); }, FromSwarm::NewExternalAddr(e) => { self.ping.on_swarm_event(FromSwarm::NewExternalAddr(e)); - self.identify.on_swarm_event(FromSwarm::NewExternalAddr(e)); - self.external_addresses.add(e.addr.clone()); + // Only notify [`Identify`] and add an external address if we do not have the + // explicit list of advertised addresses. + if self.public_addresses_only.is_none() { + self.identify.on_swarm_event(FromSwarm::NewExternalAddr(e)); + self.external_addresses.add(e.addr.clone()); + } + }, + FromSwarm::ExpiredExternalAddr(e) => { + self.ping.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); + // See `FromSwarm::NewExternalAddr` for why we do not always notify `Identify`. + if self.public_addresses_only.is_none() { + self.identify.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); + self.external_addresses.remove(e.addr); + } }, FromSwarm::AddressChange(e @ AddressChange { peer_id, old, new, .. }) => { self.ping.on_swarm_event(FromSwarm::AddressChange(e)); @@ -419,14 +456,6 @@ impl NetworkBehaviour for PeerInfoBehaviour { "Unknown peer {:?} to change address from {:?} to {:?}", peer_id, old, new); } }, - FromSwarm::NewListenAddr(e) => { - self.ping.on_swarm_event(FromSwarm::NewListenAddr(e)); - // Only report listen addresses to `Identify` if remote nodes should know about our - // listen addresses. Note that external addresses are always reported. - if !self.hide_listen_addresses { - self.identify.on_swarm_event(FromSwarm::NewListenAddr(e)); - } - }, } } diff --git a/substrate/client/network/src/service.rs b/substrate/client/network/src/service.rs index 1597b50bde0d..51ece2d8f0bc 100644 --- a/substrate/client/network/src/service.rs +++ b/substrate/client/network/src/service.rs @@ -427,6 +427,17 @@ where }; let behaviour = { + // Prepare the list of explicit external addresses. + let public_addresses_only = network_config + .public_addresses_only + .then_some(network_config.public_addresses.clone()); + + if let Some(addresses) = &public_addresses_only { + if addresses.is_empty() { + return Err(Error::NoPublicAddresses) + } + } + let result = Behaviour::new( protocol, user_agent, @@ -435,7 +446,7 @@ where request_response_protocols, params.peer_store.clone(), external_addresses.clone(), - network_config.hide_listen_addresses, + public_addresses_only, ); match result { From 19a01227d84d7b879f7eb74cb1b9780c247315f3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 14 Mar 2024 17:10:54 +0200 Subject: [PATCH 06/12] minor: comments --- substrate/client/network/src/peer_info.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index 52272296c3e8..fe33b8f865b3 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -145,7 +145,7 @@ impl PeerInfoBehaviour { target: "sub-libp2p", "PeerInfo: adding explicitly set public address {address}.", ); - // Let `Identify` know about explicit public addresses (ab)using libp2p API. + // Let [`Identify`] know about explicit public addresses (ab)using libp2p API. identify .on_swarm_event(FromSwarm::NewExternalAddr(NewExternalAddr { addr: &address })); // Make sure explicit public addresses are added to external addresses. @@ -418,7 +418,7 @@ impl NetworkBehaviour for PeerInfoBehaviour { }, FromSwarm::ExpiredListenAddr(e) => { self.ping.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); - // See `FromSwarm::NewListenAddr` for why we do not always notify `Identify`. + // See `FromSwarm::NewListenAddr` for why we do not always notify [`Identify`]. if self.public_addresses_only.is_none() { self.identify.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); } @@ -434,7 +434,7 @@ impl NetworkBehaviour for PeerInfoBehaviour { }, FromSwarm::ExpiredExternalAddr(e) => { self.ping.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); - // See `FromSwarm::NewExternalAddr` for why we do not always notify `Identify`. + // See `FromSwarm::NewExternalAddr` for why we do not always notify [`Identify`]. if self.public_addresses_only.is_none() { self.identify.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); self.external_addresses.remove(e.addr); From 714448f693fbe0bd08f39940d30e3e9d56195539 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 15 Mar 2024 13:13:40 +0200 Subject: [PATCH 07/12] Extend `--public-addr-only` on authority discovery DHT records --- .../relay-chain-minimal-node/src/lib.rs | 2 + polkadot/node/service/src/lib.rs | 2 + substrate/bin/node/cli/src/service.rs | 2 + .../client/authority-discovery/src/lib.rs | 9 +- .../client/authority-discovery/src/worker.rs | 90 ++++++++++++++----- .../client/cli/src/params/network_params.rs | 5 +- substrate/client/network/src/config.rs | 8 +- substrate/client/network/src/service.rs | 9 +- 8 files changed, 91 insertions(+), 36 deletions(-) diff --git a/cumulus/client/relay-chain-minimal-node/src/lib.rs b/cumulus/client/relay-chain-minimal-node/src/lib.rs index 4bccca59fe3e..8ee2e7fe2b75 100644 --- a/cumulus/client/relay-chain-minimal-node/src/lib.rs +++ b/cumulus/client/relay-chain-minimal-node/src/lib.rs @@ -55,6 +55,7 @@ fn build_authority_discovery_service( prometheus_registry: Option, ) -> AuthorityDiscoveryService { let auth_disc_publish_non_global_ips = config.network.allow_non_globals_in_dht; + let auth_disc_public_addresses_only = config.network.public_addresses_only.clone(); let authority_discovery_role = sc_authority_discovery::Role::Discover; let dht_event_stream = network.event_stream("authority-discovery").filter_map(|e| async move { match e { @@ -64,6 +65,7 @@ fn build_authority_discovery_service( }); let (worker, service) = sc_authority_discovery::new_worker_and_service_with_config( sc_authority_discovery::WorkerConfig { + public_addresses_only: auth_disc_public_addresses_only, publish_non_global_ips: auth_disc_publish_non_global_ips, // Require that authority discovery records are signed. strict_record_validation: true, diff --git a/polkadot/node/service/src/lib.rs b/polkadot/node/service/src/lib.rs index 83a0afc077e7..9483ada5cd08 100644 --- a/polkadot/node/service/src/lib.rs +++ b/polkadot/node/service/src/lib.rs @@ -807,6 +807,7 @@ pub fn new_full( let shared_voter_state = rpc_setup; let auth_disc_publish_non_global_ips = config.network.allow_non_globals_in_dht; + let auth_disc_public_addresses_only = config.network.public_addresses_only.clone(); let mut net_config = sc_network::config::FullNetworkConfiguration::new(&config.network); let genesis_hash = client.block_hash(0).ok().flatten().expect("Genesis block exists; qed"); @@ -1060,6 +1061,7 @@ pub fn new_full( }); let (worker, service) = sc_authority_discovery::new_worker_and_service_with_config( sc_authority_discovery::WorkerConfig { + public_addresses_only: auth_disc_public_addresses_only, publish_non_global_ips: auth_disc_publish_non_global_ips, // Require that authority discovery records are signed. strict_record_validation: true, diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index 8f2aba6b44cd..7010c86e7751 100644 --- a/substrate/bin/node/cli/src/service.rs +++ b/substrate/bin/node/cli/src/service.rs @@ -422,6 +422,7 @@ pub fn new_full_base( let shared_voter_state = rpc_setup; let auth_disc_publish_non_global_ips = config.network.allow_non_globals_in_dht; + let auth_disc_public_addresses_only = config.network.public_addresses_only.clone(); let mut net_config = sc_network::config::FullNetworkConfiguration::new(&config.network); let genesis_hash = client.block_hash(0).ok().flatten().expect("Genesis block exists; qed"); @@ -609,6 +610,7 @@ pub fn new_full_base( let (authority_discovery_worker, _service) = sc_authority_discovery::new_worker_and_service_with_config( sc_authority_discovery::WorkerConfig { + public_addresses_only: auth_disc_public_addresses_only, publish_non_global_ips: auth_disc_publish_non_global_ips, ..Default::default() }, diff --git a/substrate/client/authority-discovery/src/lib.rs b/substrate/client/authority-discovery/src/lib.rs index 6bb12804cada..52ce3e926e73 100644 --- a/substrate/client/authority-discovery/src/lib.rs +++ b/substrate/client/authority-discovery/src/lib.rs @@ -72,8 +72,14 @@ pub struct WorkerConfig { /// By default this is set to 10 minutes. pub max_query_interval: Duration, + /// Explicitly set public addresses that should be published instead of the discovered + /// external addresses. + /// + /// Defaults to `None`, meaning the discovered external addresses should be published. + pub public_addresses_only: Option>, + /// If `false`, the node won't publish on the DHT multiaddresses that contain non-global - /// IP addresses (such as 10.0.0.1). + /// IP addresses (such as 10.0.0.1). Does not apply to `public_addresses_only`. /// /// Recommended: `false` for live chains, and `true` for local chains or for testing. /// @@ -103,6 +109,7 @@ impl Default for WorkerConfig { // comparing `authority_discovery_authority_addresses_requested_total` and // `authority_discovery_dht_event_received`. max_query_interval: Duration::from_secs(10 * 60), + public_addresses_only: None, publish_non_global_ips: true, strict_record_validation: false, } diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 9bccb96ff378..bb77a107f623 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -126,7 +126,9 @@ pub struct Worker { /// List of keys onto which addresses have been published at the latest publication. /// Used to check whether they have changed. latest_published_keys: HashSet, - /// Same value as in the configuration. + /// Should we publish only these explicitly set addresses? + public_addresses_only: Option>, + /// Same value as in the configuration. Does not apply to `public_addresses_only`. publish_non_global_ips: bool, /// Same value as in the configuration. strict_record_validation: bool, @@ -224,6 +226,29 @@ where None => None, }; + // Make sure we have only valid peer ids in public addresses. + let public_addresses_only = { + let local_peer_id: Multihash = network.local_peer_id().into(); + + config.public_addresses_only.map(|addresses| { + addresses + .into_iter() + .map(|mut address| { + if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { + if peer_id != local_peer_id { + error!( + target: LOG_TARGET, + "Discarding invalid local peer ID in public address {address}.", + ); + address.pop(); + } + } + address + }) + .collect() + }) + }; + Worker { from_service: from_service.fuse(), client, @@ -232,6 +257,7 @@ where publish_interval, publish_if_changed_interval, latest_published_keys: HashSet::new(), + public_addresses_only, publish_non_global_ips: config.publish_non_global_ips, strict_record_validation: config.strict_record_validation, query_interval, @@ -306,30 +332,50 @@ where fn addresses_to_publish(&self) -> impl Iterator { let peer_id: Multihash = self.network.local_peer_id().into(); let publish_non_global_ips = self.publish_non_global_ips; - let addresses = self.network.external_addresses().into_iter().filter(move |a| { - if publish_non_global_ips { - return true - } + let addresses: Vec<_> = { + let addresses = match self.public_addresses_only { + Some(ref public_addresses) => public_addresses.clone(), + None => self + .network + .external_addresses() + .into_iter() + .filter(move |a| { + if publish_non_global_ips { + return true + } + + a.iter().all(|p| match p { + // 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(), + }; - a.iter().all(|p| match p { - // 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, - }) - }); + // The address must include the peer id if not already set. + addresses + .into_iter() + .map(move |a| { + if a.iter().any(|p| matches!(p, multiaddr::Protocol::P2p(_))) { + a + } else { + a.with(multiaddr::Protocol::P2p(peer_id)) + } + }) + .collect() + }; - debug!(target: LOG_TARGET, "Authority DHT record peer_id='{:?}' addresses='{:?}'", peer_id, addresses.clone().collect::>()); + debug!( + target: LOG_TARGET, + "Authority DHT record peer_id='{peer_id:?}' addresses='{addresses:?}'", + ); - // The address must include the peer id if not already set. - addresses.map(move |a| { - if a.iter().any(|p| matches!(p, multiaddr::Protocol::P2p(_))) { - a - } else { - a.with(multiaddr::Protocol::P2p(peer_id)) - } - }) + addresses.into_iter() } /// Publish own public addresses. diff --git a/substrate/client/cli/src/params/network_params.rs b/substrate/client/cli/src/params/network_params.rs index 721110dbfba0..1fdc94f29dd7 100644 --- a/substrate/client/cli/src/params/network_params.rs +++ b/substrate/client/cli/src/params/network_params.rs @@ -54,8 +54,6 @@ pub struct NetworkParams { /// Public address that other nodes will use to connect to this node. /// /// This can be used if there's a proxy in front of this node. - /// - /// If you use this flag, consider also passing `--public-addr-only` (see below). #[arg(long, value_name = "PUBLIC_ADDR", num_args = 1..)] pub public_addr: Vec, @@ -222,6 +220,7 @@ impl NetworkParams { }; let public_addresses = self.public_addr.clone(); + let public_addresses_only = self.public_addr_only.then_some(public_addresses.clone()); let mut boot_nodes = chain_spec.boot_nodes().to_vec(); boot_nodes.extend(self.bootnodes.clone()); @@ -257,7 +256,7 @@ impl NetworkParams { default_peers_set_num_full: self.in_peers + self.out_peers, listen_addresses, public_addresses, - public_addresses_only: self.public_addr_only, + public_addresses_only, node_key, node_name: node_name.to_string(), client_version: client_id.to_string(), diff --git a/substrate/client/network/src/config.rs b/substrate/client/network/src/config.rs index c6d46e9dba4c..0c9ece2948f0 100644 --- a/substrate/client/network/src/config.rs +++ b/substrate/client/network/src/config.rs @@ -581,11 +581,13 @@ pub struct NetworkConfiguration { /// Multiaddresses to advertise. Detected automatically if empty. pub public_addresses: Vec, - /// Do not advertise automatically discovered address. + /// Advertise `public_addresses` instead of automatically discovered address. /// /// This applies to external addresses in the authority discovery DHT record and external & /// listen addresses in Identify protocol messages. - pub public_addresses_only: bool, + /// + /// If `Some`, the contained value should match `public_addresses`. + pub public_addresses_only: Option>, /// List of initial node addresses pub boot_nodes: Vec, @@ -676,7 +678,7 @@ impl NetworkConfiguration { net_config_path, listen_addresses: Vec::new(), public_addresses: Vec::new(), - public_addresses_only: false, + public_addresses_only: None, boot_nodes: Vec::new(), node_key, default_peers_set_num_full: default_peers_set.in_peers + default_peers_set.out_peers, diff --git a/substrate/client/network/src/service.rs b/substrate/client/network/src/service.rs index 51ece2d8f0bc..e80cd21d59a3 100644 --- a/substrate/client/network/src/service.rs +++ b/substrate/client/network/src/service.rs @@ -427,12 +427,7 @@ where }; let behaviour = { - // Prepare the list of explicit external addresses. - let public_addresses_only = network_config - .public_addresses_only - .then_some(network_config.public_addresses.clone()); - - if let Some(addresses) = &public_addresses_only { + if let Some(ref addresses) = network_config.public_addresses_only { if addresses.is_empty() { return Err(Error::NoPublicAddresses) } @@ -446,7 +441,7 @@ where request_response_protocols, params.peer_store.clone(), external_addresses.clone(), - public_addresses_only, + network_config.public_addresses_only, ); match result { From 5d3e398a361be11072a48e561f34a6c531aa520a Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 15 Mar 2024 13:15:05 +0200 Subject: [PATCH 08/12] Update PRDoc --- prdoc/pr_3657.prdoc | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/prdoc/pr_3657.prdoc b/prdoc/pr_3657.prdoc index 24621b781509..97a6f5ccb350 100644 --- a/prdoc/pr_3657.prdoc +++ b/prdoc/pr_3657.prdoc @@ -1,17 +1,21 @@ # Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 # See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json -title: Do not report listen addresses via Identify protocol +title: Explicit public address for authority discovery DHT record and Identify protocol doc: - audience: Node Operator description: | - New flag `--hide-listen-addr` is added that makes the node not advertise listen addresses - to remote nodes via Identify protocol, effectively hiding these addresses from DHT discovery. - This does not affect external addresses added with `--public-addr` nor external addresses - discovered with Identify protocol. - Consider passing this flag if you manually specify `--public-addr` and your node listens - on other global addresses, which should not be dialed. + New flag `--public-addr-only` is added that forces publishing of only explicitly set public + addresses. + + If `--public-addr-only` flag is passed: + + 1. The node publishes only explicitly set `--public-addr` addresses to the authority discovery + DHT record instead af the discovered external addresses (applies to validators). + 2. Only `--public_addr` addresses are advertised via Identify protocol instead of all listen + addresses and discovered external addresses, so that remote nodes do not add them to the + node addresses in DHT (applies to all nodes). crates: - name: sc-network From ed885b5f4ec52c69a0e97438fd52890f1a6f630d Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 15 Mar 2024 15:10:01 +0200 Subject: [PATCH 09/12] Apply review suggestions --- substrate/client/network/src/peer_info.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index fe33b8f865b3..e87ae6c970e9 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -72,8 +72,9 @@ pub struct PeerInfoBehaviour { garbage_collect: Pin + Send>>, /// Record keeping of external addresses. Data is queried by the `NetworkService`. external_addresses: ExternalAddresses, - /// Whether only these addresses should be advertised to remote peers in [`Identify`] messages. - public_addresses_only: Option>, + /// Whether only explicitly set public addresses should be advertised to remote peers in + /// [`Identify`] messages. + public_addresses_only: bool, } /// Information about a node we're connected to. @@ -159,7 +160,7 @@ impl PeerInfoBehaviour { nodes_info: FnvHashMap::default(), garbage_collect: Box::pin(interval(GARBAGE_COLLECT_INTERVAL)), external_addresses, - public_addresses_only, + public_addresses_only: public_addresses_only.is_some(), } } @@ -412,14 +413,14 @@ impl NetworkBehaviour for PeerInfoBehaviour { self.ping.on_swarm_event(FromSwarm::NewListenAddr(e)); // Only notify [`Identify`] if we do not have the explicit list of advertised // addresses. - if self.public_addresses_only.is_none() { + if !self.public_addresses_only { self.identify.on_swarm_event(FromSwarm::NewListenAddr(e)); } }, FromSwarm::ExpiredListenAddr(e) => { self.ping.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); // See `FromSwarm::NewListenAddr` for why we do not always notify [`Identify`]. - if self.public_addresses_only.is_none() { + if !self.public_addresses_only { self.identify.on_swarm_event(FromSwarm::ExpiredListenAddr(e)); } }, @@ -427,7 +428,7 @@ impl NetworkBehaviour for PeerInfoBehaviour { self.ping.on_swarm_event(FromSwarm::NewExternalAddr(e)); // Only notify [`Identify`] and add an external address if we do not have the // explicit list of advertised addresses. - if self.public_addresses_only.is_none() { + if !self.public_addresses_only { self.identify.on_swarm_event(FromSwarm::NewExternalAddr(e)); self.external_addresses.add(e.addr.clone()); } @@ -435,7 +436,7 @@ impl NetworkBehaviour for PeerInfoBehaviour { FromSwarm::ExpiredExternalAddr(e) => { self.ping.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); // See `FromSwarm::NewExternalAddr` for why we do not always notify [`Identify`]. - if self.public_addresses_only.is_none() { + if !self.public_addresses_only { self.identify.on_swarm_event(FromSwarm::ExpiredExternalAddr(e)); self.external_addresses.remove(e.addr); } From d5586333fe846a0307bcc619db0ec5f21b63f9b3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 15 Mar 2024 15:19:06 +0200 Subject: [PATCH 10/12] Use `Display` for `peer_id` when printing authority DHT record --- substrate/client/authority-discovery/src/worker.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index bb77a107f623..2e18990d5b0b 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -330,7 +330,7 @@ where } fn addresses_to_publish(&self) -> impl Iterator { - let peer_id: Multihash = self.network.local_peer_id().into(); + let peer_id = self.network.local_peer_id(); let publish_non_global_ips = self.publish_non_global_ips; let addresses: Vec<_> = { let addresses = match self.public_addresses_only { @@ -364,7 +364,7 @@ where if a.iter().any(|p| matches!(p, multiaddr::Protocol::P2p(_))) { a } else { - a.with(multiaddr::Protocol::P2p(peer_id)) + a.with(multiaddr::Protocol::P2p(peer_id.into())) } }) .collect() @@ -372,7 +372,7 @@ where debug!( target: LOG_TARGET, - "Authority DHT record peer_id='{peer_id:?}' addresses='{addresses:?}'", + "Authority DHT record peer_id='{peer_id}' addresses='{addresses:?}'", ); addresses.into_iter() From 82025651330e5572c1e55d9a2d3d95dc52ebb1e1 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 15 Mar 2024 15:39:45 +0200 Subject: [PATCH 11/12] Apply review suggestions --- .../client/authority-discovery/src/worker.rs | 75 +++++++++++-------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 2e18990d5b0b..765d4093aca0 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -81,6 +81,15 @@ pub enum Role { Discover, } +/// Addreses, published as the authority discovery DHT record. +enum PublishedAddresses { + /// Publish only these addresses. + List(Vec), + /// Publish all external addresses as reported by network backend, possibly including + /// non-global ips. + External { publish_non_global_ips: bool }, +} + /// An authority discovery [`Worker`] can publish the local node's addresses as well as discover /// those of other nodes via a Kademlia DHT. /// @@ -120,16 +129,18 @@ pub struct Worker { /// Interval to be proactive, publishing own addresses. publish_interval: ExpIncInterval, + /// Pro-actively publish our own addresses at this interval, if the keys in the keystore /// have changed. publish_if_changed_interval: ExpIncInterval, + /// List of keys onto which addresses have been published at the latest publication. /// Used to check whether they have changed. latest_published_keys: HashSet, - /// Should we publish only these explicitly set addresses? - public_addresses_only: Option>, - /// Same value as in the configuration. Does not apply to `public_addresses_only`. - publish_non_global_ips: bool, + + /// Addresses to publish as the authority discovery DHT record. + published_addresses: PublishedAddresses, + /// Same value as in the configuration. strict_record_validation: bool, @@ -138,6 +149,7 @@ pub struct Worker { /// Queue of throttled lookups pending to be passed to the network. pending_lookups: Vec, + /// Set of in-flight lookups. in_flight_lookups: HashMap, @@ -226,27 +238,32 @@ where None => None, }; - // Make sure we have only valid peer ids in public addresses. - let public_addresses_only = { - let local_peer_id: Multihash = network.local_peer_id().into(); - - config.public_addresses_only.map(|addresses| { - addresses - .into_iter() - .map(|mut address| { - if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { - if peer_id != local_peer_id { - error!( - target: LOG_TARGET, - "Discarding invalid local peer ID in public address {address}.", - ); - address.pop(); + let published_addresses = match config.public_addresses_only { + Some(addresses) => { + PublishedAddresses::List({ + // Make sure we have only valid peer ids in public addresses. + let local_peer_id: Multihash = network.local_peer_id().into(); + + addresses + .into_iter() + .map(|mut address| { + if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { + if peer_id != local_peer_id { + error!( + target: LOG_TARGET, + "Discarding invalid local peer ID in public address {address}.", + ); + address.pop(); + } } - } - address - }) - .collect() - }) + address + }) + .collect() + }) + }, + None => PublishedAddresses::External { + publish_non_global_ips: config.publish_non_global_ips, + }, }; Worker { @@ -257,8 +274,7 @@ where publish_interval, publish_if_changed_interval, latest_published_keys: HashSet::new(), - public_addresses_only, - publish_non_global_ips: config.publish_non_global_ips, + published_addresses, strict_record_validation: config.strict_record_validation, query_interval, pending_lookups: Vec::new(), @@ -331,11 +347,10 @@ where fn addresses_to_publish(&self) -> impl Iterator { let peer_id = self.network.local_peer_id(); - let publish_non_global_ips = self.publish_non_global_ips; let addresses: Vec<_> = { - let addresses = match self.public_addresses_only { - Some(ref public_addresses) => public_addresses.clone(), - None => self + let addresses = match self.published_addresses { + PublishedAddresses::List(ref addresses) => addresses.clone(), + PublishedAddresses::External { publish_non_global_ips } => self .network .external_addresses() .into_iter() From a8da47870c25cd20a980e4cec10c89bbd3bbbe6b Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 18 Mar 2024 11:15:15 +0200 Subject: [PATCH 12/12] Apply review suggestions --- .../client/authority-discovery/src/worker.rs | 58 +++++++++---------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 765d4093aca0..20b992eb74ba 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -241,22 +241,26 @@ where let published_addresses = match config.public_addresses_only { Some(addresses) => { PublishedAddresses::List({ - // Make sure we have only valid peer ids in public addresses. + // Make sure we have valid peer ids in public addresses. let local_peer_id: Multihash = network.local_peer_id().into(); addresses .into_iter() .map(|mut address| { if let Some(multiaddr::Protocol::P2p(peer_id)) = address.iter().last() { - if peer_id != local_peer_id { + if peer_id == local_peer_id { + address + } else { error!( target: LOG_TARGET, "Discarding invalid local peer ID in public address {address}.", ); address.pop(); + address.with(multiaddr::Protocol::P2p(local_peer_id)) } + } else { + address.with(multiaddr::Protocol::P2p(local_peer_id)) } - address }) .collect() }) @@ -347,42 +351,34 @@ where fn addresses_to_publish(&self) -> impl Iterator { let peer_id = self.network.local_peer_id(); - let addresses: Vec<_> = { - let addresses = match self.published_addresses { - PublishedAddresses::List(ref addresses) => addresses.clone(), - PublishedAddresses::External { publish_non_global_ips } => self - .network - .external_addresses() - .into_iter() - .filter(move |a| { - if publish_non_global_ips { - return true - } - - a.iter().all(|p| match p { - // 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(), - }; - - // The address must include the peer id if not already set. - addresses + let addresses: Vec<_> = match self.published_addresses { + PublishedAddresses::List(ref addresses) => addresses.clone(), + PublishedAddresses::External { publish_non_global_ips } => self + .network + .external_addresses() .into_iter() + .filter(move |a| { + if publish_non_global_ips { + return true + } + + a.iter().all(|p| match p { + // 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, + }) + }) .map(move |a| { + // The address must include the peer id if not already set. if a.iter().any(|p| matches!(p, multiaddr::Protocol::P2p(_))) { a } else { a.with(multiaddr::Protocol::P2p(peer_id.into())) } }) - .collect() + .collect(), }; debug!(