Make NetworkService::add_reserved_peer() accept MultiaddrWithPeerId (#12102)

* Make `add_reserved_peer()` accept `MultiaddrWithPeerId`

* minor: cargo fmt

* minor: error to string conversion

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/client/finality-grandpa/src/communication/tests.rs

@@ -27,6 +27,7 @@ use futures::prelude::*;
 use parity_scale_codec::Encode;
 use sc_network::{config::Role, Multiaddr, PeerId, ReputationChange};
 use sc_network_common::{
+	config::MultiaddrWithPeerId,
 	protocol::event::{Event as NetworkEvent, ObservedRole},
 	service::{
 		NetworkBlock, NetworkEventStream, NetworkNotification, NetworkPeers,
@@ -87,7 +88,7 @@ impl NetworkPeers for TestNetwork {
 		unimplemented!();
 	}
 
-	fn add_reserved_peer(&self, _peer: String) -> Result<(), String> {
+	fn add_reserved_peer(&self, _peer: MultiaddrWithPeerId) -> Result<(), String> {
 		unimplemented!();
 	}
 

substrate/client/network-gossip/src/bridge.rs

@@ -317,6 +317,7 @@ mod tests {
 	};
 	use quickcheck::{Arbitrary, Gen, QuickCheck};
 	use sc_network_common::{
+		config::MultiaddrWithPeerId,
 		protocol::event::ObservedRole,
 		service::{
 			NetworkBlock, NetworkEventStream, NetworkNotification, NetworkPeers,
@@ -371,7 +372,7 @@ mod tests {
 			unimplemented!();
 		}
 
-		fn add_reserved_peer(&self, _peer: String) -> Result<(), String> {
+		fn add_reserved_peer(&self, _peer: MultiaddrWithPeerId) -> Result<(), String> {
 			unimplemented!();
 		}
 

substrate/client/network-gossip/src/state_machine.rs

@@ -514,6 +514,7 @@ mod tests {
 	use crate::multiaddr::Multiaddr;
 	use futures::prelude::*;
 	use sc_network_common::{
+		config::MultiaddrWithPeerId,
 		protocol::event::Event,
 		service::{
 			NetworkBlock, NetworkEventStream, NetworkNotification, NetworkPeers,
@@ -610,7 +611,7 @@ mod tests {
 			unimplemented!();
 		}
 
-		fn add_reserved_peer(&self, _peer: String) -> Result<(), String> {
+		fn add_reserved_peer(&self, _peer: MultiaddrWithPeerId) -> Result<(), String> {
 			unimplemented!();
 		}
 

substrate/client/network/common/src/service.rs

@@ -19,6 +19,7 @@
 // If you read this, you are very thorough, congratulations.
 
 use crate::{
+	config::MultiaddrWithPeerId,
 	protocol::event::Event,
 	request_responses::{IfDisconnected, RequestFailure},
 	sync::{warp::WarpSyncProgress, StateDownloadProgress, SyncState},
@@ -179,12 +180,11 @@ pub trait NetworkPeers {
 	/// purposes.
 	fn deny_unreserved_peers(&self);
 
-	/// Adds a `PeerId` and its address as reserved. The string should encode the address
-	/// and peer ID of the remote node.
+	/// Adds a `PeerId` and its `Multiaddr` as reserved.
 	///
 	/// Returns an `Err` if the given string is not a valid multiaddress
 	/// or contains an invalid peer ID (which includes the local peer ID).
-	fn add_reserved_peer(&self, peer: String) -> Result<(), String>;
+	fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String>;
 
 	/// Removes a `PeerId` from the list of reserved peers.
 	fn remove_reserved_peer(&self, peer_id: PeerId);
@@ -285,7 +285,7 @@ where
 		T::deny_unreserved_peers(self)
 	}
 
-	fn add_reserved_peer(&self, peer: String) -> Result<(), String> {
+	fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String> {
 		T::add_reserved_peer(self, peer)
 	}
 

substrate/client/network/src/service.rs

@@ -62,7 +62,7 @@ use parking_lot::Mutex;
 use sc_client_api::{BlockBackend, ProofProvider};
 use sc_consensus::{BlockImportError, BlockImportStatus, ImportQueue, Link};
 use sc_network_common::{
-	config::parse_str_addr,
+	config::MultiaddrWithPeerId,
 	protocol::event::{DhtEvent, Event},
 	request_responses::{IfDisconnected, RequestFailure},
 	service::{
@@ -702,9 +702,8 @@ where
 		self.service.remove_reserved_peer(peer);
 	}
 
-	/// Adds a `PeerId` and its address as reserved. The string should encode the address
-	/// and peer ID of the remote node.
-	pub fn add_reserved_peer(&self, peer: String) -> Result<(), String> {
+	/// Adds a `PeerId` and its `Multiaddr` as reserved.
+	pub fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String> {
 		self.service.add_reserved_peer(peer)
 	}
 
@@ -912,17 +911,16 @@ where
 		let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::SetReservedOnly(true));
 	}
 
-	fn add_reserved_peer(&self, peer: String) -> Result<(), String> {
-		let (peer_id, addr) = parse_str_addr(&peer).map_err(|e| format!("{:?}", e))?;
+	fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String> {
 		// Make sure the local peer ID is never added to the PSM.
-		if peer_id == self.local_peer_id {
+		if peer.peer_id == self.local_peer_id {
 			return Err("Local peer ID cannot be added as a reserved peer.".to_string())
 		}
 
 		let _ = self
 			.to_worker
-			.unbounded_send(ServiceToWorkerMsg::AddKnownAddress(peer_id, addr));
-		let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::AddReserved(peer_id));
+			.unbounded_send(ServiceToWorkerMsg::AddKnownAddress(peer.peer_id, peer.multiaddr));
+		let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::AddReserved(peer.peer_id));
 		Ok(())
 	}
 

substrate/client/offchain/src/api.rs

@@ -326,7 +326,10 @@ mod tests {
 	use super::*;
 	use libp2p::PeerId;
 	use sc_client_db::offchain::LocalStorage;
-	use sc_network_common::service::{NetworkPeers, NetworkStateInfo};
+	use sc_network_common::{
+		config::MultiaddrWithPeerId,
+		service::{NetworkPeers, NetworkStateInfo},
+	};
 	use sc_peerset::ReputationChange;
 	use sp_core::offchain::{DbExternalities, Externalities};
 	use std::{borrow::Cow, time::SystemTime};
@@ -362,7 +365,7 @@ mod tests {
 			unimplemented!();
 		}
 
-		fn add_reserved_peer(&self, _peer: String) -> Result<(), String> {
+		fn add_reserved_peer(&self, _peer: MultiaddrWithPeerId) -> Result<(), String> {
 			unimplemented!();
 		}
 

substrate/client/offchain/src/lib.rs

@@ -249,6 +249,7 @@ mod tests {
 	use libp2p::{Multiaddr, PeerId};
 	use sc_block_builder::BlockBuilderProvider as _;
 	use sc_client_api::Backend as _;
+	use sc_network_common::config::MultiaddrWithPeerId;
 	use sc_peerset::ReputationChange;
 	use sc_transaction_pool::{BasicPool, FullChainApi};
 	use sc_transaction_pool_api::{InPoolTransaction, TransactionPool};
@@ -300,7 +301,7 @@ mod tests {
 			unimplemented!();
 		}
 
-		fn add_reserved_peer(&self, _peer: String) -> Result<(), String> {
+		fn add_reserved_peer(&self, _peer: MultiaddrWithPeerId) -> Result<(), String> {
 			unimplemented!();
 		}
 

substrate/client/service/src/lib.rs

@@ -42,7 +42,7 @@ use jsonrpsee::{core::Error as JsonRpseeError, RpcModule};
 use log::{debug, error, warn};
 use sc_client_api::{blockchain::HeaderBackend, BlockBackend, BlockchainEvents, ProofProvider};
 use sc_network::PeerId;
-use sc_network_common::service::NetworkBlock;
+use sc_network_common::{config::MultiaddrWithPeerId, service::NetworkBlock};
 use sc_rpc_server::WsConfig;
 use sc_utils::mpsc::TracingUnboundedReceiver;
 use sp_blockchain::HeaderMetadata;
@@ -230,8 +230,15 @@ async fn build_network_future<
 						}
 					}
 					sc_rpc::system::Request::NetworkAddReservedPeer(peer_addr, sender) => {
-						let x = network.add_reserved_peer(peer_addr)
-							.map_err(sc_rpc::system::error::Error::MalformattedPeerArg);
+						let result = match MultiaddrWithPeerId::try_from(peer_addr) {
+							Ok(peer) => {
+								network.add_reserved_peer(peer)
+							},
+							Err(err) => {
+								Err(err.to_string())
+							},
+						};
+						let x = result.map_err(sc_rpc::system::error::Error::MalformattedPeerArg);
 						let _ = sender.send(x);
 					}
 					sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender) => {

substrate/client/service/test/src/lib.rs

@@ -24,9 +24,12 @@ use parking_lot::Mutex;
 use sc_client_api::{Backend, CallExecutor};
 use sc_network::{
 	config::{NetworkConfiguration, TransportConfig},
-	multiaddr, Multiaddr,
+	multiaddr,
+};
+use sc_network_common::{
+	config::MultiaddrWithPeerId,
+	service::{NetworkBlock, NetworkPeers, NetworkStateInfo},
 };
-use sc_network_common::service::{NetworkBlock, NetworkPeers, NetworkStateInfo};
 use sc_service::{
 	client::Client,
 	config::{BasePath, DatabaseSource, KeystoreConfig},
@@ -49,8 +52,8 @@ const MAX_WAIT_TIME: Duration = Duration::from_secs(60 * 3);
 
 struct TestNet<G, E, F, U> {
 	runtime: Runtime,
-	authority_nodes: Vec<(usize, F, U, Multiaddr)>,
-	full_nodes: Vec<(usize, F, U, Multiaddr)>,
+	authority_nodes: Vec<(usize, F, U, MultiaddrWithPeerId)>,
+	full_nodes: Vec<(usize, F, U, MultiaddrWithPeerId)>,
 	chain_spec: GenericChainSpec<G, E>,
 	base_port: u16,
 	nodes: usize,
@@ -320,7 +323,7 @@ where
 
 			handle.spawn(service.clone().map_err(|_| ()));
 			let addr =
-				addr.with(multiaddr::Protocol::P2p((service.network().local_peer_id()).into()));
+				MultiaddrWithPeerId { multiaddr: addr, peer_id: service.network().local_peer_id() };
 			self.authority_nodes.push((self.nodes, service, user_data, addr));
 			self.nodes += 1;
 		}
@@ -340,7 +343,7 @@ where
 
 			handle.spawn(service.clone().map_err(|_| ()));
 			let addr =
-				addr.with(multiaddr::Protocol::P2p((service.network().local_peer_id()).into()));
+				MultiaddrWithPeerId { multiaddr: addr, peer_id: service.network().local_peer_id() };
 			self.full_nodes.push((self.nodes, service, user_data, addr));
 			self.nodes += 1;
 		}
@@ -382,7 +385,7 @@ where
 			for (_, service, _, _) in network.full_nodes.iter().skip(1) {
 				service
 					.network()
-					.add_reserved_peer(first_address.to_string())
+					.add_reserved_peer(first_address.clone())
 					.expect("Error adding reserved peer");
 			}
 
@@ -414,7 +417,7 @@ where
 					if let Some((_, service, _, node_id)) = network.full_nodes.get(i) {
 						service
 							.network()
-							.add_reserved_peer(address.to_string())
+							.add_reserved_peer(address)
 							.expect("Error adding reserved peer");
 						address = node_id.clone();
 					}
@@ -479,7 +482,7 @@ pub fn sync<G, E, Fb, F, B, ExF, U>(
 	for (_, service, _, _) in network.full_nodes.iter().skip(1) {
 		service
 			.network()
-			.add_reserved_peer(first_address.to_string())
+			.add_reserved_peer(first_address.clone())
 			.expect("Error adding reserved peer");
 	}
 
@@ -532,13 +535,13 @@ pub fn consensus<G, E, Fb, F>(
 	for (_, service, _, _) in network.full_nodes.iter() {
 		service
 			.network()
-			.add_reserved_peer(first_address.to_string())
+			.add_reserved_peer(first_address.clone())
 			.expect("Error adding reserved peer");
 	}
 	for (_, service, _, _) in network.authority_nodes.iter().skip(1) {
 		service
 			.network()
-			.add_reserved_peer(first_address.to_string())
+			.add_reserved_peer(first_address.clone())
 			.expect("Error adding reserved peer");
 	}
 	network.run_until_all_full(|_index, service| {
@@ -556,7 +559,7 @@ pub fn consensus<G, E, Fb, F>(
 	for (_, service, _, _) in network.full_nodes.iter() {
 		service
 			.network()
-			.add_reserved_peer(first_address.to_string())
+			.add_reserved_peer(first_address.clone())
 			.expect("Error adding reserved peer");
 	}