From d427792b6b141e7dc23a75961a105131c9fa3757 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <git@kchr.de>
Date: Thu, 27 Apr 2023 11:28:03 +0200
Subject: [PATCH] sc-network-sync: Improve error reporting (#14025)

Before this wasn't printing a warning for `VerificationFailed` when there wasn't a peer assigned to
the error message. However, if there happens an error on importing the state there wouldn't be any
error. This pr improves the situation to also print an error in this case. Besides that there are
some other cleanups.
---
 substrate/client/network/sync/src/lib.rs | 43 +++++++++++-------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/substrate/client/network/sync/src/lib.rs b/substrate/client/network/sync/src/lib.rs
index 29c4bc840b..3f1cbebd57 100644
--- a/substrate/client/network/sync/src/lib.rs
+++ b/substrate/client/network/sync/src/lib.rs
@@ -2708,9 +2708,7 @@ where
 				break
 			}
 
-			if result.is_err() {
-				has_error = true;
-			}
+			has_error |= result.is_err();
 
 			match result {
 				Ok(BlockImportStatus::ImportedKnown(number, who)) =>
@@ -2721,9 +2719,7 @@ where
 					if aux.clear_justification_requests {
 						trace!(
 							target: "sync",
-							"Block imported clears all pending justification requests {}: {:?}",
-							number,
-							hash,
+							"Block imported clears all pending justification requests {number}: {hash:?}",
 						);
 						self.clear_justification_requests();
 					}
@@ -2731,9 +2727,7 @@ where
 					if aux.needs_justification {
 						trace!(
 							target: "sync",
-							"Block imported but requires justification {}: {:?}",
-							number,
-							hash,
+							"Block imported but requires justification {number}: {hash:?}",
 						);
 						self.request_justification(&hash, number);
 					}
@@ -2793,25 +2787,26 @@ where
 						output.push(Err(BadPeer(peer, rep::INCOMPLETE_HEADER)));
 						output.extend(self.restart());
 					},
-				Err(BlockImportError::VerificationFailed(who, e)) =>
+				Err(BlockImportError::VerificationFailed(who, e)) => {
+					let extra_message =
+						who.map_or_else(|| "".into(), |peer| format!(" received from ({peer})"));
+
+					warn!(
+						target: "sync",
+						"💔 Verification failed for block {hash:?}{extra_message}: {e:?}",
+					);
+
 					if let Some(peer) = who {
-						warn!(
-							target: "sync",
-							"💔 Verification failed for block {:?} received from peer: {}, {:?}",
-							hash,
-							peer,
-							e,
-						);
 						output.push(Err(BadPeer(peer, rep::VERIFICATION_FAIL)));
-						output.extend(self.restart());
-					},
+					}
+
+					output.extend(self.restart());
+				},
 				Err(BlockImportError::BadBlock(who)) =>
 					if let Some(peer) = who {
 						warn!(
 							target: "sync",
-							"💔 Block {:?} received from peer {} has been blacklisted",
-							hash,
-							peer,
+							"💔 Block {hash:?} received from peer {peer} has been blacklisted",
 						);
 						output.push(Err(BadPeer(peer, rep::BAD_BLOCK)));
 					},
@@ -2819,10 +2814,10 @@ where
 					// This may happen if the chain we were requesting upon has been discarded
 					// in the meantime because other chain has been finalized.
 					// Don't mark it as bad as it still may be synced if explicitly requested.
-					trace!(target: "sync", "Obsolete block {:?}", hash);
+					trace!(target: "sync", "Obsolete block {hash:?}");
 				},
 				e @ Err(BlockImportError::UnknownParent) | e @ Err(BlockImportError::Other(_)) => {
-					warn!(target: "sync", "💔 Error importing block {:?}: {}", hash, e.unwrap_err());
+					warn!(target: "sync", "💔 Error importing block {hash:?}: {}", e.unwrap_err());
 					self.state_sync = None;
 					self.warp_sync = None;
 					output.extend(self.restart());