client/beefy: drop justification on import if pallet not enabled (#13422)

BEEFY pallet allows setting on-chain BEEFY genesis to some future
block. Disregard any BEEFY justifications attached to imported blocks
that predate configured BEEFY genesis.

Signed-off-by: acatangiu <adrian@parity.io>

substrate/client/beefy/src/import.rs

@@ -92,13 +92,23 @@ where
 		number: NumberFor<Block>,
 		hash: <Block as BlockT>::Hash,
 	) -> Result<BeefyVersionedFinalityProof<Block>, ConsensusError> {
+		use ConsensusError::ClientImport as ImportError;
 		let block_id = BlockId::hash(hash);
+		let beefy_genesis = self
+			.runtime
+			.runtime_api()
+			.beefy_genesis(&block_id)
+			.map_err(|e| ImportError(e.to_string()))?
+			.ok_or_else(|| ImportError("Unknown BEEFY genesis".to_string()))?;
+		if number < beefy_genesis {
+			return Err(ImportError("BEEFY genesis is set for future block".to_string()))
+		}
 		let validator_set = self
 			.runtime
 			.runtime_api()
 			.validator_set(&block_id)
-			.map_err(|e| ConsensusError::ClientImport(e.to_string()))?
-			.ok_or_else(|| ConsensusError::ClientImport("Unknown validator set".to_string()))?;
+			.map_err(|e| ImportError(e.to_string()))?
+			.ok_or_else(|| ImportError("Unknown validator set".to_string()))?;
 
 		decode_and_verify_finality_proof::<Block>(&encoded[..], number, &validator_set)
 	}
