client: only report block import to telemetry if new best (#3548)

* client: only report block import to telemetry if new best

* grandpa: fix tests

* consensus: derive Default for ImportedAux

* network: fix test

substrate/core/client/src/client.rs

@@ -879,11 +879,15 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 			fork_choice,
 		);
 
-		telemetry!(SUBSTRATE_INFO; "block.import";
-			"height" => height,
-			"best" => ?hash,
-			"origin" => ?origin
-		);
+		if let Ok(ImportResult::Imported(ref aux)) = result {
+			if aux.is_new_best {
+				telemetry!(SUBSTRATE_INFO; "block.import";
+					"height" => height,
+					"best" => ?hash,
+					"origin" => ?origin
+				);
+			}
+		}
 
 		result
 	}
@@ -985,7 +989,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 			operation.notify_imported = Some((hash, origin, import_headers.into_post(), is_new_best, storage_changes));
 		}
 
-		Ok(ImportResult::imported())
+		Ok(ImportResult::imported(is_new_best))
 	}
 
 	fn block_execution(
@@ -1500,7 +1504,7 @@ impl<'a, B, E, Block, RA> consensus::BlockImport<Block> for &'a Client<B, E, Blo
 			BlockStatus::KnownBad => return Ok(ImportResult::KnownBad),
 		}
 
-		Ok(ImportResult::imported())
+		Ok(ImportResult::imported(false))
 	}
 }
 
@@ -1528,7 +1532,7 @@ impl<B, E, Block, RA> consensus::BlockImport<Block> for Client<B, E, Block, RA>
 	}
 }
 
-impl<B, E, Block, RA> Finalizer<Block, Blake2Hasher, B> for Client<B, E, Block, RA> where 
+impl<B, E, Block, RA> Finalizer<Block, Blake2Hasher, B> for Client<B, E, Block, RA> where
 	B: backend::Backend<Block, Blake2Hasher>,
 	E: CallExecutor<Block, Blake2Hasher>,
 	Block: BlockT<Hash=H256>,
@@ -1552,7 +1556,7 @@ impl<B, E, Block, RA> Finalizer<Block, Blake2Hasher, B> for Client<B, E, Block,
 	}
 }
 
-impl<B, E, Block, RA> Finalizer<Block, Blake2Hasher, B> for &Client<B, E, Block, RA> where 
+impl<B, E, Block, RA> Finalizer<Block, Blake2Hasher, B> for &Client<B, E, Block, RA> where
 	B: backend::Backend<Block, Blake2Hasher>,
 	E: CallExecutor<Block, Blake2Hasher>,
 	Block: BlockT<Hash=H256>,
@@ -1833,7 +1837,7 @@ impl<B, E, Block, RA> backend::AuxStore for &Client<B, E, Block, RA>
 		B: backend::Backend<Block, Blake2Hasher>,
 		E: CallExecutor<Block, Blake2Hasher>,
 		Block: BlockT<Hash=H256>,
-{ 
+{
 
 	fn insert_aux<
 		'a,

substrate/core/consensus/common/src/block_import.rs

@@ -39,7 +39,7 @@ pub enum ImportResult {
 }
 
 /// Auxiliary data associated with an imported block result.
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Debug, Default, PartialEq, Eq)]
 pub struct ImportedAux {
 	/// Clear all pending justification requests.
 	pub clear_justification_requests: bool,
@@ -49,24 +49,19 @@ pub struct ImportedAux {
 	pub bad_justification: bool,
 	/// Request a finality proof for the given block.
 	pub needs_finality_proof: bool,
-}
-
-impl Default for ImportedAux {
-	fn default() -> ImportedAux {
-		ImportedAux {
-			clear_justification_requests: false,
-			needs_justification: false,
-			bad_justification: false,
-			needs_finality_proof: false,
-		}
-	}
+	/// Whether the block that was imported is the new best block.
+	pub is_new_best: bool,
 }
 
 impl ImportResult {
-	/// Returns default value for `ImportResult::Imported` with both
-	/// `clear_justification_requests` and `needs_justification` set to false.
-	pub fn imported() -> ImportResult {
-		ImportResult::Imported(ImportedAux::default())
+	/// Returns default value for `ImportResult::Imported` with
+	/// `clear_justification_requests`, `needs_justification`,
+	/// `bad_justification` and `needs_finality_proof` set to false.
+	pub fn imported(is_new_best: bool) -> ImportResult {
+		let mut aux = ImportedAux::default();
+		aux.is_new_best = is_new_best;
+
+		ImportResult::Imported(aux)
 	}
 }
 

substrate/core/finality-grandpa/src/light_import.rs

@@ -467,7 +467,8 @@ fn do_finalize_block<B, C, Block: BlockT<Hash=H256>>(
 	// update last finalized block reference
 	data.last_finalized = hash;
 
-	Ok(ImportResult::imported())
+	// we just finalized this block, so if we were importing it, it is now the new best
+	Ok(ImportResult::imported(true))
 }
 
 /// Load light import aux data from the store.
@@ -679,6 +680,7 @@ pub mod tests {
 			needs_justification: false,
 			bad_justification: false,
 			needs_finality_proof: false,
+			is_new_best: true,
 		}));
 	}
 
@@ -690,6 +692,7 @@ pub mod tests {
 			needs_justification: false,
 			bad_justification: false,
 			needs_finality_proof: false,
+			is_new_best: true,
 		}));
 	}
 
@@ -702,6 +705,7 @@ pub mod tests {
 			needs_justification: false,
 			bad_justification: false,
 			needs_finality_proof: true,
+			is_new_best: true,
 		}));
 	}
 
@@ -717,6 +721,7 @@ pub mod tests {
 				needs_justification: false,
 				bad_justification: false,
 				needs_finality_proof: true,
+				is_new_best: false,
 			},
 		));
 	}

substrate/core/finality-grandpa/src/tests.rs

@@ -1006,6 +1006,7 @@ fn allows_reimporting_change_blocks() {
 			clear_justification_requests: false,
 			bad_justification: false,
 			needs_finality_proof: false,
+			is_new_best: true,
 		}),
 	);
 
@@ -1054,6 +1055,7 @@ fn test_bad_justification() {
 			needs_justification: true,
 			clear_justification_requests: false,
 			bad_justification: true,
+			is_new_best: true,
 			..Default::default()
 		}),
 	);

substrate/core/network/src/test/block_import.rs

@@ -16,8 +16,9 @@
 
 //! Testing block import logic.
 
+use consensus::ImportedAux;
 use consensus::import_queue::{
-	import_single_block, IncomingBlock, BasicQueue, BlockImportError, BlockImportResult
+	import_single_block, BasicQueue, BlockImportError, BlockImportResult, IncomingBlock,
 };
 use test_client::{self, prelude::*};
 use test_client::runtime::{Block, Hash};
@@ -45,9 +46,13 @@ fn prepare_good_block() -> (TestClient, Hash, u64, PeerId, IncomingBlock<Block>)
 #[test]
 fn import_single_good_block_works() {
 	let (_, _hash, number, peer_id, block) = prepare_good_block();
+
+	let mut expected_aux = ImportedAux::default();
+	expected_aux.is_new_best = true;
+
 	match import_single_block(&mut test_client::new(), BlockOrigin::File, block, &mut PassThroughVerifier(true)) {
 		Ok(BlockImportResult::ImportedUnknown(ref num, ref aux, ref org))
-			if *num == number && *aux == Default::default() && *org == Some(peer_id) => {}
+			if *num == number && *aux == expected_aux && *org == Some(peer_id) => {}
 		_ => panic!()
 	}
 }