Peer is_known robustness (#3040)

* add some test coverage around peers map (peer_addr hashing impl)

* make is_known a bit more robust

* fix typos

p2p/src/peers.rs

@@ -97,15 +97,19 @@ impl Peers {
 		self.save_peer(&peer_data)
 	}
 
-	pub fn is_known(&self, addr: PeerAddr) -> bool {
+	/// Check if this peer address is already known (are we already connected to it)?
+	/// We try to get the read lock but if we experience contention
+	/// and this attempt fails then return an error allowing the caller
+	/// to decide how best to handle this.
+	pub fn is_known(&self, addr: PeerAddr) -> Result<bool, Error> {
 		let peers = match self.peers.try_read_for(LOCK_TIMEOUT) {
 			Some(peers) => peers,
 			None => {
 				error!("is_known: failed to get peers lock");
-				return false;
+				return Err(Error::Internal);
 			}
 		};
-		peers.contains_key(&addr)
+		Ok(peers.contains_key(&addr))
 	}
 
 	/// Get vec of peers we are currently connected to.

p2p/src/serv.rs

@@ -224,9 +224,21 @@ impl Server {
 				debug!("Peer {} banned, refusing connection.", peer_addr);
 				return true;
 			}
-			if self.peers.is_known(peer_addr) {
-				debug!("Peer {} already known, refusing connection.", peer_addr);
-				return true;
+			// The call to is_known() can fail due to contention on the peers map.
+			// If it fails we want to default to refusing the connection.
+			match self.peers.is_known(peer_addr) {
+				Ok(true) => {
+					debug!("Peer {} already known, refusing connection.", peer_addr);
+					return true;
+				}
+				Err(_) => {
+					error!(
+						"Peer {} is_known check failed, refusing connection.",
+						peer_addr
+					);
+					return true;
+				}
+				_ => (),
 			}
 		}
 		false

servers/src/grin/seed.rs

@@ -239,14 +239,14 @@ fn monitor_peers(
 		max_peer_attempts as usize,
 	);
 
-	for p in new_peers.iter().filter(|p| !peers.is_known(p.addr)) {
-		trace!(
-			"monitor_peers: on {}:{}, queue to soon try {}",
-			config.host,
-			config.port,
-			p.addr,
-		);
-		tx.send(p.addr).unwrap();
+	// Only queue up connection attempts for candidate peers where we
+	// are confident we do not yet know about this peer.
+	// The call to is_known() may fail due to contention on the peers map.
+	// Do not attempt any connection where is_known() fails for any reason.
+	for p in new_peers {
+		if let Ok(false) = peers.is_known(p.addr) {
+			tx.send(p.addr).unwrap();
+		}
 	}
 }