Avoid self-lookups in Authority Discovery (#6317)

* Ensure authority discovery avoids self-lookups.

Thereby additionally guard the `NetworkService` against
adding the local peer to the PSM or registering a
"known address" for the local peer.

* Clarify comments.

* See if returning errors is ok.

substrate/client/authority-discovery/src/lib.rs

@@ -294,13 +294,26 @@ where
 			.authorities(&id)
 			.map_err(Error::CallingRuntime)?;
 
-		for authority_id in authorities.iter() {
+		let local_keys = match &self.role {
-			if let Some(metrics) = &self.metrics {
+			Role::Authority(key_store) => {
-				metrics.request.inc();
+				key_store.read()
-			}
+					.sr25519_public_keys(key_types::AUTHORITY_DISCOVERY)
+					.into_iter()
+					.collect::<HashSet<_>>()
+			},
+			Role::Sentry => HashSet::new(),
+		};
 
-			self.network
+		for authority_id in authorities.iter() {
-				.get_value(&hash_authority_id(authority_id.as_ref()));
+			// Make sure we don't look up our own keys.
+			if !local_keys.contains(authority_id.as_ref()) {
+				if let Some(metrics) = &self.metrics {
+					metrics.request.inc();
+				}
+
+				self.network
+					.get_value(&hash_authority_id(authority_id.as_ref()));
+			}
 		}
 
 		Ok(())

substrate/client/network/src/service.rs

@@ -672,8 +672,15 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkService<B, H> {
 
 	/// Adds a `PeerId` and its address as reserved. The string should encode the address
 	/// and peer ID of the remote node.
+	///
+	/// Returns an `Err` if the given string is not a valid multiaddress
+	/// or contains an invalid peer ID (which includes the local peer ID).
 	pub fn add_reserved_peer(&self, peer: String) -> Result<(), String> {
 		let (peer_id, addr) = parse_str_addr(&peer).map_err(|e| format!("{:?}", e))?;
+		// Make sure the local peer ID is never added to the PSM.
+		if peer_id == self.local_peer_id {
+			return Err("Local peer ID cannot be added as a reserved peer.".to_string())
+		}
 		self.peerset.add_reserved_peer(peer_id.clone());
 		let _ = self
 			.to_worker
@@ -694,12 +701,26 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkService<B, H> {
 	}
 
 	/// Modify a peerset priority group.
+	///
+	/// Returns an `Err` if one of the given addresses contains an invalid
+	/// peer ID (which includes the local peer ID).
 	pub fn set_priority_group(&self, group_id: String, peers: HashSet<Multiaddr>) -> Result<(), String> {
-		let peers = peers.into_iter().map(|p| {
+		let peers = peers.into_iter()
-			parse_addr(p).map_err(|e| format!("{:?}", e))
+			.map(|p| match parse_addr(p) {
-		}).collect::<Result<Vec<(PeerId, Multiaddr)>, String>>()?;
+				Err(e) => Err(format!("{:?}", e)),
+				Ok((peer, addr)) =>
+					// 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::<Result<Vec<(PeerId, Multiaddr)>, String>>()?;
 
 		let peer_ids = peers.iter().map(|(peer_id, _addr)| peer_id.clone()).collect();
+
 		self.peerset.set_priority_group(group_id, peer_ids);
 
 		for (peer_id, addr) in peers.into_iter() {