From c5b6ad146c7834eeebcdf72e228f10cbf2a1312a Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 1 Oct 2020 15:38:00 +0200 Subject: [PATCH 1/6] sc-network: expose `add_to_priority_group` and `remove_from_priority_group` in `NetworkService` --- client/network/src/service.rs | 73 +++++++++++++++++++++++++++-------- 1 file changed, 56 insertions(+), 17 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 5b675186e9065..7dc0172d17c29 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -973,23 +973,7 @@ impl NetworkService { /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). pub fn set_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { - let peers = peers.into_iter() - .map(|mut addr| { - let peer = match addr.pop() { - Some(multiaddr::Protocol::P2p(key)) => PeerId::from_multihash(key) - .map_err(|_| "Invalid PeerId format".to_string())?, - _ => return Err("Missing PeerId from address".to_string()), - }; - - // Make sure the local peer ID is never added to the PSM - // or added as a "known address", even if given. - if peer == self.local_peer_id { - Err("Local peer ID in priority group.".to_string()) - } else { - Ok((peer, addr)) - } - }) - .collect::, String>>()?; + let peers = self.parse_multiaddr(peers)?; let peer_ids = peers.iter().map(|(peer_id, _addr)| peer_id.clone()).collect(); @@ -1004,6 +988,40 @@ impl NetworkService { Ok(()) } + /// Add peers to a peerset priority group. + /// + /// Each `Multiaddr` must end with a `/p2p/` component containing the `PeerId`. + /// + /// Returns an `Err` if one of the given addresses is invalid or contains an + /// invalid peer ID (which includes the local peer ID). + pub fn add_to_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { + let peers = self.parse_multiaddr(peers)?; + + for (peer_id, addr) in peers.into_iter() { + self.peerset.add_to_priority_group(group_id.clone(), peer_id.clone()); + + let _ = self + .to_worker + .unbounded_send(ServiceToWorkerMsg::AddKnownAddress(peer_id, addr)); + } + + Ok(()) + } + + /// Remove peers to a peerset priority group. + /// + /// Each `Multiaddr` must end with a `/p2p/` component containing the `PeerId`. + /// + /// Returns an `Err` if one of the given addresses is invalid or contains an + /// invalid peer ID (which includes the local peer ID). + pub fn remove_from_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { + let peers = self.parse_multiaddr(peers)?; + for (peer_id, _) in peers.into_iter() { + self.peerset.remove_from_priority_group(group_id.clone(), peer_id); + } + Ok(()) + } + /// Returns the number of peers we're connected to. pub fn num_connected(&self) -> usize { self.num_connected.load(Ordering::Relaxed) @@ -1025,6 +1043,27 @@ impl NetworkService { .to_worker .unbounded_send(ServiceToWorkerMsg::OwnBlockImported(hash, number)); } + + // Utility function to extract `PeerId` from each `Multiaddr` for priority group updates. + fn parse_multiaddr(&self, peers: HashSet) -> Result, String> { + peers.into_iter() + .map(|mut addr| { + let peer = match addr.pop() { + Some(multiaddr::Protocol::P2p(key)) => PeerId::from_multihash(key) + .map_err(|_| "Invalid PeerId format".to_string())?, + _ => return Err("Missing PeerId from address".to_string()), + }; + + // Make sure the local peer ID is never added to the PSM + // or added as a "known address", even if given. + if peer == self.local_peer_id { + Err("Local peer ID in priority group.".to_string()) + } else { + Ok((peer, addr)) + } + }) + .collect::, String>>() + } } impl sp_consensus::SyncOracle From f9993566050dfc56643c20f2108483d41abff85f Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 1 Oct 2020 15:44:07 +0200 Subject: [PATCH 2/6] sc-network: fix a typo --- client/network/src/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 7dc0172d17c29..e6b86efeaf2f5 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1008,7 +1008,7 @@ impl NetworkService { Ok(()) } - /// Remove peers to a peerset priority group. + /// Remove peers from a peerset priority group. /// /// Each `Multiaddr` must end with a `/p2p/` component containing the `PeerId`. /// From c751e29ce6d9d0092f02e6166c969b4365bfc2f6 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 9 Oct 2020 12:06:46 +0200 Subject: [PATCH 3/6] Update client/network/src/service.rs Co-authored-by: Max Inden --- client/network/src/service.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index e6b86efeaf2f5..61b6567afb451 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1044,7 +1044,10 @@ impl NetworkService { .unbounded_send(ServiceToWorkerMsg::OwnBlockImported(hash, number)); } - // Utility function to extract `PeerId` from each `Multiaddr` for priority group updates. + /// Utility function to extract `PeerId` from each `Multiaddr` for priority group updates. + /// + /// Returns an `Err` if one of the given addresses is invalid or contains an + /// invalid peer ID (which includes the local peer ID). fn parse_multiaddr(&self, peers: HashSet) -> Result, String> { peers.into_iter() .map(|mut addr| { From 3ea90f2efd43d06d8251944c04c6dbee6dc38f4d Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 9 Oct 2020 12:07:25 +0200 Subject: [PATCH 4/6] s/parse_multiaddr/split_multiaddr_and_peer_id/g --- client/network/src/service.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index e6b86efeaf2f5..606a11f2426cc 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -973,7 +973,7 @@ impl NetworkService { /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). pub fn set_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { - let peers = self.parse_multiaddr(peers)?; + let peers = self.split_multiaddr_and_peer_id(peers)?; let peer_ids = peers.iter().map(|(peer_id, _addr)| peer_id.clone()).collect(); @@ -995,7 +995,7 @@ impl NetworkService { /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). pub fn add_to_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { - let peers = self.parse_multiaddr(peers)?; + let peers = self.split_multiaddr_and_peer_id(peers)?; for (peer_id, addr) in peers.into_iter() { self.peerset.add_to_priority_group(group_id.clone(), peer_id.clone()); @@ -1015,7 +1015,7 @@ impl NetworkService { /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). pub fn remove_from_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { - let peers = self.parse_multiaddr(peers)?; + let peers = self.split_multiaddr_and_peer_id(peers)?; for (peer_id, _) in peers.into_iter() { self.peerset.remove_from_priority_group(group_id.clone(), peer_id); } @@ -1045,7 +1045,7 @@ impl NetworkService { } // Utility function to extract `PeerId` from each `Multiaddr` for priority group updates. - fn parse_multiaddr(&self, peers: HashSet) -> Result, String> { + fn split_multiaddr_and_peer_id(&self, peers: HashSet) -> Result, String> { peers.into_iter() .map(|mut addr| { let peer = match addr.pop() { From b1c28b26df241468d6923a41f6f66ef7ec0b9973 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 9 Oct 2020 12:24:20 +0200 Subject: [PATCH 5/6] sc-network: mark new functions as async and add comments --- client/network/src/service.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index ef9eb3c64d804..5322e85de6478 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -994,7 +994,9 @@ impl NetworkService { /// /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). - pub fn add_to_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { + // NOTE: even though this function is currently sync, it's marked as async for + // future-proofing, see https://github.com/paritytech/substrate/pull/7247#discussion_r502263451. + pub async fn add_to_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { let peers = self.split_multiaddr_and_peer_id(peers)?; for (peer_id, addr) in peers.into_iter() { @@ -1014,7 +1016,10 @@ impl NetworkService { /// /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). - pub fn remove_from_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { + // NOTE: even though this function is currently sync, it's marked as async for + // future-proofing, see https://github.com/paritytech/substrate/pull/7247#discussion_r502263451. + // NOTE: technically, this function only needs `Vec`, but we use `Multiaddr` here for convenience. + pub async fn remove_from_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { let peers = self.split_multiaddr_and_peer_id(peers)?; for (peer_id, _) in peers.into_iter() { self.peerset.remove_from_priority_group(group_id.clone(), peer_id); From 3babad9a40b1af5037f037585202a87181ac7ff7 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Fri, 9 Oct 2020 15:41:43 +0200 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Max Inden --- client/network/src/service.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 5322e85de6478..af123e94fac4b 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -994,6 +994,7 @@ impl NetworkService { /// /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). + // // NOTE: even though this function is currently sync, it's marked as async for // future-proofing, see https://github.com/paritytech/substrate/pull/7247#discussion_r502263451. pub async fn add_to_priority_group(&self, group_id: String, peers: HashSet) -> Result<(), String> { @@ -1016,6 +1017,7 @@ impl NetworkService { /// /// Returns an `Err` if one of the given addresses is invalid or contains an /// invalid peer ID (which includes the local peer ID). + // // NOTE: even though this function is currently sync, it's marked as async for // future-proofing, see https://github.com/paritytech/substrate/pull/7247#discussion_r502263451. // NOTE: technically, this function only needs `Vec`, but we use `Multiaddr` here for convenience.