Call NetworkService::add_known_address before sending a request (#2726)

* Call NetworkService::add_known_address before sending a request

* Better doc

* Update Substrate

* Update Substrate

* Restore the import 🤷‍♀️ I don't know why it compiles locally

* imports correctly

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>

node/network/bridge/src/network.rs

@@ -26,6 +26,7 @@ use parity_scale_codec::Encode;
 
 use sc_network::Event as NetworkEvent;
 use sc_network::{IfDisconnected, NetworkService, OutboundFailure, RequestFailure};
+use sc_network::config::parse_addr;
 
 use polkadot_node_network_protocol::{
 	peer_set::PeerSet,
@@ -35,7 +36,7 @@ use polkadot_node_network_protocol::{
 use polkadot_primitives::v1::{Block, Hash};
 use polkadot_subsystem::{SubsystemError, SubsystemResult};
 
-use crate::validator_discovery::{peer_id_from_multiaddr, AuthorityDiscovery};
+use crate::validator_discovery::AuthorityDiscovery;
 
 use super::LOG_TARGET;
 
@@ -235,15 +236,27 @@ impl Network for Arc<NetworkService<Block, Hash>> {
 
 		let peer_id = match peer {
 			Recipient::Peer(peer_id) =>  Some(peer_id),
-			Recipient::Authority(authority) =>
-				authority_discovery
-					.get_addresses_by_authority_id(authority)
-					.await
-					.and_then(|addrs| {
-						addrs
-							.into_iter()
-							.find_map(|addr| peer_id_from_multiaddr(&addr))
-					}),
+			Recipient::Authority(authority) => {
+				let mut found_peer_id = None;
+				// Note: `get_addresses_by_authority_id` searched in a cache, and it thus expected
+				// to be very quick.
+				for addr in authority_discovery
+					.get_addresses_by_authority_id(authority).await
+					.into_iter().flat_map(|list| list.into_iter())
+				{
+					let (peer_id, addr) = match parse_addr(addr) {
+						Ok(v) => v,
+						Err(_) => continue,
+					};
+					NetworkService::add_known_address(
+						&*self,
+						peer_id.clone(),
+						addr,
+					);
+					found_peer_id = Some(peer_id);
+				}
+				found_peer_id
+			}
 		};
 
 		let peer_id = match peer_id {

node/network/bridge/src/validator_discovery.rs

@@ -24,7 +24,7 @@ use std::sync::Arc;
 use async_trait::async_trait;
 use futures::channel::mpsc;
 
-use sc_network::multiaddr::{Multiaddr, Protocol};
+use sc_network::{config::parse_addr, multiaddr::Multiaddr};
 use sc_authority_discovery::Service as AuthorityDiscoveryService;
 use polkadot_node_network_protocol::PeerId;
 use polkadot_primitives::v1::{AuthorityDiscoveryId, Block, Hash};
@@ -131,14 +131,6 @@ fn on_revoke(map: &mut HashMap<AuthorityDiscoveryId, u64>, id: AuthorityDiscover
 	None
 }
 
-pub(crate) fn peer_id_from_multiaddr(addr: &Multiaddr) -> Option<PeerId> {
-	addr.iter().last().and_then(|protocol| if let Protocol::P2p(multihash) = protocol {
-		PeerId::from_multihash(multihash).ok()
-	} else {
-		None
-	})
-}
-
 
 pub(super) struct Service<N, AD> {
 	state: PerPeerSet<StatePerPeerSet>,
@@ -197,7 +189,7 @@ impl<N: Network, AD: AuthorityDiscovery> Service<N, AD> {
 
 			// If not ask the authority discovery
 			if let Some(addresses) = authority_discovery_service.get_addresses_by_authority_id(id.clone()).await {
-				for peer_id in addresses.iter().filter_map(peer_id_from_multiaddr) {
+				for (peer_id, _) in addresses.into_iter().filter_map(|a| parse_addr(a).ok()) {
 					if let Some(ids) = state.connected_peers.get_mut(&peer_id) {
 						ids.insert(id.clone());
 						result.insert(id.clone(), peer_id);
@@ -367,6 +359,7 @@ mod tests {
 	use super::*;
 
 	use futures::stream::StreamExt as _;
+	use sc_network::multiaddr::Protocol;
 
 	use sp_keyring::Sr25519Keyring;
 

node/network/protocol/src/request_response/request.rs

@@ -90,6 +90,12 @@ pub enum Recipient {
 ///
 /// The network implementation will make use of that sender for informing the requesting subsystem
 /// about responses/errors.
+///
+/// When using `Recipient::Peer`, keep in mind that no address (as in IP address and port) might
+/// be known for that specific peer. You are encouraged to use `Peer` for peers that you are
+/// expected to be already connected to.
+/// When using `Recipient::Authority`, the addresses can be found thanks to the authority
+/// discovery system.
 #[derive(Debug)]
 pub struct OutgoingRequest<Req> {
 	/// Intendent recipient of this request.