From 131d56ed0bf45dbed9bae7da6e1a1ee8ec2bb020 Mon Sep 17 00:00:00 2001 From: Alin Dima Date: Tue, 13 Aug 2024 17:08:29 +0300 Subject: [PATCH] [stable2407] backport #4937 (#5337) Backport https://github.com/paritytech/polkadot-sdk/pull/4937 on the stable release --- Cargo.lock | 10 +- .../core/prospective-parachains/Cargo.toml | 11 +- .../core/prospective-parachains/src/error.rs | 15 - .../src/fragment_chain/mod.rs | 1361 +++++++---- .../src/fragment_chain/tests.rs | 2156 ++++++++--------- .../core/prospective-parachains/src/lib.rs | 638 ++--- .../prospective-parachains/src/metrics.rs | 105 +- .../core/prospective-parachains/src/tests.rs | 691 ++++-- polkadot/node/core/provisioner/src/lib.rs | 8 +- .../statement-distribution/src/v2/mod.rs | 4 +- polkadot/node/subsystem-types/src/messages.rs | 40 +- .../src/backing_implicit_view.rs | 76 + .../src/inclusion_emulator/mod.rs | 113 +- prdoc/pr_4937.prdoc | 21 + 14 files changed, 2922 insertions(+), 2327 deletions(-) create mode 100644 prdoc/pr_4937.prdoc diff --git a/Cargo.lock b/Cargo.lock index d6b087b6f70d..0ede9df3704f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13199,23 +13199,17 @@ name = "polkadot-node-core-prospective-parachains" version = "16.0.0" dependencies = [ "assert_matches", - "bitvec", "fatality", "futures", - "parity-scale-codec", - "polkadot-node-primitives", "polkadot-node-subsystem", "polkadot-node-subsystem-test-helpers", - "polkadot-node-subsystem-types", "polkadot-node-subsystem-util", "polkadot-primitives", "polkadot-primitives-test-helpers", + "rand", "rstest", - "sc-keystore", - "sp-application-crypto", "sp-core", - "sp-keyring", - "sp-keystore", + "sp-tracing", "thiserror", "tracing-gum", ] diff --git a/polkadot/node/core/prospective-parachains/Cargo.toml b/polkadot/node/core/prospective-parachains/Cargo.toml index 5e1614b863a3..0ca18b73e54a 100644 --- a/polkadot/node/core/prospective-parachains/Cargo.toml +++ b/polkadot/node/core/prospective-parachains/Cargo.toml @@ -13,14 +13,10 @@ workspace = true futures = { workspace = true } gum.workspace = true gum.default-features = true -codec = { workspace = true, default-features = true } thiserror = { workspace = true } fatality = { workspace = true } -bitvec = { workspace = true, default-features = true } polkadot-primitives.workspace = true polkadot-primitives.default-features = true -polkadot-node-primitives.workspace = true -polkadot-node-primitives.default-features = true polkadot-node-subsystem.workspace = true polkadot-node-subsystem.default-features = true polkadot-node-subsystem-util.workspace = true @@ -29,11 +25,8 @@ polkadot-node-subsystem-util.default-features = true [dev-dependencies] assert_matches = { workspace = true } polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" } -polkadot-node-subsystem-types = { default-features = true, path = "../../subsystem-types" } polkadot-primitives-test-helpers = { path = "../../../primitives/test-helpers" } +sp-tracing = { workspace = true } sp-core = { default-features = true, path = "../../../../substrate/primitives/core" } -sc-keystore = { default-features = true, path = "../../../../substrate/client/keystore" } -sp-application-crypto = { default-features = true, path = "../../../../substrate/primitives/application-crypto" } -sp-keyring = { default-features = true, path = "../../../../substrate/primitives/keyring" } -sp-keystore = { default-features = true, path = "../../../../substrate/primitives/keystore" } +rand = { workspace = true } rstest = { workspace = true } diff --git a/polkadot/node/core/prospective-parachains/src/error.rs b/polkadot/node/core/prospective-parachains/src/error.rs index 2b0933ab1c7e..4b332b9c5de5 100644 --- a/polkadot/node/core/prospective-parachains/src/error.rs +++ b/polkadot/node/core/prospective-parachains/src/error.rs @@ -30,18 +30,6 @@ use fatality::Nested; #[allow(missing_docs)] #[fatality::fatality(splitable)] pub enum Error { - #[fatal] - #[error("SubsystemError::Context error: {0}")] - SubsystemContext(String), - - #[fatal] - #[error("Spawning a task failed: {0}")] - SpawnFailed(SubsystemError), - - #[fatal] - #[error("Participation worker receiver exhausted.")] - ParticipationWorkerReceiverExhausted, - #[fatal] #[error("Receiving message from overseer failed: {0}")] SubsystemReceive(#[source] SubsystemError), @@ -55,9 +43,6 @@ pub enum Error { #[error(transparent)] ChainApi(#[from] ChainApiError), - #[error(transparent)] - Subsystem(SubsystemError), - #[error("Request to chain API subsystem dropped")] ChainApiRequestCanceled(oneshot::Canceled), diff --git a/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs b/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs index f87d4820ff9a..606febfce183 100644 --- a/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs +++ b/polkadot/node/core/prospective-parachains/src/fragment_chain/mod.rs @@ -18,41 +18,58 @@ //! //! # Overview //! -//! This module exposes two main types: [`FragmentChain`] and [`CandidateStorage`] which are meant -//! to be used in close conjunction. Each fragment chain is associated with a particular -//! relay-parent and each node in the chain represents a candidate. Each parachain has a single -//! candidate storage, but can have one chain for each relay chain block in the view. -//! Therefore, the same candidate can be present in multiple fragment chains of a parachain. One of -//! the purposes of the candidate storage is to deduplicate the large candidate data that is being -//! referenced from multiple fragment chains. +//! The main type exposed by this module is the [`FragmentChain`]. //! -//! A chain has an associated [`Scope`] which defines limits on candidates within the chain. -//! Candidates themselves have their own [`Constraints`] which are either the constraints from the -//! scope, or, if there are previous nodes in the chain, a modified version of the previous -//! candidate's constraints. +//! Each fragment chain is associated with a particular relay-parent (an active leaf) and has a +//! [`Scope`], which contains the allowed relay parents (up to `allowed_ancestry_len`), the pending +//! availability candidates and base constraints derived from the latest included candidate. Each +//! parachain has a single `FragmentChain` for each active leaf where it's scheduled. //! -//! Another use of the `CandidateStorage` is to keep a record of candidates which may not be yet -//! included in any chain, but which may become part of a chain in the future. This is needed for -//! elastic scaling, so that we may parallelise the backing process across different groups. As long -//! as some basic constraints are not violated by an unconnected candidate (like the relay parent -//! being in scope), we proceed with the backing process, hoping that its predecessors will be -//! backed soon enough. This is commonly called a potential candidate. Note that not all potential -//! candidates will be maintained in the CandidateStorage. The total number of connected + potential -//! candidates will be at most max_candidate_depth + 1. +//! A fragment chain consists mainly of the current best backable chain (we'll call this the best +//! chain) and a storage of unconnected potential candidates (we'll call this the unconnected +//! storage). +//! +//! The best chain contains all the candidates pending availability and a subsequent chain +//! of candidates that have reached the backing quorum and are better than any other backable forks +//! according to the fork selection rule (more on this rule later). It has a length of size at most +//! `max_candidate_depth + 1`. +//! +//! The unconnected storage keeps a record of seconded/backable candidates that may be +//! added to the best chain in the future. +//! Once a candidate is seconded, it becomes part of this unconnected storage. +//! Only after it is backed it may be added to the best chain (but not necessarily). It's only +//! added if it builds on the latest candidate in the chain and if there isn't a better backable +//! candidate according to the fork selection rule. +//! +//! An important thing to note is that the candidates present in the unconnected storage may have +//! any/no relationship between them. In other words, they may form N trees and may even form +//! cycles. This is needed so that we may begin validating candidates for which we don't yet know +//! their parent (so we may parallelize the backing process across different groups for elastic +//! scaling) and so that we accept parachain forks. +//! +//! We accept parachain forks only if the fork selection rule allows for it. In other words, if we +//! have a backed candidate, we begin seconding/validating a fork only if it has a lower candidate +//! hash. Once both forks are backed, we discard the one with the higher candidate hash. +//! We assume all validators pick the same fork according to the fork selection rule. If we decided +//! to not accept parachain forks, candidates could end up getting only half of the backing votes or +//! even less (for forks of larger arity). This would affect the validator rewards. Still, we don't +//! guarantee that a fork-producing parachains will be able to fully use elastic scaling. +//! +//! Once a candidate is backed and becomes part of the best chain, we can trim from the +//! unconnected storage candidates which constitute forks on the best chain and no longer have +//! potential. //! //! This module also makes use of types provided by the Inclusion Emulator module, such as //! [`Fragment`] and [`Constraints`]. These perform the actual job of checking for validity of //! prospective fragments. //! -//! # Parachain forks +//! # Fork choice rule //! -//! Parachains are expected to not create forks, hence the use of fragment chains as opposed to -//! fragment trees. If parachains do create forks, their performance in regards to async backing and -//! elastic scaling will suffer, because different validators will have different views of the -//! future. +//! The motivation for the fork choice rule is described in the previous chapter. //! -//! This is a compromise we can make - collators which want to use async backing and elastic scaling -//! need to cooperate for the highest throughput. +//! The current rule is: choose the candidate with the lower candidate hash. +//! The candidate hash is quite random and finding a candidate with a lower hash in order to favour +//! it would essentially mean solving a proof of work problem. //! //! # Parachain cycles //! @@ -65,70 +82,117 @@ //! resolved by having candidates reference their parent by candidate hash. //! //! However, dealing with cycles increases complexity during the backing/inclusion process for no -//! practical reason. Therefore, fragment chains will not accept such candidates. +//! practical reason. +//! These cycles may be accepted by fragment chains while candidates are part of the unconnected +//! storage, but they will definitely not make it to the best chain. //! //! On the other hand, enforcing that a parachain will NEVER be acyclic would be very complicated //! (looping through the entire parachain's history on every new candidate or changing the candidate //! receipt to reference the parent's candidate hash). //! +//! Therefore, we don't provide a guarantee that a cycle-producing parachain will work (although in +//! practice they probably will if the cycle length is larger than the number of assigned cores +//! multiplied by two). +//! //! # Spam protection //! -//! As long as the [`CandidateStorage`] has bounded input on the number of candidates supplied, -//! [`FragmentChain`] complexity is bounded. This means that higher-level code needs to be selective -//! about limiting the amount of candidates that are considered. +//! As long as the supplied number of candidates is bounded, [`FragmentChain`] complexity is +//! bounded. This means that higher-level code needs to be selective about limiting the amount of +//! candidates that are considered. +//! +//! Practically speaking, the collator-protocol will not allow more than `max_candidate_depth + 1` +//! collations to be fetched at a relay parent and statement-distribution will not allow more than +//! `max_candidate_depth + 1` seconded candidates at a relay parent per each validator in the +//! backing group. Considering the `allowed_ancestry_len` configuration value, the number of +//! candidates in a `FragmentChain` (including its unconnected storage) should not exceed: +//! +//! `allowed_ancestry_len * (max_candidate_depth + 1) * backing_group_size`. //! //! The code in this module is not designed for speed or efficiency, but conceptual simplicity. //! Our assumption is that the amount of candidates and parachains we consider will be reasonably //! bounded and in practice will not exceed a few thousand at any time. This naive implementation //! will still perform fairly well under these conditions, despite being somewhat wasteful of //! memory. +//! +//! Still, the expensive candidate data (CandidateCommitments) are wrapped in an `Arc` and shared +//! across fragment chains of the same para on different active leaves. #[cfg(test)] mod tests; use std::{ + cmp::{min, Ordering}, collections::{ hash_map::{Entry, HashMap}, - BTreeMap, HashSet, + BTreeMap, HashSet, VecDeque, }, sync::Arc, }; use super::LOG_TARGET; -use polkadot_node_subsystem::messages::{Ancestors, HypotheticalCandidate}; +use polkadot_node_subsystem::messages::Ancestors; use polkadot_node_subsystem_util::inclusion_emulator::{ - ConstraintModifications, Constraints, Fragment, ProspectiveCandidate, RelayChainBlockInfo, + self, ConstraintModifications, Constraints, Fragment, HypotheticalOrConcreteCandidate, + ProspectiveCandidate, RelayChainBlockInfo, }; use polkadot_primitives::{ - BlockNumber, CandidateHash, CommittedCandidateReceipt, Hash, HeadData, Id as ParaId, - PersistedValidationData, + BlockNumber, CandidateCommitments, CandidateHash, CommittedCandidateReceipt, Hash, HeadData, + PersistedValidationData, ValidationCodeHash, }; +use thiserror::Error; + +/// Fragment chain related errors. +#[derive(Debug, Clone, PartialEq, Error)] +pub(crate) enum Error { + #[error("Candidate already known")] + CandidateAlreadyKnown, + #[error("Candidate's parent head is equal to its output head. Would introduce a cycle.")] + ZeroLengthCycle, + #[error("Candidate would introduce a cycle")] + Cycle, + #[error("Candidate would introduce two paths to the same output state")] + MultiplePaths, + #[error("Attempting to directly introduce a Backed candidate. It should first be introduced as Seconded")] + IntroduceBackedCandidate, + #[error("Relay parent {0:?} of the candidate precedes the relay parent {1:?} of a pending availability candidate")] + RelayParentPrecedesCandidatePendingAvailability(Hash, Hash), + #[error("Candidate would introduce a fork with a pending availability candidate: {0:?}")] + ForkWithCandidatePendingAvailability(CandidateHash), + #[error("Fork selection rule favours another candidate: {0:?}")] + ForkChoiceRule(CandidateHash), + #[error("Could not find parent of the candidate")] + ParentCandidateNotFound, + #[error("Could not compute candidate constraints: {0:?}")] + ComputeConstraints(inclusion_emulator::ModificationError), + #[error("Candidate violates constraints: {0:?}")] + CheckAgainstConstraints(inclusion_emulator::FragmentValidityError), + #[error("Relay parent would move backwards from the latest candidate in the chain")] + RelayParentMovedBackwards, + #[error(transparent)] + CandidateEntry(#[from] CandidateEntryError), + #[error("Relay parent {0:?} not in scope. Earliest relay parent allowed {1:?}")] + RelayParentNotInScope(Hash, Hash), +} -/// Kinds of failures to import a candidate into storage. -#[derive(Debug, Clone, PartialEq)] -pub enum CandidateStorageInsertionError { - /// An error indicating that a supplied candidate didn't match the persisted - /// validation data provided alongside it. - PersistedValidationDataMismatch, - /// The candidate was already known. - CandidateAlreadyKnown(CandidateHash), +/// The rule for selecting between two backed candidate forks, when adding to the chain. +/// All validators should adhere to this rule, in order to not lose out on rewards in case of +/// forking parachains. +fn fork_selection_rule(hash1: &CandidateHash, hash2: &CandidateHash) -> Ordering { + hash1.cmp(hash2) } -/// Stores candidates and information about them such as their relay-parents and their backing -/// states. +/// Utility for storing candidates and information about them such as their relay-parents and their +/// backing states. This does not assume any restriction on whether or not the candidates form a +/// chain. Useful for storing all kinds of candidates. #[derive(Clone, Default)] pub(crate) struct CandidateStorage { - // Index from head data hash to candidate hashes with that head data as a parent. Purely for + // Index from head data hash to candidate hashes with that head data as a parent. Useful for // efficiency when responding to `ProspectiveValidationDataRequest`s or when trying to find a // new candidate to push to a chain. - // Even though having multiple candidates with same parent would be invalid for a parachain, it - // could happen across different relay chain forks, hence the HashSet. by_parent_head: HashMap>, - // Index from head data hash to candidate hashes outputting that head data. Purely for + // Index from head data hash to candidate hashes outputting that head data. For // efficiency when responding to `ProspectiveValidationDataRequest`s. - // Even though having multiple candidates with same output would be invalid for a parachain, - // it could happen across different relay chain forks. by_output_head: HashMap>, // Index from candidate hash to fragment node. @@ -136,65 +200,59 @@ pub(crate) struct CandidateStorage { } impl CandidateStorage { - /// Introduce a new candidate. - pub fn add_candidate( + /// Introduce a new pending availability candidate. + pub fn add_pending_availability_candidate( &mut self, + candidate_hash: CandidateHash, candidate: CommittedCandidateReceipt, persisted_validation_data: PersistedValidationData, - state: CandidateState, - ) -> Result { - let candidate_hash = candidate.hash(); - if self.by_candidate_hash.contains_key(&candidate_hash) { - return Err(CandidateStorageInsertionError::CandidateAlreadyKnown(candidate_hash)) - } + ) -> Result<(), Error> { + let entry = CandidateEntry::new( + candidate_hash, + candidate, + persisted_validation_data, + CandidateState::Backed, + )?; - if persisted_validation_data.hash() != candidate.descriptor.persisted_validation_data_hash { - return Err(CandidateStorageInsertionError::PersistedValidationDataMismatch) - } + self.add_candidate_entry(entry) + } - let entry = CandidateEntry { - candidate_hash, - parent_head_data_hash: persisted_validation_data.parent_head.hash(), - output_head_data_hash: candidate.commitments.head_data.hash(), - relay_parent: candidate.descriptor.relay_parent, - state, - candidate: Arc::new(ProspectiveCandidate { - commitments: candidate.commitments, - collator: candidate.descriptor.collator, - collator_signature: candidate.descriptor.signature, - persisted_validation_data, - pov_hash: candidate.descriptor.pov_hash, - validation_code_hash: candidate.descriptor.validation_code_hash, - }), - }; + /// Return the number of stored candidates. + pub fn len(&self) -> usize { + self.by_candidate_hash.len() + } + + /// Introduce a new candidate entry. + fn add_candidate_entry(&mut self, candidate: CandidateEntry) -> Result<(), Error> { + let candidate_hash = candidate.candidate_hash; + if self.by_candidate_hash.contains_key(&candidate_hash) { + return Err(Error::CandidateAlreadyKnown) + } self.by_parent_head - .entry(entry.parent_head_data_hash()) + .entry(candidate.parent_head_data_hash) .or_default() .insert(candidate_hash); self.by_output_head - .entry(entry.output_head_data_hash()) + .entry(candidate.output_head_data_hash) .or_default() .insert(candidate_hash); - // sanity-checked already. - self.by_candidate_hash.insert(candidate_hash, entry); + self.by_candidate_hash.insert(candidate_hash, candidate); - Ok(candidate_hash) + Ok(()) } /// Remove a candidate from the store. - pub fn remove_candidate(&mut self, candidate_hash: &CandidateHash) { + fn remove_candidate(&mut self, candidate_hash: &CandidateHash) { if let Some(entry) = self.by_candidate_hash.remove(candidate_hash) { - if let Entry::Occupied(mut e) = self.by_parent_head.entry(entry.parent_head_data_hash()) - { + if let Entry::Occupied(mut e) = self.by_parent_head.entry(entry.parent_head_data_hash) { e.get_mut().remove(&candidate_hash); if e.get().is_empty() { e.remove(); } } - if let Entry::Occupied(mut e) = self.by_output_head.entry(entry.output_head_data_hash()) - { + if let Entry::Occupied(mut e) = self.by_output_head.entry(entry.output_head_data_hash) { e.get_mut().remove(&candidate_hash); if e.get().is_empty() { e.remove(); @@ -204,7 +262,7 @@ impl CandidateStorage { } /// Note that an existing candidate has been backed. - pub fn mark_backed(&mut self, candidate_hash: &CandidateHash) { + fn mark_backed(&mut self, candidate_hash: &CandidateHash) { if let Some(entry) = self.by_candidate_hash.get_mut(candidate_hash) { gum::trace!(target: LOG_TARGET, ?candidate_hash, "Candidate marked as backed"); entry.state = CandidateState::Backed; @@ -213,38 +271,18 @@ impl CandidateStorage { } } - /// Whether a candidate is recorded as being backed. - pub fn is_backed(&self, candidate_hash: &CandidateHash) -> bool { - self.by_candidate_hash - .get(candidate_hash) - .map_or(false, |e| e.state == CandidateState::Backed) - } - /// Whether a candidate is contained within the storage already. - pub fn contains(&self, candidate_hash: &CandidateHash) -> bool { + fn contains(&self, candidate_hash: &CandidateHash) -> bool { self.by_candidate_hash.contains_key(candidate_hash) } - /// Return an iterator over the stored candidates. - pub fn candidates(&self) -> impl Iterator { + /// Return an iterator over references to the stored candidates, in arbitrary order. + fn candidates(&self) -> impl Iterator { self.by_candidate_hash.values() } - /// Retain only candidates which pass the predicate. - pub(crate) fn retain(&mut self, pred: impl Fn(&CandidateHash) -> bool) { - self.by_candidate_hash.retain(|h, _v| pred(h)); - self.by_parent_head.retain(|_parent, children| { - children.retain(|h| pred(h)); - !children.is_empty() - }); - self.by_output_head.retain(|_output, candidates| { - candidates.retain(|h| pred(h)); - !candidates.is_empty() - }); - } - - /// Get head-data by hash. - pub(crate) fn head_data_by_hash(&self, hash: &Hash) -> Option<&HeadData> { + /// Try getting head-data by hash. + fn head_data_by_hash(&self, hash: &Hash) -> Option<&HeadData> { // First, search for candidates outputting this head data and extract the head data // from their commitments if they exist. // @@ -264,16 +302,8 @@ impl CandidateStorage { }) } - /// Returns candidate's relay parent, if present. - pub(crate) fn relay_parent_of_candidate(&self, candidate_hash: &CandidateHash) -> Option { - self.by_candidate_hash.get(candidate_hash).map(|entry| entry.relay_parent) - } - - /// Returns the candidates which have the given head data hash as parent. - /// We don't allow forks in a parachain, but we may have multiple candidates with same parent - /// across different relay chain forks. That's why it returns an iterator (but only one will be - /// valid and used in the end). - fn possible_para_children<'a>( + /// Returns the backed candidates which have the given head data hash as parent. + fn possible_backed_para_children<'a>( &'a self, parent_head_hash: &'a Hash, ) -> impl Iterator + 'a { @@ -282,12 +312,11 @@ impl CandidateStorage { .get(parent_head_hash) .into_iter() .flat_map(|hashes| hashes.iter()) - .filter_map(move |h| by_candidate_hash.get(h)) - } - - #[cfg(test)] - pub fn len(&self) -> (usize, usize) { - (self.by_parent_head.len(), self.by_candidate_hash.len()) + .filter_map(move |h| { + by_candidate_hash.get(h).and_then(|candidate| { + (candidate.state == CandidateState::Backed).then_some(candidate) + }) + }) } } @@ -295,14 +324,24 @@ impl CandidateStorage { /// /// Candidates aren't even considered until they've at least been seconded. #[derive(Debug, PartialEq, Clone)] -pub(crate) enum CandidateState { +enum CandidateState { /// The candidate has been seconded. Seconded, /// The candidate has been completely backed by the group. Backed, } +#[derive(Debug, Clone, PartialEq, Error)] +/// Possible errors when construcing a candidate entry. +pub enum CandidateEntryError { + #[error("Candidate does not match the persisted validation data provided alongside it")] + PersistedValidationDataMismatch, + #[error("Candidate's parent head is equal to its output head. Would introduce a cycle")] + ZeroLengthCycle, +} + #[derive(Debug, Clone)] +/// Representation of a candidate into the [`CandidateStorage`]. pub(crate) struct CandidateEntry { candidate_hash: CandidateHash, parent_head_data_hash: Hash, @@ -313,16 +352,81 @@ pub(crate) struct CandidateEntry { } impl CandidateEntry { + /// Create a new seconded candidate entry. + pub fn new_seconded( + candidate_hash: CandidateHash, + candidate: CommittedCandidateReceipt, + persisted_validation_data: PersistedValidationData, + ) -> Result { + Self::new(candidate_hash, candidate, persisted_validation_data, CandidateState::Seconded) + } + pub fn hash(&self) -> CandidateHash { self.candidate_hash } - pub fn parent_head_data_hash(&self) -> Hash { + fn new( + candidate_hash: CandidateHash, + candidate: CommittedCandidateReceipt, + persisted_validation_data: PersistedValidationData, + state: CandidateState, + ) -> Result { + if persisted_validation_data.hash() != candidate.descriptor.persisted_validation_data_hash { + return Err(CandidateEntryError::PersistedValidationDataMismatch) + } + + let parent_head_data_hash = persisted_validation_data.parent_head.hash(); + let output_head_data_hash = candidate.commitments.head_data.hash(); + + if parent_head_data_hash == output_head_data_hash { + return Err(CandidateEntryError::ZeroLengthCycle) + } + + Ok(Self { + candidate_hash, + parent_head_data_hash, + output_head_data_hash, + relay_parent: candidate.descriptor.relay_parent, + state, + candidate: Arc::new(ProspectiveCandidate { + commitments: candidate.commitments, + collator: candidate.descriptor.collator, + collator_signature: candidate.descriptor.signature, + persisted_validation_data, + pov_hash: candidate.descriptor.pov_hash, + validation_code_hash: candidate.descriptor.validation_code_hash, + }), + }) + } +} + +impl HypotheticalOrConcreteCandidate for CandidateEntry { + fn commitments(&self) -> Option<&CandidateCommitments> { + Some(&self.candidate.commitments) + } + + fn persisted_validation_data(&self) -> Option<&PersistedValidationData> { + Some(&self.candidate.persisted_validation_data) + } + + fn validation_code_hash(&self) -> Option<&ValidationCodeHash> { + Some(&self.candidate.validation_code_hash) + } + + fn parent_head_data_hash(&self) -> Hash { self.parent_head_data_hash } - pub fn output_head_data_hash(&self) -> Hash { - self.output_head_data_hash + fn output_head_data_hash(&self) -> Option { + Some(self.output_head_data_hash) + } + + fn relay_parent(&self) -> Hash { + self.relay_parent + } + + fn candidate_hash(&self) -> CandidateHash { + self.candidate_hash } } @@ -339,8 +443,6 @@ pub(crate) struct PendingAvailability { /// The scope of a [`FragmentChain`]. #[derive(Debug, Clone)] pub(crate) struct Scope { - /// The assigned para id of this `FragmentChain`. - para: ParaId, /// The relay parent we're currently building on top of. relay_parent: RelayChainBlockInfo, /// The other relay parents candidates are allowed to build upon, mapped by the block number. @@ -358,10 +460,14 @@ pub(crate) struct Scope { /// An error variant indicating that ancestors provided to a scope /// had unexpected order. #[derive(Debug)] -pub struct UnexpectedAncestor { +pub(crate) struct UnexpectedAncestor { /// The block number that this error occurred at. + /// Allow as dead code, but it's being read in logs. + #[allow(dead_code)] pub number: BlockNumber, /// The previous seen block number, which did not match `number`. + /// Allow as dead code, but it's being read in logs. + #[allow(dead_code)] pub prev: BlockNumber, } @@ -383,7 +489,6 @@ impl Scope { /// /// It is allowed to provide zero ancestors. pub fn with_ancestors( - para: ParaId, relay_parent: RelayChainBlockInfo, base_constraints: Constraints, pending_availability: Vec, @@ -410,7 +515,6 @@ impl Scope { } Ok(Scope { - para, relay_parent, base_constraints, pending_availability, @@ -438,24 +542,29 @@ impl Scope { self.ancestors_by_hash.get(hash).map(|info| info.clone()) } + /// Get the base constraints of the scope + pub fn base_constraints(&self) -> &Constraints { + &self.base_constraints + } + /// Whether the candidate in question is one pending availability in this scope. - pub fn get_pending_availability( + fn get_pending_availability( &self, candidate_hash: &CandidateHash, ) -> Option<&PendingAvailability> { self.pending_availability.iter().find(|c| &c.candidate_hash == candidate_hash) } - - /// Get the base constraints of the scope - pub fn base_constraints(&self) -> &Constraints { - &self.base_constraints - } } -pub struct FragmentNode { +#[cfg_attr(test, derive(Clone))] +/// A node that is part of a `BackedChain`. It holds constraints based on the ancestors in the +/// chain. +struct FragmentNode { fragment: Fragment, candidate_hash: CandidateHash, cumulative_modifications: ConstraintModifications, + parent_head_data_hash: Hash, + output_head_data_hash: Hash, } impl FragmentNode { @@ -464,211 +573,336 @@ impl FragmentNode { } } -/// Response given by `can_add_candidate_as_potential` -#[derive(PartialEq, Debug)] -pub enum PotentialAddition { - /// Can be added as either connected or unconnected candidate. - Anyhow, - /// Can only be added as a connected candidate to the chain. - IfConnected, - /// Cannot be added. - None, +impl From<&FragmentNode> for CandidateEntry { + fn from(node: &FragmentNode) -> Self { + // We don't need to perform the checks done in `CandidateEntry::new()`, since a + // `FragmentNode` always comes from a `CandidateEntry` + Self { + candidate_hash: node.candidate_hash, + parent_head_data_hash: node.parent_head_data_hash, + output_head_data_hash: node.output_head_data_hash, + candidate: node.fragment.candidate_clone(), + relay_parent: node.relay_parent(), + // A fragment node is always backed. + state: CandidateState::Backed, + } + } +} + +/// A candidate chain of backed/backable candidates. +/// Includes the candidates pending availability and candidates which may be backed on-chain. +#[derive(Default)] +#[cfg_attr(test, derive(Clone))] +struct BackedChain { + // Holds the candidate chain. + chain: Vec, + // Index from head data hash to the candidate hash with that head data as a parent. + // Only contains the candidates present in the `chain`. + by_parent_head: HashMap, + // Index from head data hash to the candidate hash outputting that head data. + // Only contains the candidates present in the `chain`. + by_output_head: HashMap, + // A set of the candidate hashes in the `chain`. + candidates: HashSet, +} + +impl BackedChain { + fn push(&mut self, candidate: FragmentNode) { + self.candidates.insert(candidate.candidate_hash); + self.by_parent_head + .insert(candidate.parent_head_data_hash, candidate.candidate_hash); + self.by_output_head + .insert(candidate.output_head_data_hash, candidate.candidate_hash); + self.chain.push(candidate); + } + + fn clear(&mut self) -> Vec { + self.by_parent_head.clear(); + self.by_output_head.clear(); + self.candidates.clear(); + + std::mem::take(&mut self.chain) + } + + fn revert_to_parent_hash<'a>( + &'a mut self, + parent_head_data_hash: &Hash, + ) -> impl Iterator + 'a { + let mut found_index = None; + for index in 0..self.chain.len() { + let node = &self.chain[0]; + + if found_index.is_some() { + self.by_parent_head.remove(&node.parent_head_data_hash); + self.by_output_head.remove(&node.output_head_data_hash); + self.candidates.remove(&node.candidate_hash); + } else if &node.output_head_data_hash == parent_head_data_hash { + found_index = Some(index); + } + } + + if let Some(index) = found_index { + self.chain.drain(min(index + 1, self.chain.len())..) + } else { + // Don't remove anything, but use drain to satisfy the compiler. + self.chain.drain(0..0) + } + } + + fn contains(&self, hash: &CandidateHash) -> bool { + self.candidates.contains(hash) + } } -/// This is a chain of candidates based on some underlying storage of candidates and a scope. +/// This is the fragment chain specific to an active leaf. /// -/// All nodes in the chain must be either pending availability or within the scope. Within the scope -/// means it's built off of the relay-parent or an ancestor. +/// It holds the current best backable candidate chain, as well as potential candidates +/// which could become connected to the chain in the future or which could even overwrite the +/// existing chain. +#[cfg_attr(test, derive(Clone))] pub(crate) struct FragmentChain { + // The current scope, which dictates the on-chain operating constraints that all future + // candidates must adhere to. scope: Scope, - chain: Vec, - - candidates: HashSet, + // The current best chain of backable candidates. It only contains candidates which build on + // top of each other and which have reached the backing quorum. In the presence of potential + // forks, this chain will pick a fork according to the `fork_selection_rule`. + best_chain: BackedChain, - // Index from head data hash to candidate hashes with that head data as a parent. - by_parent_head: HashMap, - // Index from head data hash to candidate hashes outputting that head data. - by_output_head: HashMap, + // The potential candidate storage. Contains candidates which are not yet part of the `chain` + // but may become in the future. These can form any tree shape as well as contain any + // unconnected candidates for which we don't know the parent. + unconnected: CandidateStorage, } impl FragmentChain { - /// Create a new [`FragmentChain`] with given scope and populated from the storage. - pub fn populate(scope: Scope, storage: &CandidateStorage) -> Self { - gum::trace!( - target: LOG_TARGET, - relay_parent = ?scope.relay_parent.hash, - relay_parent_num = scope.relay_parent.number, - para_id = ?scope.para, - ancestors = scope.ancestors.len(), - "Instantiating Fragment Chain", - ); - + /// Create a new [`FragmentChain`] with the given scope and populate it with the candidates + /// pending availability. + pub fn init(scope: Scope, mut candidates_pending_availability: CandidateStorage) -> Self { let mut fragment_chain = Self { scope, - chain: Vec::new(), - candidates: HashSet::new(), - by_parent_head: HashMap::new(), - by_output_head: HashMap::new(), + best_chain: BackedChain::default(), + unconnected: CandidateStorage::default(), }; - fragment_chain.populate_chain(storage); + // We only need to populate the best backable chain. Candidates pending availability must + // form a chain with the latest included head. + fragment_chain.populate_chain(&mut candidates_pending_availability); fragment_chain } - /// Get the scope of the Fragment Chain. + /// Populate the [`FragmentChain`] given the new candidates pending availability and the + /// optional previous fragment chain (of the previous relay parent). + pub fn populate_from_previous(&mut self, prev_fragment_chain: &FragmentChain) { + let mut prev_storage = prev_fragment_chain.unconnected.clone(); + + for candidate in prev_fragment_chain.best_chain.chain.iter() { + // If they used to be pending availability, don't add them. This is fine + // because: + // - if they still are pending availability, they have already been added to the new + // storage. + // - if they were included, no point in keeping them. + // + // This cannot happen for the candidates in the unconnected storage. The pending + // availability candidates will always be part of the best chain. + if prev_fragment_chain + .scope + .get_pending_availability(&candidate.candidate_hash) + .is_none() + { + let _ = prev_storage.add_candidate_entry(candidate.into()); + } + } + + // First populate the best backable chain. + self.populate_chain(&mut prev_storage); + + // Now that we picked the best backable chain, trim the forks generated by candidates which + // are not present in the best chain. + self.trim_uneligible_forks(&mut prev_storage, None); + + // Finally, keep any candidates which haven't been trimmed but still have potential. + self.populate_unconnected_potential_candidates(prev_storage); + } + + /// Get the scope of the [`FragmentChain`]. pub fn scope(&self) -> &Scope { &self.scope } - /// Returns the number of candidates in the chain - pub(crate) fn len(&self) -> usize { - self.candidates.len() + /// Returns the number of candidates in the best backable chain. + pub fn best_chain_len(&self) -> usize { + self.best_chain.chain.len() } - /// Whether the candidate exists. - pub(crate) fn contains_candidate(&self, candidate: &CandidateHash) -> bool { - self.candidates.contains(candidate) + /// Returns the number of candidates in unconnected potential storage. + pub fn unconnected_len(&self) -> usize { + self.unconnected.len() + } + + /// Whether the candidate exists as part of the unconnected potential candidates. + pub fn contains_unconnected_candidate(&self, candidate: &CandidateHash) -> bool { + self.unconnected.contains(candidate) } /// Return a vector of the chain's candidate hashes, in-order. - pub(crate) fn to_vec(&self) -> Vec { - self.chain.iter().map(|candidate| candidate.candidate_hash).collect() + pub fn best_chain_vec(&self) -> Vec { + self.best_chain.chain.iter().map(|candidate| candidate.candidate_hash).collect() } - /// Try accumulating more candidates onto the chain. - /// - /// Candidates can only be added if they build on the already existing chain. - pub(crate) fn extend_from_storage(&mut self, storage: &CandidateStorage) { - self.populate_chain(storage); + /// Return a vector of the unconnected potential candidate hashes, in arbitrary order. + pub fn unconnected(&self) -> impl Iterator { + self.unconnected.candidates() } - /// Returns the hypothetical state of a candidate with the given hash and parent head data - /// in regards to the existing chain. - /// - /// Returns true if either: - /// - the candidate is already present - /// - the candidate can be added to the chain - /// - the candidate could potentially be added to the chain in the future (its ancestors are - /// still unknown but it doesn't violate other rules). - /// - /// If this returns false, the candidate could never be added to the current chain (not now, not - /// ever) - pub(crate) fn hypothetical_membership( + /// Return whether this candidate is backed in this chain or the unconnected storage. + pub fn is_candidate_backed(&self, hash: &CandidateHash) -> bool { + self.best_chain.candidates.contains(hash) || + matches!( + self.unconnected.by_candidate_hash.get(hash), + Some(candidate) if candidate.state == CandidateState::Backed + ) + } + + /// Mark a candidate as backed. This can trigger a recreation of the best backable chain. + pub fn candidate_backed(&mut self, newly_backed_candidate: &CandidateHash) { + // Already backed. + if self.best_chain.candidates.contains(newly_backed_candidate) { + return + } + let Some(parent_head_hash) = self + .unconnected + .by_candidate_hash + .get(newly_backed_candidate) + .map(|entry| entry.parent_head_data_hash) + else { + // Candidate is not in unconnected storage. + return + }; + + // Mark the candidate hash. + self.unconnected.mark_backed(newly_backed_candidate); + + // Revert to parent_head_hash + if !self.revert_to(&parent_head_hash) { + // If nothing was reverted, there is nothing we can do for now. + return + } + + let mut prev_storage = std::mem::take(&mut self.unconnected); + + // Populate the chain. + self.populate_chain(&mut prev_storage); + + // Now that we picked the best backable chain, trim the forks generated by candidates + // which are not present in the best chain. We can start trimming from this candidate + // onwards. + self.trim_uneligible_forks(&mut prev_storage, Some(parent_head_hash)); + + // Finally, keep any candidates which haven't been trimmed but still have potential. + self.populate_unconnected_potential_candidates(prev_storage); + } + + /// Checks if this candidate could be added in the future to this chain. + /// This will return `Error::CandidateAlreadyKnown` if the candidate is already in the chain or + /// the unconnected candidate storage. + pub fn can_add_candidate_as_potential( &self, - candidate: HypotheticalCandidate, - candidate_storage: &CandidateStorage, - ) -> bool { + candidate: &impl HypotheticalOrConcreteCandidate, + ) -> Result<(), Error> { let candidate_hash = candidate.candidate_hash(); - // If we've already used this candidate in the chain - if self.candidates.contains(&candidate_hash) { - return true + if self.best_chain.contains(&candidate_hash) || self.unconnected.contains(&candidate_hash) { + return Err(Error::CandidateAlreadyKnown) } - let can_add_as_potential = self.can_add_candidate_as_potential( - candidate_storage, - &candidate.candidate_hash(), - &candidate.relay_parent(), - candidate.parent_head_data_hash(), - candidate.output_head_data_hash(), - ); + self.check_potential(candidate) + } - if can_add_as_potential == PotentialAddition::None { - return false + /// Try adding a seconded candidate, if the candidate has potential. It will never be added to + /// the chain directly in the seconded state, it will only be part of the unconnected storage. + pub fn try_adding_seconded_candidate( + &mut self, + candidate: &CandidateEntry, + ) -> Result<(), Error> { + if candidate.state == CandidateState::Backed { + return Err(Error::IntroduceBackedCandidate); } - let Some(candidate_relay_parent) = self.scope.ancestor(&candidate.relay_parent()) else { - // can_add_candidate_as_potential already checked for this, but just to be safe. - return false - }; + self.can_add_candidate_as_potential(candidate)?; - let identity_modifications = ConstraintModifications::identity(); - let cumulative_modifications = if let Some(last_candidate) = self.chain.last() { - &last_candidate.cumulative_modifications - } else { - &identity_modifications - }; + // This clone is cheap, as it uses an Arc for the expensive stuff. + // We can't consume the candidate because other fragment chains may use it also. + self.unconnected.add_candidate_entry(candidate.clone())?; - let child_constraints = - match self.scope.base_constraints.apply_modifications(&cumulative_modifications) { - Err(e) => { - gum::debug!( - target: LOG_TARGET, - new_parent_head = ?cumulative_modifications.required_parent, - ?candidate_hash, - err = ?e, - "Failed to apply modifications", - ); - - return false - }, - Ok(c) => c, - }; + Ok(()) + } - let parent_head_hash = candidate.parent_head_data_hash(); - if parent_head_hash == child_constraints.required_parent.hash() { - // We do additional checks for complete candidates. - if let HypotheticalCandidate::Complete { - ref receipt, - ref persisted_validation_data, - .. - } = candidate - { - if Fragment::check_against_constraints( - &candidate_relay_parent, - &child_constraints, - &receipt.commitments, - &receipt.descriptor().validation_code_hash, - persisted_validation_data, - ) - .is_err() - { - gum::debug!( - target: LOG_TARGET, - "Fragment::check_against_constraints() returned error", - ); - return false - } - } + /// Try getting the full head data associated with this hash. + pub fn get_head_data_by_hash(&self, head_data_hash: &Hash) -> Option { + // First, see if this is the head data of the latest included candidate. + let required_parent = &self.scope.base_constraints().required_parent; + if &required_parent.hash() == head_data_hash { + return Some(required_parent.clone()) + } - // If we got this far, it can be added to the chain right now. - true - } else if can_add_as_potential == PotentialAddition::Anyhow { - // Otherwise it is or can be an unconnected candidate, but only if PotentialAddition - // does not force us to only add a connected candidate. - true - } else { - false + // Cheaply check if the head data is in the best backable chain. + let has_head_data_in_chain = self + .best_chain + .by_parent_head + .get(head_data_hash) + .or_else(|| self.best_chain.by_output_head.get(head_data_hash)) + .is_some(); + + if has_head_data_in_chain { + return self.best_chain.chain.iter().find_map(|candidate| { + if &candidate.parent_head_data_hash == head_data_hash { + Some( + candidate + .fragment + .candidate() + .persisted_validation_data + .parent_head + .clone(), + ) + } else if &candidate.output_head_data_hash == head_data_hash { + Some(candidate.fragment.candidate().commitments.head_data.clone()) + } else { + None + } + }); } + + // Lastly, try getting the head data from the unconnected candidates. + self.unconnected.head_data_by_hash(head_data_hash).cloned() } - /// Select `count` candidates after the given `ancestors` which pass - /// the predicate and have not already been backed on chain. + /// Select `count` candidates after the given `ancestors` which can be backed on chain next. /// /// The intention of the `ancestors` is to allow queries on the basis of /// one or more candidates which were previously pending availability becoming /// available or candidates timing out. - pub(crate) fn find_backable_chain( + pub fn find_backable_chain( &self, ancestors: Ancestors, count: u32, - pred: impl Fn(&CandidateHash) -> bool, - ) -> Vec { + ) -> Vec<(CandidateHash, Hash)> { if count == 0 { return vec![] } let base_pos = self.find_ancestor_path(ancestors); - let actual_end_index = std::cmp::min(base_pos + (count as usize), self.chain.len()); + let actual_end_index = + std::cmp::min(base_pos + (count as usize), self.best_chain.chain.len()); let mut res = Vec::with_capacity(actual_end_index - base_pos); - for elem in &self.chain[base_pos..actual_end_index] { - if self.scope.get_pending_availability(&elem.candidate_hash).is_none() && - pred(&elem.candidate_hash) - { - res.push(elem.candidate_hash); + for elem in &self.best_chain.chain[base_pos..actual_end_index] { + // Only supply candidates which are not yet pending availability. `ancestors` should + // have already contained them, but check just in case. + if self.scope.get_pending_availability(&elem.candidate_hash).is_none() { + res.push((elem.candidate_hash, elem.relay_parent())); } else { break } @@ -681,11 +915,11 @@ impl FragmentChain { // Stops when the ancestors are all used or when a node in the chain is not present in the // ancestor set. Returns the index in the chain were the search stopped. fn find_ancestor_path(&self, mut ancestors: Ancestors) -> usize { - if self.chain.is_empty() { + if self.best_chain.chain.is_empty() { return 0; } - for (index, candidate) in self.chain.iter().enumerate() { + for (index, candidate) in self.best_chain.chain.iter().enumerate() { if !ancestors.remove(&candidate.candidate_hash) { return index } @@ -693,16 +927,16 @@ impl FragmentChain { // This means that we found the entire chain in the ancestor set. There won't be anything // left to back. - self.chain.len() + self.best_chain.chain.len() } - // Return the earliest relay parent a new candidate can have in order to be added to the chain. - // This is the relay parent of the last candidate in the chain. + // Return the earliest relay parent a new candidate can have in order to be added to the chain + // right now. This is the relay parent of the last candidate in the chain. // The value returned may not be valid if we want to add a candidate pending availability, which // may have a relay parent which is out of scope. Special handling is needed in that case. // `None` is returned if the candidate's relay parent info cannot be found. fn earliest_relay_parent(&self) -> Option { - if let Some(last_candidate) = self.chain.last() { + if let Some(last_candidate) = self.best_chain.chain.last() { self.scope.ancestor(&last_candidate.relay_parent()).or_else(|| { // if the relay-parent is out of scope _and_ it is in the chain, // it must be a candidate pending availability. @@ -715,152 +949,239 @@ impl FragmentChain { } } - // Checks if this candidate could be added in the future to this chain. - // This assumes that the chain does not already contain this candidate. It may or may not be - // present in the `CandidateStorage`. - // Even if the candidate is a potential candidate, this function will indicate that it can be - // kept only if there's enough room for it. - pub(crate) fn can_add_candidate_as_potential( - &self, - storage: &CandidateStorage, - candidate_hash: &CandidateHash, - relay_parent: &Hash, - parent_head_hash: Hash, - output_head_hash: Option, - ) -> PotentialAddition { - // If we've got enough candidates for the configured depth, no point in adding more. - if self.chain.len() > self.scope.max_depth { - return PotentialAddition::None - } + // Return the earliest relay parent a potential candidate may have for it to ever be added to + // the chain. This is the relay parent of the last candidate pending availability or the + // earliest relay parent in scope. + fn earliest_relay_parent_pending_availability(&self) -> RelayChainBlockInfo { + self.best_chain + .chain + .iter() + .rev() + .find_map(|candidate| { + self.scope + .get_pending_availability(&candidate.candidate_hash) + .map(|c| c.relay_parent.clone()) + }) + .unwrap_or_else(|| self.scope.earliest_relay_parent()) + } - if !self.check_potential(relay_parent, parent_head_hash, output_head_hash) { - return PotentialAddition::None - } + // Populate the unconnected potential candidate storage starting from a previous storage. + fn populate_unconnected_potential_candidates(&mut self, old_storage: CandidateStorage) { + for candidate in old_storage.by_candidate_hash.into_values() { + // Sanity check, all pending availability candidates should be already present in the + // chain. + if self.scope.get_pending_availability(&candidate.candidate_hash).is_some() { + continue + } - let present_in_storage = storage.contains(candidate_hash); + match self.can_add_candidate_as_potential(&candidate) { + Ok(()) => { + let _ = self.unconnected.add_candidate_entry(candidate); + }, + // Swallow these errors as they can legitimately happen when pruning stale + // candidates. + Err(_) => {}, + }; + } + } - let unconnected = self - .find_unconnected_potential_candidates( - storage, - present_in_storage.then_some(candidate_hash), - ) - .len(); + // Check whether a candidate outputting this head data would introduce a cycle or multiple paths + // to the same state. Trivial 0-length cycles are checked in `CandidateEntry::new`. + fn check_cycles_or_invalid_tree(&self, output_head_hash: &Hash) -> Result<(), Error> { + // this should catch a cycle where this candidate would point back to the parent of some + // candidate in the chain. + if self.best_chain.by_parent_head.contains_key(output_head_hash) { + return Err(Error::Cycle) + } - if (self.chain.len() + unconnected) < self.scope.max_depth { - PotentialAddition::Anyhow - } else if (self.chain.len() + unconnected) == self.scope.max_depth { - // If we've only one slot left to fill, it must be filled with a connected candidate. - PotentialAddition::IfConnected - } else { - PotentialAddition::None + // multiple paths to the same state, which can't happen for a chain. + if self.best_chain.by_output_head.contains_key(output_head_hash) { + return Err(Error::MultiplePaths) } + + Ok(()) } - // The candidates which are present in `CandidateStorage`, are not part of this chain but could - // become part of this chain in the future. Capped at the max depth minus the existing chain - // length. - // If `ignore_candidate` is supplied and found in storage, it won't be counted. - pub(crate) fn find_unconnected_potential_candidates( + // Checks the potential of a candidate to be added to the chain now or in the future. + // It works both with concrete candidates for which we have the full PVD and committed receipt, + // but also does some more basic checks for incomplete candidates (before even fetching them). + fn check_potential( &self, - storage: &CandidateStorage, - ignore_candidate: Option<&CandidateHash>, - ) -> Vec { - let mut candidates = vec![]; - for candidate in storage.candidates() { - if let Some(ignore_candidate) = ignore_candidate { - if ignore_candidate == &candidate.candidate_hash { - continue - } - } - // We stop at max_depth + 1 with the search. There's no point in looping further. - if (self.chain.len() + candidates.len()) > self.scope.max_depth { - break - } - if !self.candidates.contains(&candidate.candidate_hash) && - self.check_potential( - &candidate.relay_parent, - candidate.candidate.persisted_validation_data.parent_head.hash(), - Some(candidate.candidate.commitments.head_data.hash()), - ) { - candidates.push(candidate.candidate_hash); + candidate: &impl HypotheticalOrConcreteCandidate, + ) -> Result<(), Error> { + let relay_parent = candidate.relay_parent(); + let parent_head_hash = candidate.parent_head_data_hash(); + + // trivial 0-length cycle. + if let Some(output_head_hash) = candidate.output_head_data_hash() { + if parent_head_hash == output_head_hash { + return Err(Error::ZeroLengthCycle) } } - candidates - } + // Check if the relay parent is in scope. + let Some(relay_parent) = self.scope.ancestor(&relay_parent) else { + return Err(Error::RelayParentNotInScope( + relay_parent, + self.scope.earliest_relay_parent().hash, + )) + }; - // Check if adding a candidate which transitions `parent_head_hash` to `output_head_hash` would - // introduce a fork or a cycle in the parachain. - // `output_head_hash` is optional because we sometimes make this check before retrieving the - // collation. - fn is_fork_or_cycle(&self, parent_head_hash: Hash, output_head_hash: Option) -> bool { - if self.by_parent_head.contains_key(&parent_head_hash) { - // fork. our parent has another child already - return true + // Check if the relay parent moved backwards from the latest candidate pending availability. + let earliest_rp_of_pending_availability = self.earliest_relay_parent_pending_availability(); + if relay_parent.number < earliest_rp_of_pending_availability.number { + return Err(Error::RelayParentPrecedesCandidatePendingAvailability( + relay_parent.hash, + earliest_rp_of_pending_availability.hash, + )) } - if let Some(output_head_hash) = output_head_hash { - if self.by_output_head.contains_key(&output_head_hash) { - // this is not a chain, there are multiple paths to the same state. - return true + // If it's a fork with a backed candidate in the current chain. + if let Some(other_candidate) = self.best_chain.by_parent_head.get(&parent_head_hash) { + if self.scope().get_pending_availability(other_candidate).is_some() { + // Cannot accept a fork with a candidate pending availability. + return Err(Error::ForkWithCandidatePendingAvailability(*other_candidate)) } - // trivial 0-length cycle. - if parent_head_hash == output_head_hash { - return true - } - - // this should catch any other cycles. our output state cannot already be the parent - // state of another candidate, unless this is a cycle, since the already added - // candidates form a chain. - if self.by_parent_head.contains_key(&output_head_hash) { - return true + // If the candidate is backed and in the current chain, accept only a candidate + // according to the fork selection rule. + if fork_selection_rule(other_candidate, &candidate.candidate_hash()) == Ordering::Less { + return Err(Error::ForkChoiceRule(*other_candidate)) } } - false - } + // Try seeing if the parent candidate is in the current chain or if it is the latest + // included candidate. If so, get the constraints the candidate must satisfy. + let (constraints, maybe_min_relay_parent_number) = + if let Some(parent_candidate) = self.best_chain.by_output_head.get(&parent_head_hash) { + let Some(parent_candidate) = + self.best_chain.chain.iter().find(|c| &c.candidate_hash == parent_candidate) + else { + // Should never really happen. + return Err(Error::ParentCandidateNotFound) + }; - // Checks the potential of a candidate to be added to the chain in the future. - // Verifies that the relay parent is in scope and not moving backwards and that we're not - // introducing forks or cycles with other candidates in the chain. - // `output_head_hash` is optional because we sometimes make this check before retrieving the - // collation. - fn check_potential( - &self, - relay_parent: &Hash, - parent_head_hash: Hash, - output_head_hash: Option, - ) -> bool { - if self.is_fork_or_cycle(parent_head_hash, output_head_hash) { - return false + ( + self.scope + .base_constraints + .apply_modifications(&parent_candidate.cumulative_modifications) + .map_err(Error::ComputeConstraints)?, + self.scope.ancestor(&parent_candidate.relay_parent()).map(|rp| rp.number), + ) + } else if self.scope.base_constraints.required_parent.hash() == parent_head_hash { + // It builds on the latest included candidate. + (self.scope.base_constraints.clone(), None) + } else { + // If the parent is not yet part of the chain, there's nothing else we can check for + // now. + return Ok(()) + }; + + // Check for cycles or invalid tree transitions. + if let Some(ref output_head_hash) = candidate.output_head_data_hash() { + self.check_cycles_or_invalid_tree(output_head_hash)?; } - let Some(earliest_rp) = self.earliest_relay_parent() else { return false }; + // Check against constraints if we have a full concrete candidate. + if let (Some(commitments), Some(pvd), Some(validation_code_hash)) = ( + candidate.commitments(), + candidate.persisted_validation_data(), + candidate.validation_code_hash(), + ) { + Fragment::check_against_constraints( + &relay_parent, + &constraints, + commitments, + validation_code_hash, + pvd, + ) + .map_err(Error::CheckAgainstConstraints)?; + } - let Some(relay_parent) = self.scope.ancestor(relay_parent) else { return false }; + if relay_parent.number < constraints.min_relay_parent_number { + return Err(Error::RelayParentMovedBackwards) + } - if relay_parent.number < earliest_rp.number { - return false // relay parent moved backwards. + if let Some(earliest_rp) = maybe_min_relay_parent_number { + if relay_parent.number < earliest_rp { + return Err(Error::RelayParentMovedBackwards) + } } - true + Ok(()) } - // Populate the fragment chain with candidates from CandidateStorage. - // Can be called by the constructor or when introducing a new candidate. - // If we're introducing a new candidate onto an existing chain, we may introduce more than one, - // since we may connect already existing candidates to the chain. - fn populate_chain(&mut self, storage: &CandidateStorage) { - let mut cumulative_modifications = if let Some(last_candidate) = self.chain.last() { - last_candidate.cumulative_modifications.clone() + // Once the backable chain was populated, trim the forks generated by candidates which + // are not present in the best chain. Fan this out into a full breadth-first search. + // If `starting_point` is `Some()`, start the search from the candidates having this parent head + // hash. + fn trim_uneligible_forks(&self, storage: &mut CandidateStorage, starting_point: Option) { + // Start out with the candidates in the chain. They are all valid candidates. + let mut queue: VecDeque<_> = if let Some(starting_point) = starting_point { + [(starting_point, true)].into_iter().collect() } else { - ConstraintModifications::identity() + if self.best_chain.chain.is_empty() { + [(self.scope.base_constraints.required_parent.hash(), true)] + .into_iter() + .collect() + } else { + self.best_chain.chain.iter().map(|c| (c.parent_head_data_hash, true)).collect() + } }; + // To make sure that cycles don't make us loop forever, keep track of the visited parent + // heads. + let mut visited = HashSet::new(); + + while let Some((parent, parent_has_potential)) = queue.pop_front() { + visited.insert(parent); + + let Some(children) = storage.by_parent_head.get(&parent) else { continue }; + // Cannot remove while iterating so store them here temporarily. + let mut to_remove = vec![]; + + for child_hash in children.iter() { + let Some(child) = storage.by_candidate_hash.get(child_hash) else { continue }; + + // Already visited this parent. Either is a cycle or multiple paths that lead to the + // same candidate. Either way, stop this branch to avoid looping forever. + if visited.contains(&child.output_head_data_hash) { + continue + } + + // Only keep a candidate if its full ancestry was already kept as potential and this + // candidate itself has potential. + if parent_has_potential && self.check_potential(child).is_ok() { + queue.push_back((child.output_head_data_hash, true)); + } else { + // Otherwise, remove this candidate and continue looping for its children, but + // mark the parent's potential as `false`. We only want to remove its + // children. + to_remove.push(*child_hash); + queue.push_back((child.output_head_data_hash, false)); + } + } + + for hash in to_remove { + storage.remove_candidate(&hash); + } + } + } + + // Populate the fragment chain with candidates from the supplied `CandidateStorage`. + // Can be called by the constructor or when backing a new candidate. + // When this is called, it may cause the previous chain to be completely erased or it may add + // more than one candidate. + fn populate_chain(&mut self, storage: &mut CandidateStorage) { + let mut cumulative_modifications = + if let Some(last_candidate) = self.best_chain.chain.last() { + last_candidate.cumulative_modifications.clone() + } else { + ConstraintModifications::identity() + }; let Some(mut earliest_rp) = self.earliest_relay_parent() else { return }; loop { - if self.chain.len() > self.scope.max_depth { + if self.best_chain.chain.len() > self.scope.max_depth { break; } @@ -880,113 +1201,157 @@ impl FragmentChain { }; let required_head_hash = child_constraints.required_parent.hash(); - // Even though we don't allow parachain forks under the same active leaf, they may still - // appear under different relay chain forks, hence the iterator below. - let possible_children = storage.possible_para_children(&required_head_hash); - let mut added_child = false; - for candidate in possible_children { - // Add one node to chain if - // 1. it does not introduce a fork or a cycle. - // 2. parent hash is correct. - // 3. relay-parent does not move backwards. - // 4. all non-pending-availability candidates have relay-parent in scope. - // 5. candidate outputs fulfill constraints - - if self.is_fork_or_cycle( - candidate.parent_head_data_hash(), - Some(candidate.output_head_data_hash()), - ) { - continue - } - - let pending = self.scope.get_pending_availability(&candidate.candidate_hash); - let Some(relay_parent) = pending - .map(|p| p.relay_parent.clone()) - .or_else(|| self.scope.ancestor(&candidate.relay_parent)) - else { - continue - }; - // require: candidates don't move backwards - // and only pending availability candidates can be out-of-scope. - // - // earliest_rp can be before the earliest relay parent in the scope - // when the parent is a pending availability candidate as well, but - // only other pending candidates can have a relay parent out of scope. - let min_relay_parent_number = pending - .map(|p| match self.chain.len() { - 0 => p.relay_parent.number, - _ => earliest_rp.number, - }) - .unwrap_or_else(|| earliest_rp.number); - - if relay_parent.number < min_relay_parent_number { - continue // relay parent moved backwards. - } + // Select the few possible backed/backable children which can be added to the chain + // right now. + let possible_children = storage + .possible_backed_para_children(&required_head_hash) + .filter_map(|candidate| { + // Only select a candidate if: + // 1. it does not introduce a fork or a cycle. + // 2. parent hash is correct. + // 3. relay-parent does not move backwards. + // 4. all non-pending-availability candidates have relay-parent in scope. + // 5. candidate outputs fulfill constraints + + let pending = self.scope.get_pending_availability(&candidate.candidate_hash); + let Some(relay_parent) = pending + .map(|p| p.relay_parent.clone()) + .or_else(|| self.scope.ancestor(&candidate.relay_parent)) + else { + return None + }; + + if self.check_cycles_or_invalid_tree(&candidate.output_head_data_hash).is_err() + { + return None + } - // don't add candidates if they're already present in the chain. - // this can never happen, as candidates can only be duplicated if there's a cycle - // and we shouldn't have allowed for a cycle to be chained. - if self.contains_candidate(&candidate.candidate_hash) { - continue - } + // require: candidates don't move backwards + // and only pending availability candidates can be out-of-scope. + // + // earliest_rp can be before the earliest relay parent in the scope + // when the parent is a pending availability candidate as well, but + // only other pending candidates can have a relay parent out of scope. + let min_relay_parent_number = pending + .map(|p| match self.best_chain.chain.len() { + 0 => p.relay_parent.number, + _ => earliest_rp.number, + }) + .unwrap_or_else(|| earliest_rp.number); + + if relay_parent.number < min_relay_parent_number { + return None // relay parent moved backwards. + } - let fragment = { - let mut constraints = child_constraints.clone(); - if let Some(ref p) = pending { - // overwrite for candidates pending availability as a special-case. - constraints.min_relay_parent_number = p.relay_parent.number; + // don't add candidates if they're already present in the chain. + // this can never happen, as candidates can only be duplicated if there's a + // cycle and we shouldn't have allowed for a cycle to be chained. + if self.best_chain.contains(&candidate.candidate_hash) { + return None } - let f = Fragment::new( - relay_parent.clone(), - constraints, - // It's cheap to clone because it's wrapped in an Arc - candidate.candidate.clone(), - ); - - match f { - Ok(f) => f, - Err(e) => { - gum::debug!( - target: LOG_TARGET, - err = ?e, - ?relay_parent, - candidate_hash = ?candidate.candidate_hash, - "Failed to instantiate fragment", - ); - - break - }, + let fragment = { + let mut constraints = child_constraints.clone(); + if let Some(ref p) = pending { + // overwrite for candidates pending availability as a special-case. + constraints.min_relay_parent_number = p.relay_parent.number; + } + + let f = Fragment::new( + relay_parent.clone(), + constraints, + // It's cheap to clone because it's wrapped in an Arc + candidate.candidate.clone(), + ); + + match f { + Ok(f) => f, + Err(e) => { + gum::debug!( + target: LOG_TARGET, + err = ?e, + ?relay_parent, + candidate_hash = ?candidate.candidate_hash, + "Failed to instantiate fragment", + ); + + return None + }, + } + }; + + Some(( + fragment, + candidate.candidate_hash, + candidate.output_head_data_hash, + candidate.parent_head_data_hash, + )) + }); + + // Choose the best candidate. + let best_candidate = + possible_children.min_by(|(_, ref child1, _, _), (_, ref child2, _, _)| { + // Always pick a candidate pending availability as best. + if self.scope.get_pending_availability(child1).is_some() { + Ordering::Less + } else if self.scope.get_pending_availability(child2).is_some() { + Ordering::Greater + } else { + // Otherwise, use the fork selection rule. + fork_selection_rule(child1, child2) } - }; + }); + + if let Some((fragment, candidate_hash, output_head_data_hash, parent_head_data_hash)) = + best_candidate + { + // Remove the candidate from storage. + storage.remove_candidate(&candidate_hash); // Update the cumulative constraint modifications. cumulative_modifications.stack(fragment.constraint_modifications()); // Update the earliest rp - earliest_rp = relay_parent; + earliest_rp = fragment.relay_parent().clone(); let node = FragmentNode { fragment, - candidate_hash: candidate.candidate_hash, + candidate_hash, + parent_head_data_hash, + output_head_data_hash, cumulative_modifications: cumulative_modifications.clone(), }; - self.chain.push(node); - self.candidates.insert(candidate.candidate_hash); - // We've already checked for forks and cycles. - self.by_parent_head - .insert(candidate.parent_head_data_hash(), candidate.candidate_hash); - self.by_output_head - .insert(candidate.output_head_data_hash(), candidate.candidate_hash); - added_child = true; - // We can only add one child for a candidate. (it's a chain, not a tree) - break; - } - - if !added_child { + // Add the candidate to the chain now. + self.best_chain.push(node); + } else { break } } } + + // Revert the best backable chain so that the last candidate will be one outputting the given + // `parent_head_hash`. If the `parent_head_hash` is exactly the required parent of the base + // constraints (builds on the latest included candidate), revert the entire chain. + // Return false if we couldn't find the parent head hash. + fn revert_to(&mut self, parent_head_hash: &Hash) -> bool { + let mut removed_items = None; + if &self.scope.base_constraints.required_parent.hash() == parent_head_hash { + removed_items = Some(self.best_chain.clear()); + } + + if removed_items.is_none() && self.best_chain.by_output_head.contains_key(parent_head_hash) + { + removed_items = Some(self.best_chain.revert_to_parent_hash(parent_head_hash).collect()); + } + + let Some(removed_items) = removed_items else { return false }; + + // Even if it's empty, we need to return true, because we'll be able to add a new candidate + // to the chain. + for node in &removed_items { + let _ = self.unconnected.add_candidate_entry(node.into()); + } + true + } } diff --git a/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs b/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs index 26ee94d59d8e..9886d19e5224 100644 --- a/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs +++ b/polkadot/node/core/prospective-parachains/src/fragment_chain/tests.rs @@ -17,8 +17,12 @@ use super::*; use assert_matches::assert_matches; use polkadot_node_subsystem_util::inclusion_emulator::InboundHrmpLimitations; -use polkadot_primitives::{BlockNumber, CandidateCommitments, CandidateDescriptor, HeadData}; +use polkadot_primitives::{ + BlockNumber, CandidateCommitments, CandidateDescriptor, HeadData, Id as ParaId, +}; use polkadot_primitives_test_helpers as test_helpers; +use rand::{seq::SliceRandom, thread_rng}; +use std::ops::Range; fn make_constraints( min_relay_parent_number: BlockNumber, @@ -54,7 +58,7 @@ fn make_committed_candidate( let persisted_validation_data = PersistedValidationData { parent_head, relay_parent_number, - relay_parent_storage_root: Hash::repeat_byte(69), + relay_parent_storage_root: Hash::zero(), max_pov_size: 1_000_000, }; @@ -83,9 +87,20 @@ fn make_committed_candidate( (persisted_validation_data, candidate) } +fn populate_chain_from_previous_storage( + scope: &Scope, + storage: &CandidateStorage, +) -> FragmentChain { + let mut chain = FragmentChain::init(scope.clone(), CandidateStorage::default()); + let mut prev_chain = chain.clone(); + prev_chain.unconnected = storage.clone(); + + chain.populate_from_previous(&prev_chain); + chain +} + #[test] fn scope_rejects_ancestors_that_skip_blocks() { - let para_id = ParaId::from(5u32); let relay_parent = RelayChainBlockInfo { number: 10, hash: Hash::repeat_byte(10), @@ -104,7 +119,6 @@ fn scope_rejects_ancestors_that_skip_blocks() { assert_matches!( Scope::with_ancestors( - para_id, relay_parent, base_constraints, pending_availability, @@ -117,7 +131,6 @@ fn scope_rejects_ancestors_that_skip_blocks() { #[test] fn scope_rejects_ancestor_for_0_block() { - let para_id = ParaId::from(5u32); let relay_parent = RelayChainBlockInfo { number: 0, hash: Hash::repeat_byte(0), @@ -136,7 +149,6 @@ fn scope_rejects_ancestor_for_0_block() { assert_matches!( Scope::with_ancestors( - para_id, relay_parent, base_constraints, pending_availability, @@ -149,7 +161,6 @@ fn scope_rejects_ancestor_for_0_block() { #[test] fn scope_only_takes_ancestors_up_to_min() { - let para_id = ParaId::from(5u32); let relay_parent = RelayChainBlockInfo { number: 5, hash: Hash::repeat_byte(0), @@ -179,7 +190,6 @@ fn scope_only_takes_ancestors_up_to_min() { let pending_availability = Vec::new(); let scope = Scope::with_ancestors( - para_id, relay_parent, base_constraints, pending_availability, @@ -194,7 +204,6 @@ fn scope_only_takes_ancestors_up_to_min() { #[test] fn scope_rejects_unordered_ancestors() { - let para_id = ParaId::from(5u32); let relay_parent = RelayChainBlockInfo { number: 5, hash: Hash::repeat_byte(0), @@ -225,7 +234,6 @@ fn scope_rejects_unordered_ancestors() { assert_matches!( Scope::with_ancestors( - para_id, relay_parent, base_constraints, pending_availability, @@ -257,718 +265,695 @@ fn candidate_storage_methods() { let mut wrong_pvd = pvd.clone(); wrong_pvd.max_pov_size = 0; assert_matches!( - storage.add_candidate(candidate.clone(), wrong_pvd, CandidateState::Seconded), - Err(CandidateStorageInsertionError::PersistedValidationDataMismatch) + CandidateEntry::new( + candidate_hash, + candidate.clone(), + wrong_pvd.clone(), + CandidateState::Seconded + ), + Err(CandidateEntryError::PersistedValidationDataMismatch) + ); + assert_matches!( + CandidateEntry::new_seconded(candidate_hash, candidate.clone(), wrong_pvd), + Err(CandidateEntryError::PersistedValidationDataMismatch) ); + // Zero-length cycle. + { + let mut candidate = candidate.clone(); + candidate.commitments.head_data = HeadData(vec![1; 10]); + let mut pvd = pvd.clone(); + pvd.parent_head = HeadData(vec![1; 10]); + candidate.descriptor.persisted_validation_data_hash = pvd.hash(); + assert_matches!( + CandidateEntry::new_seconded(candidate_hash, candidate, pvd), + Err(CandidateEntryError::ZeroLengthCycle) + ); + } assert!(!storage.contains(&candidate_hash)); - assert_eq!(storage.possible_para_children(&parent_head_hash).count(), 0); - assert_eq!(storage.relay_parent_of_candidate(&candidate_hash), None); + assert_eq!(storage.possible_backed_para_children(&parent_head_hash).count(), 0); assert_eq!(storage.head_data_by_hash(&candidate.descriptor.para_head), None); assert_eq!(storage.head_data_by_hash(&parent_head_hash), None); - assert_eq!(storage.is_backed(&candidate_hash), false); - // Add a valid candidate - storage - .add_candidate(candidate.clone(), pvd.clone(), CandidateState::Seconded) - .unwrap(); + // Add a valid candidate. + let candidate_entry = CandidateEntry::new( + candidate_hash, + candidate.clone(), + pvd.clone(), + CandidateState::Seconded, + ) + .unwrap(); + storage.add_candidate_entry(candidate_entry.clone()).unwrap(); assert!(storage.contains(&candidate_hash)); - assert_eq!(storage.possible_para_children(&parent_head_hash).count(), 1); - assert_eq!(storage.possible_para_children(&candidate.descriptor.para_head).count(), 0); - assert_eq!(storage.relay_parent_of_candidate(&candidate_hash), Some(relay_parent)); + assert_eq!(storage.possible_backed_para_children(&parent_head_hash).count(), 0); + assert_eq!(storage.possible_backed_para_children(&candidate.descriptor.para_head).count(), 0); assert_eq!( storage.head_data_by_hash(&candidate.descriptor.para_head).unwrap(), &candidate.commitments.head_data ); assert_eq!(storage.head_data_by_hash(&parent_head_hash).unwrap(), &pvd.parent_head); - assert_eq!(storage.is_backed(&candidate_hash), false); + // Now mark it as backed + storage.mark_backed(&candidate_hash); + // Marking it twice is fine. storage.mark_backed(&candidate_hash); - assert_eq!(storage.is_backed(&candidate_hash), true); + assert_eq!( + storage + .possible_backed_para_children(&parent_head_hash) + .map(|c| c.candidate_hash) + .collect::>(), + vec![candidate_hash] + ); + assert_eq!(storage.possible_backed_para_children(&candidate.descriptor.para_head).count(), 0); // Re-adding a candidate fails. assert_matches!( - storage.add_candidate(candidate.clone(), pvd.clone(), CandidateState::Seconded), - Err(CandidateStorageInsertionError::CandidateAlreadyKnown(hash)) if candidate_hash == hash + storage.add_candidate_entry(candidate_entry), + Err(Error::CandidateAlreadyKnown) ); // Remove candidate and re-add it later in backed state. storage.remove_candidate(&candidate_hash); assert!(!storage.contains(&candidate_hash)); - assert_eq!(storage.possible_para_children(&parent_head_hash).count(), 0); - assert_eq!(storage.relay_parent_of_candidate(&candidate_hash), None); + + // Removing it twice is fine. + storage.remove_candidate(&candidate_hash); + assert!(!storage.contains(&candidate_hash)); + assert_eq!(storage.possible_backed_para_children(&parent_head_hash).count(), 0); assert_eq!(storage.head_data_by_hash(&candidate.descriptor.para_head), None); assert_eq!(storage.head_data_by_hash(&parent_head_hash), None); - assert_eq!(storage.is_backed(&candidate_hash), false); storage - .add_candidate(candidate.clone(), pvd.clone(), CandidateState::Backed) + .add_pending_availability_candidate(candidate_hash, candidate.clone(), pvd) .unwrap(); - assert_eq!(storage.is_backed(&candidate_hash), true); - - // Test retain - storage.retain(|_| true); assert!(storage.contains(&candidate_hash)); - storage.retain(|_| false); - assert!(!storage.contains(&candidate_hash)); - assert_eq!(storage.possible_para_children(&parent_head_hash).count(), 0); - assert_eq!(storage.relay_parent_of_candidate(&candidate_hash), None); - assert_eq!(storage.head_data_by_hash(&candidate.descriptor.para_head), None); - assert_eq!(storage.head_data_by_hash(&parent_head_hash), None); - assert_eq!(storage.is_backed(&candidate_hash), false); + + assert_eq!( + storage + .possible_backed_para_children(&parent_head_hash) + .map(|c| c.candidate_hash) + .collect::>(), + vec![candidate_hash] + ); + assert_eq!(storage.possible_backed_para_children(&candidate.descriptor.para_head).count(), 0); + + // Now add a second candidate in Seconded state. This will be a fork. + let (pvd_2, candidate_2) = make_committed_candidate( + ParaId::from(5u32), + relay_parent, + 8, + vec![4, 5, 6].into(), + vec![2, 3, 4].into(), + 7, + ); + let candidate_hash_2 = candidate_2.hash(); + let candidate_entry_2 = + CandidateEntry::new_seconded(candidate_hash_2, candidate_2, pvd_2).unwrap(); + + storage.add_candidate_entry(candidate_entry_2).unwrap(); + assert_eq!( + storage + .possible_backed_para_children(&parent_head_hash) + .map(|c| c.candidate_hash) + .collect::>(), + vec![candidate_hash] + ); + + // Now mark it as backed. + storage.mark_backed(&candidate_hash_2); + assert_eq!( + storage + .possible_backed_para_children(&parent_head_hash) + .map(|c| c.candidate_hash) + .collect::>(), + [candidate_hash, candidate_hash_2].into_iter().collect() + ); } #[test] -fn populate_and_extend_from_storage_empty() { +fn init_and_populate_from_empty() { // Empty chain and empty storage. - let storage = CandidateStorage::default(); let base_constraints = make_constraints(0, vec![0], vec![0x0a].into()); - let pending_availability = Vec::new(); let scope = Scope::with_ancestors( - ParaId::from(2), RelayChainBlockInfo { number: 1, hash: Hash::repeat_byte(1), storage_root: Hash::repeat_byte(2), }, base_constraints, - pending_availability, + Vec::new(), 4, vec![], ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); - assert!(chain.to_vec().is_empty()); - - chain.extend_from_storage(&storage); - assert!(chain.to_vec().is_empty()); + let chain = FragmentChain::init(scope.clone(), CandidateStorage::default()); + assert_eq!(chain.best_chain_len(), 0); + assert_eq!(chain.unconnected_len(), 0); + + let mut new_chain = FragmentChain::init(scope, CandidateStorage::default()); + new_chain.populate_from_previous(&chain); + assert_eq!(chain.best_chain_len(), 0); + assert_eq!(chain.unconnected_len(), 0); } #[test] -fn populate_and_extend_from_storage_with_existing_empty_to_vec() { +fn test_populate_and_check_potential() { let mut storage = CandidateStorage::default(); let para_id = ParaId::from(5u32); - let relay_parent_a = Hash::repeat_byte(1); - let relay_parent_b = Hash::repeat_byte(2); - let relay_parent_c = Hash::repeat_byte(3); + let relay_parent_x = Hash::repeat_byte(1); + let relay_parent_y = Hash::repeat_byte(2); + let relay_parent_z = Hash::repeat_byte(3); + let relay_parent_x_info = + RelayChainBlockInfo { number: 0, hash: relay_parent_x, storage_root: Hash::zero() }; + let relay_parent_y_info = + RelayChainBlockInfo { number: 1, hash: relay_parent_y, storage_root: Hash::zero() }; + let relay_parent_z_info = + RelayChainBlockInfo { number: 2, hash: relay_parent_z, storage_root: Hash::zero() }; + + let ancestors = vec![ + // These need to be ordered in reverse. + relay_parent_y_info.clone(), + relay_parent_x_info.clone(), + ]; + + let base_constraints = make_constraints(0, vec![0], vec![0x0a].into()); + // Candidates A -> B -> C. They are all backed let (pvd_a, candidate_a) = make_committed_candidate( para_id, - relay_parent_a, - 0, + relay_parent_x_info.hash, + relay_parent_x_info.number, vec![0x0a].into(), vec![0x0b].into(), - 0, + relay_parent_x_info.number, ); let candidate_a_hash = candidate_a.hash(); - + let candidate_a_entry = + CandidateEntry::new(candidate_a_hash, candidate_a, pvd_a.clone(), CandidateState::Backed) + .unwrap(); + storage.add_candidate_entry(candidate_a_entry.clone()).unwrap(); let (pvd_b, candidate_b) = make_committed_candidate( para_id, - relay_parent_b, - 1, + relay_parent_y_info.hash, + relay_parent_y_info.number, vec![0x0b].into(), vec![0x0c].into(), - 1, + relay_parent_y_info.number, ); let candidate_b_hash = candidate_b.hash(); - + let candidate_b_entry = + CandidateEntry::new(candidate_b_hash, candidate_b, pvd_b, CandidateState::Backed).unwrap(); + storage.add_candidate_entry(candidate_b_entry.clone()).unwrap(); let (pvd_c, candidate_c) = make_committed_candidate( para_id, - relay_parent_c, - 2, + relay_parent_z_info.hash, + relay_parent_z_info.number, vec![0x0c].into(), vec![0x0d].into(), - 2, + relay_parent_z_info.number, ); let candidate_c_hash = candidate_c.hash(); - - let relay_parent_a_info = RelayChainBlockInfo { - number: pvd_a.relay_parent_number, - hash: relay_parent_a, - storage_root: pvd_a.relay_parent_storage_root, - }; - let relay_parent_b_info = RelayChainBlockInfo { - number: pvd_b.relay_parent_number, - hash: relay_parent_b, - storage_root: pvd_b.relay_parent_storage_root, - }; - let relay_parent_c_info = RelayChainBlockInfo { - number: pvd_c.relay_parent_number, - hash: relay_parent_c, - storage_root: pvd_c.relay_parent_storage_root, - }; - - let base_constraints = make_constraints(0, vec![0], vec![0x0a].into()); - let pending_availability = Vec::new(); - - let ancestors = vec![ - // These need to be ordered in reverse. - relay_parent_b_info.clone(), - relay_parent_a_info.clone(), - ]; - - storage - .add_candidate(candidate_a.clone(), pvd_a.clone(), CandidateState::Seconded) - .unwrap(); - storage - .add_candidate(candidate_b.clone(), pvd_b.clone(), CandidateState::Backed) - .unwrap(); - storage - .add_candidate(candidate_c.clone(), pvd_c.clone(), CandidateState::Backed) - .unwrap(); + let candidate_c_entry = + CandidateEntry::new(candidate_c_hash, candidate_c, pvd_c, CandidateState::Backed).unwrap(); + storage.add_candidate_entry(candidate_c_entry.clone()).unwrap(); // Candidate A doesn't adhere to the base constraints. { for wrong_constraints in [ // Different required parent - make_constraints(0, vec![0], vec![0x0e].into()), + make_constraints( + relay_parent_x_info.number, + vec![relay_parent_x_info.number], + vec![0x0e].into(), + ), // Min relay parent number is wrong - make_constraints(1, vec![0], vec![0x0a].into()), + make_constraints(relay_parent_y_info.number, vec![0], vec![0x0a].into()), ] { let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + relay_parent_z_info.clone(), wrong_constraints.clone(), - pending_availability.clone(), + vec![], 4, ancestors.clone(), ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); + let chain = populate_chain_from_previous_storage(&scope, &storage); - assert!(chain.to_vec().is_empty()); - - chain.extend_from_storage(&storage); - assert!(chain.to_vec().is_empty()); + assert!(chain.best_chain_vec().is_empty()); // If the min relay parent number is wrong, candidate A can never become valid. // Otherwise, if only the required parent doesn't match, candidate A is still a // potential candidate. - if wrong_constraints.min_relay_parent_number == 1 { - assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate_a.hash(), - &candidate_a.descriptor.relay_parent, - pvd_a.parent_head.hash(), - Some(candidate_a.commitments.head_data.hash()), - ), - PotentialAddition::None + if wrong_constraints.min_relay_parent_number == relay_parent_y_info.number { + // If A is not a potential candidate, its descendants will also not be added. + assert_eq!(chain.unconnected_len(), 0); + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_a_entry), + Err(Error::RelayParentNotInScope(_, _)) ); + // However, if taken independently, both B and C still have potential, since we + // don't know that A doesn't. + assert!(chain.can_add_candidate_as_potential(&candidate_b_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_c_entry).is_ok()); } else { assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate_a.hash(), - &candidate_a.descriptor.relay_parent, - pvd_a.parent_head.hash(), - Some(candidate_a.commitments.head_data.hash()), - ), - PotentialAddition::Anyhow - ); - } - - // All other candidates can always be potential candidates. - for (candidate, pvd) in - [(candidate_b.clone(), pvd_b.clone()), (candidate_c.clone(), pvd_c.clone())] - { - assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::Anyhow + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_a_hash, candidate_b_hash, candidate_c_hash].into_iter().collect() ); } } } - // Various max depths. + // Various depths { - // depth is 0, will only allow 1 candidate + // Depth is 0, only allows one candidate, but the others will be kept as potential. let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + relay_parent_z_info.clone(), base_constraints.clone(), - pending_availability.clone(), + vec![], 0, ancestors.clone(), ) .unwrap(); - // Before populating the chain, all candidates are potential candidates. However, they can - // only be added as connected candidates, because only one candidates is allowed by max - // depth - let chain = FragmentChain::populate(scope.clone(), &CandidateStorage::default()); - for (candidate, pvd) in [ - (candidate_a.clone(), pvd_a.clone()), - (candidate_b.clone(), pvd_b.clone()), - (candidate_c.clone(), pvd_c.clone()), - ] { - assert_eq!( - chain.can_add_candidate_as_potential( - &CandidateStorage::default(), - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::IfConnected - ); - } - let mut chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash]); - chain.extend_from_storage(&storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash]); - // since depth is maxed out, we can't add more potential candidates - // candidate A is no longer a potential candidate because it's already present. - for (candidate, pvd) in [ - (candidate_a.clone(), pvd_a.clone()), - (candidate_b.clone(), pvd_b.clone()), - (candidate_c.clone(), pvd_c.clone()), - ] { - assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::None - ); - } + let chain = FragmentChain::init(scope.clone(), CandidateStorage::default()); + assert!(chain.can_add_candidate_as_potential(&candidate_a_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_b_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_c_entry).is_ok()); + + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash]); + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_b_hash, candidate_c_hash].into_iter().collect() + ); // depth is 1, allows two candidates let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + relay_parent_z_info.clone(), base_constraints.clone(), - pending_availability.clone(), + vec![], 1, ancestors.clone(), ) .unwrap(); - // Before populating the chain, all candidates can be added as potential. - let mut modified_storage = CandidateStorage::default(); - let chain = FragmentChain::populate(scope.clone(), &modified_storage); - for (candidate, pvd) in [ - (candidate_a.clone(), pvd_a.clone()), - (candidate_b.clone(), pvd_b.clone()), - (candidate_c.clone(), pvd_c.clone()), - ] { - assert_eq!( - chain.can_add_candidate_as_potential( - &modified_storage, - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::Anyhow - ); - } - // Add an unconnected candidate. We now should only allow a Connected candidate, because max - // depth only allows one more candidate. - modified_storage - .add_candidate(candidate_b.clone(), pvd_b.clone(), CandidateState::Seconded) - .unwrap(); - let chain = FragmentChain::populate(scope.clone(), &modified_storage); - for (candidate, pvd) in - [(candidate_a.clone(), pvd_a.clone()), (candidate_c.clone(), pvd_c.clone())] - { - assert_eq!( - chain.can_add_candidate_as_potential( - &modified_storage, - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::IfConnected - ); - } + let chain = FragmentChain::init(scope.clone(), CandidateStorage::default()); + assert!(chain.can_add_candidate_as_potential(&candidate_a_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_b_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_c_entry).is_ok()); - // Now try populating from all candidates. - let mut chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - chain.extend_from_storage(&storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - // since depth is maxed out, we can't add more potential candidates - // candidate A and B are no longer a potential candidate because they're already present. - for (candidate, pvd) in [ - (candidate_a.clone(), pvd_a.clone()), - (candidate_b.clone(), pvd_b.clone()), - (candidate_c.clone(), pvd_c.clone()), - ] { - assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::None - ); - } + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash, candidate_b_hash]); + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_c_hash].into_iter().collect() + ); - // depths larger than 2, allows all candidates + // depth is larger than 2, allows all three candidates for depth in 2..6 { let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + relay_parent_z_info.clone(), base_constraints.clone(), - pending_availability.clone(), + vec![], depth, ancestors.clone(), ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); - chain.extend_from_storage(&storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); - // Candidates are no longer potential candidates because they're already part of the - // chain. - for (candidate, pvd) in [ - (candidate_a.clone(), pvd_a.clone()), - (candidate_b.clone(), pvd_b.clone()), - (candidate_c.clone(), pvd_c.clone()), - ] { - assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::None - ); - } + let chain = FragmentChain::init(scope.clone(), CandidateStorage::default()); + assert!(chain.can_add_candidate_as_potential(&candidate_a_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_b_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_c_entry).is_ok()); + + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert_eq!( + chain.best_chain_vec(), + vec![candidate_a_hash, candidate_b_hash, candidate_c_hash] + ); + assert_eq!(chain.unconnected_len(), 0); } } - // Wrong relay parents + // Relay parents out of scope { - // Candidates A has relay parent out of scope. - let ancestors_without_a = vec![relay_parent_b_info.clone()]; + // Candidate A has relay parent out of scope. Candidates B and C will also be deleted since + // they form a chain with A. + let ancestors_without_x = vec![relay_parent_y_info.clone()]; let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + relay_parent_z_info.clone(), base_constraints.clone(), - pending_availability.clone(), + vec![], 4, - ancestors_without_a, + ancestors_without_x, ) .unwrap(); + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert!(chain.best_chain_vec().is_empty()); + assert_eq!(chain.unconnected_len(), 0); - let mut chain = FragmentChain::populate(scope, &storage); - assert!(chain.to_vec().is_empty()); - - chain.extend_from_storage(&storage); - assert!(chain.to_vec().is_empty()); - - // Candidate A is not a potential candidate, but candidates B and C still are. - assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate_a.hash(), - &candidate_a.descriptor.relay_parent, - pvd_a.parent_head.hash(), - Some(candidate_a.commitments.head_data.hash()), - ), - PotentialAddition::None + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_a_entry), + Err(Error::RelayParentNotInScope(_, _)) ); - for (candidate, pvd) in - [(candidate_b.clone(), pvd_b.clone()), (candidate_c.clone(), pvd_c.clone())] - { - assert_eq!( - chain.can_add_candidate_as_potential( - &storage, - &candidate.hash(), - &candidate.descriptor.relay_parent, - pvd.parent_head.hash(), - Some(candidate.commitments.head_data.hash()), - ), - PotentialAddition::Anyhow - ); - } + // However, if taken independently, both B and C still have potential, since we + // don't know that A doesn't. + assert!(chain.can_add_candidate_as_potential(&candidate_b_entry).is_ok()); + assert!(chain.can_add_candidate_as_potential(&candidate_c_entry).is_ok()); - // Candidate C has the same relay parent as candidate A's parent. Relay parent not allowed - // to move backwards - let mut modified_storage = storage.clone(); - modified_storage.remove_candidate(&candidate_c_hash); - let (wrong_pvd_c, wrong_candidate_c) = make_committed_candidate( - para_id, - relay_parent_a, - 1, - vec![0x0c].into(), - vec![0x0d].into(), - 2, - ); - modified_storage - .add_candidate(wrong_candidate_c.clone(), wrong_pvd_c.clone(), CandidateState::Seconded) - .unwrap(); + // Candidates A and B have relay parents out of scope. Candidate C will also be deleted + // since it forms a chain with A and B. let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + relay_parent_z_info.clone(), base_constraints.clone(), - pending_availability.clone(), + vec![], 4, - ancestors.clone(), + vec![], ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - chain.extend_from_storage(&modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); + let chain = populate_chain_from_previous_storage(&scope, &storage); - // Candidate C is not even a potential candidate. - assert_eq!( - chain.can_add_candidate_as_potential( - &modified_storage, - &wrong_candidate_c.hash(), - &wrong_candidate_c.descriptor.relay_parent, - wrong_pvd_c.parent_head.hash(), - Some(wrong_candidate_c.commitments.head_data.hash()), - ), - PotentialAddition::None + assert!(chain.best_chain_vec().is_empty()); + assert_eq!(chain.unconnected_len(), 0); + + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_a_entry), + Err(Error::RelayParentNotInScope(_, _)) + ); + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_b_entry), + Err(Error::RelayParentNotInScope(_, _)) ); + // However, if taken independently, C still has potential, since we + // don't know that A and B don't + assert!(chain.can_add_candidate_as_potential(&candidate_c_entry).is_ok()); } - // Parachain fork and cycles are not allowed. + // Parachain cycle is not allowed. Make C have the same parent as A. { - // Candidate C has the same parent as candidate B. - let mut modified_storage = storage.clone(); - modified_storage.remove_candidate(&candidate_c_hash); - let (wrong_pvd_c, wrong_candidate_c) = make_committed_candidate( - para_id, - relay_parent_c, - 2, - vec![0x0b].into(), - vec![0x0d].into(), - 2, - ); - modified_storage - .add_candidate(wrong_candidate_c.clone(), wrong_pvd_c.clone(), CandidateState::Seconded) - .unwrap(); - let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), - base_constraints.clone(), - pending_availability.clone(), - 4, - ancestors.clone(), - ) - .unwrap(); - let mut chain = FragmentChain::populate(scope, &modified_storage); - // We'll either have A->B or A->C. It's not deterministic because CandidateStorage uses - // HashSets and HashMaps. - if chain.to_vec() == vec![candidate_a_hash, candidate_b_hash] { - chain.extend_from_storage(&modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - // Candidate C is not even a potential candidate. - assert_eq!( - chain.can_add_candidate_as_potential( - &modified_storage, - &wrong_candidate_c.hash(), - &wrong_candidate_c.descriptor.relay_parent, - wrong_pvd_c.parent_head.hash(), - Some(wrong_candidate_c.commitments.head_data.hash()), - ), - PotentialAddition::None - ); - } else if chain.to_vec() == vec![candidate_a_hash, wrong_candidate_c.hash()] { - chain.extend_from_storage(&modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, wrong_candidate_c.hash()]); - // Candidate B is not even a potential candidate. - assert_eq!( - chain.can_add_candidate_as_potential( - &modified_storage, - &candidate_b.hash(), - &candidate_b.descriptor.relay_parent, - pvd_b.parent_head.hash(), - Some(candidate_b.commitments.head_data.hash()), - ), - PotentialAddition::None - ); - } else { - panic!("Unexpected chain: {:?}", chain.to_vec()); - } - - // Candidate C is a 0-length cycle. - // Candidate C has the same parent as candidate B. let mut modified_storage = storage.clone(); modified_storage.remove_candidate(&candidate_c_hash); let (wrong_pvd_c, wrong_candidate_c) = make_committed_candidate( para_id, - relay_parent_c, - 2, - vec![0x0c].into(), + relay_parent_z_info.hash, + relay_parent_z_info.number, vec![0x0c].into(), - 2, + vec![0x0a].into(), + relay_parent_z_info.number, ); - modified_storage - .add_candidate(wrong_candidate_c.clone(), wrong_pvd_c.clone(), CandidateState::Seconded) - .unwrap(); - let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), - base_constraints.clone(), - pending_availability.clone(), - 4, - ancestors.clone(), + let wrong_candidate_c_entry = CandidateEntry::new( + wrong_candidate_c.hash(), + wrong_candidate_c, + wrong_pvd_c, + CandidateState::Backed, ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - chain.extend_from_storage(&modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - // Candidate C is not even a potential candidate. - assert_eq!( - chain.can_add_candidate_as_potential( - &modified_storage, - &wrong_candidate_c.hash(), - &wrong_candidate_c.descriptor.relay_parent, - wrong_pvd_c.parent_head.hash(), - Some(wrong_candidate_c.commitments.head_data.hash()), - ), - PotentialAddition::None - ); - - // Candidate C points back to the pre-state of candidate C. - let mut modified_storage = storage.clone(); - modified_storage.remove_candidate(&candidate_c_hash); - let (wrong_pvd_c, wrong_candidate_c) = make_committed_candidate( - para_id, - relay_parent_c, - 2, - vec![0x0c].into(), - vec![0x0b].into(), - 2, - ); - modified_storage - .add_candidate(wrong_candidate_c.clone(), wrong_pvd_c.clone(), CandidateState::Seconded) - .unwrap(); + modified_storage.add_candidate_entry(wrong_candidate_c_entry.clone()).unwrap(); let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + relay_parent_z_info.clone(), base_constraints.clone(), - pending_availability.clone(), + vec![], 4, ancestors.clone(), ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - chain.extend_from_storage(&modified_storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - // Candidate C is not even a potential candidate. - assert_eq!( - chain.can_add_candidate_as_potential( - &modified_storage, - &wrong_candidate_c.hash(), - &wrong_candidate_c.descriptor.relay_parent, - wrong_pvd_c.parent_head.hash(), - Some(wrong_candidate_c.commitments.head_data.hash()), - ), - PotentialAddition::None + + let chain = populate_chain_from_previous_storage(&scope, &modified_storage); + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash, candidate_b_hash]); + assert_eq!(chain.unconnected_len(), 0); + + assert_matches!( + chain.can_add_candidate_as_potential(&wrong_candidate_c_entry), + Err(Error::Cycle) ); + // However, if taken independently, C still has potential, since we don't know A and B. + let chain = FragmentChain::init(scope.clone(), CandidateStorage::default()); + assert!(chain.can_add_candidate_as_potential(&wrong_candidate_c_entry).is_ok()); } - // Test with candidates pending availability - { - // Valid options - for pending in [ - vec![PendingAvailability { - candidate_hash: candidate_a_hash, - relay_parent: relay_parent_a_info.clone(), - }], - vec![ - PendingAvailability { - candidate_hash: candidate_a_hash, - relay_parent: relay_parent_a_info.clone(), - }, - PendingAvailability { - candidate_hash: candidate_b_hash, - relay_parent: relay_parent_b_info.clone(), - }, - ], - vec![ - PendingAvailability { - candidate_hash: candidate_a_hash, - relay_parent: relay_parent_a_info.clone(), - }, - PendingAvailability { - candidate_hash: candidate_b_hash, - relay_parent: relay_parent_b_info.clone(), - }, - PendingAvailability { - candidate_hash: candidate_c_hash, - relay_parent: relay_parent_c_info.clone(), - }, - ], - ] { - let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), - base_constraints.clone(), - pending, - 3, - ancestors.clone(), - ) - .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); - chain.extend_from_storage(&storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); - } + // Candidate C has the same relay parent as candidate A's parent. Relay parent not allowed + // to move backwards + let mut modified_storage = storage.clone(); + modified_storage.remove_candidate(&candidate_c_hash); + let (wrong_pvd_c, wrong_candidate_c) = make_committed_candidate( + para_id, + relay_parent_x_info.hash, + relay_parent_x_info.number, + vec![0x0c].into(), + vec![0x0d].into(), + 0, + ); + let wrong_candidate_c_entry = CandidateEntry::new( + wrong_candidate_c.hash(), + wrong_candidate_c, + wrong_pvd_c, + CandidateState::Backed, + ) + .unwrap(); + modified_storage.add_candidate_entry(wrong_candidate_c_entry.clone()).unwrap(); + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![], + 4, + ancestors.clone(), + ) + .unwrap(); - // Relay parents of pending availability candidates can be out of scope - // Relay parent of candidate A is out of scope. - let ancestors_without_a = vec![relay_parent_b_info.clone()]; - let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), - base_constraints.clone(), - vec![PendingAvailability { - candidate_hash: candidate_a_hash, - relay_parent: relay_parent_a_info.clone(), - }], - 4, - ancestors_without_a, - ) - .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); - chain.extend_from_storage(&storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); + let chain = populate_chain_from_previous_storage(&scope, &modified_storage); - // Even relay parents of pending availability candidates which are out of scope cannot move - // backwards. - let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info.clone(), + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash, candidate_b_hash]); + assert_eq!(chain.unconnected_len(), 0); + assert_matches!( + chain.can_add_candidate_as_potential(&wrong_candidate_c_entry), + Err(Error::RelayParentMovedBackwards) + ); + + // Candidate C is an unconnected candidate. + // C's relay parent is allowed to move backwards from B's relay parent, because C may later on + // trigger a reorg and B may get removed. + let mut modified_storage = storage.clone(); + modified_storage.remove_candidate(&candidate_c_hash); + let (unconnected_pvd_c, unconnected_candidate_c) = make_committed_candidate( + para_id, + relay_parent_x_info.hash, + relay_parent_x_info.number, + vec![0x0d].into(), + vec![0x0e].into(), + 0, + ); + let unconnected_candidate_c_hash = unconnected_candidate_c.hash(); + let unconnected_candidate_c_entry = CandidateEntry::new( + unconnected_candidate_c_hash, + unconnected_candidate_c, + unconnected_pvd_c, + CandidateState::Backed, + ) + .unwrap(); + modified_storage + .add_candidate_entry(unconnected_candidate_c_entry.clone()) + .unwrap(); + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![], + 4, + ancestors.clone(), + ) + .unwrap(); + let chain = FragmentChain::init(scope.clone(), CandidateStorage::default()); + assert!(chain.can_add_candidate_as_potential(&unconnected_candidate_c_entry).is_ok()); + + let chain = populate_chain_from_previous_storage(&scope, &modified_storage); + + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash, candidate_b_hash]); + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [unconnected_candidate_c_hash].into_iter().collect() + ); + + // Candidate A is a pending availability candidate and Candidate C is an unconnected candidate, + // C's relay parent is not allowed to move backwards from A's relay parent because we're sure A + // will not get removed in the future, as it's already on-chain (unless it times out + // availability, a case for which we don't care to optimise for) + + modified_storage.remove_candidate(&candidate_a_hash); + let (modified_pvd_a, modified_candidate_a) = make_committed_candidate( + para_id, + relay_parent_y_info.hash, + relay_parent_y_info.number, + vec![0x0a].into(), + vec![0x0b].into(), + relay_parent_y_info.number, + ); + let modified_candidate_a_hash = modified_candidate_a.hash(); + modified_storage + .add_candidate_entry( + CandidateEntry::new( + modified_candidate_a_hash, + modified_candidate_a, + modified_pvd_a, + CandidateState::Backed, + ) + .unwrap(), + ) + .unwrap(); + + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![PendingAvailability { + candidate_hash: modified_candidate_a_hash, + relay_parent: relay_parent_y_info.clone(), + }], + 4, + ancestors.clone(), + ) + .unwrap(); + + let chain = populate_chain_from_previous_storage(&scope, &modified_storage); + assert_eq!(chain.best_chain_vec(), vec![modified_candidate_a_hash, candidate_b_hash]); + assert_eq!(chain.unconnected_len(), 0); + assert_matches!( + chain.can_add_candidate_as_potential(&unconnected_candidate_c_entry), + Err(Error::RelayParentPrecedesCandidatePendingAvailability(_, _)) + ); + + // Not allowed to fork from a candidate pending availability + let (wrong_pvd_c, wrong_candidate_c) = make_committed_candidate( + para_id, + relay_parent_y_info.hash, + relay_parent_y_info.number, + vec![0x0a].into(), + vec![0x0b2].into(), + 0, + ); + let wrong_candidate_c_hash = wrong_candidate_c.hash(); + let wrong_candidate_c_entry = CandidateEntry::new( + wrong_candidate_c_hash, + wrong_candidate_c, + wrong_pvd_c, + CandidateState::Backed, + ) + .unwrap(); + modified_storage.add_candidate_entry(wrong_candidate_c_entry.clone()).unwrap(); + + // Does not even matter if the fork selection rule would have picked up the new candidate, as + // the other is already pending availability. + assert_eq!( + fork_selection_rule(&wrong_candidate_c_hash, &modified_candidate_a_hash), + Ordering::Less + ); + + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![PendingAvailability { + candidate_hash: modified_candidate_a_hash, + relay_parent: relay_parent_y_info.clone(), + }], + 4, + ancestors.clone(), + ) + .unwrap(); + + let chain = populate_chain_from_previous_storage(&scope, &modified_storage); + assert_eq!(chain.best_chain_vec(), vec![modified_candidate_a_hash, candidate_b_hash]); + assert_eq!(chain.unconnected_len(), 0); + assert_matches!( + chain.can_add_candidate_as_potential(&wrong_candidate_c_entry), + Err(Error::ForkWithCandidatePendingAvailability(_)) + ); + + // Test with candidates pending availability + { + // Valid options + for pending in [ + vec![PendingAvailability { + candidate_hash: candidate_a_hash, + relay_parent: relay_parent_x_info.clone(), + }], + vec![ + PendingAvailability { + candidate_hash: candidate_a_hash, + relay_parent: relay_parent_x_info.clone(), + }, + PendingAvailability { + candidate_hash: candidate_b_hash, + relay_parent: relay_parent_y_info.clone(), + }, + ], + vec![ + PendingAvailability { + candidate_hash: candidate_a_hash, + relay_parent: relay_parent_x_info.clone(), + }, + PendingAvailability { + candidate_hash: candidate_b_hash, + relay_parent: relay_parent_y_info.clone(), + }, + PendingAvailability { + candidate_hash: candidate_c_hash, + relay_parent: relay_parent_z_info.clone(), + }, + ], + ] { + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + pending, + 3, + ancestors.clone(), + ) + .unwrap(); + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert_eq!( + chain.best_chain_vec(), + vec![candidate_a_hash, candidate_b_hash, candidate_c_hash] + ); + assert_eq!(chain.unconnected_len(), 0); + } + + // Relay parents of pending availability candidates can be out of scope + // Relay parent of candidate A is out of scope. + let ancestors_without_x = vec![relay_parent_y_info.clone()]; + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![PendingAvailability { + candidate_hash: candidate_a_hash, + relay_parent: relay_parent_x_info.clone(), + }], + 4, + ancestors_without_x, + ) + .unwrap(); + let chain = populate_chain_from_previous_storage(&scope, &storage); + + assert_eq!( + chain.best_chain_vec(), + vec![candidate_a_hash, candidate_b_hash, candidate_c_hash] + ); + assert_eq!(chain.unconnected_len(), 0); + + // Even relay parents of pending availability candidates which are out of scope cannot + // move backwards. + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), base_constraints.clone(), vec![ PendingAvailability { candidate_hash: candidate_a_hash, relay_parent: RelayChainBlockInfo { - hash: relay_parent_a_info.hash, + hash: relay_parent_x_info.hash, number: 1, - storage_root: relay_parent_a_info.storage_root, + storage_root: relay_parent_x_info.storage_root, }, }, PendingAvailability { candidate_hash: candidate_b_hash, relay_parent: RelayChainBlockInfo { - hash: relay_parent_b_info.hash, + hash: relay_parent_y_info.hash, number: 0, - storage_root: relay_parent_b_info.storage_root, + storage_root: relay_parent_y_info.storage_root, }, }, ], @@ -976,271 +961,418 @@ fn populate_and_extend_from_storage_with_existing_empty_to_vec() { vec![], ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); - assert!(chain.to_vec().is_empty()); - - chain.extend_from_storage(&storage); - assert!(chain.to_vec().is_empty()); + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert!(chain.best_chain_vec().is_empty()); + assert_eq!(chain.unconnected_len(), 0); } -} -#[test] -fn extend_from_storage_with_existing_to_vec() { - let para_id = ParaId::from(5u32); - let relay_parent_a = Hash::repeat_byte(1); - let relay_parent_b = Hash::repeat_byte(2); - let relay_parent_d = Hash::repeat_byte(3); + // More complex case: + // max_depth is 2 (a chain of max depth 3). + // A -> B -> C are the best backable chain. + // D is backed but would exceed the max depth. + // F is unconnected and seconded. + // A1 has same parent as A, is backed but has a higher candidate hash. It'll therefore be + // deleted. + // A1 has underneath a subtree that will all need to be trimmed. A1 -> B1. B1 -> C1 + // and B1 -> C2. (C1 is backed). + // A2 is seconded but is kept because it has a lower candidate hash than A. + // A2 points to B2, which is backed. + // + // Check that D, F, A2 and B2 are kept as unconnected potential candidates. - let (pvd_a, candidate_a) = make_committed_candidate( + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![], + 2, + ancestors.clone(), + ) + .unwrap(); + + // Candidate D + let (pvd_d, candidate_d) = make_committed_candidate( para_id, - relay_parent_a, - 0, - vec![0x0a].into(), - vec![0x0b].into(), - 0, + relay_parent_z_info.hash, + relay_parent_z_info.number, + vec![0x0d].into(), + vec![0x0e].into(), + relay_parent_z_info.number, ); - let candidate_a_hash = candidate_a.hash(); - - let (pvd_b, candidate_b) = make_committed_candidate( + let candidate_d_hash = candidate_d.hash(); + let candidate_d_entry = + CandidateEntry::new(candidate_d_hash, candidate_d, pvd_d, CandidateState::Backed).unwrap(); + assert!(populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_d_entry) + .is_ok()); + storage.add_candidate_entry(candidate_d_entry).unwrap(); + + // Candidate F + let (pvd_f, candidate_f) = make_committed_candidate( para_id, - relay_parent_b, - 1, - vec![0x0b].into(), - vec![0x0c].into(), - 1, + relay_parent_z_info.hash, + relay_parent_z_info.number, + vec![0x0f].into(), + vec![0xf1].into(), + 1000, ); - let candidate_b_hash = candidate_b.hash(); + let candidate_f_hash = candidate_f.hash(); + let candidate_f_entry = + CandidateEntry::new(candidate_f_hash, candidate_f, pvd_f, CandidateState::Seconded) + .unwrap(); + assert!(populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_f_entry) + .is_ok()); + storage.add_candidate_entry(candidate_f_entry.clone()).unwrap(); - let (pvd_c, candidate_c) = make_committed_candidate( + // Candidate A1 + let (pvd_a1, candidate_a1) = make_committed_candidate( para_id, - // Use the same relay parent number as B to test that it doesn't need to change between - // candidates. - relay_parent_b, - 1, - vec![0x0c].into(), - vec![0x0d].into(), - 1, + relay_parent_x_info.hash, + relay_parent_x_info.number, + vec![0x0a].into(), + vec![0xb1].into(), + relay_parent_x_info.number, ); - let candidate_c_hash = candidate_c.hash(); + let candidate_a1_hash = candidate_a1.hash(); + let candidate_a1_entry = + CandidateEntry::new(candidate_a1_hash, candidate_a1, pvd_a1, CandidateState::Backed) + .unwrap(); + // Candidate A1 is created so that its hash is greater than the candidate A hash. + assert_eq!(fork_selection_rule(&candidate_a_hash, &candidate_a1_hash), Ordering::Less); - // Candidate D will never be added to the chain. - let (pvd_d, candidate_d) = make_committed_candidate( - para_id, - relay_parent_d, - 2, - vec![0x0e].into(), - vec![0x0f].into(), - 1, + assert_matches!( + populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_a1_entry), + Err(Error::ForkChoiceRule(other)) if candidate_a_hash == other ); - let relay_parent_a_info = RelayChainBlockInfo { - number: pvd_a.relay_parent_number, - hash: relay_parent_a, - storage_root: pvd_a.relay_parent_storage_root, - }; - let relay_parent_b_info = RelayChainBlockInfo { - number: pvd_b.relay_parent_number, - hash: relay_parent_b, - storage_root: pvd_b.relay_parent_storage_root, - }; - let relay_parent_d_info = RelayChainBlockInfo { - number: pvd_d.relay_parent_number, - hash: relay_parent_d, - storage_root: pvd_d.relay_parent_storage_root, - }; + storage.add_candidate_entry(candidate_a1_entry.clone()).unwrap(); - let base_constraints = make_constraints(0, vec![0], vec![0x0a].into()); - let pending_availability = Vec::new(); + // Candidate B1. + let (pvd_b1, candidate_b1) = make_committed_candidate( + para_id, + relay_parent_x_info.hash, + relay_parent_x_info.number, + vec![0xb1].into(), + vec![0xc1].into(), + relay_parent_x_info.number, + ); + let candidate_b1_hash = candidate_b1.hash(); + let candidate_b1_entry = + CandidateEntry::new(candidate_b1_hash, candidate_b1, pvd_b1, CandidateState::Seconded) + .unwrap(); + assert!(populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_b1_entry) + .is_ok()); - let ancestors = vec![ - // These need to be ordered in reverse. - relay_parent_b_info.clone(), - relay_parent_a_info.clone(), - ]; + storage.add_candidate_entry(candidate_b1_entry).unwrap(); - // Already had A and C in the storage. Introduce B, which should add both B and C to the chain - // now. - { - let mut storage = CandidateStorage::default(); - storage - .add_candidate(candidate_a.clone(), pvd_a.clone(), CandidateState::Seconded) + // Candidate C1. + let (pvd_c1, candidate_c1) = make_committed_candidate( + para_id, + relay_parent_x_info.hash, + relay_parent_x_info.number, + vec![0xc1].into(), + vec![0xd1].into(), + relay_parent_x_info.number, + ); + let candidate_c1_hash = candidate_c1.hash(); + let candidate_c1_entry = + CandidateEntry::new(candidate_c1_hash, candidate_c1, pvd_c1, CandidateState::Backed) .unwrap(); - storage - .add_candidate(candidate_c.clone(), pvd_c.clone(), CandidateState::Seconded) + assert!(populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_c1_entry) + .is_ok()); + + storage.add_candidate_entry(candidate_c1_entry).unwrap(); + + // Candidate C2. + let (pvd_c2, candidate_c2) = make_committed_candidate( + para_id, + relay_parent_x_info.hash, + relay_parent_x_info.number, + vec![0xc1].into(), + vec![0xd2].into(), + relay_parent_x_info.number, + ); + let candidate_c2_hash = candidate_c2.hash(); + let candidate_c2_entry = + CandidateEntry::new(candidate_c2_hash, candidate_c2, pvd_c2, CandidateState::Seconded) .unwrap(); - storage - .add_candidate(candidate_d.clone(), pvd_d.clone(), CandidateState::Seconded) + assert!(populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_c2_entry) + .is_ok()); + storage.add_candidate_entry(candidate_c2_entry).unwrap(); + + // Candidate A2. + let (pvd_a2, candidate_a2) = make_committed_candidate( + para_id, + relay_parent_x_info.hash, + relay_parent_x_info.number, + vec![0x0a].into(), + vec![0xb3].into(), + relay_parent_x_info.number, + ); + let candidate_a2_hash = candidate_a2.hash(); + let candidate_a2_entry = + CandidateEntry::new(candidate_a2_hash, candidate_a2, pvd_a2, CandidateState::Seconded) .unwrap(); + // Candidate A2 is created so that its hash is greater than the candidate A hash. + assert_eq!(fork_selection_rule(&candidate_a2_hash, &candidate_a_hash), Ordering::Less); - let scope = Scope::with_ancestors( - para_id, - relay_parent_d_info.clone(), - base_constraints.clone(), - pending_availability.clone(), - 4, - ancestors.clone(), - ) - .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash]); + assert!(populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_a2_entry) + .is_ok()); - storage - .add_candidate(candidate_b.clone(), pvd_b.clone(), CandidateState::Seconded) + storage.add_candidate_entry(candidate_a2_entry).unwrap(); + + // Candidate B2. + let (pvd_b2, candidate_b2) = make_committed_candidate( + para_id, + relay_parent_y_info.hash, + relay_parent_y_info.number, + vec![0xb3].into(), + vec![0xb4].into(), + relay_parent_y_info.number, + ); + let candidate_b2_hash = candidate_b2.hash(); + let candidate_b2_entry = + CandidateEntry::new(candidate_b2_hash, candidate_b2, pvd_b2, CandidateState::Backed) .unwrap(); - chain.extend_from_storage(&storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); - } + assert!(populate_chain_from_previous_storage(&scope, &storage) + .can_add_candidate_as_potential(&candidate_b2_entry) + .is_ok()); + storage.add_candidate_entry(candidate_b2_entry).unwrap(); - // Already had A and B in the chain. Introduce C. + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_d_hash, candidate_f_hash, candidate_a2_hash, candidate_b2_hash] + .into_iter() + .collect() + ); + // Cannot add as potential an already present candidate (whether it's in the best chain or in + // unconnected storage) + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_a_entry), + Err(Error::CandidateAlreadyKnown) + ); + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_f_entry), + Err(Error::CandidateAlreadyKnown) + ); + + // Simulate a best chain reorg by backing a2. { - let mut storage = CandidateStorage::default(); - storage - .add_candidate(candidate_a.clone(), pvd_a.clone(), CandidateState::Seconded) - .unwrap(); - storage - .add_candidate(candidate_b.clone(), pvd_b.clone(), CandidateState::Seconded) - .unwrap(); - storage - .add_candidate(candidate_d.clone(), pvd_d.clone(), CandidateState::Seconded) - .unwrap(); + let mut chain = chain.clone(); + chain.candidate_backed(&candidate_a2_hash); + assert_eq!(chain.best_chain_vec(), vec![candidate_a2_hash, candidate_b2_hash]); + // F is kept as it was truly unconnected. The rest will be trimmed. + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_f_hash].into_iter().collect() + ); - let scope = Scope::with_ancestors( - para_id, - relay_parent_d_info.clone(), - base_constraints.clone(), - pending_availability.clone(), - 4, - ancestors.clone(), + // A and A1 will never have potential again. + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_a1_entry), + Err(Error::ForkChoiceRule(_)) + ); + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_a_entry), + Err(Error::ForkChoiceRule(_)) + ); + } + + // Candidate F has an invalid hrmp watermark. however, it was not checked beforehand as we don't + // have its parent yet. Add its parent now. This will not impact anything as E is not yet part + // of the best chain. + + let (pvd_e, candidate_e) = make_committed_candidate( + para_id, + relay_parent_z_info.hash, + relay_parent_z_info.number, + vec![0x0e].into(), + vec![0x0f].into(), + relay_parent_z_info.number, + ); + let candidate_e_hash = candidate_e.hash(); + storage + .add_candidate_entry( + CandidateEntry::new(candidate_e_hash, candidate_e, pvd_e, CandidateState::Seconded) + .unwrap(), ) .unwrap(); - let mut chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - storage - .add_candidate(candidate_c.clone(), pvd_c.clone(), CandidateState::Seconded) - .unwrap(); - chain.extend_from_storage(&storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); - } + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [ + candidate_d_hash, + candidate_f_hash, + candidate_a2_hash, + candidate_b2_hash, + candidate_e_hash + ] + .into_iter() + .collect() + ); + + // Simulate the fact that candidates A, B, C are now pending availability. + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![ + PendingAvailability { + candidate_hash: candidate_a_hash, + relay_parent: relay_parent_x_info, + }, + PendingAvailability { + candidate_hash: candidate_b_hash, + relay_parent: relay_parent_y_info, + }, + PendingAvailability { + candidate_hash: candidate_c_hash, + relay_parent: relay_parent_z_info.clone(), + }, + ], + 2, + ancestors.clone(), + ) + .unwrap(); + + // A2 and B2 will now be trimmed + let chain = populate_chain_from_previous_storage(&scope, &storage); + assert_eq!(chain.best_chain_vec(), vec![candidate_a_hash, candidate_b_hash, candidate_c_hash]); + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_d_hash, candidate_f_hash, candidate_e_hash].into_iter().collect() + ); + // Cannot add as potential an already pending availability candidate + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_a_entry), + Err(Error::CandidateAlreadyKnown) + ); + + // Simulate the fact that candidates A, B and C have been included. + + let base_constraints = make_constraints(0, vec![0], HeadData(vec![0x0d])); + let scope = Scope::with_ancestors( + relay_parent_z_info.clone(), + base_constraints.clone(), + vec![], + 2, + ancestors.clone(), + ) + .unwrap(); + + let prev_chain = chain; + let mut chain = FragmentChain::init(scope, CandidateStorage::default()); + chain.populate_from_previous(&prev_chain); + assert_eq!(chain.best_chain_vec(), vec![candidate_d_hash]); + assert_eq!( + chain.unconnected().map(|c| c.candidate_hash).collect::>(), + [candidate_e_hash, candidate_f_hash].into_iter().collect() + ); + + // Mark E as backed. F will be dropped for invalid watermark. No other unconnected candidates. + chain.candidate_backed(&candidate_e_hash); + assert_eq!(chain.best_chain_vec(), vec![candidate_d_hash, candidate_e_hash]); + assert_eq!(chain.unconnected_len(), 0); + + assert_matches!( + chain.can_add_candidate_as_potential(&candidate_f_entry), + Err(Error::CheckAgainstConstraints(_)) + ); } #[test] -fn test_find_ancestor_path_and_find_backable_chain_empty_to_vec() { - let para_id = ParaId::from(5u32); +fn test_find_ancestor_path_and_find_backable_chain_empty_best_chain() { let relay_parent = Hash::repeat_byte(1); let required_parent: HeadData = vec![0xff].into(); let max_depth = 10; // Empty chain - let storage = CandidateStorage::default(); let base_constraints = make_constraints(0, vec![0], required_parent.clone()); let relay_parent_info = RelayChainBlockInfo { number: 0, hash: relay_parent, storage_root: Hash::zero() }; - let scope = Scope::with_ancestors( - para_id, - relay_parent_info, - base_constraints, - vec![], - max_depth, - vec![], - ) - .unwrap(); - let chain = FragmentChain::populate(scope, &storage); - assert!(chain.to_vec().is_empty()); + let scope = + Scope::with_ancestors(relay_parent_info, base_constraints, vec![], max_depth, vec![]) + .unwrap(); + let chain = FragmentChain::init(scope, CandidateStorage::default()); + assert_eq!(chain.best_chain_len(), 0); assert_eq!(chain.find_ancestor_path(Ancestors::new()), 0); - assert_eq!(chain.find_backable_chain(Ancestors::new(), 2, |_| true), vec![]); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 2), vec![]); // Invalid candidate. let ancestors: Ancestors = [CandidateHash::default()].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 0); - assert_eq!(chain.find_backable_chain(ancestors, 2, |_| true), vec![]); + assert_eq!(chain.find_backable_chain(ancestors, 2), vec![]); } #[test] -fn test_find_ancestor_path_and_find_backable_to_vec() { +fn test_find_ancestor_path_and_find_backable_chain() { let para_id = ParaId::from(5u32); let relay_parent = Hash::repeat_byte(1); let required_parent: HeadData = vec![0xff].into(); let max_depth = 5; let relay_parent_number = 0; - let relay_parent_storage_root = Hash::repeat_byte(69); + let relay_parent_storage_root = Hash::zero(); let mut candidates = vec![]; - - // Candidate 0 - candidates.push(make_committed_candidate( - para_id, - relay_parent, - 0, - required_parent.clone(), - vec![0].into(), - 0, - )); - // Candidate 1 - candidates.push(make_committed_candidate( - para_id, - relay_parent, - 0, - vec![0].into(), - vec![1].into(), - 0, - )); - // Candidate 2 - candidates.push(make_committed_candidate( - para_id, - relay_parent, - 0, - vec![1].into(), - vec![2].into(), - 0, - )); - // Candidate 3 - candidates.push(make_committed_candidate( - para_id, - relay_parent, - 0, - vec![2].into(), - vec![3].into(), - 0, - )); - // Candidate 4 - candidates.push(make_committed_candidate( - para_id, - relay_parent, - 0, - vec![3].into(), - vec![4].into(), - 0, - )); - // Candidate 5 + + // Candidate 0 candidates.push(make_committed_candidate( para_id, relay_parent, 0, - vec![4].into(), - vec![5].into(), + required_parent.clone(), + vec![0].into(), 0, )); - let base_constraints = make_constraints(0, vec![0], required_parent.clone()); + // Candidates 1..=5 + for index in 1..=5 { + candidates.push(make_committed_candidate( + para_id, + relay_parent, + 0, + vec![index - 1].into(), + vec![index].into(), + 0, + )); + } + let mut storage = CandidateStorage::default(); + for (pvd, candidate) in candidates.iter() { + storage + .add_candidate_entry( + CandidateEntry::new_seconded(candidate.hash(), candidate.clone(), pvd.clone()) + .unwrap(), + ) + .unwrap(); + } + + let candidates = candidates + .into_iter() + .map(|(_pvd, candidate)| candidate.hash()) + .collect::>(); + let hashes = + |range: Range| range.map(|i| (candidates[i], relay_parent)).collect::>(); + let relay_parent_info = RelayChainBlockInfo { number: relay_parent_number, hash: relay_parent, storage_root: relay_parent_storage_root, }; - for (pvd, candidate) in candidates.iter() { - storage - .add_candidate(candidate.clone(), pvd.clone(), CandidateState::Seconded) - .unwrap(); - } - let candidates = candidates.into_iter().map(|(_pvd, candidate)| candidate).collect::>(); + let base_constraints = make_constraints(0, vec![0], required_parent.clone()); let scope = Scope::with_ancestors( - para_id, relay_parent_info.clone(), base_constraints.clone(), vec![], @@ -1248,506 +1380,140 @@ fn test_find_ancestor_path_and_find_backable_to_vec() { vec![], ) .unwrap(); - let chain = FragmentChain::populate(scope, &storage); + let mut chain = populate_chain_from_previous_storage(&scope, &storage); + // For now, candidates are only seconded, not backed. So the best chain is empty and no + // candidate will be returned. assert_eq!(candidates.len(), 6); - assert_eq!(chain.to_vec().len(), 6); + assert_eq!(chain.best_chain_len(), 0); + assert_eq!(chain.unconnected_len(), 6); + + for count in 0..10 { + assert_eq!(chain.find_backable_chain(Ancestors::new(), count).len(), 0); + } + + // Do tests with only a couple of candidates being backed. + { + let mut chain = chain.clone(); + chain.candidate_backed(&&candidates[5]); + for count in 0..10 { + assert_eq!(chain.find_backable_chain(Ancestors::new(), count).len(), 0); + } + chain.candidate_backed(&&candidates[3]); + chain.candidate_backed(&&candidates[4]); + for count in 0..10 { + assert_eq!(chain.find_backable_chain(Ancestors::new(), count).len(), 0); + } + + chain.candidate_backed(&&candidates[1]); + for count in 0..10 { + assert_eq!(chain.find_backable_chain(Ancestors::new(), count).len(), 0); + } + + chain.candidate_backed(&&candidates[0]); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 1), hashes(0..1)); + for count in 2..10 { + assert_eq!(chain.find_backable_chain(Ancestors::new(), count), hashes(0..2)); + } + + // Now back the missing piece. + chain.candidate_backed(&&candidates[2]); + assert_eq!(chain.best_chain_len(), 6); + for count in 0..10 { + assert_eq!( + chain.find_backable_chain(Ancestors::new(), count), + (0..6) + .take(count as usize) + .map(|i| (candidates[i], relay_parent)) + .collect::>() + ); + } + } + + // Now back all candidates. Back them in a random order. The result should always be the same. + let mut candidates_shuffled = candidates.clone(); + candidates_shuffled.shuffle(&mut thread_rng()); + for candidate in candidates.iter() { + chain.candidate_backed(candidate); + storage.mark_backed(candidate); + } // No ancestors supplied. assert_eq!(chain.find_ancestor_path(Ancestors::new()), 0); - assert_eq!(chain.find_backable_chain(Ancestors::new(), 0, |_| true), vec![]); - assert_eq!( - chain.find_backable_chain(Ancestors::new(), 1, |_| true), - [0].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - assert_eq!( - chain.find_backable_chain(Ancestors::new(), 2, |_| true), - [0, 1].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - assert_eq!( - chain.find_backable_chain(Ancestors::new(), 5, |_| true), - [0, 1, 2, 3, 4].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 0), vec![]); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 1), hashes(0..1)); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 2), hashes(0..2)); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 5), hashes(0..5)); for count in 6..10 { - assert_eq!( - chain.find_backable_chain(Ancestors::new(), count, |_| true), - [0, 1, 2, 3, 4, 5].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(Ancestors::new(), count), hashes(0..6)); } - assert_eq!( - chain.find_backable_chain(Ancestors::new(), 7, |_| true), - [0, 1, 2, 3, 4, 5].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - assert_eq!( - chain.find_backable_chain(Ancestors::new(), 10, |_| true), - [0, 1, 2, 3, 4, 5].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 7), hashes(0..6)); + assert_eq!(chain.find_backable_chain(Ancestors::new(), 10), hashes(0..6)); // Ancestor which is not part of the chain. Will be ignored. let ancestors: Ancestors = [CandidateHash::default()].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 0); - assert_eq!( - chain.find_backable_chain(ancestors, 4, |_| true), - [0, 1, 2, 3].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - let ancestors: Ancestors = - [candidates[1].hash(), CandidateHash::default()].into_iter().collect(); + assert_eq!(chain.find_backable_chain(ancestors, 4), hashes(0..4)); + + let ancestors: Ancestors = [candidates[1], CandidateHash::default()].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 0); - assert_eq!( - chain.find_backable_chain(ancestors, 4, |_| true), - [0, 1, 2, 3].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - let ancestors: Ancestors = - [candidates[0].hash(), CandidateHash::default()].into_iter().collect(); + assert_eq!(chain.find_backable_chain(ancestors, 4), hashes(0..4)); + + let ancestors: Ancestors = [candidates[0], CandidateHash::default()].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 1); - assert_eq!( - chain.find_backable_chain(ancestors, 4, |_| true), - [1, 2, 3, 4].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(ancestors, 4), hashes(1..5)); // Ancestors which are part of the chain but don't form a path from root. Will be ignored. - let ancestors: Ancestors = [candidates[1].hash(), candidates[2].hash()].into_iter().collect(); + let ancestors: Ancestors = [candidates[1], candidates[2]].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 0); - assert_eq!( - chain.find_backable_chain(ancestors, 4, |_| true), - [0, 1, 2, 3].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(ancestors, 4), hashes(0..4)); // Valid ancestors. - let ancestors: Ancestors = [candidates[2].hash(), candidates[0].hash(), candidates[1].hash()] - .into_iter() - .collect(); + let ancestors: Ancestors = [candidates[2], candidates[0], candidates[1]].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 3); - assert_eq!( - chain.find_backable_chain(ancestors.clone(), 2, |_| true), - [3, 4].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(ancestors.clone(), 2), hashes(3..5)); for count in 3..10 { - assert_eq!( - chain.find_backable_chain(ancestors.clone(), count, |_| true), - [3, 4, 5].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(ancestors.clone(), count), hashes(3..6)); } // Valid ancestors with candidates which have been omitted due to timeouts - let ancestors: Ancestors = [candidates[0].hash(), candidates[2].hash()].into_iter().collect(); + let ancestors: Ancestors = [candidates[0], candidates[2]].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 1); - assert_eq!( - chain.find_backable_chain(ancestors.clone(), 3, |_| true), - [1, 2, 3].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - assert_eq!( - chain.find_backable_chain(ancestors.clone(), 4, |_| true), - [1, 2, 3, 4].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(ancestors.clone(), 3), hashes(1..4)); + assert_eq!(chain.find_backable_chain(ancestors.clone(), 4), hashes(1..5)); for count in 5..10 { - assert_eq!( - chain.find_backable_chain(ancestors.clone(), count, |_| true), - [1, 2, 3, 4, 5].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(ancestors.clone(), count), hashes(1..6)); } - let ancestors: Ancestors = [candidates[0].hash(), candidates[1].hash(), candidates[3].hash()] - .into_iter() - .collect(); + let ancestors: Ancestors = [candidates[0], candidates[1], candidates[3]].into_iter().collect(); assert_eq!(chain.find_ancestor_path(ancestors.clone()), 2); - assert_eq!( - chain.find_backable_chain(ancestors.clone(), 4, |_| true), - [2, 3, 4, 5].into_iter().map(|i| candidates[i].hash()).collect::>() - ); + assert_eq!(chain.find_backable_chain(ancestors.clone(), 4), hashes(2..6)); // Requested count is 0. - assert_eq!(chain.find_backable_chain(ancestors, 0, |_| true), vec![]); - - // Stop when we've found a candidate for which pred returns false. - let ancestors: Ancestors = [candidates[2].hash(), candidates[0].hash(), candidates[1].hash()] - .into_iter() - .collect(); - for count in 1..10 { - assert_eq!( - // Stop at 4. - chain.find_backable_chain(ancestors.clone(), count, |hash| hash != - &candidates[4].hash()), - [3].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - } + assert_eq!(chain.find_backable_chain(ancestors, 0), vec![]); // Stop when we've found a candidate which is pending availability { let scope = Scope::with_ancestors( - para_id, relay_parent_info.clone(), base_constraints, // Mark the third candidate as pending availability vec![PendingAvailability { - candidate_hash: candidates[3].hash(), + candidate_hash: candidates[3], relay_parent: relay_parent_info, }], max_depth, vec![], ) .unwrap(); - let chain = FragmentChain::populate(scope, &storage); - let ancestors: Ancestors = - [candidates[0].hash(), candidates[1].hash()].into_iter().collect(); + let chain = populate_chain_from_previous_storage(&scope, &storage); + let ancestors: Ancestors = [candidates[0], candidates[1]].into_iter().collect(); assert_eq!( // Stop at 4. - chain.find_backable_chain(ancestors.clone(), 3, |_| true), - [2].into_iter().map(|i| candidates[i].hash()).collect::>() - ); - } -} - -#[test] -fn hypothetical_membership() { - let mut storage = CandidateStorage::default(); - - let para_id = ParaId::from(5u32); - let relay_parent_a = Hash::repeat_byte(1); - - let (pvd_a, candidate_a) = make_committed_candidate( - para_id, - relay_parent_a, - 0, - vec![0x0a].into(), - vec![0x0b].into(), - 0, - ); - let candidate_a_hash = candidate_a.hash(); - - let (pvd_b, candidate_b) = make_committed_candidate( - para_id, - relay_parent_a, - 0, - vec![0x0b].into(), - vec![0x0c].into(), - 0, - ); - let candidate_b_hash = candidate_b.hash(); - - let base_constraints = make_constraints(0, vec![0], vec![0x0a].into()); - - let relay_parent_a_info = RelayChainBlockInfo { - number: pvd_a.relay_parent_number, - hash: relay_parent_a, - storage_root: pvd_a.relay_parent_storage_root, - }; - - let max_depth = 4; - storage.add_candidate(candidate_a, pvd_a, CandidateState::Seconded).unwrap(); - storage.add_candidate(candidate_b, pvd_b, CandidateState::Seconded).unwrap(); - let scope = Scope::with_ancestors( - para_id, - relay_parent_a_info.clone(), - base_constraints.clone(), - vec![], - max_depth, - vec![], - ) - .unwrap(); - let chain = FragmentChain::populate(scope, &storage); - - assert_eq!(chain.to_vec().len(), 2); - - // Check candidates which are already present - assert!(chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0a]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: candidate_a_hash, - }, - &storage, - )); - assert!(chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0b]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: candidate_b_hash, - }, - &storage, - )); - - // Forks not allowed. - assert!(!chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0a]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: CandidateHash(Hash::repeat_byte(21)), - }, - &storage, - )); - assert!(!chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0b]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: CandidateHash(Hash::repeat_byte(22)), - }, - &storage, - )); - - // Unknown candidate which builds on top of the current chain. - assert!(chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0c]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: CandidateHash(Hash::repeat_byte(23)), - }, - &storage, - )); - - // Unknown unconnected candidate which may be valid. - assert!(chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0e]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: CandidateHash(Hash::repeat_byte(23)), - }, - &storage, - )); - - // The number of unconnected candidates is limited (chain.len() + unconnected) <= max_depth - { - // C will be an unconnected candidate. - let (pvd_c, candidate_c) = make_committed_candidate( - para_id, - relay_parent_a, - 0, - vec![0x0e].into(), - vec![0x0f].into(), - 0, - ); - let candidate_c_hash = candidate_c.hash(); - - // Add an invalid candidate in the storage. This would introduce a fork. Just to test that - // it's ignored. - let (invalid_pvd, invalid_candidate) = make_committed_candidate( - para_id, - relay_parent_a, - 1, - vec![0x0a].into(), - vec![0x0b].into(), - 0, + chain.find_backable_chain(ancestors.clone(), 3), + hashes(2..3) ); - - let scope = Scope::with_ancestors( - para_id, - relay_parent_a_info, - base_constraints, - vec![], - 2, - vec![], - ) - .unwrap(); - let mut storage = storage.clone(); - storage.add_candidate(candidate_c, pvd_c, CandidateState::Seconded).unwrap(); - - let chain = FragmentChain::populate(scope, &storage); - assert_eq!(chain.to_vec(), vec![candidate_a_hash, candidate_b_hash]); - - storage - .add_candidate(invalid_candidate, invalid_pvd, CandidateState::Seconded) - .unwrap(); - - // Check that C is accepted as a potential unconnected candidate. - assert!(!chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0e]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_hash: candidate_c_hash, - candidate_para: para_id - }, - &storage, - )); - - // Since C is already an unconnected candidate in the storage. - assert!(!chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0f]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: CandidateHash(Hash::repeat_byte(23)), - }, - &storage, - )); } } - -#[test] -fn hypothetical_membership_stricter_on_complete_candidates() { - let storage = CandidateStorage::default(); - - let para_id = ParaId::from(5u32); - let relay_parent_a = Hash::repeat_byte(1); - - let (pvd_a, candidate_a) = make_committed_candidate( - para_id, - relay_parent_a, - 0, - vec![0x0a].into(), - vec![0x0b].into(), - 1000, // watermark is illegal - ); - - let candidate_a_hash = candidate_a.hash(); - - let base_constraints = make_constraints(0, vec![0], vec![0x0a].into()); - let pending_availability = Vec::new(); - - let relay_parent_a_info = RelayChainBlockInfo { - number: pvd_a.relay_parent_number, - hash: relay_parent_a, - storage_root: pvd_a.relay_parent_storage_root, - }; - - let max_depth = 4; - let scope = Scope::with_ancestors( - para_id, - relay_parent_a_info, - base_constraints, - pending_availability, - max_depth, - vec![], - ) - .unwrap(); - let chain = FragmentChain::populate(scope, &storage); - - assert!(chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0a]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_para: para_id, - candidate_hash: candidate_a_hash, - }, - &storage, - )); - - assert!(!chain.hypothetical_membership( - HypotheticalCandidate::Complete { - receipt: Arc::new(candidate_a), - persisted_validation_data: pvd_a, - candidate_hash: candidate_a_hash, - }, - &storage, - )); -} - -#[test] -fn hypothetical_membership_with_pending_availability_in_scope() { - let mut storage = CandidateStorage::default(); - - let para_id = ParaId::from(5u32); - let relay_parent_a = Hash::repeat_byte(1); - let relay_parent_b = Hash::repeat_byte(2); - let relay_parent_c = Hash::repeat_byte(3); - - let (pvd_a, candidate_a) = make_committed_candidate( - para_id, - relay_parent_a, - 0, - vec![0x0a].into(), - vec![0x0b].into(), - 0, - ); - let candidate_a_hash = candidate_a.hash(); - - let (pvd_b, candidate_b) = make_committed_candidate( - para_id, - relay_parent_b, - 1, - vec![0x0b].into(), - vec![0x0c].into(), - 1, - ); - - // Note that relay parent `a` is not allowed. - let base_constraints = make_constraints(1, vec![], vec![0x0a].into()); - - let relay_parent_a_info = RelayChainBlockInfo { - number: pvd_a.relay_parent_number, - hash: relay_parent_a, - storage_root: pvd_a.relay_parent_storage_root, - }; - let pending_availability = vec![PendingAvailability { - candidate_hash: candidate_a_hash, - relay_parent: relay_parent_a_info, - }]; - - let relay_parent_b_info = RelayChainBlockInfo { - number: pvd_b.relay_parent_number, - hash: relay_parent_b, - storage_root: pvd_b.relay_parent_storage_root, - }; - let relay_parent_c_info = RelayChainBlockInfo { - number: pvd_b.relay_parent_number + 1, - hash: relay_parent_c, - storage_root: Hash::zero(), - }; - - let max_depth = 4; - storage.add_candidate(candidate_a, pvd_a, CandidateState::Seconded).unwrap(); - storage.add_candidate(candidate_b, pvd_b, CandidateState::Backed).unwrap(); - storage.mark_backed(&candidate_a_hash); - - let scope = Scope::with_ancestors( - para_id, - relay_parent_c_info, - base_constraints, - pending_availability, - max_depth, - vec![relay_parent_b_info], - ) - .unwrap(); - let chain = FragmentChain::populate(scope, &storage); - - assert_eq!(chain.to_vec().len(), 2); - - let candidate_d_hash = CandidateHash(Hash::repeat_byte(0xAA)); - - assert!(chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0a]).hash(), - candidate_relay_parent: relay_parent_a, - candidate_hash: candidate_a_hash, - candidate_para: para_id - }, - &storage, - )); - - assert!(!chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0a]).hash(), - candidate_relay_parent: relay_parent_c, - candidate_para: para_id, - candidate_hash: candidate_d_hash, - }, - &storage, - )); - - assert!(!chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0b]).hash(), - candidate_relay_parent: relay_parent_c, - candidate_para: para_id, - candidate_hash: candidate_d_hash, - }, - &storage, - )); - - assert!(chain.hypothetical_membership( - HypotheticalCandidate::Incomplete { - parent_head_data_hash: HeadData::from(vec![0x0c]).hash(), - candidate_relay_parent: relay_parent_b, - candidate_para: para_id, - candidate_hash: candidate_d_hash, - }, - &storage, - )); -} diff --git a/polkadot/node/core/prospective-parachains/src/lib.rs b/polkadot/node/core/prospective-parachains/src/lib.rs index e4b6deffdf4a..ecb1f3a476ec 100644 --- a/polkadot/node/core/prospective-parachains/src/lib.rs +++ b/polkadot/node/core/prospective-parachains/src/lib.rs @@ -26,9 +26,11 @@ //! //! This subsystem also handles concerns such as the relay-chain being forkful and session changes. +#![deny(unused_crate_dependencies)] + use std::collections::{HashMap, HashSet}; -use fragment_chain::{FragmentChain, PotentialAddition}; +use fragment_chain::CandidateStorage; use futures::{channel::oneshot, prelude::*}; use polkadot_node_subsystem::{ @@ -41,6 +43,7 @@ use polkadot_node_subsystem::{ overseer, ActiveLeavesUpdate, FromOrchestra, OverseerSignal, SpawnedSubsystem, SubsystemError, }; use polkadot_node_subsystem_util::{ + backing_implicit_view::{BlockInfoProspectiveParachains as BlockInfo, View as ImplicitView}, inclusion_emulator::{Constraints, RelayChainBlockInfo}, request_session_index_for_child, runtime::{prospective_parachains_mode, ProspectiveParachainsMode}, @@ -55,8 +58,7 @@ use polkadot_primitives::{ use crate::{ error::{FatalError, FatalResult, JfyiError, JfyiErrorResult, Result}, fragment_chain::{ - CandidateState, CandidateStorage, CandidateStorageInsertionError, - Scope as FragmentChainScope, + CandidateEntry, Error as FragmentChainError, FragmentChain, Scope as FragmentChainScope, }, }; @@ -71,20 +73,33 @@ use self::metrics::Metrics; const LOG_TARGET: &str = "parachain::prospective-parachains"; struct RelayBlockViewData { - // Scheduling info for paras and upcoming paras. + // The fragment chains for current and upcoming scheduled paras. fragment_chains: HashMap, - pending_availability: HashSet, } struct View { - // Active or recent relay-chain blocks by block hash. - active_leaves: HashMap, - candidate_storage: HashMap, + // Per relay parent fragment chains. These includes all relay parents under the implicit view. + per_relay_parent: HashMap, + // The hashes of the currently active leaves. This is a subset of the keys in + // `per_relay_parent`. + active_leaves: HashSet, + // The backing implicit view. + implicit_view: ImplicitView, } impl View { + // Initialize with empty values. fn new() -> Self { - View { active_leaves: HashMap::new(), candidate_storage: HashMap::new() } + View { + per_relay_parent: HashMap::new(), + active_leaves: HashSet::new(), + implicit_view: ImplicitView::default(), + } + } + + // Get the fragment chains of this leaf. + fn get_fragment_chains(&self, leaf: &Hash) -> Option<&HashMap> { + self.per_relay_parent.get(&leaf).map(|view_data| &view_data.fragment_chains) } } @@ -142,9 +157,9 @@ async fn run_iteration( FromOrchestra::Signal(OverseerSignal::BlockFinalized(..)) => {}, FromOrchestra::Communication { msg } => match msg { ProspectiveParachainsMessage::IntroduceSecondedCandidate(request, tx) => - handle_introduce_seconded_candidate(&mut *ctx, view, request, tx, metrics).await, + handle_introduce_seconded_candidate(view, request, tx, metrics).await, ProspectiveParachainsMessage::CandidateBacked(para, candidate_hash) => - handle_candidate_backed(&mut *ctx, view, para, candidate_hash).await, + handle_candidate_backed(view, para, candidate_hash, metrics).await, ProspectiveParachainsMessage::GetBackableCandidates( relay_parent, para, @@ -170,17 +185,32 @@ async fn handle_active_leaves_update( update: ActiveLeavesUpdate, metrics: &Metrics, ) -> JfyiErrorResult<()> { - // 1. clean up inactive leaves - // 2. determine all scheduled paras at the new block - // 3. construct new fragment chain for each para for each new leaf - // 4. prune candidate storage. + // For any new active leaf: + // - determine the scheduled paras + // - pre-populate the candidate storage with pending availability candidates and candidates from + // the parent leaf + // - populate the fragment chain + // - add it to the implicit view + // + // Then mark the newly-deactivated leaves as deactivated and update the implicit view. + // Finally, remove any relay parents that are no longer part of the implicit view. + + let _timer = metrics.time_handle_active_leaves_update(); - for deactivated in &update.deactivated { - view.active_leaves.remove(deactivated); - } + gum::trace!( + target: LOG_TARGET, + activated = ?update.activated, + deactivated = ?update.deactivated, + "Handle ActiveLeavesUpdate" + ); let mut temp_header_cache = HashMap::new(); + // There can only be one newly activated leaf, `update.activated` is an `Option`. for activated in update.activated.into_iter() { + if update.deactivated.contains(&activated.hash) { + continue + } + let hash = activated.hash; let mode = prospective_parachains_mode(ctx.sender(), hash) @@ -199,38 +229,34 @@ async fn handle_active_leaves_update( return Ok(()) }; - let scheduled_paras = fetch_upcoming_paras(&mut *ctx, hash).await?; + let scheduled_paras = fetch_upcoming_paras(ctx, hash).await?; - let block_info: RelayChainBlockInfo = - match fetch_block_info(&mut *ctx, &mut temp_header_cache, hash).await? { - None => { - gum::warn!( - target: LOG_TARGET, - block_hash = ?hash, - "Failed to get block info for newly activated leaf block." - ); + let block_info = match fetch_block_info(ctx, &mut temp_header_cache, hash).await? { + None => { + gum::warn!( + target: LOG_TARGET, + block_hash = ?hash, + "Failed to get block info for newly activated leaf block." + ); - // `update.activated` is an option, but we can use this - // to exit the 'loop' and skip this block without skipping - // pruning logic. - continue - }, - Some(info) => info, - }; + // `update.activated` is an option, but we can use this + // to exit the 'loop' and skip this block without skipping + // pruning logic. + continue + }, + Some(info) => info, + }; let ancestry = - fetch_ancestry(&mut *ctx, &mut temp_header_cache, hash, allowed_ancestry_len).await?; + fetch_ancestry(ctx, &mut temp_header_cache, hash, allowed_ancestry_len).await?; - let mut all_pending_availability = HashSet::new(); + let prev_fragment_chains = + ancestry.first().and_then(|prev_leaf| view.get_fragment_chains(&prev_leaf.hash)); - // Find constraints. let mut fragment_chains = HashMap::new(); for para in scheduled_paras { - let candidate_storage = - view.candidate_storage.entry(para).or_insert_with(CandidateStorage::default); - - let backing_state = fetch_backing_state(&mut *ctx, hash, para).await?; - + // Find constraints and pending availability candidates. + let backing_state = fetch_backing_state(ctx, hash, para).await?; let Some((constraints, pending_availability)) = backing_state else { // This indicates a runtime conflict of some kind. gum::debug!( @@ -243,8 +269,6 @@ async fn handle_active_leaves_update( continue }; - all_pending_availability.extend(pending_availability.iter().map(|c| c.candidate_hash)); - let pending_availability = preprocess_candidates_pending_availability( ctx, &mut temp_header_cache, @@ -254,16 +278,18 @@ async fn handle_active_leaves_update( .await?; let mut compact_pending = Vec::with_capacity(pending_availability.len()); + let mut pending_availability_storage = CandidateStorage::default(); + for c in pending_availability { - let res = candidate_storage.add_candidate( + let candidate_hash = c.compact.candidate_hash; + let res = pending_availability_storage.add_pending_availability_candidate( + candidate_hash, c.candidate, c.persisted_validation_data, - CandidateState::Backed, ); - let candidate_hash = c.compact.candidate_hash; match res { - Ok(_) | Err(CandidateStorageInsertionError::CandidateAlreadyKnown(_)) => {}, + Ok(_) | Err(FragmentChainError::CandidateAlreadyKnown) => {}, Err(err) => { gum::warn!( target: LOG_TARGET, @@ -280,119 +306,138 @@ async fn handle_active_leaves_update( compact_pending.push(c.compact); } - let scope = FragmentChainScope::with_ancestors( - para, - block_info.clone(), + let scope = match FragmentChainScope::with_ancestors( + block_info.clone().into(), constraints, compact_pending, max_candidate_depth, - ancestry.iter().cloned(), - ) - .expect("ancestors are provided in reverse order and correctly; qed"); + ancestry + .iter() + .map(|a| RelayChainBlockInfo::from(a.clone())) + .collect::>(), + ) { + Ok(scope) => scope, + Err(unexpected_ancestors) => { + gum::warn!( + target: LOG_TARGET, + para_id = ?para, + max_candidate_depth, + ?ancestry, + leaf = ?hash, + "Relay chain ancestors have wrong order: {:?}", + unexpected_ancestors + ); + continue + }, + }; gum::trace!( target: LOG_TARGET, relay_parent = ?hash, min_relay_parent = scope.earliest_relay_parent().number, para_id = ?para, + ancestors = ?ancestry, "Creating fragment chain" ); - let chain = FragmentChain::populate(scope, &*candidate_storage); + let number_of_pending_candidates = pending_availability_storage.len(); + + // Init the fragment chain with the pending availability candidates. + let mut chain = FragmentChain::init(scope, pending_availability_storage); + + if chain.best_chain_len() < number_of_pending_candidates { + gum::warn!( + target: LOG_TARGET, + relay_parent = ?hash, + para_id = ?para, + "Not all pending availability candidates could be introduced. Actual vs expected count: {}, {}", + chain.best_chain_len(), + number_of_pending_candidates + ) + } + + // If we know the previous fragment chain, use that for further populating the fragment + // chain. + if let Some(prev_fragment_chain) = + prev_fragment_chains.and_then(|chains| chains.get(¶)) + { + chain.populate_from_previous(prev_fragment_chain); + } gum::trace!( target: LOG_TARGET, relay_parent = ?hash, para_id = ?para, - "Populated fragment chain with {} candidates", - chain.len() + "Populated fragment chain with {} candidates: {:?}", + chain.best_chain_len(), + chain.best_chain_vec() + ); + + gum::trace!( + target: LOG_TARGET, + relay_parent = ?hash, + para_id = ?para, + "Potential candidate storage for para: {:?}", + chain.unconnected().map(|candidate| candidate.hash()).collect::>() ); fragment_chains.insert(para, chain); } - view.active_leaves.insert( - hash, - RelayBlockViewData { fragment_chains, pending_availability: all_pending_availability }, - ); - } + view.per_relay_parent.insert(hash, RelayBlockViewData { fragment_chains }); - if !update.deactivated.is_empty() { - // This has potential to be a hotspot. - prune_view_candidate_storage(view, metrics); - } + view.active_leaves.insert(hash); - Ok(()) -} + view.implicit_view + .activate_leaf_from_prospective_parachains(block_info, &ancestry); + } -fn prune_view_candidate_storage(view: &mut View, metrics: &Metrics) { - let _timer = metrics.time_prune_view_candidate_storage(); + for deactivated in update.deactivated { + view.active_leaves.remove(&deactivated); + view.implicit_view.deactivate_leaf(deactivated); + } - let active_leaves = &view.active_leaves; - let mut live_candidates = HashSet::new(); - let mut live_paras = HashSet::new(); - for sub_view in active_leaves.values() { - live_candidates.extend(sub_view.pending_availability.iter().cloned()); + { + let remaining: HashSet<_> = view.implicit_view.all_allowed_relay_parents().collect(); - for (para_id, fragment_chain) in &sub_view.fragment_chains { - live_candidates.extend(fragment_chain.to_vec()); - live_paras.insert(*para_id); - } + view.per_relay_parent.retain(|r, _| remaining.contains(&r)); } - let connected_candidates_count = live_candidates.len(); - for (leaf, sub_view) in active_leaves.iter() { - for (para_id, fragment_chain) in &sub_view.fragment_chains { - if let Some(storage) = view.candidate_storage.get(para_id) { - let unconnected_potential = - fragment_chain.find_unconnected_potential_candidates(storage, None); - if !unconnected_potential.is_empty() { - gum::trace!( - target: LOG_TARGET, - ?leaf, - "Keeping {} unconnected candidates for paraid {} in storage: {:?}", - unconnected_potential.len(), - para_id, - unconnected_potential - ); + if metrics.0.is_some() { + let mut active_connected = 0; + let mut active_unconnected = 0; + let mut candidates_in_implicit_view = 0; + + for (hash, RelayBlockViewData { fragment_chains, .. }) in view.per_relay_parent.iter() { + if view.active_leaves.contains(hash) { + for chain in fragment_chains.values() { + active_connected += chain.best_chain_len(); + active_unconnected += chain.unconnected_len(); + } + } else { + for chain in fragment_chains.values() { + candidates_in_implicit_view += chain.best_chain_len(); + candidates_in_implicit_view += chain.unconnected_len(); } - live_candidates.extend(unconnected_potential); } } - } - - view.candidate_storage.retain(|para_id, storage| { - if !live_paras.contains(¶_id) { - return false - } - - storage.retain(|h| live_candidates.contains(&h)); - - // Even if `storage` is now empty, we retain. - // This maintains a convenient invariant that para-id storage exists - // as long as there's an active head which schedules the para. - true - }); - for (para_id, storage) in view.candidate_storage.iter() { - gum::trace!( - target: LOG_TARGET, - "Keeping a total of {} connected candidates for paraid {} in storage", - storage.candidates().count(), - para_id, - ); + metrics.record_candidate_count(active_connected as u64, active_unconnected as u64); + metrics.record_candidate_count_in_implicit_view(candidates_in_implicit_view as u64); } - metrics.record_candidate_storage_size( - connected_candidates_count as u64, - live_candidates.len().saturating_sub(connected_candidates_count) as u64, - ); + let num_active_leaves = view.active_leaves.len() as u64; + let num_inactive_leaves = + (view.per_relay_parent.len() as u64).saturating_sub(num_active_leaves); + metrics.record_leaves_count(num_active_leaves, num_inactive_leaves); + + Ok(()) } struct ImportablePendingAvailability { candidate: CommittedCandidateReceipt, persisted_validation_data: PersistedValidationData, - compact: crate::fragment_chain::PendingAvailability, + compact: fragment_chain::PendingAvailability, } #[overseer::contextbounds(ProspectiveParachains, prefix = self::overseer)] @@ -435,9 +480,9 @@ async fn preprocess_candidates_pending_availability( relay_parent_number: relay_parent.number, relay_parent_storage_root: relay_parent.storage_root, }, - compact: crate::fragment_chain::PendingAvailability { + compact: fragment_chain::PendingAvailability { candidate_hash: pending.candidate_hash, - relay_parent, + relay_parent: relay_parent.into(), }, }); @@ -447,9 +492,7 @@ async fn preprocess_candidates_pending_availability( Ok(importable) } -#[overseer::contextbounds(ProspectiveParachains, prefix = self::overseer)] -async fn handle_introduce_seconded_candidate( - _ctx: &mut Context, +async fn handle_introduce_seconded_candidate( view: &mut View, request: IntroduceSecondedCandidateRequest, tx: oneshot::Sender, @@ -463,167 +506,163 @@ async fn handle_introduce_seconded_candidate( persisted_validation_data: pvd, } = request; - let Some(storage) = view.candidate_storage.get_mut(¶) else { - gum::warn!( - target: LOG_TARGET, - para_id = ?para, - candidate_hash = ?candidate.hash(), - "Received seconded candidate for inactive para", - ); + let candidate_hash = candidate.hash(); + let candidate_entry = match CandidateEntry::new_seconded(candidate_hash, candidate, pvd) { + Ok(candidate) => candidate, + Err(err) => { + gum::warn!( + target: LOG_TARGET, + para = ?para, + "Cannot add seconded candidate: {}", + err + ); - let _ = tx.send(false); - return + let _ = tx.send(false); + return + }, }; - let parent_head_hash = pvd.parent_head.hash(); - let output_head_hash = Some(candidate.commitments.head_data.hash()); + let mut added = false; + let mut para_scheduled = false; + // We don't iterate only through the active leaves. We also update the deactivated parents in + // the implicit view, so that their upcoming children may see these candidates. + for (relay_parent, rp_data) in view.per_relay_parent.iter_mut() { + let Some(chain) = rp_data.fragment_chains.get_mut(¶) else { continue }; + let is_active_leaf = view.active_leaves.contains(relay_parent); - // We first introduce the candidate in the storage and then try to extend the chain. - // If the candidate gets included in the chain, we can keep it in storage. - // If it doesn't, check that it's still a potential candidate in at least one fragment chain. - // If it's not, we can remove it. + para_scheduled = true; - let candidate_hash = - match storage.add_candidate(candidate.clone(), pvd, CandidateState::Seconded) { - Ok(c) => c, - Err(CandidateStorageInsertionError::CandidateAlreadyKnown(_)) => { + match chain.try_adding_seconded_candidate(&candidate_entry) { + Ok(()) => { gum::debug!( target: LOG_TARGET, - para = ?para, - "Attempting to introduce an already known candidate: {:?}", - candidate.hash() + ?para, + ?relay_parent, + ?is_active_leaf, + "Added seconded candidate {:?}", + candidate_hash ); - // Candidate already known. - let _ = tx.send(true); - return + added = true; }, - Err(CandidateStorageInsertionError::PersistedValidationDataMismatch) => { - // We can't log the candidate hash without either doing more ~expensive - // hashing but this branch indicates something is seriously wrong elsewhere - // so it's doubtful that it would affect debugging. - - gum::warn!( + Err(FragmentChainError::CandidateAlreadyKnown) => { + gum::debug!( target: LOG_TARGET, - para = ?para, - "Received seconded candidate had mismatching validation data", + ?para, + ?relay_parent, + ?is_active_leaf, + "Attempting to introduce an already known candidate: {:?}", + candidate_hash ); - - let _ = tx.send(false); - return + added = true; }, - }; - - let mut keep_in_storage = false; - for (relay_parent, leaf_data) in view.active_leaves.iter_mut() { - if let Some(chain) = leaf_data.fragment_chains.get_mut(¶) { - gum::trace!( - target: LOG_TARGET, - para = ?para, - ?relay_parent, - "Candidates in chain before trying to introduce a new one: {:?}", - chain.to_vec() - ); - chain.extend_from_storage(&*storage); - if chain.contains_candidate(&candidate_hash) { - keep_in_storage = true; - - gum::trace!( + Err(err) => { + gum::debug!( target: LOG_TARGET, + ?para, ?relay_parent, - para = ?para, ?candidate_hash, - "Added candidate to chain.", - ); - } else { - match chain.can_add_candidate_as_potential( - &storage, - &candidate_hash, - &candidate.descriptor.relay_parent, - parent_head_hash, - output_head_hash, - ) { - PotentialAddition::Anyhow => { - gum::trace!( - target: LOG_TARGET, - para = ?para, - ?relay_parent, - ?candidate_hash, - "Kept candidate as unconnected potential.", - ); - - keep_in_storage = true; - }, - _ => { - gum::trace!( - target: LOG_TARGET, - para = ?para, - ?relay_parent, - "Not introducing a new candidate: {:?}", - candidate_hash - ); - }, - } - } + ?is_active_leaf, + "Cannot introduce seconded candidate: {}", + err + ) + }, } } - // If there is at least one leaf where this candidate can be added or potentially added in the - // future, keep it in storage. - if !keep_in_storage { - storage.remove_candidate(&candidate_hash); + if !para_scheduled { + gum::warn!( + target: LOG_TARGET, + para_id = ?para, + ?candidate_hash, + "Received seconded candidate for inactive para", + ); + } + if !added { gum::debug!( target: LOG_TARGET, para = ?para, candidate = ?candidate_hash, - "Newly-seconded candidate cannot be kept under any active leaf", + "Newly-seconded candidate cannot be kept under any relay parent", ); } - let _ = tx.send(keep_in_storage); + let _ = tx.send(added); } -#[overseer::contextbounds(ProspectiveParachains, prefix = self::overseer)] -async fn handle_candidate_backed( - _ctx: &mut Context, +async fn handle_candidate_backed( view: &mut View, para: ParaId, candidate_hash: CandidateHash, + metrics: &Metrics, ) { - let Some(storage) = view.candidate_storage.get_mut(¶) else { - gum::warn!( - target: LOG_TARGET, - para_id = ?para, - ?candidate_hash, - "Received instruction to back a candidate for unscheduled para", - ); + let _timer = metrics.time_candidate_backed(); - return - }; + let mut found_candidate = false; + let mut found_para = false; + + // We don't iterate only through the active leaves. We also update the deactivated parents in + // the implicit view, so that their upcoming children may see these candidates. + for (relay_parent, rp_data) in view.per_relay_parent.iter_mut() { + let Some(chain) = rp_data.fragment_chains.get_mut(¶) else { continue }; + let is_active_leaf = view.active_leaves.contains(relay_parent); + + found_para = true; + if chain.is_candidate_backed(&candidate_hash) { + gum::debug!( + target: LOG_TARGET, + ?para, + ?candidate_hash, + ?is_active_leaf, + "Received redundant instruction to mark as backed an already backed candidate", + ); + found_candidate = true; + } else if chain.contains_unconnected_candidate(&candidate_hash) { + found_candidate = true; + // Mark the candidate as backed. This can recreate the fragment chain. + chain.candidate_backed(&candidate_hash); + + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + ?para, + ?is_active_leaf, + "Candidate backed. Candidate chain for para: {:?}", + chain.best_chain_vec() + ); + + gum::trace!( + target: LOG_TARGET, + ?relay_parent, + ?para, + ?is_active_leaf, + "Potential candidate storage for para: {:?}", + chain.unconnected().map(|candidate| candidate.hash()).collect::>() + ); + } + } - if !storage.contains(&candidate_hash) { + if !found_para { gum::warn!( target: LOG_TARGET, - para_id = ?para, + ?para, ?candidate_hash, - "Received instruction to back unknown candidate", + "Received instruction to back a candidate for unscheduled para", ); return } - if storage.is_backed(&candidate_hash) { + if !found_candidate { + // This can be harmless. It can happen if we received a better backed candidate before and + // dropped this other candidate already. gum::debug!( target: LOG_TARGET, - para_id = ?para, + ?para, ?candidate_hash, - "Received redundant instruction to mark candidate as backed", + "Received instruction to back unknown candidate", ); - - return } - - storage.mark_backed(&candidate_hash); } fn answer_get_backable_candidates( @@ -634,7 +673,7 @@ fn answer_get_backable_candidates( ancestors: Ancestors, tx: oneshot::Sender>, ) { - let Some(data) = view.active_leaves.get(&relay_parent) else { + if !view.active_leaves.contains(&relay_parent) { gum::debug!( target: LOG_TARGET, ?relay_parent, @@ -644,26 +683,25 @@ fn answer_get_backable_candidates( let _ = tx.send(vec![]); return - }; - - let Some(chain) = data.fragment_chains.get(¶) else { + } + let Some(data) = view.per_relay_parent.get(&relay_parent) else { gum::debug!( target: LOG_TARGET, ?relay_parent, para_id = ?para, - "Requested backable candidate for inactive para." + "Requested backable candidate for inexistent relay-parent." ); let _ = tx.send(vec![]); return }; - let Some(storage) = view.candidate_storage.get(¶) else { - gum::warn!( + let Some(chain) = data.fragment_chains.get(¶) else { + gum::debug!( target: LOG_TARGET, ?relay_parent, para_id = ?para, - "No candidate storage for active para", + "Requested backable candidate for inactive para." ); let _ = tx.send(vec![]); @@ -674,38 +712,19 @@ fn answer_get_backable_candidates( target: LOG_TARGET, ?relay_parent, para_id = ?para, - "Candidate storage for para: {:?}", - storage.candidates().map(|candidate| candidate.hash()).collect::>() + "Candidate chain for para: {:?}", + chain.best_chain_vec() ); gum::trace!( target: LOG_TARGET, ?relay_parent, para_id = ?para, - "Candidate chain for para: {:?}", - chain.to_vec() + "Potential candidate storage for para: {:?}", + chain.unconnected().map(|candidate| candidate.hash()).collect::>() ); - let backable_candidates: Vec<_> = chain - .find_backable_chain(ancestors.clone(), count, |candidate| storage.is_backed(candidate)) - .into_iter() - .filter_map(|child_hash| { - storage.relay_parent_of_candidate(&child_hash).map_or_else( - || { - // Here, we'd actually need to trim all of the candidates that follow. Or - // not, the runtime will do this. Impossible scenario anyway. - gum::error!( - target: LOG_TARGET, - ?child_hash, - para_id = ?para, - "Candidate is present in fragment chain but not in candidate's storage!", - ); - None - }, - |parent_hash| Some((child_hash, parent_hash)), - ) - }) - .collect(); + let backable_candidates = chain.find_backable_chain(ancestors.clone(), count); if backable_candidates.is_empty() { gum::trace!( @@ -742,19 +761,32 @@ fn answer_hypothetical_membership_request( } let required_active_leaf = request.fragment_chain_relay_parent; - for (active_leaf, leaf_view) in view + for active_leaf in view .active_leaves .iter() - .filter(|(h, _)| required_active_leaf.as_ref().map_or(true, |x| h == &x)) + .filter(|h| required_active_leaf.as_ref().map_or(true, |x| h == &x)) { + let Some(leaf_view) = view.per_relay_parent.get(&active_leaf) else { continue }; for &mut (ref candidate, ref mut membership) in &mut response { let para_id = &candidate.candidate_para(); let Some(fragment_chain) = leaf_view.fragment_chains.get(para_id) else { continue }; - let Some(candidate_storage) = view.candidate_storage.get(para_id) else { continue }; - if fragment_chain.hypothetical_membership(candidate.clone(), candidate_storage) { - membership.push(*active_leaf); - } + let res = fragment_chain.can_add_candidate_as_potential(candidate); + match res { + Err(FragmentChainError::CandidateAlreadyKnown) | Ok(()) => { + membership.push(*active_leaf); + }, + Err(err) => { + gum::debug!( + target: LOG_TARGET, + para = ?para_id, + leaf = ?active_leaf, + candidate = ?candidate.candidate_hash(), + "Candidate is not a hypothetical member: {}", + err + ) + }, + }; } } @@ -767,9 +799,11 @@ fn answer_minimum_relay_parents_request( tx: oneshot::Sender>, ) { let mut v = Vec::new(); - if let Some(leaf_data) = view.active_leaves.get(&relay_parent) { - for (para_id, fragment_chain) in &leaf_data.fragment_chains { - v.push((*para_id, fragment_chain.scope().earliest_relay_parent().number)); + if view.active_leaves.contains(&relay_parent) { + if let Some(leaf_data) = view.per_relay_parent.get(&relay_parent) { + for (para_id, fragment_chain) in &leaf_data.fragment_chains { + v.push((*para_id, fragment_chain.scope().earliest_relay_parent().number)); + } } } @@ -781,37 +815,21 @@ fn answer_prospective_validation_data_request( request: ProspectiveValidationDataRequest, tx: oneshot::Sender>, ) { - // 1. Try to get the head-data from the candidate store if known. - // 2. Otherwise, it might exist as the base in some relay-parent and we can find it by iterating - // fragment chains. - // 3. Otherwise, it is unknown. - // 4. Also try to find the relay parent block info by scanning fragment chains. - // 5. If head data and relay parent block info are found - success. Otherwise, failure. - - let storage = match view.candidate_storage.get(&request.para_id) { - None => { - let _ = tx.send(None); - return - }, - Some(s) => s, - }; + // Try getting the needed data from any fragment chain. let (mut head_data, parent_head_data_hash) = match request.parent_head_data { - ParentHeadData::OnlyHash(parent_head_data_hash) => ( - storage.head_data_by_hash(&parent_head_data_hash).map(|x| x.clone()), - parent_head_data_hash, - ), + ParentHeadData::OnlyHash(parent_head_data_hash) => (None, parent_head_data_hash), ParentHeadData::WithData { head_data, hash } => (Some(head_data), hash), }; let mut relay_parent_info = None; let mut max_pov_size = None; - for fragment_chain in view - .active_leaves - .values() - .filter_map(|x| x.fragment_chains.get(&request.para_id)) - { + for fragment_chain in view.active_leaves.iter().filter_map(|x| { + view.per_relay_parent + .get(&x) + .and_then(|data| data.fragment_chains.get(&request.para_id)) + }) { if head_data.is_some() && relay_parent_info.is_some() && max_pov_size.is_some() { break } @@ -819,10 +837,7 @@ fn answer_prospective_validation_data_request( relay_parent_info = fragment_chain.scope().ancestor(&request.candidate_relay_parent); } if head_data.is_none() { - let required_parent = &fragment_chain.scope().base_constraints().required_parent; - if required_parent.hash() == parent_head_data_hash { - head_data = Some(required_parent.clone()); - } + head_data = fragment_chain.get_head_data_by_hash(&parent_head_data_hash); } if max_pov_size.is_none() { let contains_ancestor = @@ -925,7 +940,7 @@ async fn fetch_ancestry( cache: &mut HashMap, relay_hash: Hash, ancestors: usize, -) -> JfyiErrorResult> { +) -> JfyiErrorResult> { if ancestors == 0 { return Ok(Vec::new()) } @@ -1004,12 +1019,13 @@ async fn fetch_block_info( ctx: &mut Context, cache: &mut HashMap, relay_hash: Hash, -) -> JfyiErrorResult> { +) -> JfyiErrorResult> { let header = fetch_block_header_with_cache(ctx, cache, relay_hash).await?; - Ok(header.map(|header| RelayChainBlockInfo { + Ok(header.map(|header| BlockInfo { hash: relay_hash, number: header.number, + parent_hash: header.parent_hash, storage_root: header.state_root, })) } diff --git a/polkadot/node/core/prospective-parachains/src/metrics.rs b/polkadot/node/core/prospective-parachains/src/metrics.rs index 5abd9f56f306..78561bc878ac 100644 --- a/polkadot/node/core/prospective-parachains/src/metrics.rs +++ b/polkadot/node/core/prospective-parachains/src/metrics.rs @@ -17,15 +17,18 @@ use polkadot_node_subsystem::prometheus::Opts; use polkadot_node_subsystem_util::metrics::{ self, - prometheus::{self, GaugeVec, U64}, + prometheus::{self, Gauge, GaugeVec, U64}, }; #[derive(Clone)] pub(crate) struct MetricsInner { - prune_view_candidate_storage: prometheus::Histogram, - introduce_seconded_candidate: prometheus::Histogram, - hypothetical_membership: prometheus::Histogram, - candidate_storage_count: prometheus::GaugeVec, + time_active_leaves_update: prometheus::Histogram, + time_introduce_seconded_candidate: prometheus::Histogram, + time_candidate_backed: prometheus::Histogram, + time_hypothetical_membership: prometheus::Histogram, + candidate_count: prometheus::GaugeVec, + active_leaves_count: prometheus::GaugeVec, + implicit_view_candidate_count: prometheus::Gauge, } /// Candidate backing metrics. @@ -33,13 +36,11 @@ pub(crate) struct MetricsInner { pub struct Metrics(pub(crate) Option); impl Metrics { - /// Provide a timer for handling `prune_view_candidate_storage` which observes on drop. - pub fn time_prune_view_candidate_storage( + /// Provide a timer for handling `ActiveLeavesUpdate` which observes on drop. + pub fn time_handle_active_leaves_update( &self, ) -> Option { - self.0 - .as_ref() - .map(|metrics| metrics.prune_view_candidate_storage.start_timer()) + self.0.as_ref().map(|metrics| metrics.time_active_leaves_update.start_timer()) } /// Provide a timer for handling `IntroduceSecondedCandidate` which observes on drop. @@ -48,31 +49,47 @@ impl Metrics { ) -> Option { self.0 .as_ref() - .map(|metrics| metrics.introduce_seconded_candidate.start_timer()) + .map(|metrics| metrics.time_introduce_seconded_candidate.start_timer()) + } + + /// Provide a timer for handling `CandidateBacked` which observes on drop. + pub fn time_candidate_backed(&self) -> Option { + self.0.as_ref().map(|metrics| metrics.time_candidate_backed.start_timer()) } /// Provide a timer for handling `GetHypotheticalMembership` which observes on drop. pub fn time_hypothetical_membership_request( &self, ) -> Option { - self.0.as_ref().map(|metrics| metrics.hypothetical_membership.start_timer()) + self.0 + .as_ref() + .map(|metrics| metrics.time_hypothetical_membership.start_timer()) } - /// Record the size of the candidate storage. First param is the connected candidates count, - /// second param is the unconnected candidates count. - pub fn record_candidate_storage_size(&self, connected_count: u64, unconnected_count: u64) { + /// Record number of candidates across all fragment chains. First param is the connected + /// candidates count, second param is the unconnected candidates count. + pub fn record_candidate_count(&self, connected_count: u64, unconnected_count: u64) { self.0.as_ref().map(|metrics| { + metrics.candidate_count.with_label_values(&["connected"]).set(connected_count); metrics - .candidate_storage_count - .with_label_values(&["connected"]) - .set(connected_count) + .candidate_count + .with_label_values(&["unconnected"]) + .set(unconnected_count); }); + } + /// Record the number of candidates present in the implicit view of the subsystem. + pub fn record_candidate_count_in_implicit_view(&self, count: u64) { self.0.as_ref().map(|metrics| { - metrics - .candidate_storage_count - .with_label_values(&["unconnected"]) - .set(unconnected_count) + metrics.implicit_view_candidate_count.set(count); + }); + } + + /// Record the number of active/inactive leaves kept by the subsystem. + pub fn record_leaves_count(&self, active_count: u64, inactive_count: u64) { + self.0.as_ref().map(|metrics| { + metrics.active_leaves_count.with_label_values(&["active"]).set(active_count); + metrics.active_leaves_count.with_label_values(&["inactive"]).set(inactive_count); }); } } @@ -80,37 +97,61 @@ impl Metrics { impl metrics::Metrics for Metrics { fn try_register(registry: &prometheus::Registry) -> Result { let metrics = MetricsInner { - prune_view_candidate_storage: prometheus::register( + time_active_leaves_update: prometheus::register( prometheus::Histogram::with_opts(prometheus::HistogramOpts::new( - "polkadot_parachain_prospective_parachains_prune_view_candidate_storage", - "Time spent within `prospective_parachains::prune_view_candidate_storage`", + "polkadot_parachain_prospective_parachains_time_active_leaves_update", + "Time spent within `prospective_parachains::handle_active_leaves_update`", ))?, registry, )?, - introduce_seconded_candidate: prometheus::register( + time_introduce_seconded_candidate: prometheus::register( prometheus::Histogram::with_opts(prometheus::HistogramOpts::new( - "polkadot_parachain_prospective_parachains_introduce_seconded_candidate", + "polkadot_parachain_prospective_parachains_time_introduce_seconded_candidate", "Time spent within `prospective_parachains::handle_introduce_seconded_candidate`", ))?, registry, )?, - hypothetical_membership: prometheus::register( + time_candidate_backed: prometheus::register( prometheus::Histogram::with_opts(prometheus::HistogramOpts::new( - "polkadot_parachain_prospective_parachains_hypothetical_membership", + "polkadot_parachain_prospective_parachains_time_candidate_backed", + "Time spent within `prospective_parachains::handle_candidate_backed`", + ))?, + registry, + )?, + time_hypothetical_membership: prometheus::register( + prometheus::Histogram::with_opts(prometheus::HistogramOpts::new( + "polkadot_parachain_prospective_parachains_time_hypothetical_membership", "Time spent responding to `GetHypotheticalMembership`", ))?, registry, )?, - candidate_storage_count: prometheus::register( + candidate_count: prometheus::register( GaugeVec::new( Opts::new( - "polkadot_parachain_prospective_parachains_candidate_storage_count", - "Number of candidates present in the candidate storage, split by connected and unconnected" + "polkadot_parachain_prospective_parachains_candidate_count", + "Number of candidates present across all fragment chains, split by connected and unconnected" ), &["type"], )?, registry, )?, + active_leaves_count: prometheus::register( + GaugeVec::new( + Opts::new( + "polkadot_parachain_prospective_parachains_active_leaves_count", + "Number of leaves kept by the subsystem, split by active/inactive" + ), + &["type"], + )?, + registry, + )?, + implicit_view_candidate_count: prometheus::register( + Gauge::new( + "polkadot_parachain_prospective_parachains_implicit_view_candidate_count", + "Number of candidates present in the implicit view" + )?, + registry + )?, }; Ok(Metrics(Some(metrics))) } diff --git a/polkadot/node/core/prospective-parachains/src/tests.rs b/polkadot/node/core/prospective-parachains/src/tests.rs index 221fbf4c4e60..14a093239e8e 100644 --- a/polkadot/node/core/prospective-parachains/src/tests.rs +++ b/polkadot/node/core/prospective-parachains/src/tests.rs @@ -111,6 +111,8 @@ fn get_parent_hash(hash: Hash) -> Hash { fn test_harness>( test: impl FnOnce(VirtualOverseer) -> T, ) -> View { + sp_tracing::init_for_tests(); + let pool = sp_core::testing::TaskExecutor::new(); let (mut context, virtual_overseer) = @@ -203,6 +205,32 @@ async fn activate_leaf( activate_leaf_with_params(virtual_overseer, leaf, test_state, ASYNC_BACKING_PARAMETERS).await; } +async fn activate_leaf_with_parent_hash_fn( + virtual_overseer: &mut VirtualOverseer, + leaf: &TestLeaf, + test_state: &TestState, + parent_hash_fn: impl Fn(Hash) -> Hash, +) { + let TestLeaf { number, hash, .. } = leaf; + + let activated = new_leaf(*hash, *number); + + virtual_overseer + .send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves(ActiveLeavesUpdate::start_work( + activated, + )))) + .await; + + handle_leaf_activation( + virtual_overseer, + leaf, + test_state, + ASYNC_BACKING_PARAMETERS, + parent_hash_fn, + ) + .await; +} + async fn activate_leaf_with_params( virtual_overseer: &mut VirtualOverseer, leaf: &TestLeaf, @@ -219,7 +247,14 @@ async fn activate_leaf_with_params( )))) .await; - handle_leaf_activation(virtual_overseer, leaf, test_state, async_backing_params).await; + handle_leaf_activation( + virtual_overseer, + leaf, + test_state, + async_backing_params, + get_parent_hash, + ) + .await; } async fn handle_leaf_activation( @@ -227,6 +262,7 @@ async fn handle_leaf_activation( leaf: &TestLeaf, test_state: &TestState, async_backing_params: AsyncBackingParams, + parent_hash_fn: impl Fn(Hash) -> Hash, ) { let TestLeaf { number, hash, para_data } = leaf; @@ -281,7 +317,7 @@ async fn handle_leaf_activation( let min_min = para_data.iter().map(|(_, data)| data.min_relay_parent).min().unwrap_or(*number); let ancestry_len = number - min_min; let ancestry_hashes: Vec = - std::iter::successors(Some(*hash), |h| Some(get_parent_hash(*h))) + std::iter::successors(Some(*hash), |h| Some(parent_hash_fn(*h))) .skip(1) .take(ancestry_len as usize) .collect(); @@ -307,16 +343,20 @@ async fn handle_leaf_activation( ); } + let mut used_relay_parents = HashSet::new(); for (hash, number) in ancestry_iter { - send_block_header(virtual_overseer, hash, number).await; - assert_matches!( - virtual_overseer.recv().await, - AllMessages::RuntimeApi( - RuntimeApiMessage::Request(parent, RuntimeApiRequest::SessionIndexForChild(tx)) - ) if parent == hash => { - tx.send(Ok(1)).unwrap(); - } - ); + if !used_relay_parents.contains(&hash) { + send_block_header(virtual_overseer, hash, number).await; + assert_matches!( + virtual_overseer.recv().await, + AllMessages::RuntimeApi( + RuntimeApiMessage::Request(parent, RuntimeApiRequest::SessionIndexForChild(tx)) + ) if parent == hash => { + tx.send(Ok(1)).unwrap(); + } + ); + used_relay_parents.insert(hash); + } } let paras: HashSet<_> = test_state.claim_queue.values().flatten().collect(); @@ -353,12 +393,16 @@ async fn handle_leaf_activation( ); for pending in pending_availability { - send_block_header( - virtual_overseer, - pending.descriptor.relay_parent, - pending.relay_parent_number, - ) - .await; + if !used_relay_parents.contains(&pending.descriptor.relay_parent) { + send_block_header( + virtual_overseer, + pending.descriptor.relay_parent, + pending.relay_parent_number, + ) + .await; + + used_relay_parents.insert(pending.descriptor.relay_parent); + } } } @@ -513,6 +557,26 @@ async fn get_pvd( assert_eq!(resp, expected_pvd); } +macro_rules! make_and_back_candidate { + ($test_state:ident, $virtual_overseer:ident, $leaf:ident, $parent:expr, $index:expr) => {{ + let (mut candidate, pvd) = make_candidate( + $leaf.hash, + $leaf.number, + 1.into(), + $parent.commitments.head_data.clone(), + HeadData(vec![$index]), + $test_state.validation_code_hash, + ); + // Set a field to make this candidate unique. + candidate.descriptor.para_head = Hash::from_low_u64_le($index); + let candidate_hash = candidate.hash(); + introduce_seconded_candidate(&mut $virtual_overseer, candidate.clone(), pvd).await; + back_candidate(&mut $virtual_overseer, &candidate, candidate_hash).await; + + (candidate, candidate_hash) + }}; +} + #[test] fn should_do_no_work_if_async_backing_disabled_for_leaf() { async fn activate_leaf_async_backing_disabled(virtual_overseer: &mut VirtualOverseer) { @@ -542,7 +606,6 @@ fn should_do_no_work_if_async_backing_disabled_for_leaf() { }); assert!(view.active_leaves.is_empty()); - assert!(view.candidate_storage.is_empty()); } // Send some candidates and make sure all are found: @@ -718,10 +781,6 @@ fn introduce_candidates_basic() { }); assert_eq!(view.active_leaves.len(), 3); - assert_eq!(view.candidate_storage.len(), 2); - // Two parents and two candidates per para. - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (2, 2)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (2, 2)); } #[test] @@ -766,28 +825,36 @@ fn introduce_candidate_multiple_times() { 1.into(), Ancestors::default(), 5, - response_a, + response_a.clone(), ) .await; - // Introduce the same candidate multiple times. It'll return true but it won't be added. - // We'll check below that the candidate count remains 1. + // Introduce the same candidate multiple times. It'll return true but it will only be added + // once. for _ in 0..5 { introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a.clone()) .await; } + // Check candidate tree membership. + get_backable_candidates( + &mut virtual_overseer, + &leaf_a, + 1.into(), + Ancestors::default(), + 5, + response_a, + ) + .await; + virtual_overseer }); assert_eq!(view.active_leaves.len(), 1); - assert_eq!(view.candidate_storage.len(), 2); - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (1, 1)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (0, 0)); } #[test] -fn fragment_chain_length_is_bounded() { +fn fragment_chain_best_chain_length_is_bounded() { let test_state = TestState::default(); let view = test_harness(|mut virtual_overseer| async move { // Leaf A @@ -835,12 +902,11 @@ fn fragment_chain_length_is_bounded() { ); // Introduce candidates A and B. Since max depth is 1, only these two will be allowed. - introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a.clone()) - .await; - introduce_seconded_candidate(&mut virtual_overseer, candidate_b.clone(), pvd_b.clone()) - .await; + introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a).await; + introduce_seconded_candidate(&mut virtual_overseer, candidate_b.clone(), pvd_b).await; - // Back candidates. Otherwise, we cannot check membership with GetBackableCandidates. + // Back candidates. Otherwise, we cannot check membership with GetBackableCandidates and + // they won't be part of the best chain. back_candidate(&mut virtual_overseer, &candidate_a, candidate_a.hash()).await; back_candidate(&mut virtual_overseer, &candidate_b, candidate_b.hash()).await; @@ -855,103 +921,25 @@ fn fragment_chain_length_is_bounded() { ) .await; - // Introducing C will fail. - introduce_seconded_candidate_failed(&mut virtual_overseer, candidate_c, pvd_c.clone()) - .await; - - virtual_overseer - }); - - assert_eq!(view.active_leaves.len(), 1); - assert_eq!(view.candidate_storage.len(), 2); - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (2, 2)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (0, 0)); -} - -#[test] -fn unconnected_candidate_count_is_bounded() { - let test_state = TestState::default(); - let view = test_harness(|mut virtual_overseer| async move { - // Leaf A - let leaf_a = TestLeaf { - number: 100, - hash: Hash::from_low_u64_be(130), - para_data: vec![ - (1.into(), PerParaData::new(97, HeadData(vec![1, 2, 3]))), - (2.into(), PerParaData::new(100, HeadData(vec![2, 3, 4]))), - ], - }; - // Activate leaves. - activate_leaf_with_params( - &mut virtual_overseer, - &leaf_a, - &test_state, - AsyncBackingParams { max_candidate_depth: 1, allowed_ancestry_len: 3 }, - ) - .await; - - // Candidates A, B and C are all potential candidates but don't form a chain. - let (candidate_a, pvd_a) = make_candidate( - leaf_a.hash, - leaf_a.number, - 1.into(), - HeadData(vec![1]), - HeadData(vec![2]), - test_state.validation_code_hash, - ); - let (candidate_b, pvd_b) = make_candidate( - leaf_a.hash, - leaf_a.number, - 1.into(), - HeadData(vec![3]), - HeadData(vec![4]), - test_state.validation_code_hash, - ); - let (candidate_c, pvd_c) = make_candidate( - leaf_a.hash, - leaf_a.number, - 1.into(), - HeadData(vec![4]), - HeadData(vec![5]), - test_state.validation_code_hash, - ); - - // Introduce candidates A and B. Although max depth is 1 (which should allow for two - // candidates), only 1 is allowed, because the last candidate must be a connected candidate. - introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a.clone()) - .await; - introduce_seconded_candidate_failed( - &mut virtual_overseer, - candidate_b.clone(), - pvd_b.clone(), - ) - .await; - - // Back candidates. Otherwise, we cannot check membership with GetBackableCandidates. - back_candidate(&mut virtual_overseer, &candidate_a, candidate_a.hash()).await; + // Introducing C will not fail. It will be kept as unconnected storage. + introduce_seconded_candidate(&mut virtual_overseer, candidate_c.clone(), pvd_c).await; + // When being backed, candidate C will be dropped. + back_candidate(&mut virtual_overseer, &candidate_c, candidate_c.hash()).await; - // Check candidate tree membership. Should be empty. get_backable_candidates( &mut virtual_overseer, &leaf_a, 1.into(), Ancestors::default(), 5, - vec![], + vec![(candidate_a.hash(), leaf_a.hash), (candidate_b.hash(), leaf_a.hash)], ) .await; - // Introducing C will also fail. - introduce_seconded_candidate_failed(&mut virtual_overseer, candidate_c, pvd_c.clone()) - .await; - virtual_overseer }); assert_eq!(view.active_leaves.len(), 1); - assert_eq!(view.candidate_storage.len(), 2); - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (1, 1)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (0, 0)); } // Send some candidates, check if the candidate won't be found once its relay parent leaves the @@ -1178,7 +1166,6 @@ fn introduce_candidate_parent_leaving_view() { }); assert_eq!(view.active_leaves.len(), 0); - assert_eq!(view.candidate_storage.len(), 0); } // Introduce a candidate to multiple forks, see how the membership is returned. @@ -1249,13 +1236,12 @@ fn introduce_candidate_on_multiple_forks() { }); assert_eq!(view.active_leaves.len(), 2); - assert_eq!(view.candidate_storage.len(), 2); - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (1, 1)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (0, 0)); } #[test] fn unconnected_candidates_become_connected() { + // This doesn't test all the complicated cases with many unconnected candidates, as it's more + // extensively tested in the `fragment_chain::tests` module. let test_state = TestState::default(); let view = test_harness(|mut virtual_overseer| async move { // Leaf A @@ -1351,9 +1337,6 @@ fn unconnected_candidates_become_connected() { }); assert_eq!(view.active_leaves.len(), 1); - assert_eq!(view.candidate_storage.len(), 2); - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (4, 4)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (0, 0)); } // Backs some candidates and tests `GetBackableCandidates` when requesting a single candidate. @@ -1435,6 +1418,10 @@ fn check_backable_query_single_candidate() { back_candidate(&mut virtual_overseer, &candidate_a, candidate_hash_a).await; back_candidate(&mut virtual_overseer, &candidate_b, candidate_hash_b).await; + // Back an unknown candidate. It doesn't return anything but it's ignored. Will not have any + // effect on the backable candidates. + back_candidate(&mut virtual_overseer, &candidate_b, CandidateHash(Hash::random())).await; + // Should not get any backable candidates for the other para. get_backable_candidates( &mut virtual_overseer, @@ -1490,35 +1477,11 @@ fn check_backable_query_single_candidate() { }); assert_eq!(view.active_leaves.len(), 1); - assert_eq!(view.candidate_storage.len(), 2); - // Two parents and two candidates on para 1. - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (2, 2)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (0, 0)); } // Backs some candidates and tests `GetBackableCandidates` when requesting a multiple candidates. #[test] fn check_backable_query_multiple_candidates() { - macro_rules! make_and_back_candidate { - ($test_state:ident, $virtual_overseer:ident, $leaf:ident, $parent:expr, $index:expr) => {{ - let (mut candidate, pvd) = make_candidate( - $leaf.hash, - $leaf.number, - 1.into(), - $parent.commitments.head_data.clone(), - HeadData(vec![$index]), - $test_state.validation_code_hash, - ); - // Set a field to make this candidate unique. - candidate.descriptor.para_head = Hash::from_low_u64_le($index); - let candidate_hash = candidate.hash(); - introduce_seconded_candidate(&mut $virtual_overseer, candidate.clone(), pvd).await; - back_candidate(&mut $virtual_overseer, &candidate, candidate_hash).await; - - (candidate, candidate_hash) - }}; - } - let test_state = TestState::default(); let view = test_harness(|mut virtual_overseer| async move { // Leaf A @@ -1786,10 +1749,6 @@ fn check_backable_query_multiple_candidates() { }); assert_eq!(view.active_leaves.len(), 1); - assert_eq!(view.candidate_storage.len(), 2); - // 4 candidates on para 1. - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (4, 4)); - assert_eq!(view.candidate_storage.get(&2.into()).unwrap().len(), (0, 0)); } // Test hypothetical membership query. @@ -1885,11 +1844,13 @@ fn check_hypothetical_membership_query() { introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a.clone()) .await; - // Get membership of candidates after adding A. C is not a potential candidate because we - // may only add one more candidate, which must be a connected candidate. - for (candidate, pvd) in - [(candidate_a.clone(), pvd_a.clone()), (candidate_b.clone(), pvd_b.clone())] - { + // Get membership of candidates after adding A. They all are still unconnected candidates + // (not part of the best backable chain). + for (candidate, pvd) in [ + (candidate_a.clone(), pvd_a.clone()), + (candidate_b.clone(), pvd_b.clone()), + (candidate_c.clone(), pvd_c.clone()), + ] { get_hypothetical_membership( &mut virtual_overseer, candidate.hash(), @@ -1900,14 +1861,24 @@ fn check_hypothetical_membership_query() { .await; } - get_hypothetical_membership( - &mut virtual_overseer, - candidate_c.hash(), - candidate_c.clone(), - pvd_c.clone(), - vec![], - ) - .await; + // Back A. Now A is part of the best chain the rest can be added as unconnected. + + back_candidate(&mut virtual_overseer, &candidate_a, candidate_a.hash()).await; + + for (candidate, pvd) in [ + (candidate_a.clone(), pvd_a.clone()), + (candidate_b.clone(), pvd_b.clone()), + (candidate_c.clone(), pvd_c.clone()), + ] { + get_hypothetical_membership( + &mut virtual_overseer, + candidate.hash(), + candidate, + pvd, + vec![leaf_a.hash, leaf_b.hash], + ) + .await; + } // Candidate D has invalid relay parent. let (candidate_d, pvd_d) = make_candidate( @@ -1920,14 +1891,17 @@ fn check_hypothetical_membership_query() { ); introduce_seconded_candidate_failed(&mut virtual_overseer, candidate_d, pvd_d).await; - // Add candidate B. + // Add candidate B and back it. introduce_seconded_candidate(&mut virtual_overseer, candidate_b.clone(), pvd_b.clone()) .await; + back_candidate(&mut virtual_overseer, &candidate_b, candidate_b.hash()).await; // Get membership of candidates after adding B. - for (candidate, pvd) in - [(candidate_a.clone(), pvd_a.clone()), (candidate_b.clone(), pvd_b.clone())] - { + for (candidate, pvd) in [ + (candidate_a.clone(), pvd_a.clone()), + (candidate_b.clone(), pvd_b.clone()), + (candidate_c.clone(), pvd_c.clone()), + ] { get_hypothetical_membership( &mut virtual_overseer, candidate.hash(), @@ -1938,24 +1912,10 @@ fn check_hypothetical_membership_query() { .await; } - get_hypothetical_membership( - &mut virtual_overseer, - candidate_c.hash(), - candidate_c.clone(), - pvd_c.clone(), - vec![], - ) - .await; - - // Add candidate C. It will fail because we have enough candidates for the configured depth. - introduce_seconded_candidate_failed(&mut virtual_overseer, candidate_c, pvd_c).await; - virtual_overseer }); assert_eq!(view.active_leaves.len(), 2); - assert_eq!(view.candidate_storage.len(), 2); - assert_eq!(view.candidate_storage.get(&1.into()).unwrap().len(), (2, 2)); } #[test] @@ -2005,6 +1965,16 @@ fn check_pvd_query() { test_state.validation_code_hash, ); + // Candidate E. + let (candidate_e, pvd_e) = make_candidate( + leaf_a.hash, + leaf_a.number, + 1.into(), + HeadData(vec![5]), + HeadData(vec![6]), + test_state.validation_code_hash, + ); + // Get pvd of candidate A before adding it. get_pvd( &mut virtual_overseer, @@ -2067,20 +2037,20 @@ fn check_pvd_query() { introduce_seconded_candidate(&mut virtual_overseer, candidate_c, pvd_c.clone()).await; // Get pvd of candidate C after adding it. - get_pvd( - &mut virtual_overseer, - 1.into(), - leaf_a.hash, - HeadData(vec![2]), - Some(pvd_c.clone()), - ) - .await; + get_pvd(&mut virtual_overseer, 1.into(), leaf_a.hash, HeadData(vec![2]), Some(pvd_c)).await; + + // Get pvd of candidate E before adding it. It won't be found, as we don't have its parent. + get_pvd(&mut virtual_overseer, 1.into(), leaf_a.hash, HeadData(vec![5]), None).await; + + // Add candidate E and check again. Should succeed this time. + introduce_seconded_candidate(&mut virtual_overseer, candidate_e, pvd_e.clone()).await; + + get_pvd(&mut virtual_overseer, 1.into(), leaf_a.hash, HeadData(vec![5]), Some(pvd_e)).await; virtual_overseer }); assert_eq!(view.active_leaves.len(), 1); - assert_eq!(view.candidate_storage.len(), 2); } // Test simultaneously activating and deactivating leaves, and simultaneously deactivating @@ -2150,6 +2120,7 @@ fn correctly_updates_leaves(#[case] runtime_api_version: u32) { &leaf_c, &test_state, ASYNC_BACKING_PARAMETERS, + get_parent_hash, ) .await; @@ -2171,13 +2142,6 @@ fn correctly_updates_leaves(#[case] runtime_api_version: u32) { virtual_overseer .send(FromOrchestra::Signal(OverseerSignal::ActiveLeaves(update))) .await; - handle_leaf_activation( - &mut virtual_overseer, - &leaf_a, - &test_state, - ASYNC_BACKING_PARAMETERS, - ) - .await; // Remove the leaf again. Send some unnecessary hashes. let update = ActiveLeavesUpdate { @@ -2192,7 +2156,322 @@ fn correctly_updates_leaves(#[case] runtime_api_version: u32) { }); assert_eq!(view.active_leaves.len(), 0); - assert_eq!(view.candidate_storage.len(), 0); +} + +#[test] +fn handle_active_leaves_update_gets_candidates_from_parent() { + let para_id = ParaId::from(1); + let mut test_state = TestState::default(); + test_state.claim_queue = test_state + .claim_queue + .into_iter() + .filter(|(_, paras)| matches!(paras.front(), Some(para) if para == ¶_id)) + .collect(); + assert_eq!(test_state.claim_queue.len(), 1); + let view = test_harness(|mut virtual_overseer| async move { + // Leaf A + let leaf_a = TestLeaf { + number: 100, + hash: Hash::from_low_u64_be(130), + para_data: vec![(para_id, PerParaData::new(97, HeadData(vec![1, 2, 3])))], + }; + // Activate leaf A. + activate_leaf(&mut virtual_overseer, &leaf_a, &test_state).await; + + // Candidates A, B, C and D all form a chain + let (candidate_a, pvd_a) = make_candidate( + leaf_a.hash, + leaf_a.number, + para_id, + HeadData(vec![1, 2, 3]), + HeadData(vec![1]), + test_state.validation_code_hash, + ); + let candidate_hash_a = candidate_a.hash(); + introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a).await; + back_candidate(&mut virtual_overseer, &candidate_a, candidate_hash_a).await; + + let (candidate_b, candidate_hash_b) = + make_and_back_candidate!(test_state, virtual_overseer, leaf_a, &candidate_a, 2); + let (candidate_c, candidate_hash_c) = + make_and_back_candidate!(test_state, virtual_overseer, leaf_a, &candidate_b, 3); + let (candidate_d, candidate_hash_d) = + make_and_back_candidate!(test_state, virtual_overseer, leaf_a, &candidate_c, 4); + + let mut all_candidates_resp = vec![ + (candidate_hash_a, leaf_a.hash), + (candidate_hash_b, leaf_a.hash), + (candidate_hash_c, leaf_a.hash), + (candidate_hash_d, leaf_a.hash), + ]; + + // Check candidate tree membership. + get_backable_candidates( + &mut virtual_overseer, + &leaf_a, + para_id, + Ancestors::default(), + 5, + all_candidates_resp.clone(), + ) + .await; + + // Activate leaf B, which makes candidates A and B pending availability. + // Leaf B + let leaf_b = TestLeaf { + number: 101, + hash: Hash::from_low_u64_be(129), + para_data: vec![( + para_id, + PerParaData::new_with_pending( + 98, + HeadData(vec![1, 2, 3]), + vec![ + CandidatePendingAvailability { + candidate_hash: candidate_a.hash(), + descriptor: candidate_a.descriptor.clone(), + commitments: candidate_a.commitments.clone(), + relay_parent_number: leaf_a.number, + max_pov_size: MAX_POV_SIZE, + }, + CandidatePendingAvailability { + candidate_hash: candidate_b.hash(), + descriptor: candidate_b.descriptor.clone(), + commitments: candidate_b.commitments.clone(), + relay_parent_number: leaf_a.number, + max_pov_size: MAX_POV_SIZE, + }, + ], + ), + )], + }; + // Activate leaf B. + activate_leaf(&mut virtual_overseer, &leaf_b, &test_state).await; + get_backable_candidates( + &mut virtual_overseer, + &leaf_b, + para_id, + Ancestors::default(), + 5, + vec![], + ) + .await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_b, + para_id, + [candidate_a.hash(), candidate_b.hash()].into_iter().collect(), + 5, + vec![(candidate_c.hash(), leaf_a.hash), (candidate_d.hash(), leaf_a.hash)], + ) + .await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_b, + para_id, + Ancestors::default(), + 5, + vec![], + ) + .await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_a, + para_id, + Ancestors::default(), + 5, + all_candidates_resp.clone(), + ) + .await; + + // Now deactivate leaf A. + deactivate_leaf(&mut virtual_overseer, leaf_a.hash).await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_b, + para_id, + Ancestors::default(), + 5, + vec![], + ) + .await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_b, + para_id, + [candidate_a.hash(), candidate_b.hash()].into_iter().collect(), + 5, + vec![(candidate_c.hash(), leaf_a.hash), (candidate_d.hash(), leaf_a.hash)], + ) + .await; + + // Now add leaf C, which will be a sibling (fork) of leaf B. It should also inherit the + // candidates of leaf A (their common parent). + let leaf_c = TestLeaf { + number: 101, + hash: Hash::from_low_u64_be(12), + para_data: vec![( + para_id, + PerParaData::new_with_pending(98, HeadData(vec![1, 2, 3]), vec![]), + )], + }; + + activate_leaf_with_parent_hash_fn(&mut virtual_overseer, &leaf_c, &test_state, |hash| { + if hash == leaf_c.hash { + leaf_a.hash + } else { + get_parent_hash(hash) + } + }) + .await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_b, + para_id, + [candidate_a.hash(), candidate_b.hash()].into_iter().collect(), + 5, + vec![(candidate_c.hash(), leaf_a.hash), (candidate_d.hash(), leaf_a.hash)], + ) + .await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_c, + para_id, + Ancestors::new(), + 5, + all_candidates_resp.clone(), + ) + .await; + + // Deactivate C and add another candidate that will be present on the deactivated parent A. + // When activating C again it should also get the new candidate. Deactivated leaves are + // still updated with new candidates. + deactivate_leaf(&mut virtual_overseer, leaf_c.hash).await; + + let (candidate_e, _) = + make_and_back_candidate!(test_state, virtual_overseer, leaf_a, &candidate_d, 5); + activate_leaf_with_parent_hash_fn(&mut virtual_overseer, &leaf_c, &test_state, |hash| { + if hash == leaf_c.hash { + leaf_a.hash + } else { + get_parent_hash(hash) + } + }) + .await; + + get_backable_candidates( + &mut virtual_overseer, + &leaf_b, + para_id, + [candidate_a.hash(), candidate_b.hash()].into_iter().collect(), + 5, + vec![ + (candidate_c.hash(), leaf_a.hash), + (candidate_d.hash(), leaf_a.hash), + (candidate_e.hash(), leaf_a.hash), + ], + ) + .await; + + all_candidates_resp.push((candidate_e.hash(), leaf_a.hash)); + get_backable_candidates( + &mut virtual_overseer, + &leaf_c, + para_id, + Ancestors::new(), + 5, + all_candidates_resp, + ) + .await; + + // Querying the backable candidates for deactivated leaf won't work. + get_backable_candidates( + &mut virtual_overseer, + &leaf_a, + para_id, + Ancestors::new(), + 5, + vec![], + ) + .await; + + virtual_overseer + }); + + assert_eq!(view.active_leaves.len(), 2); + assert_eq!(view.per_relay_parent.len(), 3); +} + +#[test] +fn handle_active_leaves_update_bounded_implicit_view() { + let para_id = ParaId::from(1); + let mut test_state = TestState::default(); + test_state.claim_queue = test_state + .claim_queue + .into_iter() + .filter(|(_, paras)| matches!(paras.front(), Some(para) if para == ¶_id)) + .collect(); + assert_eq!(test_state.claim_queue.len(), 1); + + let mut leaves = vec![TestLeaf { + number: 100, + hash: Hash::from_low_u64_be(130), + para_data: vec![( + para_id, + PerParaData::new(100 - ALLOWED_ANCESTRY_LEN, HeadData(vec![1, 2, 3])), + )], + }]; + + for index in 1..10 { + let prev_leaf = &leaves[index - 1]; + leaves.push(TestLeaf { + number: prev_leaf.number - 1, + hash: get_parent_hash(prev_leaf.hash), + para_data: vec![( + para_id, + PerParaData::new( + prev_leaf.number - 1 - ALLOWED_ANCESTRY_LEN, + HeadData(vec![1, 2, 3]), + ), + )], + }); + } + leaves.reverse(); + + let view = test_harness(|mut virtual_overseer| async { + // Activate first 10 leaves. + for leaf in &leaves[0..10] { + activate_leaf(&mut virtual_overseer, leaf, &test_state).await; + } + + // Now deactivate first 9 leaves. + for leaf in &leaves[0..9] { + deactivate_leaf(&mut virtual_overseer, leaf.hash).await; + } + + virtual_overseer + }); + + // Only latest leaf is active. + assert_eq!(view.active_leaves.len(), 1); + // We keep allowed_ancestry_len implicit leaves. The latest leaf is also present here. + assert_eq!( + view.per_relay_parent.len() as u32, + ASYNC_BACKING_PARAMETERS.allowed_ancestry_len + 1 + ); + + assert_eq!(view.active_leaves, [leaves[9].hash].into_iter().collect()); + assert_eq!( + view.per_relay_parent.into_keys().collect::>(), + leaves[6..].into_iter().map(|l| l.hash).collect::>() + ); } #[test] @@ -2251,7 +2530,8 @@ fn persists_pending_availability_candidate() { ); let candidate_hash_b = candidate_b.hash(); - introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a).await; + introduce_seconded_candidate(&mut virtual_overseer, candidate_a.clone(), pvd_a.clone()) + .await; back_candidate(&mut virtual_overseer, &candidate_a, candidate_hash_a).await; let candidate_a_pending_av = CandidatePendingAvailability { @@ -2275,6 +2555,15 @@ fn persists_pending_availability_candidate() { }; activate_leaf(&mut virtual_overseer, &leaf_b, &test_state).await; + get_hypothetical_membership( + &mut virtual_overseer, + candidate_hash_a, + candidate_a, + pvd_a, + vec![leaf_a.hash, leaf_b.hash], + ) + .await; + introduce_seconded_candidate(&mut virtual_overseer, candidate_b.clone(), pvd_b).await; back_candidate(&mut virtual_overseer, &candidate_b, candidate_hash_b).await; diff --git a/polkadot/node/core/provisioner/src/lib.rs b/polkadot/node/core/provisioner/src/lib.rs index 3f622a60a059..ffc5859b7756 100644 --- a/polkadot/node/core/provisioner/src/lib.rs +++ b/polkadot/node/core/provisioner/src/lib.rs @@ -273,7 +273,7 @@ async fn handle_communication( let span = state.span.child("provisionable-data"); let _timer = metrics.time_provisionable_data(); - gum::trace!(target: LOG_TARGET, ?relay_parent, "Received provisionable data."); + gum::trace!(target: LOG_TARGET, ?relay_parent, "Received provisionable data: {:?}", &data); note_provisionable_data(state, &span, data); } @@ -794,9 +794,11 @@ async fn select_candidates( relay_parent: Hash, sender: &mut impl overseer::ProvisionerSenderTrait, ) -> Result, Error> { - gum::trace!(target: LOG_TARGET, + gum::trace!( + target: LOG_TARGET, leaf_hash=?relay_parent, - "before GetBackedCandidates"); + "before GetBackedCandidates" + ); let selected_candidates = match prospective_parachains_mode { ProspectiveParachainsMode::Enabled { .. } => diff --git a/polkadot/node/network/statement-distribution/src/v2/mod.rs b/polkadot/node/network/statement-distribution/src/v2/mod.rs index 47d350849b20..109c29f520c5 100644 --- a/polkadot/node/network/statement-distribution/src/v2/mod.rs +++ b/polkadot/node/network/statement-distribution/src/v2/mod.rs @@ -2237,7 +2237,9 @@ async fn fragment_chain_update_inner( // 2. find out which are in the frontier gum::debug!( target: LOG_TARGET, - "Calling getHypotheticalMembership from statement distribution" + active_leaf_hash = ?active_leaf_hash, + "Calling getHypotheticalMembership from statement distribution for candidates: {:?}", + &hypotheticals.iter().map(|hypo| hypo.candidate_hash()).collect::>() ); let candidate_memberships = { let (tx, rx) = oneshot::channel(); diff --git a/polkadot/node/subsystem-types/src/messages.rs b/polkadot/node/subsystem-types/src/messages.rs index ee937bca05bf..129a179586bf 100644 --- a/polkadot/node/subsystem-types/src/messages.rs +++ b/polkadot/node/subsystem-types/src/messages.rs @@ -43,13 +43,13 @@ use polkadot_node_primitives::{ }; use polkadot_primitives::{ async_backing, slashing, ApprovalVotingParams, AuthorityDiscoveryId, BackedCandidate, - BlockNumber, CandidateEvent, CandidateHash, CandidateIndex, CandidateReceipt, CollatorId, - CommittedCandidateReceipt, CoreIndex, CoreState, DisputeState, ExecutorParams, GroupIndex, - GroupRotationInfo, Hash, HeadData, Header as BlockHeader, Id as ParaId, InboundDownwardMessage, - InboundHrmpMessage, MultiDisputeStatementSet, NodeFeatures, OccupiedCoreAssumption, - PersistedValidationData, PvfCheckStatement, PvfExecKind, SessionIndex, SessionInfo, - SignedAvailabilityBitfield, SignedAvailabilityBitfields, ValidationCode, ValidationCodeHash, - ValidatorId, ValidatorIndex, ValidatorSignature, + BlockNumber, CandidateCommitments, CandidateEvent, CandidateHash, CandidateIndex, + CandidateReceipt, CollatorId, CommittedCandidateReceipt, CoreIndex, CoreState, DisputeState, + ExecutorParams, GroupIndex, GroupRotationInfo, Hash, HeadData, Header as BlockHeader, + Id as ParaId, InboundDownwardMessage, InboundHrmpMessage, MultiDisputeStatementSet, + NodeFeatures, OccupiedCoreAssumption, PersistedValidationData, PvfCheckStatement, PvfExecKind, + SessionIndex, SessionInfo, SignedAvailabilityBitfield, SignedAvailabilityBitfields, + ValidationCode, ValidationCodeHash, ValidatorId, ValidatorIndex, ValidatorSignature, }; use polkadot_statement_table::v2::Misbehavior; use std::{ @@ -1116,6 +1116,32 @@ impl HypotheticalCandidate { HypotheticalCandidate::Incomplete { .. } => None, } } + + /// Get the candidate commitments, if the candidate is complete. + pub fn commitments(&self) -> Option<&CandidateCommitments> { + match *self { + HypotheticalCandidate::Complete { ref receipt, .. } => Some(&receipt.commitments), + HypotheticalCandidate::Incomplete { .. } => None, + } + } + + /// Get the persisted validation data, if the candidate is complete. + pub fn persisted_validation_data(&self) -> Option<&PersistedValidationData> { + match *self { + HypotheticalCandidate::Complete { ref persisted_validation_data, .. } => + Some(persisted_validation_data), + HypotheticalCandidate::Incomplete { .. } => None, + } + } + + /// Get the validation code hash, if the candidate is complete. + pub fn validation_code_hash(&self) -> Option<&ValidationCodeHash> { + match *self { + HypotheticalCandidate::Complete { ref receipt, .. } => + Some(&receipt.descriptor.validation_code_hash), + HypotheticalCandidate::Incomplete { .. } => None, + } + } } /// Request specifying which candidates are either already included diff --git a/polkadot/node/subsystem-util/src/backing_implicit_view.rs b/polkadot/node/subsystem-util/src/backing_implicit_view.rs index 23a758d25715..a805ef8165e5 100644 --- a/polkadot/node/subsystem-util/src/backing_implicit_view.rs +++ b/polkadot/node/subsystem-util/src/backing_implicit_view.rs @@ -25,6 +25,7 @@ use polkadot_primitives::{BlockNumber, Hash, Id as ParaId}; use std::collections::HashMap; use crate::{ + inclusion_emulator::RelayChainBlockInfo, request_session_index_for_child, runtime::{self, prospective_parachains_mode, recv_runtime, ProspectiveParachainsMode}, }; @@ -121,6 +122,26 @@ struct BlockInfo { parent_hash: Hash, } +/// Information about a relay-chain block, to be used when calling this module from prospective +/// parachains. +#[derive(Debug, Clone, PartialEq)] +pub struct BlockInfoProspectiveParachains { + /// The hash of the relay-chain block. + pub hash: Hash, + /// The hash of the parent relay-chain block. + pub parent_hash: Hash, + /// The number of the relay-chain block. + pub number: BlockNumber, + /// The storage-root of the relay-chain block. + pub storage_root: Hash, +} + +impl From for RelayChainBlockInfo { + fn from(value: BlockInfoProspectiveParachains) -> Self { + Self { hash: value.hash, number: value.number, storage_root: value.storage_root } + } +} + impl View { /// Get an iterator over active leaves in the view. pub fn leaves(&self) -> impl Iterator { @@ -178,6 +199,61 @@ impl View { } } + /// Activate a leaf in the view. To be used by the prospective parachains subsystem. + /// + /// This will not request any additional data, as prospective parachains already provides all + /// the required info. + /// NOTE: using `activate_leaf` instead of this function will result in a + /// deadlock, as it calls prospective-parachains under the hood. + /// + /// No-op for known leaves. + pub fn activate_leaf_from_prospective_parachains( + &mut self, + leaf: BlockInfoProspectiveParachains, + ancestors: &[BlockInfoProspectiveParachains], + ) { + if self.leaves.contains_key(&leaf.hash) { + return + } + + // Retain at least `MINIMUM_RETAIN_LENGTH` blocks in storage. + // This helps to avoid Chain API calls when activating leaves in the + // same chain. + let retain_minimum = std::cmp::min( + ancestors.last().map(|a| a.number).unwrap_or(0), + leaf.number.saturating_sub(MINIMUM_RETAIN_LENGTH), + ); + + self.leaves.insert(leaf.hash, ActiveLeafPruningInfo { retain_minimum }); + let mut allowed_relay_parents = AllowedRelayParents { + allowed_relay_parents_contiguous: Vec::with_capacity(ancestors.len()), + // In this case, initialise this to an empty map, as prospective parachains already has + // this data and it won't query the implicit view for it. + minimum_relay_parents: HashMap::new(), + }; + + for ancestor in ancestors { + self.block_info_storage.insert( + ancestor.hash, + BlockInfo { + block_number: ancestor.number, + maybe_allowed_relay_parents: None, + parent_hash: ancestor.parent_hash, + }, + ); + allowed_relay_parents.allowed_relay_parents_contiguous.push(ancestor.hash); + } + + self.block_info_storage.insert( + leaf.hash, + BlockInfo { + block_number: leaf.number, + maybe_allowed_relay_parents: Some(allowed_relay_parents), + parent_hash: leaf.parent_hash, + }, + ); + } + /// Deactivate a leaf in the view. This prunes any outdated implicit ancestors as well. /// /// Returns hashes of blocks pruned from storage. diff --git a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs index b5aef325c8b4..37750cdfeb2f 100644 --- a/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs +++ b/polkadot/node/subsystem-util/src/inclusion_emulator/mod.rs @@ -39,8 +39,8 @@ /// /// # Usage /// -/// It's expected that the users of this module will be building up chains of -/// [`Fragment`]s and consistently pruning and adding to the chains. +/// It's expected that the users of this module will be building up chains or trees of +/// [`Fragment`]s and consistently pruning and adding to them. /// /// ## Operating Constraints /// @@ -56,55 +56,19 @@ /// /// ## Fragment Chains /// -/// For simplicity and practicality, we expect that collators of the same parachain are -/// cooperating and don't create parachain forks or cycles on the same relay chain active leaf. -/// Therefore, higher-level code should maintain one fragment chain for each active leaf (not a -/// fragment tree). If parachains do create forks, their performance in regards to async -/// backing and elastic scaling will suffer, because different validators will have different -/// predictions of the future. +/// For the sake of this module, we don't care how higher-level code is managing parachain +/// fragments, whether or not they're kept as a chain or tree. In reality, +/// prospective-parachains is maintaining for every active leaf, a chain of the "best" backable +/// candidates and a storage of potential candidates which may be added to this chain in the +/// future. /// /// As the relay-chain grows, some predictions come true and others come false. -/// And new predictions get made. These three changes correspond distinctly to the -/// 3 primary operations on fragment chains. +/// And new predictions get made. Higher-level code is responsible for adding and pruning the +/// fragments chains. /// /// Avoiding fragment-chain blowup is beyond the scope of this module. Higher-level must ensure /// proper spam protection. /// -/// ### Pruning Fragment Chains -/// -/// When the relay-chain advances, we want to compare the new constraints of that relay-parent -/// to the root of the fragment chain we have. There are 3 cases: -/// -/// 1. The root fragment is still valid under the new constraints. In this case, we do nothing. -/// This is the "prediction still uncertain" case. (Corresponds to some candidates still -/// being pending availability). -/// -/// 2. The root fragment (potentially along with a number of descendants) is invalid under the -/// new constraints because it has been included by the relay-chain. In this case, we can -/// discard the included chain and split & re-root the chain under its descendants and -/// compare to the new constraints again. This is the "prediction came true" case. -/// -/// 3. The root fragment becomes invalid under the new constraints for any reason (if for -/// example the parachain produced a fork and the block producer picked a different -/// candidate to back). In this case we can discard the entire fragment chain. This is the -/// "prediction came false" case. -/// -/// This is all a bit of a simplification because it assumes that the relay-chain advances -/// without forks and is finalized instantly. In practice, the set of fragment-chains needs to -/// be observable from the perspective of a few different possible forks of the relay-chain and -/// not pruned too eagerly. -/// -/// Note that the fragments themselves don't need to change and the only thing we care about -/// is whether the predictions they represent are still valid. -/// -/// ### Extending Fragment Chains -/// -/// As predictions fade into the past, new ones should be stacked on top. -/// -/// Every new relay-chain block is an opportunity to make a new prediction about the future. -/// Higher-level logic should decide whether to build upon an existing chain or whether -/// to create a new fragment-chain. -/// /// ### Code Upgrades /// /// Code upgrades are the main place where this emulation fails. The on-chain PVF upgrade @@ -116,10 +80,11 @@ /// /// That means a few blocks of execution time lost, which is not a big deal for code upgrades /// in practice at most once every few weeks. +use polkadot_node_subsystem::messages::HypotheticalCandidate; use polkadot_primitives::{ async_backing::Constraints as PrimitiveConstraints, BlockNumber, CandidateCommitments, - CollatorId, CollatorSignature, Hash, HeadData, Id as ParaId, PersistedValidationData, - UpgradeRestriction, ValidationCodeHash, + CandidateHash, CollatorId, CollatorSignature, Hash, HeadData, Id as ParaId, + PersistedValidationData, UpgradeRestriction, ValidationCodeHash, }; use std::{collections::HashMap, sync::Arc}; @@ -708,6 +673,11 @@ impl Fragment { &self.candidate } + /// Get a cheap ref-counted copy of the underlying prospective candidate. + pub fn candidate_clone(&self) -> Arc { + self.candidate.clone() + } + /// Modifications to constraints based on the outputs of the candidate. pub fn constraint_modifications(&self) -> &ConstraintModifications { &self.modifications @@ -797,6 +767,55 @@ fn validate_against_constraints( .map_err(FragmentValidityError::OutputsInvalid) } +/// Trait for a hypothetical or concrete candidate, as needed when assessing the validity of a +/// potential candidate. +pub trait HypotheticalOrConcreteCandidate { + /// Return a reference to the candidate commitments, if present. + fn commitments(&self) -> Option<&CandidateCommitments>; + /// Return a reference to the persisted validation data, if present. + fn persisted_validation_data(&self) -> Option<&PersistedValidationData>; + /// Return a reference to the validation code hash, if present. + fn validation_code_hash(&self) -> Option<&ValidationCodeHash>; + /// Return the parent head hash. + fn parent_head_data_hash(&self) -> Hash; + /// Return the output head hash, if present. + fn output_head_data_hash(&self) -> Option; + /// Return the relay parent hash. + fn relay_parent(&self) -> Hash; + /// Return the candidate hash. + fn candidate_hash(&self) -> CandidateHash; +} + +impl HypotheticalOrConcreteCandidate for HypotheticalCandidate { + fn commitments(&self) -> Option<&CandidateCommitments> { + self.commitments() + } + + fn persisted_validation_data(&self) -> Option<&PersistedValidationData> { + self.persisted_validation_data() + } + + fn validation_code_hash(&self) -> Option<&ValidationCodeHash> { + self.validation_code_hash() + } + + fn parent_head_data_hash(&self) -> Hash { + self.parent_head_data_hash() + } + + fn output_head_data_hash(&self) -> Option { + self.output_head_data_hash() + } + + fn relay_parent(&self) -> Hash { + self.relay_parent() + } + + fn candidate_hash(&self) -> CandidateHash { + self.candidate_hash() + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/prdoc/pr_4937.prdoc b/prdoc/pr_4937.prdoc new file mode 100644 index 000000000000..37b7bc3dda59 --- /dev/null +++ b/prdoc/pr_4937.prdoc @@ -0,0 +1,21 @@ +title: "prospective-parachains rework: take II" + +doc: + - audience: Node Dev + description: | + Add back support for backing parachain forks. Once a candidate reaches the backing quorum, + validators use a shared way of picking the winning fork to back on-chain. This was done in + order to increase the likelihood that all backers will vote on the winning fork. + The functionality of backing unconnected candidates introduced by the previous rework is preserved. + +crates: + - name: polkadot-node-core-prospective-parachains + bump: minor + - name: polkadot-node-subsystem-types + bump: minor + - name: polkadot-node-subsystem-util + bump: minor + - name: polkadot-node-core-provisioner + bump: none + - name: polkadot-statement-distribution + bump: none