Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

find_potential_parents: Enforce max_depth from the pending_block #7335

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

find_potential_parents: Enforce max_depth from the pending_block #7335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 10 additions & 17 deletions cumulus/client/consensus/common/src/parent_search.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub struct ParentSearchParams {
/// A limitation on the age of relay parents for parachain blocks that are being
/// considered. This is relative to the `relay_parent` number.
pub ancestry_lookback: usize,
/// How "deep" parents can be relative to the included parachain block at the relay-parent.
/// How "deep" parents can be relative to the pending parachain block at the relay-parent.
/// The included block has depth 0.
pub max_depth: usize,
/// Whether to only ignore "alternative" branches, i.e. branches of the chain
Expand Down Expand Up @@ -84,11 +84,11 @@ impl<B: BlockT> std::fmt::Debug for PotentialParent<B> {
/// relay-parent according to the search parameters.
///
/// A parachain block is a potential parent if it is either the last included parachain block, the
/// pending parachain block (when `max_depth` >= 1), or all of the following hold:
/// pending parachain block, or all of the following hold:
/// * its parent is a potential parent
/// * its relay-parent is within `ancestry_lookback` of the targeted relay-parent.
/// * its relay-parent is within the same session as the targeted relay-parent.
/// * the block number is within `max_depth` blocks of the included block
/// * the block number is within `max_depth` blocks of the parent block
pub async fn find_potential_parents<B: BlockT>(
params: ParentSearchParams,
backend: &impl Backend<B>,
Expand All @@ -110,10 +110,6 @@ pub async fn find_potential_parents<B: BlockT>(
aligned_with_pending: true,
}];

if params.max_depth == 0 {
return Ok(only_included)
};

// Pending header and hash.
let maybe_pending = {
// Fetch the most recent pending header from the relay chain. We use
Expand Down Expand Up @@ -160,7 +156,7 @@ pub async fn find_potential_parents<B: BlockT>(

// If we want to ignore alternative branches there is no reason to start
// the parent search at the included block. We can add the included block and
// the path to the pending block to the potential parents directly (limited by max_depth).
// the path to the pending block to the potential parents directly.
let (frontier, potential_parents) = match (
&maybe_pending,
params.ignore_alternative_branches,
Expand All @@ -175,10 +171,8 @@ pub async fn find_potential_parents<B: BlockT>(
return Ok(Default::default())
}

// Add all items on the path included -> pending - 1 to the potential parents, but
// not more than `max_depth`.
let num_parents_on_path =
route_to_pending.enacted().len().saturating_sub(1).min(params.max_depth);
// Add all items on the path included -> pending - 1 to the potential parents.
let num_parents_on_path = route_to_pending.enacted().len().saturating_sub(1);
for (num, block) in
route_to_pending.enacted().iter().take(num_parents_on_path).enumerate()
{
Expand Down Expand Up @@ -207,10 +201,6 @@ pub async fn find_potential_parents<B: BlockT>(
_ => (only_included, Default::default()),
};

if potential_parents.len() > params.max_depth {
return Ok(potential_parents);
}

// Build up the ancestry record of the relay chain to compare against.
let rp_ancestry =
build_relay_parent_ancestry(params.ancestry_lookback, params.relay_parent, relay_client)
Expand Down Expand Up @@ -386,7 +376,10 @@ pub fn search_child_branches_for_parents<Block: BlockT>(
potential_parents.push(entry);
}

if !is_potential || child_depth > max_depth {
let child_depth_from_pending =
pending_distance.map_or(child_depth, |pd| child_depth.saturating_sub(pd));

if !is_potential || child_depth_from_pending > max_depth {
continue
}

Expand Down
93 changes: 80 additions & 13 deletions cumulus/client/consensus/common/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ use cumulus_test_relay_sproof_builder::RelayStateSproofBuilder;
use futures::{channel::mpsc, executor::block_on, select, FutureExt, Stream, StreamExt};
use futures_timer::Delay;
use polkadot_primitives::HeadData;
use sc_client_api::{Backend as _, UsageProvider};
use sc_client_api::{Backend as _, HeaderBackend, UsageProvider};
use sc_consensus::{BlockImport, BlockImportParams, ForkChoiceStrategy};
use sp_blockchain::Backend as BlockchainBackend;
use sp_consensus::{BlockOrigin, BlockStatus};
Expand Down Expand Up @@ -1404,7 +1404,7 @@ fn find_potential_parents_aligned_with_late_pending() {
// the other is not.
let mut aligned_blocks = Vec::new();
let mut parent = pending_block.header().hash();
for _ in 2..NON_INCLUDED_CHAIN_LEN {
for _ in 2..NON_INCLUDED_CHAIN_LEN * 2 {
let block = build_and_import_block_ext(
&client,
BlockOrigin::Own,
Expand All @@ -1420,7 +1420,7 @@ fn find_potential_parents_aligned_with_late_pending() {

let mut alt_blocks = Vec::new();
let mut parent = included_block.header().hash();
for _ in 0..NON_INCLUDED_CHAIN_LEN {
for _ in 0..NON_INCLUDED_CHAIN_LEN * 2 {
let block = build_and_import_block_ext(
&client,
BlockOrigin::NetworkInitialSync,
Expand Down Expand Up @@ -1449,7 +1449,10 @@ fn find_potential_parents_aligned_with_late_pending() {
))
.unwrap();

assert_eq!(potential_parents.len(), max_depth + 1);
// `max_depth` starts from the pending candidate.
// So, we have at least the `included_block`, the `in_between_block` and the `pending_block`
// included in the `potential_parents`
assert_eq!(potential_parents.len(), max_depth + 3);
let expected_parents: Vec<_> = [&included_block, &in_between_block, &pending_block]
.into_iter()
.chain(aligned_blocks.iter())
Expand Down Expand Up @@ -1482,14 +1485,14 @@ fn find_potential_parents_aligned_with_late_pending() {
))
.unwrap();

let expected_len = 2 * max_depth + 1;
let expected_len = 2 * (max_depth + 2) + 1;
assert_eq!(potential_parents.len(), expected_len);
let expected_aligned: Vec<_> = [&included_block, &in_between_block, &pending_block]
.into_iter()
.chain(aligned_blocks.iter())
.take(max_depth + 1)
.take(max_depth + 3)
.collect();
let expected_alt = alt_blocks.iter().take(max_depth);
let expected_alt = alt_blocks.iter().take(max_depth + 2);

let expected_parents: Vec<_> =
expected_aligned.clone().into_iter().chain(expected_alt).collect();
Expand Down Expand Up @@ -1560,7 +1563,7 @@ fn find_potential_parents_aligned_with_pending() {
// Build two sibling chains from the included block.
let mut aligned_blocks = Vec::new();
let mut parent = pending_block.header().hash();
for _ in 1..NON_INCLUDED_CHAIN_LEN {
for _ in 1..NON_INCLUDED_CHAIN_LEN * 2 {
let block = build_and_import_block_ext(
&client,
BlockOrigin::Own,
Expand All @@ -1576,7 +1579,7 @@ fn find_potential_parents_aligned_with_pending() {

let mut alt_blocks = Vec::new();
let mut parent = included_block.header().hash();
for _ in 0..NON_INCLUDED_CHAIN_LEN {
for _ in 0..NON_INCLUDED_CHAIN_LEN * 2 {
let block = build_and_import_block_ext(
&client,
BlockOrigin::NetworkInitialSync,
Expand Down Expand Up @@ -1604,7 +1607,7 @@ fn find_potential_parents_aligned_with_pending() {
&relay_chain,
))
.unwrap();
assert_eq!(potential_parents.len(), max_depth + 1);
assert_eq!(potential_parents.len(), max_depth + 2);
let expected_parents: Vec<_> = [&included_block, &pending_block]
.into_iter()
.chain(aligned_blocks.iter())
Expand Down Expand Up @@ -1638,14 +1641,17 @@ fn find_potential_parents_aligned_with_pending() {
))
.unwrap();

let expected_len = 2 * max_depth + 1;
let expected_len = 2 * (max_depth + 1) + 1;
assert_eq!(potential_parents.len(), expected_len);
let expected_aligned: Vec<_> = [&included_block, &pending_block]
.into_iter()
.chain(aligned_blocks.iter())
.take(max_depth + 1)
// At least the `included_block` and `pending_block` are included.
.take(max_depth + 2)
.collect();
let expected_alt = alt_blocks.iter().take(max_depth);

// At least the block that is at the same height of `pending_block` is included.
let expected_alt = alt_blocks.iter().take(max_depth + 1);

let expected_parents: Vec<_> =
expected_aligned.clone().into_iter().chain(expected_alt).collect();
Expand Down Expand Up @@ -1765,3 +1771,64 @@ fn find_potential_parents_aligned_no_pending() {
assert_eq!(potential_parents, potential_parents_aligned);
}
}

/// Ensure that `max_depth` is counted from the pending block onwards.
#[test]
fn find_potential_parents_max_depth_from_pending() {
sp_tracing::try_init_simple();

const MAX_DEPTH: usize = 5;

let backend = Arc::new(Backend::new_test(1000, 1));
let client = Arc::new(TestClientBuilder::with_backend(backend.clone()).build());
let mut para_import = ParachainBlockImport::new(client.clone(), backend.clone());

let relay_parent = relay_hash_from_block_num(10);
// Choose different relay parent for alternative chain to get new hashes.
let search_relay_parent = relay_hash_from_block_num(11);
let included_block = client.hash(0).unwrap().unwrap();
let relay_chain = Relaychain::new();
{
let included_map = &mut relay_chain.inner.lock().unwrap().relay_chain_hash_to_header;
included_map.insert(search_relay_parent, client.header(included_block).unwrap().unwrap());
}

// Build a chain from the included block.
let mut parent = included_block;
for _ in 0..MAX_DEPTH * 2 {
let block = build_and_import_block_ext(
&client,
BlockOrigin::Own,
true,
&mut para_import,
Some(parent),
None,
Some(relay_parent),
);
parent = block.header().hash();
}

// The pending block is `MAX_DEPTH` blocks away from the included
{
let pending_map = &mut relay_chain.inner.lock().unwrap().relay_chain_hash_to_header_pending;
pending_map.insert(
search_relay_parent,
client.header(client.hash(MAX_DEPTH as u32).unwrap().unwrap()).unwrap().unwrap(),
);
}

let potential_parents = block_on(find_potential_parents(
ParentSearchParams {
relay_parent: search_relay_parent,
para_id: ParaId::from(100),
ancestry_lookback: 1, // aligned chain is in ancestry.
max_depth: MAX_DEPTH,
ignore_alternative_branches: true,
},
&*backend,
&relay_chain,
))
.unwrap();

assert_eq!(potential_parents.len(), 2 * MAX_DEPTH + 1);
}
Loading