diff --git a/Cargo.lock b/Cargo.lock index 6151ed33c5b6..792b18e4af93 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -22161,7 +22161,6 @@ dependencies = [ "async-trait", "futures", "futures-timer", - "ip_network", "linked_hash_set", "log", "multihash 0.19.1", @@ -23266,6 +23265,7 @@ dependencies = [ "bs58", "bytes", "ed25519-dalek", + "ip_network", "libp2p-identity", "libp2p-kad", "litep2p", diff --git a/prdoc/pr_6545.prdoc b/prdoc/pr_6545.prdoc new file mode 100644 index 000000000000..182bb82d612f --- /dev/null +++ b/prdoc/pr_6545.prdoc @@ -0,0 +1,33 @@ +title: Ignore multi-addresses without IP from DHT records +doc: +- audience: Node Dev + description: |- + Kusama has several authority-discovery records published without a proper mutlti-address format. + + ```bash + authority="CzHcp6nEsT4NJuAbf4yCS2J8KKYpvuAe63bkWrPWhjaaT6w" + .. + addresses={.., /p2p/12D3KooWFegWJubWCeJCbWpDEAwukQmXKaWTyRgRRZ8NNdzmuxSr, ..} + ``` + In the previous example, an multiaddress of form `/p2p/12D3KooWFegWJubWCeJCbWpDEAwukQmXKaWTyRgRRZ8NNdzmuxSr` cannot establish a connection to the authority. + + This PR changes the following: + - publishing DHT records no longer includes multi addresses that are empty or just contain `/p2p/[peer]` format + - received DHT records are filtered in a similar manner + - litep2p and libp2p backends verify discovered external addresses before pushing them to other subcomponents + + + ### Next Steps + - [x] versi-net burnin with detailed logs + - [x] kusama nodes (litep2p + libp2p) to ensure the warnings are not excessive + + Closes: https://github.com/paritytech/polkadot-sdk/issues/6518 + + cc @paritytech/networking +crates: +- name: sc-authority-discovery + bump: patch +- name: sc-network-types + bump: patch +- name: sc-network + bump: patch diff --git a/substrate/client/authority-discovery/Cargo.toml b/substrate/client/authority-discovery/Cargo.toml index ac1891451ec0..7fbcdf48f6fa 100644 --- a/substrate/client/authority-discovery/Cargo.toml +++ b/substrate/client/authority-discovery/Cargo.toml @@ -24,7 +24,6 @@ async-trait = { workspace = true } codec = { workspace = true } futures = { workspace = true } futures-timer = { workspace = true } -ip_network = { workspace = true } linked_hash_set = { workspace = true } log = { workspace = true, default-features = true } multihash = { workspace = true } diff --git a/substrate/client/authority-discovery/src/worker.rs b/substrate/client/authority-discovery/src/worker.rs index 6630b7157d96..6332b58bd71c 100644 --- a/substrate/client/authority-discovery/src/worker.rs +++ b/substrate/client/authority-discovery/src/worker.rs @@ -33,7 +33,6 @@ use futures::{channel::mpsc, future, stream::Fuse, FutureExt, Stream, StreamExt} use addr_cache::AddrCache; use codec::{Decode, Encode}; -use ip_network::IpNetwork; use linked_hash_set::LinkedHashSet; use sc_network_types::kad::{Key, PeerRecord, Record}; @@ -280,7 +279,9 @@ where config .public_addresses .into_iter() - .map(|address| AddressType::PublicAddress(address).without_p2p(local_peer_id)) + .filter_map(|address| { + AddressType::PublicAddress(address).without_p2p(local_peer_id) + }) .collect() }; @@ -374,17 +375,6 @@ where 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). // @@ -397,8 +387,9 @@ where .listen_addresses() .into_iter() .filter_map(|address| { - address_is_global(&address) + (address.is_external_address_valid() && address.is_global()) .then(|| AddressType::GlobalListenAddress(address).without_p2p(local_peer_id)) + .flatten() }) .take(MAX_GLOBAL_LISTEN_ADDRESSES) .peekable(); @@ -409,8 +400,10 @@ where .external_addresses() .into_iter() .filter_map(|address| { - (publish_non_global_ips || address_is_global(&address)) - .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) + (publish_non_global_ips || + (address.is_external_address_valid() && address.is_global())) + .then(|| AddressType::ExternalAddress(address).without_p2p(local_peer_id)) + .flatten() }) .peekable(); @@ -846,10 +839,20 @@ where _ => None, }; + log::debug!( + target: LOG_TARGET, + "Received DHT record for authority {:?} with addresses {:?}", + authority_id, + addresses + ); + // Ignore [`Multiaddr`]s without [`PeerId`] or with own addresses. let addresses: Vec = addresses .into_iter() - .filter(|a| get_peer_id(&a).filter(|p| *p != local_peer_id).is_some()) + .filter(|addr| { + addr.is_external_address_valid() && + get_peer_id(&addr).filter(|p| *p != local_peer_id).is_some() + }) .collect(); let remote_peer_id = single(addresses.iter().map(|a| get_peer_id(&a))) @@ -1001,7 +1004,9 @@ impl AddressType { /// /// 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 { + /// + /// Returns `None` if the address is empty after removing the `/p2p/..` part. + fn without_p2p(self, local_peer_id: PeerId) -> Option { // Get the address and the source str for logging. let (mut address, source) = match self { AddressType::PublicAddress(address) => (address, "public address"), @@ -1019,7 +1024,13 @@ impl AddressType { } address.pop(); } - address + + // Empty addresses cannot be published. + if address.is_empty() { + None + } else { + Some(address) + } } } diff --git a/substrate/client/network/src/litep2p/discovery.rs b/substrate/client/network/src/litep2p/discovery.rs index 2bea2e5a80dc..6d05d781308f 100644 --- a/substrate/client/network/src/litep2p/discovery.rs +++ b/substrate/client/network/src/litep2p/discovery.rs @@ -511,6 +511,48 @@ impl Discovery { (false, None) } + + /// Handle the observed address from the network. + fn handle_observed_addresses(&mut self, observed_address: Multiaddr, peer: PeerId) { + // Converting to and from `sc_network_types::multiaddr::Multiaddr` is cheap considering + // it is a small wrapper over litep2p `Multiaddr`. + let observed_address: sc_network_types::multiaddr::Multiaddr = observed_address.into(); + if !observed_address.is_external_address_valid() { + log::debug!( + target: LOG_TARGET, + "Ignoring invalid external address {observed_address} from {peer:?}", + ); + return; + } + + let Some(observed_address) = observed_address.ensure_peer_id(self.local_peer_id.into()) + else { + log::debug!( + target: LOG_TARGET, + "Ignoring external address with different peer ID from {peer:?}", + ); + return + }; + let observed_address = observed_address.into(); + + let (is_new, expired_address) = self.is_new_external_address(&observed_address, peer); + + if let Some(expired_address) = expired_address { + log::trace!( + target: LOG_TARGET, + "Removing expired external address expired={expired_address} is_new={is_new} observed={observed_address}", + ); + + self.pending_events + .push_back(DiscoveryEvent::ExternalAddressExpired { address: expired_address }); + } + + if is_new { + self.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { + address: observed_address.clone(), + }); + } + } } impl Stream for Discovery { @@ -633,44 +675,7 @@ impl Stream for Discovery { observed_address, .. })) => { - let observed_address = - if let Some(Protocol::P2p(peer_id)) = observed_address.iter().last() { - if peer_id != *this.local_peer_id.as_ref() { - log::warn!( - target: LOG_TARGET, - "Discovered external address for a peer that is not us: {observed_address}", - ); - None - } else { - Some(observed_address) - } - } else { - Some(observed_address.with(Protocol::P2p(this.local_peer_id.into()))) - }; - - // Ensure that an external address with a different peer ID does not have - // side effects of evicting other external addresses via `ExternalAddressExpired`. - if let Some(observed_address) = observed_address { - let (is_new, expired_address) = - this.is_new_external_address(&observed_address, peer); - - if let Some(expired_address) = expired_address { - log::trace!( - target: LOG_TARGET, - "Removing expired external address expired={expired_address} is_new={is_new} observed={observed_address}", - ); - - this.pending_events.push_back(DiscoveryEvent::ExternalAddressExpired { - address: expired_address, - }); - } - - if is_new { - this.pending_events.push_back(DiscoveryEvent::ExternalAddressDiscovered { - address: observed_address.clone(), - }); - } - } + this.handle_observed_addresses(observed_address, peer); return Poll::Ready(Some(DiscoveryEvent::Identified { peer, diff --git a/substrate/client/network/src/litep2p/mod.rs b/substrate/client/network/src/litep2p/mod.rs index 52b2970525df..c3bdd4d9d9a6 100644 --- a/substrate/client/network/src/litep2p/mod.rs +++ b/substrate/client/network/src/litep2p/mod.rs @@ -762,13 +762,15 @@ impl NetworkBackend for Litep2pNetworkBac }; } NetworkServiceCommand::AddKnownAddress { peer, address } => { - let mut address: Multiaddr = address.into(); - - if !address.iter().any(|protocol| std::matches!(protocol, Protocol::P2p(_))) { - address.push(Protocol::P2p(litep2p::PeerId::from(peer).into())); + if !address.is_external_address_valid() { + log::debug!( + target: LOG_TARGET, + "ignoring invalid external address {address} for {peer:?}", + ); + continue } - if self.litep2p.add_known_address(peer.into(), iter::once(address.clone())) == 0usize { + if self.litep2p.add_known_address(peer.into(), iter::once(address.clone().into())) == 0usize { log::debug!( target: LOG_TARGET, "couldn't add known address ({address}) for {peer:?}, unsupported transport" diff --git a/substrate/client/network/src/peer_info.rs b/substrate/client/network/src/peer_info.rs index a673f06fd622..f23266bf6ac1 100644 --- a/substrate/client/network/src/peer_info.rs +++ b/substrate/client/network/src/peer_info.rs @@ -66,6 +66,9 @@ pub struct PeerInfoBehaviour { ping: Ping, /// Periodically identifies the remote and responds to incoming requests. identify: Identify, + /// Identity of our local node. + local_peer_id: PeerId, + /// Information that we know about all nodes. nodes_info: FnvHashMap, /// Interval at which we perform garbage collection in `nodes_info`. @@ -123,6 +126,7 @@ impl PeerInfoBehaviour { local_public_key: PublicKey, external_addresses: Arc>>, ) -> Self { + let local_peer_id = local_public_key.to_peer_id(); let identify = { let cfg = IdentifyConfig::new("/substrate/1.0".to_string(), local_public_key) .with_agent_version(user_agent) @@ -134,6 +138,7 @@ impl PeerInfoBehaviour { Self { ping: Ping::new(PingConfig::new()), identify, + local_peer_id, nodes_info: FnvHashMap::default(), garbage_collect: Box::pin(interval(GARBAGE_COLLECT_INTERVAL)), external_addresses: ExternalAddresses { addresses: external_addresses }, @@ -406,17 +411,23 @@ impl NetworkBehaviour for PeerInfoBehaviour { self.external_addresses.remove(e.addr); }, FromSwarm::NewExternalAddrCandidate(e @ NewExternalAddrCandidate { addr }) => { - self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); - self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); - - // Manually confirm all external address candidates. - // TODO: consider adding [AutoNAT protocol](https://docs.rs/libp2p/0.52.3/libp2p/autonat/index.html) - // (must go through the polkadot protocol spec) or implemeting heuristics for - // approving external address candidates. This can be done, for example, by - // approving only addresses reported by multiple peers. - // See also https://github.com/libp2p/rust-libp2p/pull/4721 introduced - // in libp2p v0.53 for heuristics approach. - self.pending_actions.push_back(ToSwarm::ExternalAddrConfirmed(addr.clone())); + // Verify the external address is valid. + let multiaddr: sc_network_types::multiaddr::Multiaddr = addr.clone().into(); + if multiaddr.is_external_address_valid() && + multiaddr.ensure_peer_id(self.local_peer_id.into()).is_some() + { + self.ping.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); + self.identify.on_swarm_event(FromSwarm::NewExternalAddrCandidate(e)); + + // Manually confirm all external address candidates. + // TODO: consider adding [AutoNAT protocol](https://docs.rs/libp2p/0.52.3/libp2p/autonat/index.html) + // (must go through the polkadot protocol spec) or implemeting heuristics for + // approving external address candidates. This can be done, for example, by + // approving only addresses reported by multiple peers. + // See also https://github.com/libp2p/rust-libp2p/pull/4721 introduced + // in libp2p v0.53 for heuristics approach. + self.pending_actions.push_back(ToSwarm::ExternalAddrConfirmed(addr.clone())); + } }, FromSwarm::ExternalAddrConfirmed(e @ ExternalAddrConfirmed { addr }) => { self.ping.on_swarm_event(FromSwarm::ExternalAddrConfirmed(e)); diff --git a/substrate/client/network/types/Cargo.toml b/substrate/client/network/types/Cargo.toml index 67814f135d39..8901bf0fa727 100644 --- a/substrate/client/network/types/Cargo.toml +++ b/substrate/client/network/types/Cargo.toml @@ -13,6 +13,7 @@ documentation = "https://docs.rs/sc-network-types" bs58 = { workspace = true, default-features = true } bytes = { version = "1.4.0", default-features = false } ed25519-dalek = { workspace = true, default-features = true } +ip_network = { workspace = true } libp2p-identity = { features = ["ed25519", "peerid", "rand"], workspace = true } libp2p-kad = { version = "0.46.2", default-features = false } litep2p = { workspace = true } diff --git a/substrate/client/network/types/src/multiaddr.rs b/substrate/client/network/types/src/multiaddr.rs index 925e24fe70d6..befc5656b6c1 100644 --- a/substrate/client/network/types/src/multiaddr.rs +++ b/substrate/client/network/types/src/multiaddr.rs @@ -32,6 +32,7 @@ pub use protocol::Protocol; // Re-export the macro under shorter name under `multiaddr`. pub use crate::build_multiaddr as multiaddr; +use crate::PeerId; /// [`Multiaddr`] type used in Substrate. Converted to libp2p's `Multiaddr` /// or litep2p's `Multiaddr` when passed to the corresponding network backend. @@ -42,6 +43,11 @@ pub struct Multiaddr { } impl Multiaddr { + /// Returns `true` if this multiaddress is empty. + pub fn is_empty(&self) -> bool { + self.multiaddr.is_empty() + } + /// Create a new, empty multiaddress. pub fn empty() -> Self { Self { multiaddr: LiteP2pMultiaddr::empty() } @@ -71,6 +77,68 @@ impl Multiaddr { pub fn to_vec(&self) -> Vec { self.multiaddr.to_vec() } + + // Checks that the address is global. + pub fn is_global(&self) -> bool { + self.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. + Protocol::Ip4(ip) => ip_network::IpNetwork::from(ip).is_global(), + Protocol::Ip6(ip) => ip_network::IpNetwork::from(ip).is_global(), + _ => true, + }) + } + + /// Verify the external address is valid. + /// + /// An external address address discovered by the network is valid when: + /// - the address is not empty + /// - the address contains a valid IP address + pub fn is_external_address_valid(&self) -> bool { + // Empty addresses are not reachable. + if self.is_empty() { + return false; + } + + // For the address to be reachable we need an IP address with a protocol. + let mut iter = self.iter(); + match iter.next() { + Some(Protocol::Ip4(address)) => + if address.is_unspecified() { + return false; + }, + Some(Protocol::Ip6(address)) => + if address.is_unspecified() { + return false; + }, + Some(Protocol::Dns(_)) | Some(Protocol::Dns4(_)) | Some(Protocol::Dns6(_)) => {}, + _ => return false, + } + // Ensure TCP or UDP (future compatibility with QUIC) is present. + match iter.next() { + Some(Protocol::Tcp(_)) | Some(Protocol::Udp(_)) => {}, + _ => return false, + } + + true + } + + /// Ensure the peer ID is present in the multiaddress. + /// + /// Returns None when the peer ID of the address is different from the local peer ID. + pub fn ensure_peer_id(self, local_peer_id: PeerId) -> Option { + if let Some(Protocol::P2p(peer_id)) = self.iter().last() { + // Invalid address if the reported peer ID is not the local peer ID. + if peer_id != *local_peer_id.as_ref() { + return None + } + + return Some(self) + } + + // Ensure the address contains the local peer ID. + Some(self.with(Protocol::P2p(local_peer_id.into()))) + } } impl Display for Multiaddr { @@ -284,3 +352,110 @@ macro_rules! build_multiaddr { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn check_is_external_address_valid() { + let peer_id = PeerId::random(); + + // Plain empty address. + let empty_address = Multiaddr::empty(); + assert!(!empty_address.is_external_address_valid()); + + // Address is still unusable. + // `/p2p/[random]` + let address_with_p2p = Multiaddr::from(Protocol::P2p(peer_id.into())); + assert!(!address_with_p2p.is_external_address_valid()); + + // Address is not empty. + let valid_address: Multiaddr = "/dns/domain1.com/tcp/30333".parse().unwrap(); + assert!(valid_address.is_external_address_valid()); + } + + #[test] + fn check_is_external_address_valid_ip() { + let peer_id = PeerId::random(); + + // Check ip4/tcp. + let address_with_ip4_tcp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip4_tcp.is_external_address_valid()); + + let address_with_ip4_tcp = + Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))).with(Protocol::Tcp(30333)); + assert!(address_with_ip4_tcp.is_external_address_valid()); + + // Check ip4/udp. + let address_with_ip4_udp = Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))) + .with(Protocol::Udp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip4_udp.is_external_address_valid()); + + let address_with_ip4_udp = + Multiaddr::from(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1))).with(Protocol::Udp(30333)); + assert!(address_with_ip4_udp.is_external_address_valid()); + + // Check ip6/tcp. + let address_with_ip6_tcp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip6_tcp.is_external_address_valid()); + + let address_with_ip6_tcp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Tcp(30333)); + assert!(address_with_ip6_tcp.is_external_address_valid()); + + // Check ip6/udp. + let address_with_ip6_udp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Udp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_ip6_udp.is_external_address_valid()); + + let address_with_ip6_udp = + Multiaddr::from(Protocol::Ip6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 1))) + .with(Protocol::Udp(30333)); + assert!(address_with_ip6_udp.is_external_address_valid()); + } + + #[test] + fn check_is_external_address_valid_dns() { + let peer_id = PeerId::random(); + + // Check dns/tcp. + let address_with_dns = Multiaddr::from(Protocol::Dns("domain1.com".into())) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_dns.is_external_address_valid()); + + let address_with_dns = + Multiaddr::from(Protocol::Dns("domain1.com".into())).with(Protocol::Tcp(30333)); + assert!(address_with_dns.is_external_address_valid()); + + // Check dns4/tcp. + let address_with_dns4 = Multiaddr::from(Protocol::Dns4("domain1.com".into())) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_dns4.is_external_address_valid()); + + let address_with_dns4 = + Multiaddr::from(Protocol::Dns4("domain1.com".into())).with(Protocol::Tcp(30333)); + assert!(address_with_dns4.is_external_address_valid()); + + // Check dns6/tcp. + let address_with_dns6 = Multiaddr::from(Protocol::Dns6("domain1.com".into())) + .with(Protocol::Tcp(30333)) + .with(Protocol::P2p(peer_id.into())); + assert!(address_with_dns6.is_external_address_valid()); + + let address_with_dns6 = + Multiaddr::from(Protocol::Dns6("domain1.com".into())).with(Protocol::Tcp(30333)); + assert!(address_with_dns6.is_external_address_valid()); + } +}