Import blocks with bad justification (#1977)

* init version

* chore: improve code

* fix test

* fix: log error resulting of bad justification

* fix: add test to check for disconnected peer

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

@@ -40,6 +40,8 @@ pub struct ImportedAux {
 	pub clear_justification_requests: bool,
 	/// Request a justification for the given block.
 	pub needs_justification: bool,
+	/// Received a bad justification.
+	pub bad_justification: bool,
 }
 
 impl Default for ImportedAux {
@@ -47,6 +49,7 @@ impl Default for ImportedAux {
 		ImportedAux {
 			clear_justification_requests: false,
 			needs_justification: false,
+			bad_justification: false,
 		}
 	}
 }

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

@@ -309,7 +309,7 @@ impl<B: BlockT> BlockImporter<B> {
 
 			match result {
 				Ok(BlockImportResult::ImportedKnown(number)) => link.block_imported(&hash, number),
-				Ok(BlockImportResult::ImportedUnknown(number, aux)) => {
+				Ok(BlockImportResult::ImportedUnknown(number, aux, who)) => {
 					link.block_imported(&hash, number);
 
 					if aux.clear_justification_requests {
@@ -321,6 +321,12 @@ impl<B: BlockT> BlockImporter<B> {
 						trace!(target: "sync", "Block imported but requires justification {}: {:?}", number, hash);
 						link.request_justification(&hash, number);
 					}
+
+					if aux.bad_justification {
+						if let Some(peer) = who {
+							link.useless_peer(peer, "Sent block with bad justification to import");
+						}
+					}
 				},
 				Err(BlockImportError::IncompleteHeader(who)) => {
 					if let Some(peer) = who {
@@ -428,12 +434,12 @@ impl<B: BlockT, V: 'static + Verifier<B>> BlockImportWorker<B, V> {
 			let import_result = if has_error {
 				Err(BlockImportError::Error)
 			} else {
-   				import_single_block(
-   					&*self.block_import,
-   					origin.clone(),
-   					block.clone(),
-   					self.verifier.clone(),
-   				)
+				import_single_block(
+					&*self.block_import,
+					origin.clone(),
+					block.clone(),
+					self.verifier.clone(),
+				)
 			};
 			let was_ok = import_result.is_ok();
 			results.push((import_result, block.hash));
@@ -479,7 +485,7 @@ pub enum BlockImportResult<N: ::std::fmt::Debug + PartialEq> {
 	/// Imported known block.
 	ImportedKnown(N),
 	/// Imported unknown block.
-	ImportedUnknown(N, ImportedAux),
+	ImportedUnknown(N, ImportedAux, Option<Origin>),
 }
 
 /// Block import error.
@@ -528,7 +534,7 @@ pub fn import_single_block<B: BlockT, V: Verifier<B>>(
 				trace!(target: "sync", "Block already in chain {}: {:?}", number, hash);
 				Ok(BlockImportResult::ImportedKnown(number))
 			},
-			Ok(ImportResult::Imported(aux)) => Ok(BlockImportResult::ImportedUnknown(number, aux)),
+			Ok(ImportResult::Imported(aux)) => Ok(BlockImportResult::ImportedUnknown(number, aux, peer)),
 			Ok(ImportResult::UnknownParent) => {
 				debug!(target: "sync", "Block with unknown parent {}: {:?}, parent: {:?}", number, hash, parent);
 				Err(BlockImportError::UnknownParent)
@@ -627,10 +633,18 @@ mod tests {
 		assert_eq!(link_port.recv(), Ok(LinkMsg::BlockImported));
 
 		// Send an unknown
-		let results = vec![(Ok(BlockImportResult::ImportedUnknown(Default::default(), Default::default())), Default::default())];
+		let results = vec![(Ok(BlockImportResult::ImportedUnknown(Default::default(), Default::default(), None)), Default::default())];
 		let _ = result_sender.send(BlockImportWorkerMsg::Imported(results)).ok().unwrap();
 		assert_eq!(link_port.recv(), Ok(LinkMsg::BlockImported));
 
+		// Send an unknown with peer and bad justification
+		let results = vec![(Ok(BlockImportResult::ImportedUnknown(Default::default(),
+			ImportedAux { needs_justification: true, clear_justification_requests: false, bad_justification: true },
+			Some(0))), Default::default())];
+		let _ = result_sender.send(BlockImportWorkerMsg::Imported(results)).ok().unwrap();
+		assert_eq!(link_port.recv(), Ok(LinkMsg::BlockImported));
+		assert_eq!(link_port.recv(), Ok(LinkMsg::Disconnected));
+
 		// Send an incomplete header
 		let results = vec![(Err(BlockImportError::IncompleteHeader(Some(Default::default()))), Default::default())];
 		let _ = result_sender.send(BlockImportWorkerMsg::Imported(results)).ok().unwrap();

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

@@ -460,7 +460,12 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 
 		match justification {
 			Some(justification) => {
-				self.import_justification(hash, number, justification, needs_justification)?;
+				self.import_justification(hash, number, justification, needs_justification).unwrap_or_else(|err| {
+					debug!(target: "finality", "Imported block #{} that enacts authority set change with \
+						invalid justification: {:?}, requesting justification from peers.", number, err);
+					imported_aux.bad_justification = true;
+					imported_aux.needs_justification = true;
+				});
 			},
 			None => {
 				if needs_justification {
@@ -476,7 +481,6 @@ impl<B, E, Block: BlockT<Hash=H256>, RA, PRA> BlockImport<Block>
 				if enacts_consensus_change {
 					self.consensus_changes.lock().note_change((number, hash));
 				}
-
 				imported_aux.needs_justification = true;
 			}
 		}

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

@@ -992,7 +992,50 @@ fn allows_reimporting_change_blocks() {
 
 	assert_eq!(
 		block_import.import_block(block(), None).unwrap(),
-		ImportResult::Imported(ImportedAux { needs_justification: true, clear_justification_requests: false }),
+		ImportResult::Imported(ImportedAux { needs_justification: true, clear_justification_requests: false, bad_justification: false }),
+	);
+
+	assert_eq!(
+		block_import.import_block(block(), None).unwrap(),
+		ImportResult::AlreadyInChain
+	);
+}
+
+#[test]
+fn test_bad_justification() {
+	let peers_a = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];
+	let peers_b = &[Keyring::Alice, Keyring::Bob];
+	let voters = make_ids(peers_a);
+	let api = TestApi::new(voters);
+	let net = GrandpaTestNet::new(api.clone(), 3);
+
+	let client = net.peer(0).client().clone();
+	let (block_import, ..) = net.make_block_import(client.clone());
+
+	let builder = client.new_block_at(&BlockId::Number(0)).unwrap();
+	let block = builder.bake().unwrap();
+	api.scheduled_changes.lock().insert(*block.header.parent_hash(), ScheduledChange {
+		next_authorities: make_ids(peers_b),
+		delay: 0,
+	});
+
+	let block = || {
+		let block = block.clone();
+		ImportBlock {
+			origin: BlockOrigin::File,
+			header: block.header,
+			justification: Some(Vec::new()),
+			post_digests: Vec::new(),
+			body: Some(block.extrinsics),
+			finalized: false,
+			auxiliary: Vec::new(),
+			fork_choice: ForkChoiceStrategy::LongestChain,
+		}
+	};
+
+	assert_eq!(
+		block_import.import_block(block(), None).unwrap(),
+		ImportResult::Imported(ImportedAux { needs_justification: true, clear_justification_requests: false, bad_justification: true }),
 	);
 
 	assert_eq!(

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

@@ -48,7 +48,7 @@ fn import_single_good_block_works() {
 	let (_, _hash, number, block) = prepare_good_block();
 	assert_eq!(
 		import_single_block(&test_client::new(), BlockOrigin::File, block, Arc::new(PassThroughVerifier(true))),
-		Ok(BlockImportResult::ImportedUnknown(number, Default::default()))
+		Ok(BlockImportResult::ImportedUnknown(number, Default::default(), Some(0)))
 	);
 }