diff --git a/cumulus/client/consensus/aura/src/collators/lookahead.rs b/cumulus/client/consensus/aura/src/collators/lookahead.rs index 7bce12a31618b..789cb78a8a4e3 100644 --- a/cumulus/client/consensus/aura/src/collators/lookahead.rs +++ b/cumulus/client/consensus/aura/src/collators/lookahead.rs @@ -63,7 +63,10 @@ use sp_consensus_aura::{AuraApi, Slot}; use sp_core::crypto::Pair; use sp_inherents::CreateInherentDataProviders; use sp_keystore::KeystorePtr; -use sp_runtime::traits::{Block as BlockT, Header as HeaderT, Member}; +use sp_runtime::{ + traits::{Block as BlockT, Header as HeaderT, Member}, + Saturating, +}; use sp_timestamp::Timestamp; use std::{path::PathBuf, sync::Arc, time::Duration}; @@ -314,7 +317,7 @@ where }, }; - let (included_block, initial_parent) = match crate::collators::find_parent( + let parent_search_result = match crate::collators::find_parent( relay_parent, params.para_id, &*params.para_backend, @@ -322,10 +325,11 @@ where ) .await { - Some(value) => value, + Some(result) => result, None => continue, }; + let included_header = &parent_search_result.included_header; let para_client = &*params.para_client; let keystore = ¶ms.keystore; let can_build_upon = |block_hash| { @@ -341,7 +345,7 @@ where relay_slot, timestamp, block_hash, - included_block.hash(), + included_header.hash(), para_client, &keystore, )) @@ -349,8 +353,11 @@ where // Build in a loop until not allowed. Note that the authorities can change // at any block, so we need to re-claim our slot every time. - let mut parent_hash = initial_parent.hash; - let mut parent_header = initial_parent.header; + let mut parent_hash = parent_search_result.best_parent_header.hash(); + let mut parent_header = parent_search_result.best_parent_header; + // Distance from included block to best parent. + let initial_parent_depth = + (*parent_header.number()).saturating_sub(*included_header.number()); let overseer_handle = &mut params.overseer_handle; // Do not try to build upon an unknown, pruned or bad block @@ -373,7 +380,7 @@ where // This needs to change to support elastic scaling, but for continuously // scheduled chains this ensures that the backlog will grow steadily. - for n_built in 0..2 { + for n_built in 0..2u32 { let slot_claim = match can_build_upon(parent_hash) { Some(fut) => match fut.await { None => break, @@ -385,7 +392,7 @@ where tracing::debug!( target: crate::LOG_TARGET, ?relay_parent, - unincluded_segment_len = initial_parent.depth + n_built, + unincluded_segment_len = ?initial_parent_depth.saturating_add(n_built.into()), "Slot claimed. Building" ); diff --git a/cumulus/client/consensus/aura/src/collators/mod.rs b/cumulus/client/consensus/aura/src/collators/mod.rs index 5af3ac81465f2..966bb62af6655 100644 --- a/cumulus/client/consensus/aura/src/collators/mod.rs +++ b/cumulus/client/consensus/aura/src/collators/mod.rs @@ -257,15 +257,14 @@ where .then(|| SlotClaim::unchecked::

