Add relay_parent to block announce data (#640)

* Add `relay_parent` to block announce data

This is required to make it more future proof when contextual execution is coming. While this
changes the `BlockAnnounceData` type, it is written in a way to make it backwards compatible.

* FMT

cumulus/client/network/src/lib.rs

@@ -34,14 +34,14 @@ use sp_runtime::{
 };
 
 use polkadot_client::ClientHandle;
-use polkadot_node_primitives::{CollationSecondedSignal, SignedFullStatement, Statement};
+use polkadot_node_primitives::{CollationSecondedSignal, Statement};
 use polkadot_parachain::primitives::HeadData;
 use polkadot_primitives::v1::{
 	Block as PBlock, CandidateReceipt, CompactStatement, Hash as PHash, Id as ParaId,
 	OccupiedCoreAssumption, ParachainHost, SigningContext, UncheckedSigned,
 };
 
-use codec::{Decode, Encode};
+use codec::{Decode, DecodeAll, Encode};
 use futures::{
 	channel::oneshot,
 	future::{ready, FutureExt},
@@ -74,10 +74,29 @@ impl fmt::Display for BlockAnnounceError {
 ///
 /// This will be used to prove that a header belongs to a block that is probably being backed by
 /// the relay chain.
-#[derive(Encode, Decode, Debug)]
+#[derive(Encode, Debug)]
 pub struct BlockAnnounceData {
+	/// The receipt identifying the candidate.
 	receipt: CandidateReceipt,
+	/// The seconded statement issued by a relay chain validator that approves the candidate.
 	statement: UncheckedSigned<CompactStatement>,
+	/// The relay parent that was used as context to sign the [`Self::statement`].
+	relay_parent: PHash,
+}
+
+impl Decode for BlockAnnounceData {
+	fn decode<I: codec::Input>(input: &mut I) -> Result<Self, codec::Error> {
+		let receipt = CandidateReceipt::decode(input)?;
+		let statement = UncheckedSigned::<CompactStatement>::decode(input)?;
+
+		let relay_parent = match PHash::decode(input) {
+			Ok(p) => p,
+			// For being backwards compatible, we support missing relay-chain parent.
+			Err(_) => receipt.descriptor.relay_parent,
+		};
+
+		Ok(Self { receipt, statement, relay_parent })
+	}
 }
 
 impl BlockAnnounceData {
@@ -127,14 +146,13 @@ impl BlockAnnounceData {
 		let runtime_api = relay_chain_client.runtime_api();
 		let validator_index = self.statement.unchecked_validator_index();
 
-		let runtime_api_block_id = BlockId::Hash(self.receipt.descriptor.relay_parent);
+		let runtime_api_block_id = BlockId::Hash(self.relay_parent);
 		let session_index = match runtime_api.session_index_for_child(&runtime_api_block_id) {
 			Ok(r) => r,
 			Err(e) => return Err(BlockAnnounceError(format!("{:?}", e))),
 		};
 
-		let signing_context =
-			SigningContext { parent_hash: self.receipt.descriptor.relay_parent, session_index };
+		let signing_context = SigningContext { parent_hash: self.relay_parent, session_index };
 
 		// Check that the signer is a legit validator.
 		let authorities = match runtime_api.validators(&runtime_api_block_id) {
@@ -167,17 +185,21 @@ impl BlockAnnounceData {
 	}
 }
 
-impl TryFrom<&'_ SignedFullStatement> for BlockAnnounceData {
+impl TryFrom<&'_ CollationSecondedSignal> for BlockAnnounceData {
 	type Error = ();
 
-	fn try_from(stmt: &SignedFullStatement) -> Result<BlockAnnounceData, ()> {
-		let receipt = if let Statement::Seconded(receipt) = stmt.payload() {
+	fn try_from(signal: &CollationSecondedSignal) -> Result<BlockAnnounceData, ()> {
+		let receipt = if let Statement::Seconded(receipt) = signal.statement.payload() {
 			receipt.to_plain()
 		} else {
 			return Err(())
 		};
 
-		Ok(BlockAnnounceData { receipt, statement: stmt.convert_payload().into() })
+		Ok(BlockAnnounceData {
+			receipt,
+			statement: signal.statement.convert_payload().into(),
+			relay_parent: signal.relay_parent,
+		})
 	}
 }
 
@@ -347,12 +369,15 @@ where
 			return self.handle_empty_block_announce_data(header.clone()).boxed()
 		}
 
-		let block_announce_data = match BlockAnnounceData::decode(&mut data) {
+		let block_announce_data = match BlockAnnounceData::decode_all(&mut data) {
 			Ok(r) => r,
-			Err(_) =>
-				return ready(Err(Box::new(BlockAnnounceError(
-					"Can not decode the `BlockAnnounceData`".into(),
-				)) as Box<_>))
+			Err(err) =>
+				return async move {
+					Err(Box::new(BlockAnnounceError(format!(
+						"Can not decode the `BlockAnnounceData`: {:?}",
+						err
+					))) as Box<_>)
+				}
 				.boxed(),
 		};
 
@@ -520,8 +545,8 @@ async fn wait_to_announce<Block: BlockT>(
 	announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 	signed_stmt_recv: oneshot::Receiver<CollationSecondedSignal>,
 ) {
-	let statement = match signed_stmt_recv.await {
-		Ok(s) => s.statement,
+	let signal = match signed_stmt_recv.await {
+		Ok(s) => s,
 		Err(_) => {
 			tracing::debug!(
 				target: "cumulus-network",
@@ -532,12 +557,12 @@ async fn wait_to_announce<Block: BlockT>(
 		},
 	};
 
-	if let Ok(data) = BlockAnnounceData::try_from(&statement) {
+	if let Ok(data) = BlockAnnounceData::try_from(&signal) {
 		announce_block(block_hash, Some(data.encode()));
 	} else {
 		tracing::debug!(
 			target: "cumulus-network",
-			statement = ?statement,
+			?signal,
 			block = ?block_hash,
 			"Received invalid statement while waiting to announce block.",
 		);

cumulus/client/network/src/tests.rs

@@ -91,7 +91,7 @@ fn default_header() -> Header {
 async fn make_gossip_message_and_header_using_genesis(
 	api: Arc<TestApi>,
 	validator_index: u32,
-) -> (SignedFullStatement, Header) {
+) -> (CollationSecondedSignal, Header) {
 	let relay_parent = api.relay_client.hash(0).ok().flatten().expect("Genesis hash exists");
 
 	make_gossip_message_and_header(api, relay_parent, validator_index).await
@@ -101,7 +101,7 @@ async fn make_gossip_message_and_header(
 	api: Arc<TestApi>,
 	relay_parent: H256,
 	validator_index: u32,
-) -> (SignedFullStatement, Header) {
+) -> (CollationSecondedSignal, Header) {
 	let keystore: SyncCryptoStorePtr = Arc::new(KeyStore::new());
 	let alice_public = SyncCryptoStore::sr25519_generate_new(
 		&*keystore,
@@ -138,7 +138,7 @@ async fn make_gossip_message_and_header(
 	.flatten()
 	.expect("Signing statement");
 
-	(signed, header)
+	(CollationSecondedSignal { statement: signed, relay_parent }, header)
 }
 
 #[test]
@@ -184,8 +184,7 @@ fn check_statement_is_encoded_correctly() {
 	let mut validator = make_validator_and_api().0;
 	let header = default_header();
 	let res = block_on(validator.validate(&header, &[0x42]))
-		.err()
-		.expect("Should fail on invalid encoded statement");
+		.expect_err("Should fail on invalid encoded statement");
 
 	check_error(res, |error| {
 		matches!(
@@ -195,12 +194,63 @@ fn check_statement_is_encoded_correctly() {
 	});
 }
 
+#[test]
+fn block_announce_data_decoding_should_reject_extra_data() {
+	let (mut validator, api) = make_validator_and_api();
+
+	let (signal, header) = block_on(make_gossip_message_and_header_using_genesis(api, 1));
+	let mut data = BlockAnnounceData::try_from(&signal).unwrap().encode();
+	data.push(0x42);
+
+	let res = block_on(validator.validate(&header, &data)).expect_err("Should return an error ");
+
+	check_error(res, |error| {
+		matches!(
+			error,
+			BlockAnnounceError(x) if x.contains("Input buffer has still data left after decoding!")
+		)
+	});
+}
+
+#[derive(Encode, Decode, Debug)]
+struct LegacyBlockAnnounceData {
+	receipt: CandidateReceipt,
+	statement: UncheckedSigned<CompactStatement>,
+}
+
+#[test]
+fn legacy_block_announce_data_handling() {
+	let (_, api) = make_validator_and_api();
+
+	let (signal, _) = block_on(make_gossip_message_and_header_using_genesis(api, 1));
+
+	let receipt = if let Statement::Seconded(receipt) = signal.statement.payload() {
+		receipt.to_plain()
+	} else {
+		panic!("Invalid")
+	};
+
+	let legacy = LegacyBlockAnnounceData {
+		receipt: receipt.clone(),
+		statement: signal.statement.convert_payload().into(),
+	};
+
+	let data = legacy.encode();
+
+	let block_data =
+		BlockAnnounceData::decode(&mut &data[..]).expect("Decoding works from legacy works");
+	assert_eq!(receipt.descriptor.relay_parent, block_data.relay_parent);
+
+	let data = block_data.encode();
+	LegacyBlockAnnounceData::decode(&mut &data[..]).expect("Decoding works");
+}
+
 #[test]
 fn check_signer_is_legit_validator() {
 	let (mut validator, api) = make_validator_and_api();
 
-	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 (signal, header) = block_on(make_gossip_message_and_header_using_genesis(api, 1));
+	let data = BlockAnnounceData::try_from(&signal).unwrap().encode();
 
 	let res = block_on(validator.validate(&header, &data));
 	assert_eq!(Validation::Failure { disconnect: true }, res.unwrap());
@@ -210,9 +260,9 @@ fn check_signer_is_legit_validator() {
 fn check_statement_is_correctly_signed() {
 	let (mut validator, api) = make_validator_and_api();
 
-	let (signed_statement, header) = block_on(make_gossip_message_and_header_using_genesis(api, 0));
+	let (signal, header) = block_on(make_gossip_message_and_header_using_genesis(api, 0));
 
-	let mut data = BlockAnnounceData::try_from(&signed_statement).unwrap().encode();
+	let mut data = BlockAnnounceData::try_from(&signal).unwrap().encode();
 
 	// The signature comes at the end of the type, so change a bit to make the signature invalid.
 	let last = data.len() - 1;
@@ -255,6 +305,7 @@ fn check_statement_seconded() {
 	let data = BlockAnnounceData {
 		receipt: Default::default(),
 		statement: signed_statement.convert_payload().into(),
+		relay_parent,
 	}
 	.encode();
 
@@ -266,9 +317,8 @@ fn check_statement_seconded() {
 fn check_header_match_candidate_receipt_header() {
 	let (mut validator, api) = make_validator_and_api();
 
-	let (signed_statement, mut header) =
-		block_on(make_gossip_message_and_header_using_genesis(api, 0));
-	let data = BlockAnnounceData::try_from(&signed_statement).unwrap().encode();
+	let (signal, mut header) = block_on(make_gossip_message_and_header_using_genesis(api, 0));
+	let data = BlockAnnounceData::try_from(&signal).unwrap().encode();
 	header.number = 300;
 
 	let res = block_on(validator.validate(&header, &data));
@@ -287,9 +337,9 @@ fn relay_parent_not_imported_when_block_announce_is_processed() {
 		let mut client = api.relay_client.clone();
 		let block = client.init_polkadot_block_builder().build().expect("Build new block").block;
 
-		let (signed_statement, header) = make_gossip_message_and_header(api, block.hash(), 0).await;
+		let (signal, header) = make_gossip_message_and_header(api, block.hash(), 0).await;
 
-		let data = BlockAnnounceData::try_from(&signed_statement).unwrap().encode();
+		let data = BlockAnnounceData::try_from(&signal).unwrap().encode();
 
 		let mut validation = validator.validate(&header, &data);