Ensure any call to disable peer has a reason

polkadot/network/src/lib.rs

@@ -355,7 +355,7 @@ impl PolkadotProtocol {
 					};
 
 					if !info.validator {
-						ctx.disable_peer(peer_id);
+						ctx.disable_peer(peer_id, "Unknown Polkadot-protocol reason");
 						return;
 					}
 
@@ -395,7 +395,7 @@ impl PolkadotProtocol {
 				self.pending.push(req);
 				self.dispatch_pending_requests(ctx);
 			}
-			None => ctx.disable_peer(peer_id),
+			None => ctx.disable_peer(peer_id, "Unknown Polkadot-protocol reason"),
 		}
 	}
 }
@@ -415,7 +415,7 @@ impl Specialization<Block> for PolkadotProtocol {
 
 		if let Some((ref acc_id, ref para_id)) = local_status.collating_for {
 			if self.collator_peer_id(acc_id.clone()).is_some() {
-				ctx.disable_peer(peer_id);
+				ctx.disable_peer(peer_id, "Unknown Polkadot-protocol reason");
 				return
 			}
 
@@ -501,7 +501,7 @@ impl Specialization<Block> for PolkadotProtocol {
 					Ok(msg) => self.on_polkadot_message(ctx, peer_id, raw, msg),
 					Err(e) => {
 						trace!(target: "p_net", "Bad message from {}: {}", peer_id, e);
-						ctx.disable_peer(peer_id);
+						ctx.disable_peer(peer_id, "Unknown Polkadot-protocol reason");
 					}
 				}
 			}
@@ -546,13 +546,13 @@ impl PolkadotProtocol {
 		match self.peers.get(&from) {
 			None => ctx.disconnect_peer(from),
 			Some(peer_info) => match peer_info.status.collating_for {
-				None => ctx.disable_peer(from),
+				None => ctx.disable_peer(from, "Unknown Polkadot-protocol reason"),
 				Some((ref acc_id, ref para_id)) => {
 					let structurally_valid = para_id == &collation_para && acc_id == &collated_acc;
 					if structurally_valid && collation.receipt.check_signature().is_ok() {
 						self.collators.on_collation(acc_id.clone(), relay_parent, collation)
 					} else {
-						ctx.disable_peer(from)
+						ctx.disable_peer(from, "Unknown Polkadot-protocol reason")
 					};
 				}
 			},
@@ -583,7 +583,7 @@ impl PolkadotProtocol {
 	// disconnect a collator by account-id.
 	fn disconnect_bad_collator(&self, ctx: &mut Context<Block>, account_id: AccountId) {
 		if let Some(peer_id) = self.collator_peer_id(account_id) {
-			ctx.disable_peer(peer_id)
+			ctx.disable_peer(peer_id, "Unknown Polkadot-protocol reason")
 		}
 	}
 }

polkadot/network/src/tests.rs

@@ -41,7 +41,7 @@ impl Context<Block> for TestContext {
 		unimplemented!()
 	}
 
-	fn disable_peer(&mut self, peer: PeerId) {
+	fn disable_peer(&mut self, peer: PeerId, _reason: &str) {
 		self.disabled.push(peer);
 	}
 

substrate/network-libp2p/src/custom_proto.rs

@@ -124,7 +124,7 @@ where C: AsyncRead + AsyncWrite + 'static,		// TODO: 'static :-/
 		self,
 		socket: C,
 		protocol_version: Self::UpgradeIdentifier,
-		endpoint: Endpoint,
+		_endpoint: Endpoint,
 		remote_addr: Maf
 	) -> Self::Future {
 		let packet_count = self.supported_versions

substrate/network-libp2p/src/network_state.rs

@@ -236,6 +236,7 @@ impl NetworkState {
 
 	/// Reports the ping of the peer. Returned later by `session_info()`.
 	/// No-op if the `peer_id` is not valid/expired.
+	#[allow(dead_code)]
 	pub fn report_ping(&self, peer_id: PeerId, ping: Duration) {
 		let connections = self.connections.read();
 		let info = match connections.info_by_peer.get(&peer_id) {
@@ -610,10 +611,11 @@ impl NetworkState {
 	/// list of disabled peers, and  drops any existing connections if
 	/// necessary (ie. drops the sender that was passed to
 	/// `accept_custom_proto`).
-	pub fn disable_peer(&self, peer_id: PeerId) {
+	pub fn disable_peer(&self, peer_id: PeerId, reason: &str) {
 		// TODO: what do we do if the peer is reserved?
 		let mut connections = self.connections.write();
 		let peer_info = if let Some(peer_info) = connections.info_by_peer.remove(&peer_id) {
+			info!(target: "network", "Peer {} disabled. {}", peer_id, reason);
 			let old = connections.peer_by_nodeid.remove(&peer_info.id);
 			debug_assert_eq!(old, Some(peer_id));
 			peer_info
@@ -870,7 +872,7 @@ mod tests {
 			mpsc::unbounded().0
 		).unwrap();
 
-		state.disable_peer(peer_id);
+		state.disable_peer(peer_id, "Just a test");
 
 		assert!(state.accept_custom_proto(
 			example_peer.clone(),

substrate/network-libp2p/src/service.rs

@@ -328,9 +328,9 @@ impl NetworkContext for NetworkContextImpl {
 		}
 	}
 
-	fn disable_peer(&self, peer: PeerId) {
+	fn disable_peer(&self, peer: PeerId, reason: &str) {
 		debug!(target: "sub-libp2p", "Request to disable peer {}", peer);
-		self.inner.network_state.disable_peer(peer);
+		self.inner.network_state.disable_peer(peer, reason);
 	}
 
 	fn disconnect_peer(&self, peer: PeerId) {

substrate/network-libp2p/src/transport.rs

@@ -57,6 +57,7 @@ pub fn build_transport(
 /// Specifies whether unencrypted communications are allowed or denied.
 #[derive(Debug, Copy, Clone)]
 pub enum UnencryptedAllowed {
+	#[allow(dead_code)]
 	Allowed,
 	Denied,
 }

substrate/network/src/io.rs

@@ -20,7 +20,7 @@ use network::{NetworkContext, PeerId, Error as NetworkError, SessionInfo};
 /// Provides peer connection management and an interface to the blockchain client.
 pub trait SyncIo {
 	/// Disable a peer
-	fn disable_peer(&mut self, peer_id: PeerId);
+	fn disable_peer(&mut self, peer_id: PeerId, reason: &str);
 	/// Disconnect peer
 	fn disconnect_peer(&mut self, peer_id: PeerId);
 	/// Send a packet to a peer.
@@ -50,8 +50,8 @@ impl<'s> NetSyncIo<'s> {
 }
 
 impl<'s> SyncIo for NetSyncIo<'s> {
-	fn disable_peer(&mut self, peer_id: PeerId) {
-		self.network.disable_peer(peer_id);
+	fn disable_peer(&mut self, peer_id: PeerId, reason: &str) {
+		self.network.disable_peer(peer_id, reason);
 	}
 
 	fn disconnect_peer(&mut self, peer_id: PeerId) {

substrate/network/src/protocol.rs

@@ -110,7 +110,7 @@ pub trait Context<B: BlockT> {
 	fn client(&self) -> &::chain::Client<B>;
 
 	/// Disable a peer
-	fn disable_peer(&mut self, peer_id: PeerId);
+	fn disable_peer(&mut self, peer_id: PeerId, reason: &str);
 
 	/// Disconnect peer
 	fn disconnect_peer(&mut self, peer_id: PeerId);
@@ -141,8 +141,8 @@ impl<'a, B: BlockT + 'a> ProtocolContext<'a, B> {
 		send_message(&self.context_data.peers, self.io, peer_id, message)
 	}
 
-	pub fn disable_peer(&mut self, peer_id: PeerId) {
-		self.io.disable_peer(peer_id);
+	pub fn disable_peer(&mut self, peer_id: PeerId, reason: &str) {
+		self.io.disable_peer(peer_id, reason);
 	}
 
 	pub fn disconnect_peer(&mut self, peer_id: PeerId) {
@@ -167,8 +167,8 @@ impl<'a, B: BlockT + 'a> Context<B> for ProtocolContext<'a, B> {
 		ProtocolContext::send_message(self, peer_id, message);
 	}
 
-	fn disable_peer(&mut self, peer_id: PeerId) {
-		ProtocolContext::disable_peer(self, peer_id);
+	fn disable_peer(&mut self, peer_id: PeerId, reason: &str) {
+		ProtocolContext::disable_peer(self, peer_id, reason);
 	}
 
 	fn disconnect_peer(&mut self, peer_id: PeerId) {
@@ -237,9 +237,8 @@ impl<B: BlockT, S: Specialization<B>> Protocol<B, S> {
 		let message: Message<B> = match serde_json::from_slice(data) {
 			Ok(m) => m,
 			Err(e) => {
-				debug!(target: "sync", "Invalid packet from {}: {}", peer_id, e);
 				trace!(target: "sync", "Invalid packet: {}", String::from_utf8_lossy(data));
-				io.disable_peer(peer_id);
+				io.disable_peer(peer_id, &format!("Peer sent us a packet with invalid format ({})", e));
 				return;
 			}
 		};
@@ -255,14 +254,12 @@ impl<B: BlockT, S: Specialization<B>> Protocol<B, S> {
 						match mem::replace(&mut peer.block_request, None) {
 							Some(r) => r,
 							None => {
-								debug!(target: "sync", "Unexpected response packet from {}", peer_id);
-								io.disable_peer(peer_id);
+								io.disable_peer(peer_id, "Unexpected response packet received from peer");
 								return;
 							}
 						}
 					} else {
-						debug!(target: "sync", "Unexpected packet from {}", peer_id);
-						io.disable_peer(peer_id);
+						io.disable_peer(peer_id, "Unexpected packet received from peer");
 						return;
 					}
 				};
@@ -428,13 +425,11 @@ impl<B: BlockT, S: Specialization<B>> Protocol<B, S> {
 				return;
 			}
 			if status.genesis_hash != self.genesis_hash {
-				io.disable_peer(peer_id);
-				trace!(target: "sync", "Peer {} genesis hash mismatch (ours: {}, theirs: {})", peer_id, self.genesis_hash, status.genesis_hash);
+				io.disable_peer(peer_id, &format!("Peer is on different chain (our genesis: {} theirs: {})", self.genesis_hash, status.genesis_hash));
 				return;
 			}
 			if status.version != CURRENT_VERSION {
-				io.disable_peer(peer_id);
-				trace!(target: "sync", "Peer {} unsupported eth protocol ({})", peer_id, status.version);
+				io.disable_peer(peer_id, &format!("Peer using unsupported protocol version {}", status.version));
 				return;
 			}
 

substrate/network/src/sync.rs

@@ -118,12 +118,10 @@ impl<B: BlockT> ChainSync<B> {
 					protocol.disconnect_peer(peer_id);
 				},
 				(Ok(BlockStatus::KnownBad), _) => {
-					debug!(target:"sync", "New peer with known bad best block {} ({}).", info.best_hash, info.best_number);
-					protocol.disable_peer(peer_id);
+					protocol.disable_peer(peer_id, &format!("New peer with known bad best block {} ({}).", info.best_hash, info.best_number));
 				},
 				(Ok(BlockStatus::Unknown), b) if b == As::sa(0) => {
-					debug!(target:"sync", "New peer with unknown genesis hash {} ({}).", info.best_hash, info.best_number);
-					protocol.disable_peer(peer_id);
+					protocol.disable_peer(peer_id, &format!("New peer with unknown genesis hash {} ({}).", info.best_hash, info.best_number));
 				},
 				(Ok(BlockStatus::Unknown), _) => {
 					let our_best = self.best_queued_number;
@@ -206,7 +204,7 @@ impl<B: BlockT> ChainSync<B> {
 								},
 								Ok(_) => { // genesis mismatch
 									trace!(target:"sync", "Ancestry search: genesis mismatch for peer {}", peer_id);
-									protocol.disable_peer(peer_id);
+									protocol.disable_peer(peer_id, "Ancestry search: genesis mismatch for peer");
 									return;
 								},
 								Err(e) => {
@@ -278,8 +276,7 @@ impl<B: BlockT> ChainSync<B> {
 							return;
 						},
 						Ok(ImportResult::KnownBad) => {
-							debug!(target: "sync", "Bad block {}: {:?}", number, hash);
-							protocol.disable_peer(origin); //TODO: use persistent ID
+							protocol.disable_peer(origin, &format!("Peer gave us a bad block {}: {:?}", number, hash)); //TODO: use persistent ID
 							self.restart(protocol);
 							return;
 						}
@@ -291,13 +288,11 @@ impl<B: BlockT> ChainSync<B> {
 					}
 				},
 				(None, _) => {
-					debug!(target: "sync", "Header {} was not provided by {} ", block.hash, origin);
-					protocol.disable_peer(origin); //TODO: use persistent ID
+					protocol.disable_peer(origin, &format!("Header {} was not provided by peer ", block.hash)); //TODO: use persistent ID
 					return;
 				},
 				(_, None) => {
-					debug!(target: "sync", "Justification set for block {} was not provided by {} ", block.hash, origin);
-					protocol.disable_peer(origin); //TODO: use persistent ID
+					protocol.disable_peer(origin, &format!("Justification set for block {} was not provided by peer", block.hash)); //TODO: use persistent ID
 					return;
 				}
 			}

substrate/network/src/test/mod.rs

@@ -80,7 +80,7 @@ impl<'p> Drop for TestIo<'p> {
 }
 
 impl<'p> SyncIo for TestIo<'p> {
-	fn disable_peer(&mut self, peer_id: PeerId) {
+	fn disable_peer(&mut self, peer_id: PeerId, _reason: &str) {
 		self.disconnect_peer(peer_id);
 	}