diff --git a/substrate/core/consensus/common/src/block_import.rs b/substrate/core/consensus/common/src/block_import.rs index 01b3d24452..06c78d74af 100644 --- a/substrate/core/consensus/common/src/block_import.rs +++ b/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, } } } diff --git a/substrate/core/consensus/common/src/import_queue.rs b/substrate/core/consensus/common/src/import_queue.rs index 08a25e566b..997b30ed6f 100644 --- a/substrate/core/consensus/common/src/import_queue.rs +++ b/substrate/core/consensus/common/src/import_queue.rs @@ -309,7 +309,7 @@ impl BlockImporter { 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 BlockImporter { 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> BlockImportWorker { 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 { /// Imported known block. ImportedKnown(N), /// Imported unknown block. - ImportedUnknown(N, ImportedAux), + ImportedUnknown(N, ImportedAux, Option), } /// Block import error. @@ -528,7 +534,7 @@ pub fn import_single_block>( 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(); diff --git a/substrate/core/finality-grandpa/src/import.rs b/substrate/core/finality-grandpa/src/import.rs index 7e1582645f..9ff3486b86 100644 --- a/substrate/core/finality-grandpa/src/import.rs +++ b/substrate/core/finality-grandpa/src/import.rs @@ -460,7 +460,12 @@ impl, RA, PRA> BlockImport 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, RA, PRA> BlockImport if enacts_consensus_change { self.consensus_changes.lock().note_change((number, hash)); } - imported_aux.needs_justification = true; } } diff --git a/substrate/core/finality-grandpa/src/tests.rs b/substrate/core/finality-grandpa/src/tests.rs index 9aed819b25..c6efee1f39 100644 --- a/substrate/core/finality-grandpa/src/tests.rs +++ b/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!( diff --git a/substrate/core/network/src/test/block_import.rs b/substrate/core/network/src/test/block_import.rs index bc25078caa..0916d698c3 100644 --- a/substrate/core/network/src/test/block_import.rs +++ b/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))) ); }