@@ -142,26 +152,28 @@ where
 
 		match (beefy_encoded, &inner_import_result) {
 			(Some(encoded), ImportResult::Imported(_)) => {
-				if let Ok(proof) = self.decode_and_verify(&encoded, number, hash) {
-					// The proof is valid and the block is imported and final, we can import.
-					debug!(
-						target: LOG_TARGET,
-						"🥩 import justif {:?} for block number {:?}.", proof, number
-					);
-					// Send the justification to the BEEFY voter for processing.
-					self.justification_sender
-						.notify(|| Ok::<_, ()>(proof))
-						.expect("forwards closure result; the closure always returns Ok; qed.");
-
-					metric_inc!(self, beefy_good_justification_imports);
-				} else {
-					debug!(
-						target: LOG_TARGET,
-						"🥩 error decoding justification: {:?} for imported block {:?}",
-						encoded,
-						number,
-					);
-					metric_inc!(self, beefy_bad_justification_imports);
+				match self.decode_and_verify(&encoded, number, hash) {
+					Ok(proof) => {
+						// The proof is valid and the block is imported and final, we can import.
+						debug!(
+							target: LOG_TARGET,
+							"🥩 import justif {:?} for block number {:?}.", proof, number
+						);
+						// Send the justification to the BEEFY voter for processing.
+						self.justification_sender
+							.notify(|| Ok::<_, ()>(proof))
+							.expect("the closure always returns Ok; qed.");
+						metric_inc!(self, beefy_good_justification_imports);
+					},
+					Err(err) => {
+						debug!(
+							target: LOG_TARGET,
+							"🥩 error importing BEEFY justification for block {:?}: {:?}",
+							number,
+							err,
+						);
+						metric_inc!(self, beefy_bad_justification_imports);
+					},
 				}
 			},
 			_ => (),

substrate/client/beefy/src/tests.rs

@@ -60,7 +60,7 @@ use sp_runtime::{
 	codec::Encode,
 	generic::BlockId,
 	traits::{Header as HeaderT, NumberFor},
-	BuildStorage, DigestItem, Justifications, Storage,
+	BuildStorage, DigestItem, EncodedJustification, Justifications, Storage,
 };
 use std::{collections::HashMap, marker::PhantomData, sync::Arc, task::Poll};
 use substrate_test_runtime_client::{runtime::Header, ClientExt};
@@ -107,11 +107,13 @@ pub(crate) struct PeerData {
 #[derive(Default)]
 pub(crate) struct BeefyTestNet {
 	peers: Vec<BeefyPeer>,
+	pub beefy_genesis: NumberFor<Block>,
 }
 
 impl BeefyTestNet {
 	pub(crate) fn new(n_authority: usize) -> Self {
-		let mut net = BeefyTestNet { peers: Vec::with_capacity(n_authority) };
+		let beefy_genesis = 1;
+		let mut net = BeefyTestNet { peers: Vec::with_capacity(n_authority), beefy_genesis };
 
 		for i in 0..n_authority {
 			let (rx, cfg) = on_demand_justifications_protocol_config(GENESIS_HASH, None);
@@ -202,7 +204,7 @@ impl TestNetFactory for BeefyTestNet {
 	) {
 		let keys = &[BeefyKeyring::Alice, BeefyKeyring::Bob];
 		let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap();
-		let api = Arc::new(TestApi::with_validator_set(&validator_set));
+		let api = Arc::new(TestApi::new(self.beefy_genesis, &validator_set, GOOD_MMR_ROOT));
 		let inner = BlockImportAdapter::new(client.clone());
 		let (block_import, voter_links, rpc_links) =
 			beefy_block_import_and_links(inner, client.as_backend(), api, None);
@@ -716,7 +718,7 @@ async fn correct_beefy_payload() {
 }
 
 #[tokio::test]
-async fn beefy_importing_blocks() {
+async fn beefy_importing_justifications() {
 	use futures::{future::poll_fn, task::Poll};
 	use sc_block_builder::BlockBuilderProvider;
 	use sc_client_api::BlockBackend;
@@ -726,11 +728,15 @@ async fn beefy_importing_blocks() {
 	let mut net = BeefyTestNet::new(2);
 	let keys = &[BeefyKeyring::Alice, BeefyKeyring::Bob];
 	let good_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap();
+	// Set BEEFY genesis to block 3.
+	net.beefy_genesis = 3;
 
 	let client = net.peer(0).client().clone();
+	let full_client = client.as_client();
 	let (mut block_import, _, peer_data) = net.make_block_import(client.clone());
 	let PeerData { beefy_voter_links, .. } = peer_data;
 	let justif_stream = beefy_voter_links.lock().take().unwrap().from_block_import_justif_stream;
+	let mut justif_recv = justif_stream.subscribe(100_000);
 
 	let params = |block: Block, justifications: Option<Justifications>| {
 		let mut import = BlockImportParams::new(BlockOrigin::File, block.header);
@@ -740,15 +746,19 @@ async fn beefy_importing_blocks() {
 		import.fork_choice = Some(ForkChoiceStrategy::LongestChain);
 		import
 	};
+	let backend_justif_for = |block_hash: H256| -> Option<EncodedJustification> {
+		full_client
+			.justifications(block_hash)
+			.unwrap()
+			.and_then(|j| j.get(BEEFY_ENGINE_ID).cloned())
+	};
 
-	let full_client = client.as_client();
 	let parent_id = BlockId::Number(0);
 	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
 	let block = builder.build().unwrap().block;
 	let hashof1 = block.header.hash();
 
-	// Import without justifications.
-	let mut justif_recv = justif_stream.subscribe(100_000);
+	// Import block 1 without justifications.
 	assert_eq!(
 		block_import
 			.import_block(params(block.clone(), None), HashMap::new())
@@ -760,16 +770,32 @@ async fn beefy_importing_blocks() {
 		block_import.import_block(params(block, None), HashMap::new()).await.unwrap(),
 		ImportResult::AlreadyInChain,
 	);
-	// Verify no BEEFY justifications present:
+
+	// Import block 2 with "valid" justification (beefy pallet genesis block not yet reached).
+	let parent_id = BlockId::Number(1);
+	let block_num = 2;
+	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
+	let block = builder.build().unwrap().block;
+	let hashof2 = block.header.hash();
+
+	let proof = crate::justification::tests::new_finality_proof(block_num, &good_set, keys);
+	let versioned_proof: VersionedFinalityProof<NumberFor<Block>, Signature> = proof.into();
+	let encoded = versioned_proof.encode();
+	let justif = Some(Justifications::from((BEEFY_ENGINE_ID, encoded)));
+	assert_eq!(
+		block_import.import_block(params(block, justif), HashMap::new()).await.unwrap(),
+		ImportResult::Imported(ImportedAux {
+			bad_justification: false,
+			is_new_best: true,
+			..Default::default()
+		}),
+	);
+
+	// Verify no BEEFY justifications present (for either block 1 or 2):
 	{
 		// none in backend,
-		assert_eq!(
-			full_client
-				.justifications(hashof1)
-				.unwrap()
-				.and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()),
-			None
-		);
+		assert_eq!(backend_justif_for(hashof1), None);
+		assert_eq!(backend_justif_for(hashof2), None);
 		// and none sent to BEEFY worker.
 		poll_fn(move |cx| {
 			assert_eq!(justif_recv.poll_next_unpin(cx), Poll::Pending);
@@ -778,17 +804,16 @@ async fn beefy_importing_blocks() {
 		.await;
 	}
 
-	// Import with valid justification.
-	let parent_id = BlockId::Number(1);
-	let block_num = 2;
+	// Import block 3 with valid justification.
+	let parent_id = BlockId::Number(2);
+	let block_num = 3;
+	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
+	let block = builder.build().unwrap().block;
+	let hashof3 = block.header.hash();
 	let proof = crate::justification::tests::new_finality_proof(block_num, &good_set, keys);
 	let versioned_proof: VersionedFinalityProof<NumberFor<Block>, Signature> = proof.into();
 	let encoded = versioned_proof.encode();
 	let justif = Some(Justifications::from((BEEFY_ENGINE_ID, encoded)));
-
-	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
-	let block = builder.build().unwrap().block;
-	let hashof2 = block.header.hash();
 	let mut justif_recv = justif_stream.subscribe(100_000);
 	assert_eq!(
 		block_import.import_block(params(block, justif), HashMap::new()).await.unwrap(),
@@ -801,13 +826,7 @@ async fn beefy_importing_blocks() {
 	// Verify BEEFY justification successfully imported:
 	{
 		// still not in backend (worker is responsible for appending to backend),
-		assert_eq!(
-			full_client
-				.justifications(hashof2)
-				.unwrap()
-				.and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()),
-			None
-		);
+		assert_eq!(backend_justif_for(hashof3), None);
 		// but sent to BEEFY worker
 		// (worker will append it to backend when all previous mandatory justifs are there as well).
 		poll_fn(move |cx| {
@@ -820,19 +839,18 @@ async fn beefy_importing_blocks() {
 		.await;
 	}
 
-	// Import with invalid justification (incorrect validator set).
-	let parent_id = BlockId::Number(2);
-	let block_num = 3;
+	// Import block 4 with invalid justification (incorrect validator set).
+	let parent_id = BlockId::Number(3);
+	let block_num = 4;
+	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
+	let block = builder.build().unwrap().block;
+	let hashof4 = block.header.hash();
 	let keys = &[BeefyKeyring::Alice];
 	let bad_set = ValidatorSet::new(make_beefy_ids(keys), 1).unwrap();
 	let proof = crate::justification::tests::new_finality_proof(block_num, &bad_set, keys);
 	let versioned_proof: VersionedFinalityProof<NumberFor<Block>, Signature> = proof.into();
 	let encoded = versioned_proof.encode();
 	let justif = Some(Justifications::from((BEEFY_ENGINE_ID, encoded)));
-
-	let builder = full_client.new_block_at(&parent_id, Default::default(), false).unwrap();
-	let block = builder.build().unwrap().block;
-	let hashof3 = block.header.hash();
 	let mut justif_recv = justif_stream.subscribe(100_000);
 	assert_eq!(
 		block_import.import_block(params(block, justif), HashMap::new()).await.unwrap(),
@@ -846,13 +864,7 @@ async fn beefy_importing_blocks() {
 	// Verify bad BEEFY justifications was not imported:
 	{
 		// none in backend,
-		assert_eq!(
-			full_client
-				.justifications(hashof3)
-				.unwrap()
-				.and_then(|j| j.get(BEEFY_ENGINE_ID).cloned()),
-			None
-		);
+		assert_eq!(backend_justif_for(hashof4), None);
 		// and none sent to BEEFY worker.
 		poll_fn(move |cx| {
 			assert_eq!(justif_recv.poll_next_unpin(cx), Poll::Pending);