diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index 047c014ebe..e183fcd461 100644 --- a/client/network/src/lib.rs +++ b/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, + /// The relay parent that was used as context to sign the [`Self::statement`]. + relay_parent: PHash, +} + +impl Decode for BlockAnnounceData { + fn decode(input: &mut I) -> Result { + let receipt = CandidateReceipt::decode(input)?; + let statement = UncheckedSigned::::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 { - let receipt = if let Statement::Seconded(receipt) = stmt.payload() { + fn try_from(signal: &CollationSecondedSignal) -> Result { + 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( announce_block: Arc>) + Send + Sync>, signed_stmt_recv: oneshot::Receiver, ) { - 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( }, }; - 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.", ); diff --git a/client/network/src/tests.rs b/client/network/src/tests.rs index 9b47bdadcf..a17f8138c4 100644 --- a/client/network/src/tests.rs +++ b/client/network/src/tests.rs @@ -91,7 +91,7 @@ fn default_header() -> Header { async fn make_gossip_message_and_header_using_genesis( api: Arc, 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, 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, +} + +#[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);