From 75c5c3d4efadf636db8ec4b5beb6c0dbf76121fb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <bkchr@users.noreply.github.com>
Date: Sat, 30 Jan 2021 00:49:14 +0100
Subject: [PATCH] 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 | 66 +++++++++++++++++++++---------------
cumulus/network/src/tests.rs | 46 +++++--------------------
2 files changed, 47 insertions(+), 65 deletions(-)
diff --git a/cumulus/network/src/lib.rs b/cumulus/network/src/lib.rs
index b24b7bbbd14..6225233e9cf 100644
--- a/cumulus/network/src/lib.rs
+++ b/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()
}
diff --git a/cumulus/network/src/tests.rs b/cumulus/network/src/tests.rs
index 19c2938e0c9..2937c06ac99 100644
--- a/cumulus/network/src/tests.rs
+++ b/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
--
GitLab