Block announce validation should use the correct `Validation` result (#315)

* Block announce validation should use the correct `Validation` result

The error variant is just for internal errors and we need to return
`Failure` always when the other node send us an invalid statement.

* Update network/src/lib.rs

Co-authored-by: Sergei Shulepov <sergei@parity.io>

Co-authored-by: Sergei Shulepov <sergei@parity.io>

cumulus/network/src/lib.rs

@@ -54,10 +54,12 @@ use futures::{
 };
 use log::trace;
 
-use std::{fmt, marker::PhantomData, pin::Pin, sync::Arc, convert::TryFrom};
+use std::{convert::TryFrom, fmt, marker::PhantomData, pin::Pin, sync::Arc};
 
 use wait_on_relay_chain_block::WaitOnRelayChainBlock;
 
+const LOG_TARGET: &str = "cumulus-network";
+
 type BoxedError = Box<dyn std::error::Error + Send>;
 
 #[derive(Debug)]
@@ -84,26 +86,33 @@ impl BlockAnnounceData {
 	/// Validate that the receipt, statement and announced header match.
 	///
 	/// This will not check the signature, for this you should use [`BlockAnnounceData::check_signature`].
-	fn validate(&self, encoded_header: Vec<u8>) -> Result<(), BlockAnnounceError> {
+	fn validate(&self, encoded_header: Vec<u8>) -> Result<(), Validation> {
 		let candidate_hash = if let CompactStatement::Candidate(h) = self.statement.payload() {
 			h
 		} else {
-			return Err(BlockAnnounceError(
-				"`CompactStatement` isn't the candidate variant!".into(),
-			));
+			log::debug!(
+				target: LOG_TARGET,
+				"`CompactStatement` isn't the candidate variant!",
+			);
+			return Err(Validation::Failure { disconnect: true });
 		};
 
 		if *candidate_hash != self.receipt.hash() {
-			return Err(BlockAnnounceError(
-				"Receipt candidate hash doesn't match candidate hash in statement".into(),
-			));
+			log::debug!(
+				target: LOG_TARGET,
+				"Receipt candidate hash doesn't match candidate hash in statement",
+			);
+			return Err(Validation::Failure { disconnect: true });
 		}
 
-		if polkadot_parachain::primitives::HeadData(encoded_header).hash() != self.receipt.descriptor.para_head
+		if polkadot_parachain::primitives::HeadData(encoded_header).hash()
+			!= self.receipt.descriptor.para_head
 		{
-			return Err(BlockAnnounceError(
-				"Receipt para head hash doesn't match the hash of the header in the block announcement".into(),
-			));
+			log::debug!(
+				target: LOG_TARGET,
+				"Receipt para head hash doesn't match the hash of the header in the block announcement",
+			);
+			return Err(Validation::Failure { disconnect: true });
 		}
 
 		Ok(())
@@ -112,7 +121,7 @@ impl BlockAnnounceData {
 	/// Check the signature of the statement.
 	///
 	/// Returns an `Err(_)` if it failed.
-	fn check_signature<P>(&self, relay_chain_client: &Arc<P>) -> Result<(), BlockAnnounceError>
+	fn check_signature<P>(&self, relay_chain_client: &Arc<P>) -> Result<Validation, BlockAnnounceError>
 	where
 		P: ProvideRuntimeApi<PBlock> + Send + Sync + 'static,
 		P::Api: ParachainHost<PBlock>,
@@ -143,10 +152,12 @@ impl BlockAnnounceData {
 		let signer = match authorities.get(validator_index as usize) {
 			Some(r) => r,
 			None => {
-				return Err(BlockAnnounceError(
-					"block accouncement justification signer is a validator index out of bound"
-						.to_string(),
-				));
+				log::debug!(
+					target: LOG_TARGET,
+					"Block announcement justification signer is a validator index out of bound",
+				);
+
+				return Ok(Validation::Failure { disconnect: true })
 			}
 		};
 
@@ -156,12 +167,15 @@ impl BlockAnnounceData {
 			.check_signature(&signing_context, &signer)
 			.is_err()
 		{
-			return Err(BlockAnnounceError(
-				"block announcement justification signature is invalid".to_string(),
-			));
+			log::debug!(
+				target: LOG_TARGET,
+				"Block announcement justification signature is invalid.",
+			);
+
+			return Ok(Validation::Failure { disconnect: true });
 		}
 
-		Ok(())
+		Ok(Validation::Success { is_new_best: true })
 	}
 }
 
@@ -340,9 +354,9 @@ where
 		let wait_on_relay_chain_block = self.wait_on_relay_chain_block.clone();
 
 		async move {
-			block_announce_data
-				.validate(header_encoded)
-				.map_err(|e| Box::new(e) as Box<_>)?;
+			if let Err(e) = block_announce_data.validate(header_encoded) {
+				return Ok(e);
+			}
 
 			let relay_parent = block_announce_data.receipt.descriptor.relay_parent;
 
@@ -353,9 +367,7 @@ where
 
 			block_announce_data
 				.check_signature(&relay_chain_client)
-				.map_err(|e| Box::new(e) as Box<_>)?;
-
-			Ok(Validation::Success { is_new_best: true })
+				.map_err(|e| Box::new(e) as Box<_>)
 		}
 		.boxed()
 	}

cumulus/network/src/tests.rs

@@ -205,14 +205,8 @@ fn check_signer_is_legit_validator() {
 	let (signed_statement, header) = block_on(make_gossip_message_and_header_using_genesis(api, 1));
 	let data = BlockAnnounceData::try_from(signed_statement).unwrap().encode();
 
-	let res = block_on(validator.validate(&header, &data))
-		.err()
-		.expect("Should fail on invalid validator");
-
-	assert!(matches!(
-		*res.downcast::<BlockAnnounceError>().unwrap(),
-		BlockAnnounceError(x) if x.contains("signer is a validator")
-	));
+	let res = block_on(validator.validate(&header, &data));
+	assert_eq!(Validation::Failure { disconnect: true }, res.unwrap());
 }
 
 #[test]
@@ -227,16 +221,8 @@ fn check_statement_is_correctly_signed() {
 	let last = data.len() - 1;
 	data[last] = data[last].wrapping_add(1);
 
-	let res = block_on(validator.validate(&header, &data))
-		.err()
-		.expect("Validation should fail if the statement is not signed correctly");
-
-	check_error(res, |error| {
-		matches!(
-			error,
-			BlockAnnounceError(x) if x.contains("signature is invalid")
-		)
-	});
+	let res = block_on(validator.validate(&header, &data));
+	assert_eq!(Validation::Failure { disconnect: true }, res.unwrap());
 }
 
 #[test]
@@ -276,16 +262,8 @@ fn check_statement_seconded() {
 		statement: signed_statement.convert_payload(),
 	}.encode();
 
-	let res = block_on(validator.validate(&header, &data))
-		.err()
-		.expect("validation should fail if not seconded statement");
-
-	check_error(res, |error| {
-		matches!(
-			error,
-			BlockAnnounceError(x) if x.contains("`CompactStatement` isn't the candidate variant")
-		)
-	});
+	let res = block_on(validator.validate(&header, &data));
+	assert_eq!(Validation::Failure { disconnect: true }, res.unwrap());
 }
 
 #[test]
@@ -297,16 +275,8 @@ fn check_header_match_candidate_receipt_header() {
 	let data = BlockAnnounceData::try_from(signed_statement).unwrap().encode();
 	header.number = 300;
 
-	let res = block_on(validator.validate(&header, &data))
-		.err()
-		.expect("validation should fail if the header in doesn't match");
-
-	check_error(res, |error| {
-		matches!(
-			error,
-			BlockAnnounceError(x) if x.contains("Receipt para head hash doesn't match")
-		)
-	});
+	let res = block_on(validator.validate(&header, &data));
+	assert_eq!(Validation::Failure { disconnect: true }, res.unwrap());
 }
 
 /// Test that ensures that we postpone the block announce verification until