RPCs for reserved peers (#4423)

* RPC forwarders for adding reserved peers

* implement service side of reserved-peer RPCs

* Clean up unnecessary format! invocation

Co-Authored-By: Niklas Adolfsson <niklasadolfsson1@gmail.com>

* add some tests for the new RPC methods

* remove redundant `data` field

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

substrate/client/network/src/service.rs

@@ -408,6 +408,17 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> NetworkWorker
 			.map(|(id, info)| (id.clone(), info.clone()))
 			.collect()
 	}
+
+	/// Removes a `PeerId` from the list of reserved peers.
+	pub fn remove_reserved_peer(&self, peer: PeerId) {
+		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> {
+		self.service.add_reserved_peer(peer)
+	}
 }
 
 impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> NetworkService<B, S, H> {
@@ -553,7 +564,8 @@ impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> NetworkServic
 		self.peerset.remove_reserved_peer(peer);
 	}
 
-	/// Adds a `PeerId` and its address as reserved.
+	/// 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> {
 		let (peer_id, addr) = parse_str_addr(&peer).map_err(|e| format!("{:?}", e))?;
 		self.peerset.add_reserved_peer(peer_id.clone());

substrate/client/rpc-api/src/system/error.rs

@@ -28,6 +28,8 @@ pub enum Error {
 	/// Provided block range couldn't be resolved to a list of blocks.
 	#[display(fmt = "Node is not fully functional: {}", _0)]
 	NotHealthy(Health),
+	/// Peer argument is malformatted.
+	MalformattedPeerArg(String),
 }
 
 impl std::error::Error for Error {}
@@ -43,6 +45,11 @@ impl From<Error> for rpc::Error {
 				message: format!("{}", e),
 				data: serde_json::to_value(h).ok(),
 			},
+			Error::MalformattedPeerArg(ref e) => rpc::Error {
+				code :rpc::ErrorCode::ServerError(BASE_ERROR + 2),
+				message: e.clone(),
+				data: None,
+			}
 		}
 	}
 }

substrate/client/rpc-api/src/system/mod.rs

@@ -21,8 +21,10 @@ pub mod helpers;
 
 use crate::helpers::Receiver;
 use jsonrpc_derive::rpc;
+use futures::{future::BoxFuture, compat::Compat};
+use std::pin::Pin;
 
-use self::error::Result;
+use self::error::{Error, Result};
 
 pub use self::helpers::{Properties, SystemInfo, Health, PeerInfo, NodeRole};
 pub use self::gen_client::Client as SystemClient;
