Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions collator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,10 @@ where
+ Sync
+ 'static,
Backend: sc_client_api::Backend<Block> + 'static,
Client: Finalizer<Block, Backend> + UsageProvider<Block> + Send + Sync + 'static,
Client: Finalizer<Block, Backend> + UsageProvider<Block> + HeaderBackend<Block>
+ Send
+ Sync
+ 'static,
{
type ParachainContext = Collator<Block, PF, BI>;

Expand All @@ -359,7 +362,7 @@ where
Extrinsic: codec::Codec + Send + Sync + 'static,
{
self.delayed_block_announce_validator.set(
Box::new(JustifiedBlockAnnounceValidator::new(polkadot_client.clone())),
Box::new(JustifiedBlockAnnounceValidator::new(polkadot_client.clone(), self.para_id)),
);

let follow =
Expand Down
45 changes: 39 additions & 6 deletions network/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ mod tests;
use sp_api::ProvideRuntimeApi;
use sp_blockchain::{Error as ClientError, HeaderBackend};
use sp_consensus::block_validation::{BlockAnnounceValidator, Validation};
use sp_runtime::{generic::BlockId, traits::Block as BlockT};
use sp_runtime::{generic::BlockId, traits::{Block as BlockT, Header as HeaderT}};

use polkadot_collator::Network as CollatorNetwork;
use polkadot_network::legacy::gossip::{GossipMessage, GossipStatement};
use polkadot_primitives::{
parachain::ParachainHost,
parachain::{ParachainHost, Id as ParaId},
Block as PBlock, Hash as PHash,
};
use polkadot_statement_table::{SignedStatement, Statement};
Expand All @@ -45,6 +45,12 @@ use log::{error, trace};
use std::{marker::PhantomData, sync::Arc};
use parking_lot::Mutex;

/// The head data of the parachain, stored in the relay chain.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move that into primitives and consolidate with the one in collator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#[derive(Decode, Encode, Debug)]
struct HeadData<Block: BlockT> {
header: Block::Header,
}

/// Validate that data is a valid justification from a relay-chain validator that the block is a
/// valid parachain-block candidate.
/// Data encoding is just `GossipMessage`, the relay-chain validator candidate statement message is
Expand All @@ -54,13 +60,15 @@ use parking_lot::Mutex;
pub struct JustifiedBlockAnnounceValidator<B, P> {
phantom: PhantomData<B>,
polkadot_client: Arc<P>,
para_id: ParaId,
}

impl<B, P> JustifiedBlockAnnounceValidator<B, P> {
pub fn new(polkadot_client: Arc<P>) -> Self {
pub fn new(polkadot_client: Arc<P>, para_id: ParaId) -> Self {
Self {
phantom: Default::default(),
polkadot_client,
para_id,
}
}
}
Expand All @@ -75,8 +83,34 @@ where
header: &B::Header,
mut data: &[u8],
) -> Result<Validation, Box<dyn std::error::Error + Send>> {
// If no data is provided the announce is valid.
let runtime_api = self.polkadot_client.runtime_api();
let polkadot_info = self.polkadot_client.info();

if data.is_empty() {
// Check if block is one higher than best
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a justification if this is a new best block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like this? 2f9131c

let runtime_api_block_id = BlockId::Hash(polkadot_info.best_hash);
let block_number: <<B as BlockT>::Header as HeaderT>::Number = *header.number();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let block_number: <<B as BlockT>::Header as HeaderT>::Number = *header.number();
let block_number = header.number().clone();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


let local_validation_data = runtime_api
.local_validation_data(&runtime_api_block_id, self.para_id)
.map_err(|e| Box::new(ClientError::Msg(format!("{:?}", e))) as Box<_>)?
.ok_or_else(|| {
Box::new(ClientError::Msg("no local validation data".to_string())) as Box<_>
})?;
let parent_head = HeadData::<B>::decode(&mut &local_validation_data.parent_head.0[..])
.map_err(|e| Box::new(ClientError::Msg(format!("{:?}", e))) as Box<_>)?;
let known_best_number = *parent_head.header.number();

if !(block_number < known_best_number) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !(block_number < known_best_number) {
let res = if block_number >= known_best_number {

Negations always makes stuff much more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

trace!(
target: "cumulus-network",
"validation failed because the block number is not at least the best block \
number known",
);

return Ok(Validation::Failure);
}

return Ok(Validation::Success);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Ok(Validation::Failure);
}
return Ok(Validation::Success);
Validation::Failure
} else {
Validation::Success
};
return Ok(res);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eww no, the rest of the function follows the pattern:

if <condition> {
    return ... // short-circuit
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but here a else makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

Expand Down Expand Up @@ -108,7 +142,7 @@ where
} = gossip_statement;

// Check that the relay chain parent of the block is the relay chain head
let best_number = self.polkadot_client.info().best_number;
let best_number = polkadot_info.best_number;

match self.polkadot_client.number(relay_chain_leaf) {
Err(err) => {
Expand All @@ -133,7 +167,6 @@ where
},
}

let runtime_api = self.polkadot_client.runtime_api();
let runtime_api_block_id = BlockId::Hash(relay_chain_leaf);
let signing_context = runtime_api
.signing_context(&runtime_api_block_id)
Expand Down
43 changes: 35 additions & 8 deletions network/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,15 @@ fn make_validator_and_client() -> (
let builder = TestClientBuilder::new();
let client = Arc::new(TestApi::new(Arc::new(builder.build())));

(JustifiedBlockAnnounceValidator::new(client.clone()), client)
(
JustifiedBlockAnnounceValidator::new(client.clone(), ParaId::from(56)),
client,
)
}

fn default_header() -> Header {
Header {
number: Default::default(),
number: 1,
digest: Default::default(),
extrinsics_root: Default::default(),
parent_hash: Default::default(),
Expand Down Expand Up @@ -91,13 +94,34 @@ fn make_gossip_message_and_header(
}

#[test]
fn valid_if_no_data() {
fn valid_if_no_data_and_less_than_best_known_number() {
let mut validator = make_validator();
let header = default_header();
let header = Header {
number: 0,
..default_header()
};
let res = validator.validate(&header, &[]);

assert!(
matches!(validator.validate(&header, &[]), Ok(Validation::Success)),
"validating without data is always a success",
assert_eq!(
res.unwrap(),
Validation::Success,
"validating without data with block number < best known number is always a success",
);
}

#[test]
fn invalid_if_no_data_exceeds_best_known_number() {
let mut validator = make_validator();
let header = Header {
number: 1,
..default_header()
};
let res = validator.validate(&header, &[]);

assert_eq!(
res.unwrap(),
Validation::Failure,
"validation fails if no justification and block number >= best known number",
);
}

Expand Down Expand Up @@ -399,7 +423,10 @@ sp_api::mock_impl_runtime_apis! {
}

fn local_validation_data(_: ParaId) -> Option<LocalValidationData> {
Some(Default::default())
Some(LocalValidationData {
parent_head: HeadData::<Block> { header: default_header() }.encode().into(),
..Default::default()
})
}

fn get_heads(_: Vec<<PBlock as BlockT>::Extrinsic>) -> Option<Vec<AbridgedCandidateReceipt>> {
Expand Down