(author_pub, para_slot, timestamp)) } -/// Use [`cumulus_client_consensus_common::find_potential_parents`] to find parachain blocks that -/// we can build on. Once a list of potential parents is retrieved, return the last one of the -/// longest chain. +/// Use [`cumulus_client_consensus_common::find_parent_for_building`] to find the best parachain +/// block to build on. async fn find_parent( relay_parent: RelayHash, para_id: ParaId, para_backend: &impl sc_client_api::Backend, relay_client: &impl RelayChainInterface, -) -> Option<(::Header, consensus_common::PotentialParent)> +) -> Option> where Block: BlockT, { @@ -276,35 +275,26 @@ where .await .unwrap_or(DEFAULT_SCHEDULING_LOOKAHEAD) .saturating_sub(1) as usize, - ignore_alternative_branches: true, }; - let potential_parents = cumulus_client_consensus_common::find_potential_parents::( + match cumulus_client_consensus_common::find_parent_for_building::( parent_search_params, para_backend, relay_client, ) - .await; - - let potential_parents = match potential_parents { + .await + { + Ok(result) => result, Err(e) => { tracing::error!( target: crate::LOG_TARGET, ?relay_parent, err = ?e, - "Could not fetch potential parents to build upon" + "Could not find parent to build upon" ); - - return None; + None }, - Ok(x) => x, - }; - - let included_block = potential_parents.iter().find(|x| x.depth == 0)?.header.clone(); - potential_parents - .into_iter() - .max_by_key(|a| a.depth) - .map(|parent| (included_block, parent)) + } } #[cfg(test)] diff --git a/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs b/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs index 816c6c39f9135..e11e555aea21c 100644 --- a/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs +++ b/cumulus/client/consensus/aura/src/collators/slot_based/block_builder_task.rs @@ -54,7 +54,10 @@ use sp_consensus_aura::AuraApi; use sp_core::crypto::Pair; use sp_inherents::CreateInherentDataProviders; use sp_keystore::KeystorePtr; -use sp_runtime::traits::{Block as BlockT, Header as HeaderT, Member, Zero}; +use sp_runtime::{ + traits::{Block as BlockT, Header as HeaderT, Member, Zero}, + Saturating, +}; use std::{collections::VecDeque, sync::Arc, time::Duration}; /// Parameters for [`run_block_builder`]. @@ -237,15 +240,19 @@ where let relay_parent = rp_data.relay_parent().hash(); let relay_parent_header = rp_data.relay_parent().clone(); - let Some((included_header, parent)) = + let Some(parent_search_result) = crate::collators::find_parent(relay_parent, para_id, &*para_backend, &relay_client) .await else { continue; }; - let parent_hash = parent.hash; - let parent_header = &parent.header; + let parent_hash = parent_search_result.best_parent_header.hash(); + let included_header = parent_search_result.included_header; + let parent_header = &parent_search_result.best_parent_header; + // Distance from included block to best parent (unincluded segment length). + let unincluded_segment_len = + parent_header.number().saturating_sub(*included_header.number()); // Retrieve the core. let core = match determine_core( @@ -329,7 +336,7 @@ where None => { tracing::debug!( target: crate::LOG_TARGET, - unincluded_segment_len = parent.depth, + ?unincluded_segment_len, relay_parent = ?relay_parent, relay_parent_num = %relay_parent_header.number(), included_hash = ?included_header_hash, @@ -344,7 +351,7 @@ where tracing::debug!( target: crate::LOG_TARGET, - unincluded_segment_len = parent.depth, + ?unincluded_segment_len, relay_parent = %relay_parent, relay_parent_num = %relay_parent_header.number(), relay_parent_offset, @@ -417,7 +424,7 @@ where let Some(adjusted_authoring_duration) = adjusted_authoring_duration else { tracing::debug!( target: crate::LOG_TARGET, - unincluded_segment_len = parent.depth, + ?unincluded_segment_len, relay_parent = ?relay_parent, relay_parent_num = %relay_parent_header.number(), included_hash = ?included_header_hash, diff --git a/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs b/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs index 71d1f5d931904..89d7dd50bee30 100644 --- a/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs +++ b/cumulus/client/consensus/aura/src/collators/slot_based/mod.rs @@ -29,8 +29,8 @@ //! //! 1. Awaits the next production signal from the internal timer //! 2. Retrieves the current best relay chain block and identifies a valid parent block (see -//! [find_potential_parents][cumulus_client_consensus_common::find_potential_parents] for parent -//! selection criteria) +//! [find_parent_for_building][cumulus_client_consensus_common::find_parent_for_building] for +//! parent selection criteria) //! 3. Validates that: //! - The parachain has an assigned core on the relay chain //! - No block has been previously built on the target core diff --git a/cumulus/client/consensus/common/src/parent_search.rs b/cumulus/client/consensus/common/src/parent_search.rs index 078383c067193..1a469d23fde21 100644 --- a/cumulus/client/consensus/common/src/parent_search.rs +++ b/cumulus/client/consensus/common/src/parent_search.rs @@ -26,11 +26,11 @@ use cumulus_relay_chain_interface::{RelayChainError, RelayChainInterface}; use sc_client_api::{Backend, HeaderBackend}; -use sp_blockchain::{Backend as BlockchainBackend, TreeRoute}; +use sp_blockchain::Backend as BlockchainBackend; use sp_runtime::traits::{Block as BlockT, Header as HeaderT}; -const PARENT_SEARCH_LOG_TARGET: &str = "consensus::common::find_potential_parents"; +const LOG_TARGET: &str = "consensus::common::parent_search"; /// Parameters when searching for suitable parents to build on top of. #[derive(Debug)] @@ -42,72 +42,51 @@ 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, - /// Whether to only ignore "alternative" branches, i.e. branches of the chain - /// which do not contain the block pending availability. - pub ignore_alternative_branches: bool, } -/// A potential parent block returned from [`find_potential_parents`] -#[derive(PartialEq)] -pub struct PotentialParent { - /// The hash of the block. - pub hash: B::Hash, - /// The header of the block. - pub header: B::Header, - /// The depth of the block with respect to the included block. - pub depth: usize, - /// Whether the block is the included block, is itself pending on-chain, or descends - /// from the block pending availability. - pub aligned_with_pending: bool, +/// Result of the parent search, containing the included block and the best parent to build on. +pub struct ParentSearchResult { + /// The header of the included block (confirmed on relay chain). + pub included_header: B::Header, + /// The header of the best parent block to build on. + pub best_parent_header: B::Header, } -impl std::fmt::Debug for PotentialParent { +impl std::fmt::Debug for ParentSearchResult { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("PotentialParent") - .field("hash", &self.hash) - .field("depth", &self.depth) - .field("aligned_with_pending", &self.aligned_with_pending) - .field("number", &self.header.number()) + f.debug_struct("ParentSearchResult") + .field("included_number", &self.included_header.number()) + .field("best_parent_hash", &self.best_parent_header.hash()) + .field("best_parent_number", &self.best_parent_header.number()) .finish() } } -/// Perform a recursive search through blocks to find potential -/// parent blocks for a new block. +/// Find the best parent block to build on. /// -/// This accepts a relay-chain block to be used as an anchor along with some arguments for -/// filtering parachain blocks and performs a recursive search for parachain blocks. -/// The search begins at the last included parachain block and returns a set of -/// [`PotentialParent`]s which could be potential parents of a new block with this -/// relay-parent according to the search parameters. +/// This accepts a relay-chain block to be used as an anchor and searches for the best +/// parachain block to use as a parent for a new block. /// -/// A parachain block is a potential parent if it is either the last included parachain block, the -/// 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. -pub async fn find_potential_parents( +/// The search starts from either the pending block (if one exists) or the included block, +/// and finds the deepest descendant whose relay-parent is within the allowed ancestry. +/// +/// Returns `None` if no suitable parent can be found (e.g., included block unknown locally). +pub async fn find_parent_for_building( params: ParentSearchParams, backend: &impl Backend, relay_client: &impl RelayChainInterface, -) -> Result>, RelayChainError> { +) -> Result>, RelayChainError> { tracing::trace!("Parent search parameters: {params:?}"); + // Get the included block. let Some((included_header, included_hash)) = fetch_included_from_relay_chain(relay_client, backend, params.para_id, params.relay_parent) .await? else { - return Ok(Default::default()); + return Ok(None); }; - let only_included = vec![PotentialParent { - hash: included_hash, - header: included_header.clone(), - depth: 0, - aligned_with_pending: true, - }]; - - // Pending header and hash. + // Fetch the pending block if one exists. let maybe_pending = { // Fetch the most recent pending header from the relay chain. We use // `OccupiedCoreAssumption::Included` so the candidate pending availability gets enacted @@ -122,80 +101,39 @@ pub async fn find_potential_parents( .and_then(|p| B::Header::decode(&mut &p.parent_head.0[..]).ok()) .filter(|x| x.hash() != included_hash); - // If the pending block is not locally known, we can't do anything. + // If the pending block is not locally known, we can't proceed. if let Some(header) = pending_header { let pending_hash = header.hash(); - match backend.blockchain().header(pending_hash) { - // We are supposed to ignore branches that don't contain the pending block, but we - // do not know the pending block locally. - Ok(None) | Err(_) if params.ignore_alternative_branches => { - tracing::warn!( - target: PARENT_SEARCH_LOG_TARGET, - %pending_hash, - "Failed to get header for pending block.", - ); - return Ok(Default::default()); - }, - Ok(Some(_)) => Some((header, pending_hash)), - _ => None, - } + let Ok(Some(header)) = backend.blockchain().header(pending_hash) else { + tracing::warn!( + target: LOG_TARGET, + %pending_hash, + "Failed to get header for pending block.", + ); + return Ok(None); + }; + Some((header, pending_hash)) } else { None } }; - let maybe_route_to_last_pending = maybe_pending - .as_ref() - .map(|(_, pending)| { - sp_blockchain::tree_route(backend.blockchain(), included_hash, *pending) - }) - .transpose()?; - - // 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. - let (frontier, potential_parents) = match ( - &maybe_pending, - params.ignore_alternative_branches, - &maybe_route_to_last_pending, - ) { - (Some((pending_header, pending_hash)), true, Some(ref route_to_pending)) => { - let mut potential_parents = only_included; - - // This is a defensive check, should never happen. - if !route_to_pending.retracted().is_empty() { - tracing::warn!(target: PARENT_SEARCH_LOG_TARGET, "Included block not an ancestor of pending block. This should not happen."); - return Ok(Default::default()); + // Determine the starting point for the search. + let (start_hash, start_header) = match &maybe_pending { + Some((pending_header, pending_hash)) => { + // Verify pending is a descendant of included. + let route = + sp_blockchain::tree_route(backend.blockchain(), included_hash, *pending_hash)?; + if !route.retracted().is_empty() { + tracing::warn!( + target: LOG_TARGET, + "Included block not an ancestor of pending block. This should not happen." + ); + return Ok(None); } - - // 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() - { - let Ok(Some(header)) = backend.blockchain().header(block.hash) else { continue }; - - potential_parents.push(PotentialParent { - hash: block.hash, - header, - depth: 1 + num, - aligned_with_pending: true, - }); - } - - // The search for additional potential parents should now start at the children of - // the pending block. - ( - vec![PotentialParent { - hash: *pending_hash, - header: pending_header.clone(), - depth: route_to_pending.enacted().len(), - aligned_with_pending: true, - }], - potential_parents, - ) + (*pending_hash, pending_header.clone()) }, - _ => (only_included, Default::default()), + None => (included_hash, included_header.clone()), }; // Build up the ancestry record of the relay chain to compare against. @@ -203,16 +141,11 @@ pub async fn find_potential_parents( build_relay_parent_ancestry(params.ancestry_lookback, params.relay_parent, relay_client) .await?; - Ok(search_child_branches_for_parents( - frontier, - maybe_route_to_last_pending, - included_header, - maybe_pending.map(|(_, hash)| hash), - backend, - params.ignore_alternative_branches, - rp_ancestry, - potential_parents, - )) + // Search for the deepest valid parent starting from the pending/included block. + let best_parent_header = + find_deepest_valid_parent(start_hash, start_header, backend, &rp_ancestry); + + Ok(Some(ParentSearchResult { included_header, best_parent_header })) } /// Fetch the included block from the relay chain. @@ -241,19 +174,10 @@ async fn fetch_included_from_relay_chain( let included_hash = included_header.hash(); // If the included block is not locally known, we can't do anything. match backend.blockchain().header(included_hash) { - Ok(None) => { - tracing::warn!( - target: PARENT_SEARCH_LOG_TARGET, - %included_hash, - "Failed to get header for included block.", - ); - return Ok(None); - }, - Err(e) => { + Ok(None) | Err(_) => { tracing::warn!( - target: PARENT_SEARCH_LOG_TARGET, + target: LOG_TARGET, %included_hash, - %e, "Failed to get header for included block.", ); return Ok(None); @@ -301,101 +225,60 @@ async fn build_relay_parent_ancestry( Ok(ancestry) } -/// Start search for child blocks that can be used as parents. -pub fn search_child_branches_for_parents( - mut frontier: Vec>, - maybe_route_to_last_pending: Option>, - included_header: Block::Header, - pending_hash: Option, +/// Find the deepest valid parent block starting from `start`. +/// +/// The `start` block (pending or included) is always valid by construction. +/// This function explores its descendants via DFS, returning the deepest block +/// whose relay-parent is within the allowed ancestry. +fn find_deepest_valid_parent( + start_hash: Block::Hash, + start_header: Block::Header, backend: &impl Backend, - ignore_alternative_branches: bool, - rp_ancestry: Vec<(RelayHash, RelayHash)>, - mut potential_parents: Vec>, -) -> Vec> { - let included_hash = included_header.hash(); - let is_hash_in_ancestry = |hash| rp_ancestry.iter().any(|x| x.0 == hash); - let is_root_in_ancestry = |root| rp_ancestry.iter().any(|x| x.1 == root); - - // The distance between pending and included block. Is later used to check if a child - // is aligned with pending when it is between pending and included block. - let pending_distance = maybe_route_to_last_pending.as_ref().map(|route| route.enacted().len()); - - // If a block is on the path included -> pending, we consider it `aligned_with_pending`. - let is_child_pending = |hash| { - maybe_route_to_last_pending - .as_ref() - .map_or(true, |route| route.enacted().iter().any(|x| x.hash == hash)) - }; + rp_ancestry: &[(RelayHash, RelayHash)], +) -> Block::Header { + let mut best = start_header; + + let mut frontier: Vec = + backend.blockchain().children(start_hash).ok().into_iter().flatten().collect(); tracing::trace!( - target: PARENT_SEARCH_LOG_TARGET, - ?included_hash, - included_num = ?included_header.number(), - ?pending_hash , - ?rp_ancestry, - "Searching relay chain ancestry." + target: LOG_TARGET, + ?start_hash, + num_children = frontier.len(), + "Searching for deepest valid parent." ); - while let Some(entry) = frontier.pop() { - let is_pending = pending_hash.as_ref().map_or(false, |h| &entry.hash == h); - let is_included = included_hash == entry.hash; - - // Note: even if the pending block or included block have a relay parent - // outside of the expected part of the relay chain, they are always allowed - // because they have already been posted on chain. - let is_potential = is_pending || is_included || { - let digest = entry.header.digest(); - let is_hash_in_ancestry_check = cumulus_primitives_core::extract_relay_parent(digest) - .map_or(false, is_hash_in_ancestry); - let is_root_in_ancestry_check = - cumulus_primitives_core::rpsr_digest::extract_relay_parent_storage_root(digest) - .map(|(r, _n)| r) - .map_or(false, is_root_in_ancestry); - - is_hash_in_ancestry_check || is_root_in_ancestry_check - }; - - let parent_aligned_with_pending = entry.aligned_with_pending; - let child_depth = entry.depth + 1; - let hash = entry.hash; - - tracing::trace!( - target: PARENT_SEARCH_LOG_TARGET, - ?hash, - is_potential, - is_pending, - is_included, - "Checking potential parent." - ); - - if is_potential { - potential_parents.push(entry); - } else { - continue; - } - - // Push children onto search frontier. - for child in backend.blockchain().children(hash).ok().into_iter().flatten() { - tracing::trace!(target: PARENT_SEARCH_LOG_TARGET, ?child, child_depth, ?pending_distance, "Looking at child."); - let aligned_with_pending = parent_aligned_with_pending && - (pending_distance.map_or(true, |dist| child_depth > dist) || - is_child_pending(child)); + while let Some(hash) = frontier.pop() { + let Ok(Some(header)) = backend.blockchain().header(hash) else { continue }; - if ignore_alternative_branches && !aligned_with_pending { - tracing::trace!(target: PARENT_SEARCH_LOG_TARGET, ?child, "Child is not aligned with pending block."); - continue; - } - - let Ok(Some(header)) = backend.blockchain().header(child) else { continue }; + if !is_relay_parent_in_ancestry::(&header, rp_ancestry) { + continue; + } - frontier.push(PotentialParent { - hash: child, - header, - depth: child_depth, - aligned_with_pending, - }); + // This block is valid - update best if it's deeper. + if header.number() > best.number() { + best = header.clone(); } + + frontier.extend(backend.blockchain().children(hash).ok().into_iter().flatten()); } - potential_parents + best +} + +/// Check if a block's relay parent is within the allowed ancestry. +fn is_relay_parent_in_ancestry( + header: &Block::Header, + rp_ancestry: &[(RelayHash, RelayHash)], +) -> bool { + let digest = header.digest(); + let relay_parent = cumulus_primitives_core::extract_relay_parent(digest); + let storage_root = + cumulus_primitives_core::rpsr_digest::extract_relay_parent_storage_root(digest) + .map(|(r, _)| r); + + rp_ancestry.iter().any(|(rp_hash, rp_storage_root)| { + relay_parent.map_or(false, |rp| *rp_hash == rp) || + storage_root.map_or(false, |sr| *rp_storage_root == sr) + }) } diff --git a/cumulus/client/consensus/common/src/tests.rs b/cumulus/client/consensus/common/src/tests.rs index c26bbe0ba6ef8..d82ccd7654503 100644 --- a/cumulus/client/consensus/common/src/tests.rs +++ b/cumulus/client/consensus/common/src/tests.rs @@ -966,8 +966,9 @@ fn restore_limit_monitor() { assert_eq!(*monitor.freshness.get(&block13.header.hash()).unwrap(), monitor.import_counter); } +/// Tests that the best parent is found within allowed ancestry. #[test] -fn find_potential_parents_in_allowed_ancestry() { +fn find_best_parent_in_allowed_ancestry() { sp_tracing::try_init_simple(); let backend = Arc::new(Backend::new_test(1000, 1)); @@ -975,7 +976,7 @@ fn find_potential_parents_in_allowed_ancestry() { let mut para_import = ParachainBlockImport::new(client.clone(), backend.clone()); let relay_parent = relay_hash_from_block_num(10); - let block = build_and_import_block_ext( + let included_block = build_and_import_block_ext( &client, BlockOrigin::Own, true, @@ -988,84 +989,75 @@ fn find_potential_parents_in_allowed_ancestry() { let relay_chain = Relaychain::new(); { let included_map = &mut relay_chain.inner.lock().unwrap().relay_chain_hash_to_header; - included_map.insert(relay_parent, block.header().clone()); + included_map.insert(relay_parent, included_block.header().clone()); } - let potential_parents = block_on(find_potential_parents( - ParentSearchParams { - relay_parent, - para_id: ParaId::from(100), - ancestry_lookback: 0, - ignore_alternative_branches: true, - }, + // When there's only the included block, it should be the best parent. + let result = block_on(find_parent_for_building( + ParentSearchParams { relay_parent, para_id: ParaId::from(100), ancestry_lookback: 0 }, &*backend, &relay_chain, )) - .unwrap(); - assert_eq!(potential_parents.len(), 1); - let parent = &potential_parents[0]; + .unwrap() + .expect("Should find a parent"); - assert_eq!(parent.hash, block.hash()); - assert_eq!(&parent.header, block.header()); - assert_eq!(parent.depth, 0); - assert!(parent.aligned_with_pending); + assert_eq!(result.best_parent_header.hash(), included_block.hash()); + assert_eq!(&result.best_parent_header, included_block.header()); + assert_eq!(&result.included_header, included_block.header()); - // New block is not pending or included. + // Add a child block with a different relay parent. let block_relay_parent = relay_hash_from_block_num(11); let search_relay_parent = relay_hash_from_block_num(13); { let included_map = &mut relay_chain.inner.lock().unwrap().relay_chain_hash_to_header; - included_map.insert(search_relay_parent, block.header().clone()); + included_map.insert(search_relay_parent, included_block.header().clone()); } - let block = build_and_import_block_ext( + let child_block = build_and_import_block_ext( &client, BlockOrigin::Own, true, &mut para_import, - Some(block.header().hash()), + Some(included_block.header().hash()), None, Some(block_relay_parent), ); - let potential_parents = block_on(find_potential_parents( + + // With ancestry_lookback: 2, the child block should be the best parent. + let result = block_on(find_parent_for_building( ParentSearchParams { relay_parent: search_relay_parent, para_id: ParaId::from(100), ancestry_lookback: 2, - ignore_alternative_branches: true, }, &*backend, &relay_chain, )) - .unwrap(); + .unwrap() + .expect("Should find a parent"); - assert_eq!(potential_parents.len(), 2); - let parent = &potential_parents[1]; + assert_eq!(result.best_parent_header.hash(), child_block.hash()); + assert_eq!(&result.best_parent_header, child_block.header()); - assert_eq!(parent.hash, block.hash()); - assert_eq!(&parent.header, block.header()); - assert_eq!(parent.depth, 1); - assert!(parent.aligned_with_pending); - - // Reduce allowed ancestry. - let potential_parents = block_on(find_potential_parents( + // With ancestry_lookback: 0, child block's relay parent is too old, + // so included block should be the best parent. + let result = block_on(find_parent_for_building( ParentSearchParams { relay_parent: search_relay_parent, para_id: ParaId::from(100), - ancestry_lookback: 1, - ignore_alternative_branches: true, + ancestry_lookback: 0, }, &*backend, &relay_chain, )) - .unwrap(); - assert_eq!(potential_parents.len(), 1); - let parent = &potential_parents[0]; - assert_ne!(parent.hash, block.hash()); + .unwrap() + .expect("Should find a parent"); + + assert_eq!(result.best_parent_header.hash(), included_block.hash()); } -/// Tests that pending availability block is always potential parent. +/// Tests that pending availability block is used as starting point for search. #[test] -fn find_potential_pending_parent() { +fn find_best_parent_with_pending() { sp_tracing::try_init_simple(); let backend = Arc::new(Backend::new_test(1000, 1)); @@ -1105,41 +1097,32 @@ fn find_potential_pending_parent() { .insert(search_relay_parent, pending_block.header().clone()); } - let potential_parents = block_on(find_potential_parents( + let result = block_on(find_parent_for_building( ParentSearchParams { relay_parent: search_relay_parent, para_id: ParaId::from(100), ancestry_lookback: 0, - ignore_alternative_branches: true, }, &*backend, &relay_chain, )) - .unwrap(); - assert_eq!(potential_parents.len(), 2); - let included_parent = &potential_parents[0]; - - assert_eq!(included_parent.hash, included_block.hash()); - assert_eq!(&included_parent.header, included_block.header()); - assert_eq!(included_parent.depth, 0); - assert!(included_parent.aligned_with_pending); - - let pending_parent = &potential_parents[1]; - - assert_eq!(pending_parent.hash, pending_block.hash()); - assert_eq!(&pending_parent.header, pending_block.header()); - assert_eq!(pending_parent.depth, 1); - assert!(pending_parent.aligned_with_pending); + .unwrap() + .expect("Should find a parent"); + + // Best parent should be the pending block. + assert_eq!(result.best_parent_header.hash(), pending_block.hash()); + assert_eq!(&result.best_parent_header, pending_block.header()); + // Included header should be the included block. + assert_eq!(&result.included_header, included_block.header()); } #[test] -fn find_potential_parents_unknown_included() { +fn find_best_parent_unknown_included_returns_none() { sp_tracing::try_init_simple(); let backend = Arc::new(Backend::new_test(1000, 1)); let client = Arc::new(TestClientBuilder::with_backend(backend.clone()).build()); 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 sproof = sproof_with_best_parent(&client); @@ -1153,24 +1136,22 @@ fn find_potential_parents_unknown_included() { .insert(search_relay_parent, included_but_unknown.header().clone()); } - // Ignore alternative branch: - let potential_parents = block_on(find_potential_parents( + let result = block_on(find_parent_for_building( ParentSearchParams { relay_parent: search_relay_parent, para_id: ParaId::from(100), - ancestry_lookback: 1, // aligned chain is in ancestry. - ignore_alternative_branches: true, + ancestry_lookback: 1, }, &*backend, &relay_chain, )) .unwrap(); - assert_eq!(potential_parents.len(), 0); + assert!(result.is_none()); } #[test] -fn find_potential_parents_unknown_pending() { +fn find_best_parent_unknown_pending_returns_none() { sp_tracing::try_init_simple(); let backend = Arc::new(Backend::new_test(1000, 1)); @@ -1179,7 +1160,6 @@ fn find_potential_parents_unknown_pending() { ParachainBlockImport::new_with_delayed_best_block(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 = build_and_import_block_ext( &client, @@ -1211,24 +1191,23 @@ fn find_potential_parents_unknown_pending() { .insert(search_relay_parent, pending_but_unknown.header().clone()); } - // Ignore alternative branch: - let potential_parents = block_on(find_potential_parents( + let result = block_on(find_parent_for_building( ParentSearchParams { relay_parent: search_relay_parent, para_id: ParaId::from(100), - ancestry_lookback: 1, // aligned chain is in ancestry. - ignore_alternative_branches: true, + ancestry_lookback: 1, }, &*backend, &relay_chain, )) .unwrap(); - assert!(potential_parents.is_empty()); + assert!(result.is_none()); } +/// Tests that the deepest block is found when there are multiple forks. #[test] -fn find_potential_parents_unknown_pending_include_alternative_branches() { +fn find_best_parent_with_forks_returns_deepest() { sp_tracing::try_init_simple(); let backend = Arc::new(Backend::new_test(1000, 1)); @@ -1237,10 +1216,7 @@ fn find_potential_parents_unknown_pending_include_alternative_branches() { ParachainBlockImport::new_with_delayed_best_block(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 search_relay_parent = relay_hash_from_block_num(20); let included_block = build_and_import_block_ext( &client, BlockOrigin::NetworkInitialSync, @@ -1250,25 +1226,14 @@ fn find_potential_parents_unknown_pending_include_alternative_branches() { None, Some(relay_parent), ); - - let alt_block = build_and_import_block_ext( + let pending_block = build_and_import_block_ext( &client, - BlockOrigin::NetworkInitialSync, + BlockOrigin::Own, true, &mut para_import, Some(included_block.header().hash()), None, - Some(search_relay_parent), - ); - - tracing::info!(hash = %alt_block.header().hash(), "Alt block."); - let sproof = sproof_with_parent_by_hash(&client, included_block.header().hash()); - let pending_but_unknown = build_block( - &*client, - sproof, - Some(included_block.header().hash()), - None, - Some(relay_parent), + Some(relay_hash_from_block_num(11)), ); let relay_chain = Relaychain::new(); @@ -1279,190 +1244,99 @@ fn find_potential_parents_unknown_pending_include_alternative_branches() { .insert(search_relay_parent, included_block.header().clone()); relay_inner .relay_chain_hash_to_header_pending - .insert(search_relay_parent, pending_but_unknown.header().clone()); + .insert(search_relay_parent, pending_block.header().clone()); } - // Ignore alternative branch: - 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. - ignore_alternative_branches: false, - }, - &*backend, - &relay_chain, - )) - .unwrap(); - - let expected_parents: Vec<_> = vec![&included_block, &alt_block]; - assert_eq!(potential_parents.len(), 2); - assert_eq!(expected_parents[0].hash(), potential_parents[0].hash); - assert_eq!(expected_parents[1].hash(), potential_parents[1].hash); -} - -/// Test where there are multiple pending blocks. -#[test] -fn find_potential_parents_aligned_with_late_pending() { - sp_tracing::try_init_simple(); - - const NON_INCLUDED_CHAIN_LEN: 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_with_delayed_best_block(client.clone(), backend.clone()); + // Build forked chains from pending using different relay parents to create distinct blocks: + // + // included -> pending -> fork1_block1 -> fork1_block2 (depth 2) + // \-> fork2_block1 -> fork2_block2 -> fork2_block3 (depth 3, deepest) + // \-> fork3_block1 (depth 1) - 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 = build_and_import_block_ext( + // Fork 1: depth 2 + let fork1_block1 = build_and_import_block_ext( &client, - BlockOrigin::NetworkInitialSync, - true, + BlockOrigin::Own, + false, &mut para_import, + Some(pending_block.header().hash()), None, + Some(relay_hash_from_block_num(12)), + ); + let _fork1_block2 = build_and_import_block_ext( + &client, + BlockOrigin::Own, + false, + &mut para_import, + Some(fork1_block1.header().hash()), None, - Some(relay_parent), + Some(relay_hash_from_block_num(13)), ); - let in_between_block = build_and_import_block_ext( + // Fork 2: depth 3 (deepest) + let fork2_block1 = build_and_import_block_ext( &client, - BlockOrigin::NetworkInitialSync, - true, + BlockOrigin::Own, + false, &mut para_import, - Some(included_block.header().hash()), + Some(pending_block.header().hash()), None, - Some(relay_parent), + Some(relay_hash_from_block_num(14)), ); - - let pending_block = build_and_import_block_ext( + let fork2_block2 = build_and_import_block_ext( &client, BlockOrigin::Own, - true, + false, &mut para_import, - Some(in_between_block.header().hash()), + Some(fork2_block1.header().hash()), None, - Some(relay_parent), + Some(relay_hash_from_block_num(15)), + ); + let fork2_block3 = build_and_import_block_ext( + &client, + BlockOrigin::Own, + false, + &mut para_import, + Some(fork2_block2.header().hash()), + None, + Some(relay_hash_from_block_num(16)), ); - let relay_chain = Relaychain::new(); - { - let relay_inner = &mut relay_chain.inner.lock().unwrap(); - relay_inner - .relay_chain_hash_to_header - .insert(search_relay_parent, included_block.header().clone()); - relay_inner - .relay_chain_hash_to_header_pending - .insert(search_relay_parent, in_between_block.header().clone()); - relay_inner - .relay_chain_hash_to_header_pending - .insert(search_relay_parent, pending_block.header().clone()); - } - - // Build some blocks on the pending block and on the included block. - // We end up with two sibling chains, one is aligned with the pending block, - // the other is not. - let mut aligned_blocks = Vec::new(); - let mut parent = pending_block.header().hash(); - for _ in 2..NON_INCLUDED_CHAIN_LEN { - let block = build_and_import_block_ext( - &client, - BlockOrigin::Own, - true, - &mut para_import, - Some(parent), - None, - Some(relay_parent), - ); - parent = block.header().hash(); - aligned_blocks.push(block); - } - - let mut alt_blocks = Vec::new(); - let mut parent = included_block.header().hash(); - for _ in 0..NON_INCLUDED_CHAIN_LEN { - let block = build_and_import_block_ext( - &client, - BlockOrigin::NetworkInitialSync, - true, - &mut para_import, - Some(parent), - None, - Some(search_relay_parent), - ); - parent = block.header().hash(); - alt_blocks.push(block); - } - - // Ignore alternative branch: - 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. - ignore_alternative_branches: true, - }, - &*backend, - &relay_chain, - )) - .unwrap(); - - let expected_parents: Vec<_> = [&included_block, &in_between_block, &pending_block] - .into_iter() - .chain(aligned_blocks.iter()) - .collect(); - - assert_eq!(potential_parents.len(), expected_parents.len()); - for (i, (parent, expected)) in potential_parents.iter().zip(expected_parents.iter()).enumerate() - { - assert_eq!(parent.hash, expected.hash()); - assert_eq!(&parent.header, expected.header()); - assert_eq!(parent.depth, i); - assert!(parent.aligned_with_pending); - } + // Fork 3: depth 1 + let _fork3_block1 = build_and_import_block_ext( + &client, + BlockOrigin::Own, + false, + &mut para_import, + Some(pending_block.header().hash()), + None, + Some(relay_hash_from_block_num(17)), + ); - // Do not ignore: - let potential_parents = block_on(find_potential_parents( + let result = block_on(find_parent_for_building( ParentSearchParams { relay_parent: search_relay_parent, para_id: ParaId::from(100), - ancestry_lookback: 1, // aligned chain is in ancestry. - ignore_alternative_branches: false, + ancestry_lookback: 10, }, &*backend, &relay_chain, )) - .unwrap(); - - let expected_aligned: Vec<_> = [&included_block, &in_between_block, &pending_block] - .into_iter() - .chain(aligned_blocks.iter()) - .collect(); - - let expected_parents: Vec<_> = - expected_aligned.clone().into_iter().chain(alt_blocks.iter()).collect(); + .unwrap() + .expect("Should find a parent"); - assert_eq!(potential_parents.len(), expected_parents.len()); - for parent in potential_parents.iter() { - let expected = expected_parents - .iter() - .find(|block| block.header().hash() == parent.hash) - .expect("missing parent"); - - let is_aligned = expected_aligned.contains(&expected); - - assert_eq!(parent.hash, expected.hash()); - assert_eq!(&parent.header, expected.header()); - assert_eq!(parent.aligned_with_pending, is_aligned); - } + // The deepest block (fork2_block3) should be the best parent. + assert_eq!(result.best_parent_header.hash(), fork2_block3.hash()); + assert_eq!(&result.best_parent_header, fork2_block3.header()); + assert_eq!(&result.included_header, included_block.header()); } +/// Tests that the deepest block in a chain is returned as best parent. #[test] -fn find_potential_parents_aligned_with_pending() { +fn find_best_parent_returns_deepest_block() { sp_tracing::try_init_simple(); - const NON_INCLUDED_CHAIN_LEN: usize = 5; + const CHAIN_LEN: usize = 5; let backend = Arc::new(Backend::new_test(1000, 1)); let client = Arc::new(TestClientBuilder::with_backend(backend.clone()).build()); @@ -1470,7 +1344,6 @@ fn find_potential_parents_aligned_with_pending() { ParachainBlockImport::new_with_delayed_best_block(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 = build_and_import_block_ext( &client, @@ -1502,262 +1375,35 @@ fn find_potential_parents_aligned_with_pending() { .insert(search_relay_parent, pending_block.header().clone()); } - // 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 { + // Build a chain on top of the pending block. + let mut last_block = pending_block; + for _ in 1..CHAIN_LEN { let block = build_and_import_block_ext( &client, BlockOrigin::Own, true, &mut para_import, - Some(parent), + Some(last_block.header().hash()), None, Some(relay_parent), ); - parent = block.header().hash(); - aligned_blocks.push(block); + last_block = block; } - let mut alt_blocks = Vec::new(); - let mut parent = included_block.header().hash(); - for _ in 0..NON_INCLUDED_CHAIN_LEN { - let block = build_and_import_block_ext( - &client, - BlockOrigin::NetworkInitialSync, - true, - &mut para_import, - Some(parent), - None, - Some(search_relay_parent), - ); - parent = block.header().hash(); - alt_blocks.push(block); - } - - // Ignore alternative branch: - 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. - ignore_alternative_branches: true, - }, - &*backend, - &relay_chain, - )) - .unwrap(); - - let expected_parents: Vec<_> = [&included_block, &pending_block] - .into_iter() - .chain(aligned_blocks.iter()) - .collect(); - - assert_eq!(potential_parents.len(), expected_parents.len()); - for (i, (parent, expected)) in potential_parents.iter().zip(expected_parents.iter()).enumerate() - { - assert_eq!(parent.hash, expected.hash()); - assert_eq!(&parent.header, expected.header()); - assert_eq!(parent.depth, i); - assert!(parent.aligned_with_pending); - } - - // Do not ignore: - 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. - ignore_alternative_branches: false, - }, - &*backend, - &relay_chain, - )) - .unwrap(); - - let expected_aligned: Vec<_> = [&included_block, &pending_block] - .into_iter() - .chain(aligned_blocks.iter()) - .collect(); - - let expected_parents: Vec<_> = - expected_aligned.clone().into_iter().chain(alt_blocks.iter()).collect(); - - assert_eq!(potential_parents.len(), expected_parents.len()); - for parent in potential_parents.iter() { - let expected = expected_parents - .iter() - .find(|block| block.header().hash() == parent.hash) - .expect("missing parent"); - - let is_aligned = expected_aligned.contains(&expected); - - assert_eq!(parent.hash, expected.hash()); - assert_eq!(&parent.header, expected.header()); - assert_eq!(parent.aligned_with_pending, is_aligned); - } -} - -/// Tests that no potential parent gets discarded if there's no pending availability block. -#[test] -fn find_potential_parents_aligned_no_pending() { - sp_tracing::try_init_simple(); - - const NON_INCLUDED_CHAIN_LEN: 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_with_delayed_best_block(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 = build_and_import_block_ext( - &client, - BlockOrigin::Own, - true, - &mut para_import, - None, - None, - Some(relay_parent), - ); - - 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, included_block.header().clone()); - } - - // Build two sibling chains from the included block. - let mut parent = included_block.header().hash(); - for _ in 0..NON_INCLUDED_CHAIN_LEN { - let block = build_and_import_block_ext( - &client, - BlockOrigin::Own, - true, - &mut para_import, - Some(parent), - None, - Some(relay_parent), - ); - parent = block.header().hash(); - } - - let mut parent = included_block.header().hash(); - for _ in 0..NON_INCLUDED_CHAIN_LEN { - let block = build_and_import_block_ext( - &client, - BlockOrigin::NetworkInitialSync, - true, - &mut para_import, - Some(parent), - None, - Some(search_relay_parent), - ); - parent = block.header().hash(); - } - - let potential_parents_aligned = block_on(find_potential_parents( - ParentSearchParams { - relay_parent: search_relay_parent, - para_id: ParaId::from(100), - ancestry_lookback: 1, // aligned chain is in ancestry. - ignore_alternative_branches: true, - }, - &*backend, - &relay_chain, - )) - .unwrap(); - let potential_parents = block_on(find_potential_parents( + let result = block_on(find_parent_for_building( ParentSearchParams { relay_parent: search_relay_parent, para_id: ParaId::from(100), ancestry_lookback: 1, - ignore_alternative_branches: false, - }, - &*backend, - &relay_chain, - )) - .unwrap(); - // Both chains have NON_INCLUDED_CHAIN_LEN blocks each, plus the included block. - assert_eq!(potential_parents.len(), 2 * NON_INCLUDED_CHAIN_LEN + 1); - assert_eq!(potential_parents, potential_parents_aligned); -} - -#[test] -fn find_potential_parents_no_duplicates() { - sp_tracing::try_init_simple(); - - 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_with_delayed_best_block(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 = build_and_import_block_ext( - &client, - BlockOrigin::NetworkInitialSync, - true, - &mut para_import, - None, - None, - Some(relay_parent), - ); - - let mut blocks = Vec::new(); - - for _ in 0..10 { - blocks.push(build_and_import_block_ext( - &client, - BlockOrigin::Own, - true, - &mut para_import, - Some( - blocks - .last() - .map_or_else(|| included_block.header.hash(), |b: &Block| b.header.hash()), - ), - None, - Some(relay_parent), - )); - } - - let pending_block = blocks.last().unwrap().clone(); - - let relay_chain = Relaychain::new(); - { - let relay_inner = &mut relay_chain.inner.lock().unwrap(); - relay_inner - .relay_chain_hash_to_header - .insert(search_relay_parent, included_block.header().clone()); - relay_inner - .relay_chain_hash_to_header_pending - .insert(search_relay_parent, pending_block.header().clone()); - } - - // Ignore alternative branch: - 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. - ignore_alternative_branches: true, }, &*backend, &relay_chain, )) - .unwrap(); + .unwrap() + .expect("Should find a parent"); - let expected_parents: Vec<_> = [included_block.header.clone()] - .into_iter() - .chain(blocks.iter().map(|b| b.header.clone())) - .collect(); - - assert_eq!( - potential_parents.into_iter().map(|p| p.header).collect::>(), - expected_parents - ); + // The deepest block should be the best parent. + assert_eq!(result.best_parent_header.hash(), last_block.hash()); + assert_eq!(&result.best_parent_header, last_block.header()); + assert_eq!(&result.included_header, included_block.header()); } diff --git a/prdoc/pr_10998.prdoc b/prdoc/pr_10998.prdoc new file mode 100644 index 0000000000000..021268f6d6211 --- /dev/null +++ b/prdoc/pr_10998.prdoc @@ -0,0 +1,14 @@ +title: 'Cumulus: Simplify parent search for block-building' +doc: +- audience: Node Dev + description: |- + While reviewing #10973 I found once more that our parent search is totally overengineered: + - It offers the option to search branches that do not contain the pending block -> These branches can never be taken + - It returns a list of potential parents -> Nobody uses the list, we only care about the latest block that we should build on + + By eliminating these two annoyances, the code is a lot more simple and easier to follow. There are still some defensive checks that are not strictly necessary, but does not hurt to keep them. +crates: +- name: cumulus-client-consensus-common + bump: major +- name: cumulus-client-consensus-aura + bump: major