@@ -65,6 +67,21 @@ pub trait SystemApi<Hash, Number> {
 	#[rpc(name = "system_networkState", returns = "jsonrpc_core::Value")]
 	fn system_network_state(&self) -> Receiver<jsonrpc_core::Value>;
 
+	/// Adds a reserved peer. Returns the empty string or an error. The string
+	/// parameter should encode a `p2p` multiaddr.
+	///
+	/// `/ip4/198.51.100.19/tcp/30333/p2p/QmSk5HQbn6LhUwDiNMseVUjuRYhEtYj4aUZ6WfWoGURpdV`
+	/// is an example of a valid, passing multiaddr with PeerId attached.
+	#[rpc(name = "system_addReservedPeer", returns = "()")]
+	fn system_add_reserved_peer(&self, peer: String)
+		-> Compat<BoxFuture<'static, std::result::Result<(), jsonrpc_core::Error>>>;
+
+	/// Remove a reserved peer. Returns the empty string or an error. The string
+	/// should encode only the PeerId e.g. `QmSk5HQbn6LhUwDiNMseVUjuRYhEtYj4aUZ6WfWoGURpdV`.
+	#[rpc(name = "system_removeReservedPeer", returns = "()")]
+	fn system_remove_reserved_peer(&self, peer_id: String)
+		-> Compat<BoxFuture<'static, std::result::Result<(), jsonrpc_core::Error>>>;
+
 	/// Returns the roles the node is running as.
 	#[rpc(name = "system_nodeRoles", returns = "Vec<NodeRole>")]
 	fn system_node_roles(&self) -> Receiver<Vec<NodeRole>>;

substrate/client/rpc/src/system/mod.rs

@@ -19,9 +19,11 @@
 #[cfg(test)]
 mod tests;
 
+use futures::{future::BoxFuture, FutureExt, TryFutureExt};
 use futures::{channel::{mpsc, oneshot}, compat::Compat};
 use sc_rpc_api::Receiver;
 use sp_runtime::traits::{self, Header as HeaderT};
+
 use self::error::Result;
 
 pub use sc_rpc_api::system::*;
@@ -42,6 +44,10 @@ pub enum Request<B: traits::Block> {
 	Peers(oneshot::Sender<Vec<PeerInfo<B::Hash, <B::Header as HeaderT>::Number>>>),
 	/// Must return the state of the network.
 	NetworkState(oneshot::Sender<rpc::Value>),
+	/// Must return any potential parse error.
+	NetworkAddReservedPeer(String, oneshot::Sender<Result<()>>),
+	/// Must return any potential parse error.
+	NetworkRemoveReservedPeer(String, oneshot::Sender<Result<()>>),
 	/// Must return the node role.
 	NodeRoles(oneshot::Sender<Vec<NodeRole>>)
 }
@@ -53,7 +59,7 @@ impl<B: traits::Block> System<B> {
 	/// reading from that channel and answering the requests.
 	pub fn new(
 		info: SystemInfo,
-		send_back: mpsc::UnboundedSender<Request<B>>
+		send_back: mpsc::UnboundedSender<Request<B>>,
 	) -> Self {
 		System {
 			info,
@@ -97,6 +103,34 @@ impl<B: traits::Block> SystemApi<B::Hash, <B::Header as HeaderT>::Number> for Sy
 		Receiver(Compat::new(rx))
 	}
 
+	fn system_add_reserved_peer(&self, peer: String)
+		-> Compat<BoxFuture<'static, std::result::Result<(), rpc::Error>>>
+	{
+		let (tx, rx) = oneshot::channel();
+		let _ = self.send_back.unbounded_send(Request::NetworkAddReservedPeer(peer, tx));
+		async move {
+			match rx.await {
+				Ok(Ok(())) => Ok(()),
+				Ok(Err(e)) => Err(rpc::Error::from(e)),
+				Err(_) => Err(rpc::Error::internal_error()),
+			}
+		}.boxed().compat()
+	}
+
+	fn system_remove_reserved_peer(&self, peer: String)
+		-> Compat<BoxFuture<'static, std::result::Result<(), rpc::Error>>>
+	{
+		let (tx, rx) = oneshot::channel();
+		let _ = self.send_back.unbounded_send(Request::NetworkRemoveReservedPeer(peer, tx));
+		async move {
+			match rx.await {
+				Ok(Ok(())) => Ok(()),
+				Ok(Err(e)) => Err(rpc::Error::from(e)),
+				Err(_) => Err(rpc::Error::internal_error()),
+			}
+		}.boxed().compat()
+	}
+
 	fn system_node_roles(&self) -> Receiver<Vec<NodeRole>> {
 		let (tx, rx) = oneshot::channel();
 		let _ = self.send_back.unbounded_send(Request::NodeRoles(tx));

substrate/client/rpc/src/system/tests.rs

@@ -80,6 +80,18 @@ fn api<T: Into<Option<Status>>>(sync: T) -> System<Block> {
 						peerset: serde_json::Value::Null,
 					}).unwrap());
 				},
+				Request::NetworkAddReservedPeer(peer, sender) => {
+					let _ = match sc_network::config::parse_str_addr(&peer) {
+						Ok(_) => sender.send(Ok(())),
+						Err(s) => sender.send(Err(error::Error::MalformattedPeerArg(s.to_string()))),
+					};
+				},
+				Request::NetworkRemoveReservedPeer(peer, sender) => {
+					let _ = match peer.parse::<PeerId>() {
+						Ok(_) => sender.send(Ok(())),
+						Err(s) => sender.send(Err(error::Error::MalformattedPeerArg(s.to_string()))),
+					};
+				}
 				Request::NodeRoles(sender) => {
 					let _ = sender.send(vec![NodeRole::Authority]);
 				}
@@ -232,3 +244,27 @@ fn system_node_roles() {
 		vec![NodeRole::Authority]
 	);
 }
+
+#[test]
+fn system_network_add_reserved() {
+	let good_peer_id = "/ip4/198.51.100.19/tcp/30333/p2p/QmSk5HQbn6LhUwDiNMseVUjuRYhEtYj4aUZ6WfWoGURpdV";
+	let bad_peer_id = "/ip4/198.51.100.19/tcp/30333";
+	let mut runtime = tokio::runtime::current_thread::Runtime::new().unwrap();
+
+	let good_fut = api(None).system_add_reserved_peer(good_peer_id.into());
+	let bad_fut = api(None).system_add_reserved_peer(bad_peer_id.into());
+	assert_eq!(runtime.block_on(good_fut), Ok(()));
+	assert!(runtime.block_on(bad_fut).is_err());
+}
+
+#[test]
+fn system_network_remove_reserved() {
+	let good_peer_id = "QmSk5HQbn6LhUwDiNMseVUjuRYhEtYj4aUZ6WfWoGURpdV";
+	let bad_peer_id = "/ip4/198.51.100.19/tcp/30333/p2p/QmSk5HQbn6LhUwDiNMseVUjuRYhEtYj4aUZ6WfWoGURpdV";
+	let mut runtime = tokio::runtime::current_thread::Runtime::new().unwrap();
+
+	let good_fut = api(None).system_remove_reserved_peer(good_peer_id.into());
+	let bad_fut = api(None).system_remove_reserved_peer(bad_peer_id.into());
+	assert_eq!(runtime.block_on(good_fut), Ok(()));
+	assert!(runtime.block_on(bad_fut).is_err());
+}

substrate/client/service/src/lib.rs

@@ -429,6 +429,22 @@ fn build_network_future<
 						let _ = sender.send(network_state);
 					}
 				}
+				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 _ = sender.send(x);
+				}
+				sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender) => {
+					let _ = match peer_id.parse::<PeerId>() {
+						Ok(peer_id) => {
+							network.remove_reserved_peer(peer_id);
+							sender.send(Ok(()))
+						}
+						Err(e) => sender.send(Err(sc_rpc::system::error::Error::MalformattedPeerArg(
+							e.to_string(),
+						))),
+					};
+				}
 				sc_rpc::system::Request::NodeRoles(sender) => {
 					use sc_rpc::system::NodeRole;