diff --git a/bin/light-base/src/sync_service/parachain.rs b/bin/light-base/src/sync_service/parachain.rs index 85b3800e03..8265c9928a 100644 --- a/bin/light-base/src/sync_service/parachain.rs +++ b/bin/light-base/src/sync_service/parachain.rs @@ -482,7 +482,7 @@ pub(super) async fn start_parachain( let decoded_header_hash = decoded.header.hash(); sync_sources.add_known_block(local_id, decoded.header.number, decoded_header_hash); if decoded.is_best { - sync_sources.set_best_block(local_id, decoded.header.number, decoded_header_hash); + sync_sources.add_known_block_and_set_best(local_id, decoded.header.number, decoded_header_hash); } }, _ => { diff --git a/src/sync/all.rs b/src/sync/all.rs index 54d7fc8a8e..b6fa266ad0 100644 --- a/src/sync/all.rs +++ b/src/sync/all.rs @@ -2218,6 +2218,7 @@ impl Shared { max_disjoint_headers: self.max_disjoint_headers, max_requests_per_block: self.max_requests_per_block, full: false, + banned_blocks: iter::empty(), // TODO: not implemented, should be passed by config after the optimistic sync supports banned blocks too }); debug_assert!(self diff --git a/src/sync/all_forks.rs b/src/sync/all_forks.rs index defbce981c..0db2e9f16d 100644 --- a/src/sync/all_forks.rs +++ b/src/sync/all_forks.rs @@ -102,7 +102,7 @@ pub use pending_blocks::{RequestId, RequestParams, SourceId}; /// Configuration for the [`AllForksSync`]. #[derive(Debug)] -pub struct Config { +pub struct Config { /// Information about the latest finalized block and its ancestors. pub chain_information: chain_information::ValidChainInformation, @@ -150,6 +150,13 @@ pub struct Config { /// If true, the block bodies and storage are also synchronized. pub full: bool, + + /// List of block hashes that are known to be bad and shouldn't be downloaded or verified. + /// + /// > **Note**: This list is typically filled with a list of blocks found in the chain + /// > specification. It is part of the "trusted setup" of the node, in other words + /// > the information that is passed by the user and blindly assumed to be true. + pub banned_blocks: TBannedBlocksIter, } pub struct AllForksSync { @@ -171,6 +178,9 @@ struct Inner { /// These justifications came with a block header that has been successfully verified in the /// past. pending_justifications_verify: vec::IntoIter<([u8; 4], Vec)>, + + /// Same value as [`Config::banned_blocks`]. + banned_blocks: hashbrown::HashSet<[u8; 32], fnv::FnvBuildHasher>, } struct PendingBlock { @@ -186,7 +196,7 @@ struct Block { impl AllForksSync { /// Initializes a new [`AllForksSync`]. - pub fn new(config: Config) -> Self { + pub fn new(config: Config>) -> Self { let finalized_block_height = config .chain_information .as_ref() @@ -207,9 +217,9 @@ impl AllForksSync { max_requests_per_block: config.max_requests_per_block, sources_capacity: config.sources_capacity, verify_bodies: config.full, - banned_blocks: Vec::new(), // TODO: }), pending_justifications_verify: Vec::new().into_iter(), + banned_blocks: config.banned_blocks.collect(), }, } } @@ -296,7 +306,7 @@ impl AllForksSync { let is_in_disjoints_list = self .inner .blocks - .contains_block(best_block_number, &best_block_hash); + .contains_unverified_block(best_block_number, &best_block_hash); debug_assert!(!(!needs_verification && is_in_disjoints_list)); if needs_verification && !is_in_disjoints_list { @@ -310,6 +320,12 @@ impl AllForksSync { justifications: Vec::new(), }, ); + + if self.inner.banned_blocks.contains(&best_block_hash) { + self.inner + .blocks + .mark_unverified_block_as_bad(best_block_number, &best_block_hash); + } } source_id @@ -604,7 +620,7 @@ impl AllForksSync { // Assume that the source doesn't know this block, as it is apparently unable to // serve it anyway. This avoids sending the same request to the same source over and // over again. - self.inner.blocks.remove_known_block( + self.inner.blocks.remove_known_block_of_source( source_id, requested_block_height, &requested_block_hash, @@ -749,19 +765,23 @@ impl AllForksSync { // No matter what is done below, start by updating the view the state machine maintains // for this source. if known_to_be_source_best { - self.inner - .blocks - .set_best_block(source_id, header.number, *header_hash); + self.inner.blocks.add_known_block_to_source_and_set_best( + source_id, + header.number, + *header_hash, + ); } else { self.inner .blocks - .add_known_block(source_id, header.number, *header_hash); + .add_known_block_to_source(source_id, header.number, *header_hash); } // Source also knows the parent of the announced block. - self.inner - .blocks - .add_known_block(source_id, header.number - 1, *header.parent_hash); + self.inner.blocks.add_known_block_to_source( + source_id, + header.number - 1, + *header.parent_hash, + ); // It is assumed that all sources will eventually agree on the same finalized chain. If // the block number is lower or equal than the locally-finalized block number, it is @@ -783,7 +803,11 @@ impl AllForksSync { // At this point, we have excluded blocks that are already part of the chain or too old. // We insert the block in the list of unverified blocks so as to treat all blocks the // same. - if !self.inner.blocks.contains_block(header.number, header_hash) { + if !self + .inner + .blocks + .contains_unverified_block(header.number, header_hash) + { self.inner.blocks.insert_unverified_block( header.number, *header_hash, @@ -804,15 +828,36 @@ impl AllForksSync { .collect::>(), }, ); + + if self.inner.banned_blocks.contains(header_hash) { + self.inner + .blocks + .mark_unverified_block_as_bad(header.number, header_hash); + } + + // If there are too many blocks stored in the blocks list, remove unnecessary ones. + // Not doing this could lead to an explosion of the size of the collections. + // TODO: removing blocks should only be done explicitly through an API endpoint, because we want to store user datas in unverified blocks too; see https://github.com/paritytech/smoldot/issues/1572 + while self.inner.blocks.num_unverified_blocks() >= 100 { + // TODO: arbitrary constant + let (height, hash) = match self.inner.blocks.unnecessary_unverified_blocks().next() + { + Some((n, h)) => (n, *h), + None => break, + }; + + self.inner.blocks.remove_sources_known_block(height, &hash); + self.inner.blocks.remove_unverified_block(height, &hash); + } } else { if body.is_some() { - self.inner.blocks.set_block_header_body_known( + self.inner.blocks.set_unverified_block_header_body_known( header.number, header_hash, *header.parent_hash, ); } else { - self.inner.blocks.set_block_header_known( + self.inner.blocks.set_unverified_block_header_known( header.number, header_hash, *header.parent_hash, @@ -822,7 +867,7 @@ impl AllForksSync { let block_user_data = self .inner .blocks - .block_user_data_mut(header.number, header_hash); + .unverified_block_user_data_mut(header.number, header_hash); if block_user_data.header.is_none() { block_user_data.header = Some(header.clone().into()); // TODO: copying bytes :-/ } @@ -1054,7 +1099,7 @@ impl HeaderVerify { .parent .inner .blocks - .block_user_data( + .unverified_block_user_data( self.block_to_verify.block_number, &self.block_to_verify.block_hash, ) @@ -1096,13 +1141,17 @@ impl HeaderVerify { // Remove the verified block from `pending_blocks`. let justifications = if result.is_ok() { - let outcome = self.parent.inner.blocks.remove( + self.parent.inner.blocks.remove_sources_known_block( + self.block_to_verify.block_number, + &self.block_to_verify.block_hash, + ); + let outcome = self.parent.inner.blocks.remove_unverified_block( self.block_to_verify.block_number, &self.block_to_verify.block_hash, ); outcome.justifications } else { - self.parent.inner.blocks.set_block_bad( + self.parent.inner.blocks.mark_unverified_block_as_bad( self.block_to_verify.block_number, &self.block_to_verify.block_hash, ); diff --git a/src/sync/all_forks/disjoint.rs b/src/sync/all_forks/disjoint.rs index b977966bf9..d80376a461 100644 --- a/src/sync/all_forks/disjoint.rs +++ b/src/sync/all_forks/disjoint.rs @@ -219,6 +219,8 @@ impl DisjointBlocks { /// # Panic /// /// Panics if the block with the given height and hash hasn't been inserted before. + /// Panics if the parent hash of that block was already known, and is different from the one + /// passed as parameter. /// #[track_caller] pub fn set_parent_hash(&mut self, height: u64, hash: &[u8; 32], parent_hash: [u8; 32]) { @@ -233,7 +235,7 @@ impl DisjointBlocks { let block = self.blocks.get_mut(&(height, *hash)).unwrap(); match &mut block.parent_hash { - &mut Some(ph) => debug_assert_eq!(ph, parent_hash), + &mut Some(ph) => assert_eq!(ph, parent_hash), ph @ &mut None => *ph = Some(parent_hash), } @@ -349,6 +351,29 @@ impl DisjointBlocks { (None, None) => None, }) } + + /// Returns whether a block is marked as bad. + /// + /// Returns `None` if the block isn't known. + pub fn is_bad(&self, height: u64, hash: &[u8; 32]) -> Option { + Some(self.blocks.get(&(height, *hash))?.bad) + } + + /// Returns whether the parent of a block is bad. + /// + /// Returns `None` if either the block or its parent isn't known. + pub fn is_parent_bad(&self, height: u64, hash: &[u8; 32]) -> Option { + let parent_hash = self.blocks.get(&(height, *hash))?.parent_hash?; + let parent_height = match height.checked_sub(1) { + Some(h) => h, + None => return Some(false), // Parent is known and isn't present in the data structure. + }; + Some( + self.blocks + .get(&(parent_height, parent_hash)) + .map_or(false, |parent| parent.bad), + ) + } } impl fmt::Debug for DisjointBlocks { diff --git a/src/sync/all_forks/pending_blocks.rs b/src/sync/all_forks/pending_blocks.rs index 181d412b85..971ab69df8 100644 --- a/src/sync/all_forks/pending_blocks.rs +++ b/src/sync/all_forks/pending_blocks.rs @@ -42,18 +42,21 @@ //! - A list of non-finalized blocks known by this source. //! - An opaque user data, of type `TSrc`. //! -//! # Blocks +//! # Unverified blocks //! -//! The [`PendingBlocks`] collection stores a list of pending blocks. +//! The [`PendingBlocks`] collection also stores a list of unverified blocks. //! -//! Blocks are expected to be added to this collection whenever we hear about them from a source -//! of blocks (such as a peer) and that it is not possible to verify them immediately (because -//! their parent isn't known). +//! Note that unverified blocks are added/removed completely separately from blocks known by +//! sources. //! -//! Blocks can be added by calling [`PendingBlocks::insert_unverified_block`] and remove by -//! calling [`PendingBlocks::remove`]. +//! Unverified blocks are expected to be added to this collection whenever this node hears about them +//! from a source of blocks (such as a peer) and that it is not possible to verify them +//! immediately (because their parent isn't known). //! -//! Each block stored in this collection has the following properties associated to it: +//! Blocks can be added by calling [`PendingBlocks::insert_unverified_block`] and removed by +//! calling [`PendingBlocks::remove_unverified_block`]. +//! +//! Each unverified block stored in this collection has the following properties associated to it: //! //! - A height. //! - A hash. @@ -127,14 +130,6 @@ pub struct Config { /// /// The higher the value, the more bandwidth is potentially wasted. pub max_requests_per_block: NonZeroU32, - - /// List of block hashes that are known to be bad and shouldn't be downloaded or verified. - /// - /// > **Note**: This list is typically filled with a list of blocks found in the chain - /// > specification. It is part of the "trusted setup" of the node, in other words - /// > the information that is passed by the user and blindly assumed to be true. - // TODO: unused - pub banned_blocks: Vec<[u8; 64]>, } /// State of a block in the data structure. @@ -343,7 +338,7 @@ impl PendingBlocks { /// /// Panics if the [`SourceId`] is out of range. /// - pub fn add_known_block(&mut self, source_id: SourceId, height: u64, hash: [u8; 32]) { + pub fn add_known_block_to_source(&mut self, source_id: SourceId, height: u64, hash: [u8; 32]) { self.sources.add_known_block(source_id, height, hash); } @@ -358,12 +353,20 @@ impl PendingBlocks { /// /// Panics if the [`SourceId`] is out of range. /// - pub fn remove_known_block(&mut self, source_id: SourceId, height: u64, hash: &[u8; 32]) { + pub fn remove_known_block_of_source( + &mut self, + source_id: SourceId, + height: u64, + hash: &[u8; 32], + ) { self.sources .source_remove_known_block(source_id, height, hash); } - /// Sets the best block of this source. + /// Registers a new block that the source is aware of and sets it as its best block. + /// + /// If the block height is inferior or equal to the finalized block height, the block itself + /// isn't kept in memory but is still set as the source's best block. /// /// The block does not need to be known by the data structure. /// @@ -371,13 +374,19 @@ impl PendingBlocks { /// /// Panics if the [`SourceId`] is out of range. /// - pub fn set_best_block(&mut self, source_id: SourceId, height: u64, hash: [u8; 32]) { - self.sources.set_best_block(source_id, height, hash); + pub fn add_known_block_to_source_and_set_best( + &mut self, + source_id: SourceId, + height: u64, + hash: [u8; 32], + ) { + self.sources + .add_known_block_and_set_best(source_id, height, hash); } /// Returns the current best block of the given source. /// - /// This corresponds either the latest call to [`PendingBlocks::set_best_block`], + /// This corresponds either the latest call to [`PendingBlocks::add_known_block_to_source_and_set_best`], /// or to the parameter passed to [`PendingBlocks::add_source`]. /// /// # Panic @@ -420,9 +429,10 @@ impl PendingBlocks { self.sources.knows_non_finalized_block(height, hash) } - /// Returns true if [`PendingBlocks::add_known_block`] or [`PendingBlocks::set_best_block`] - /// has earlier been called on this source with this height and hash, or if the source was - /// originally created (using [`PendingBlocks::add_source`]) with this height and hash. + /// Returns true if [`PendingBlocks::add_known_block_to_source`] or + /// [`PendingBlocks::add_known_block_to_source_and_set_best`] has earlier been called on this + /// source with this height and hash, or if the source was originally created (using + /// [`PendingBlocks::add_source`]) with this height and hash. /// /// # Panic /// @@ -444,8 +454,8 @@ impl PendingBlocks { /// Updates the height of the finalized block. /// - /// This removes from the collection, and will ignore in the future, all blocks whose height - /// is inferior or equal to this value. + /// This removes from the collection all blocks (both source-known and unverified) whose + /// height is inferior or equal to this value. /// /// # Panic /// @@ -464,6 +474,9 @@ impl PendingBlocks { /// Inserts an unverified block in the collection. /// /// Returns the previous user data associated to this block, if any. + /// + /// > **Note**: You should probably also call [`PendingBlocks::add_known_block_to_source`] or + /// > [`PendingBlocks::add_known_block_to_source_and_set_best`]. pub fn insert_unverified_block( &mut self, height: u64, @@ -488,7 +501,7 @@ impl PendingBlocks { } /// Returns `true` if the block with the given height and hash is in the collection. - pub fn contains_block(&self, height: u64, hash: &[u8; 32]) -> bool { + pub fn contains_unverified_block(&self, height: u64, hash: &[u8; 32]) -> bool { self.blocks.contains(height, hash) } @@ -498,7 +511,7 @@ impl PendingBlocks { /// /// Panics if the block wasn't present in the data structure. /// - pub fn block_user_data(&self, height: u64, hash: &[u8; 32]) -> &TBl { + pub fn unverified_block_user_data(&self, height: u64, hash: &[u8; 32]) -> &TBl { &self.blocks.user_data(height, hash).unwrap().user_data } @@ -508,7 +521,7 @@ impl PendingBlocks { /// /// Panics if the block wasn't present in the data structure. /// - pub fn block_user_data_mut(&mut self, height: u64, hash: &[u8; 32]) -> &mut TBl { + pub fn unverified_block_user_data_mut(&mut self, height: u64, hash: &[u8; 32]) -> &mut TBl { &mut self.blocks.user_data_mut(height, hash).unwrap().user_data } @@ -520,7 +533,12 @@ impl PendingBlocks { /// /// Panics if the block wasn't present in the data structure. /// - pub fn set_block_state(&mut self, height: u64, hash: &[u8; 32], state: UnverifiedBlockState) { + pub fn set_unverified_block_state( + &mut self, + height: u64, + hash: &[u8; 32], + state: UnverifiedBlockState, + ) { if let Some(parent_hash) = state.parent_hash() { self.blocks.set_parent_hash(height, hash, *parent_hash); } @@ -529,16 +547,26 @@ impl PendingBlocks { } /// Modifies the state of the given block. This is a convenience around - /// [`PendingBlocks::set_block_state`]. + /// [`PendingBlocks::set_unverified_block_state`]. /// /// If the current block's state implies that the header isn't known yet, updates it to a /// state where the header is known. /// + /// > **Note**: A user of this data structure is expected to manually add the parent block to + /// this data structure as well in case it is unverified. + /// /// # Panic /// /// Panics if the block wasn't present in the data structure. + /// Panics if the block's header was already known and the its parent hash doesn't match + /// the one passed as parameter. /// - pub fn set_block_header_known(&mut self, height: u64, hash: &[u8; 32], parent_hash: [u8; 32]) { + pub fn set_unverified_block_header_known( + &mut self, + height: u64, + hash: &[u8; 32], + parent_hash: [u8; 32], + ) { let curr = &mut self.blocks.user_data_mut(height, hash).unwrap().state; match curr { @@ -560,16 +588,21 @@ impl PendingBlocks { } /// Modifies the state of the given block. This is a convenience around - /// [`PendingBlocks::set_block_state`]. + /// [`PendingBlocks::set_unverified_block_state`]. /// /// If the current block's state implies that the header or body isn't known yet, updates it /// to a state where the header and body are known. /// + /// > **Note**: A user of this data structure is expected to manually add the parent block to + /// this data structure as well in case it is unverified. + /// /// # Panic /// /// Panics if the block wasn't present in the data structure. + /// Panics if the block's header was already known and the its parent hash doesn't match + /// the one passed as parameter. /// - pub fn set_block_header_body_known( + pub fn set_unverified_block_header_body_known( &mut self, height: u64, hash: &[u8; 32], @@ -595,7 +628,15 @@ impl PendingBlocks { self.blocks.set_parent_hash(height, hash, parent_hash); } - /// Removes the given block from the collection. + /// Removes the given block from the list of known blocks of all from the sources. + /// + /// This is equivalent to calling [`PendingBlocks::remove_known_block_of_source`] for each + /// source. + pub fn remove_sources_known_block(&mut self, height: u64, hash: &[u8; 32]) { + self.sources.remove_known_block(height, hash); + } + + /// Removes the given unverified block from the collection. /// /// > **Note**: Use this method after a block has been successfully verified, or in order to /// > remove uninteresting blocks if there are too many blocks in the collection. @@ -604,11 +645,11 @@ impl PendingBlocks { /// /// Panics if the block wasn't present in the data structure. /// - pub fn remove(&mut self, height: u64, hash: &[u8; 32]) -> TBl { + pub fn remove_unverified_block(&mut self, height: u64, hash: &[u8; 32]) -> TBl { self.blocks.remove(height, hash).user_data } - /// Marks the given block and all its known children as "bad". + /// Marks the given unverified block and all its known children as "bad". /// /// If a child of this block is later added to the collection, it is also automatically /// marked as bad. @@ -618,12 +659,12 @@ impl PendingBlocks { /// Panics if the block wasn't present in the data structure. /// #[track_caller] - pub fn set_block_bad(&mut self, height: u64, hash: &[u8; 32]) { + pub fn mark_unverified_block_as_bad(&mut self, height: u64, hash: &[u8; 32]) { self.blocks.set_block_bad(height, hash); } - /// Returns the number of blocks stored in the data structure. - pub fn num_blocks(&self) -> usize { + /// Returns the number of unverified blocks stored in the data structure. + pub fn num_unverified_blocks(&self) -> usize { self.blocks.len() } @@ -633,6 +674,10 @@ impl PendingBlocks { /// All the returned block are guaranteed to be in a "header known" state. If /// [`Config::verify_bodies`] if `true`, they they are also guaranteed to be in a "body known" /// state. + /// + /// > **Note**: The naming of this function assumes that all blocks that are referenced by + /// > this data structure but absent from this data structure are known by the + /// > API user. pub fn unverified_leaves(&'_ self) -> impl Iterator + '_ { self.blocks.good_tree_roots().filter(move |pending| { match self @@ -648,11 +693,79 @@ impl PendingBlocks { }) } + /// Returns an iterator to a list of unverified blocks in the data structure that aren't + /// necessary to keep in order to complete the chain. + /// + /// The returned blocks are ordered by increasing order of importance. In other words, the + /// earlier blocks are less useful. + /// + /// In details, this returns: + /// + /// - Blocks that have a bad parent and that aren't the best block of any given source. + /// - Blocks whose parent is in the data structure and that aren't the best block of any given + /// source. + /// - Blocks that are bad and that aren't the best block of any given source. + /// + /// It is guaranteed that, even if you always immediately remove all the blocks provided by + /// this iterator, the chain will eventually become fully synchronized (assuming that block + /// requests eventually succeed). + /// + /// > **Note**: You are encouraged to use this method to remove blocks in order to prevent the + /// > data structure from reaching unreasonable sizes. Please keep in mind, however, + /// > that removing blocks will lead to redownloading these blocks later. In other + /// > words, it is better to keep these blocks. + pub fn unnecessary_unverified_blocks( + &'_ self, + ) -> impl Iterator + '_ { + // TODO: this entire function is O(n) everywhere + + // List of blocks that have a bad parent. + // If a block has a bad parent, it is also bad itself, hence why we use `bad_blocks()`. + let bad_parent_iter = self + .blocks + .iter() + .filter(|(height, hash, _)| self.blocks.is_parent_bad(*height, *hash).unwrap_or(false)); + + // List of blocks whose parent is in the data structure. + let parent_known_iter = self.blocks.iter().filter(|(height, hash, _)| { + match ( + height.checked_sub(1), + self.blocks.parent_hash(*height, *hash), + ) { + (Some(n), Some(h)) => self.blocks.contains(n, h), + _ => false, + } + }); + + // List of blocks that are bad but don't have a bad parent. + // This is the same as `bad_parent_iter`, but the filter is reversed. + let bad_iter = self + .blocks + .iter() + .filter(|(height, hash, _)| self.blocks.is_bad(*height, *hash).unwrap()) + .filter(|(height, hash, _)| { + !self.blocks.is_parent_bad(*height, *hash).unwrap_or(false) + }); + + // Never return any block that is the best block of a source. + bad_parent_iter + .chain(parent_known_iter) + .chain(bad_iter) + .map(|(height, hash, _)| (height, hash)) + .filter(|(height, hash)| { + !self + .sources + .iter() + .any(|source_id| self.sources.best_block(source_id) == (*height, hash)) + }) + } + /// Inserts a new request in the data structure. /// /// > **Note**: The request doesn't necessarily have to match a request returned by /// > [`PendingBlocks::desired_requests`] or - /// > [`PendingBlocks::source_desired_requests`]. + /// > [`PendingBlocks::source_desired_requests`]. Any arbitrary blocks request can + /// > be added. /// /// # Panic /// @@ -696,6 +809,9 @@ impl PendingBlocks { /// /// Returns the parameters that were passed to [`PendingBlocks::add_request`]. /// + /// Note that this function does nothing else but remove the given request from the state + /// machine. Nothing in the state concerning sources or blocks is updated. + /// /// The next call to [`PendingBlocks::desired_requests`] might return the same request again. /// In order to avoid that, you are encouraged to update the state of the sources and blocks /// in the container with the outcome of the request. @@ -709,6 +825,7 @@ impl PendingBlocks { assert!(self.requests.contains(request_id.0)); let request = self.requests.remove(request_id.0); + // Update `requested_blocks`. let blocks_to_remove = self .requested_blocks .range( @@ -745,8 +862,9 @@ impl PendingBlocks { /// /// # Panic /// - /// Panics if the [`RequestId`] is out of range. + /// Panics if the [`RequestId`] is invalid. /// + #[track_caller] pub fn request_source(&self, request_id: RequestId) -> SourceId { self.requests.get(request_id.0).unwrap().source_id } @@ -754,8 +872,9 @@ impl PendingBlocks { /// Returns a list of requests that are considered obsolete and can be removed using /// [`PendingBlocks::finish_request`]. /// - /// A request becomes obsolete if the state of the request blocks changes in such a way that - /// they don't need to be requested anymore. The response to the request will be useless. + /// A request is considered obsolete if the state of the requested blocks changes in such a + /// way that they don't need to be requested anymore. The request wouldn't be returned by + /// [`PendingBlocks::desired_requests`]. /// /// > **Note**: It is in no way mandatory to actually call this function and cancel the /// > requests that are returned. @@ -769,20 +888,42 @@ impl PendingBlocks { .map(|(id, rq)| (RequestId(id), &rq.user_data)) } - /// Returns the details of a request to start towards a source. + /// Returns a list of requests that should be started in order to learn about the missing + /// unverified blocks. + /// + /// In details, the requests concern: + /// + /// - If [`Config::verify_bodies`] was `true`, downloading the body of blocks whose body is + /// unknown. + /// - Downloading headers of blocks whose state is [`UnverifiedBlockState::HeightHashKnown`]. + /// + /// Requests are ordered by increasing block height. In other words, the most important + /// requests are returned first. /// /// This method doesn't modify the state machine in any way. [`PendingBlocks::add_request`] - /// must be called in order for the request to actually be marked as started. + /// must be called in order for the request to actually be marked as started. Once a request + /// has been started with [`PendingBlocks::add_request`] it will no longer be returned by this + /// method. /// /// No request concerning the finalized block (as set using /// [`PendingBlocks::set_finalized_block_height`]) or below will ever be returned. + /// + /// > **Note**: The API user is encouraged to iterate over the requests until they find a + /// > request that is appropriate, then stop iterating and start said request. + /// + /// > **Note**: This state machine does in no way enforce a limit to the number of simultaneous + /// > requests per source, as this is out of scope of this module. However, there is + /// > limit to the number of simultaneous requests per block. See + /// > [`Config::max_requests_per_block`]. pub fn desired_requests(&'_ self) -> impl Iterator + '_ { self.desired_requests_inner(None) } - /// Returns the details of a request to start towards the source. + /// Returns a list of requests that should be started in order to learn about the missing + /// unverified blocks. /// - /// This method is similar to [`PendingBlocks::desired_requests`]. + /// This method is similar to [`PendingBlocks::desired_requests`], except that only requests + /// concerning the given source will be returned. /// /// # Panic /// @@ -811,7 +952,7 @@ impl PendingBlocks { &'_ self, force_source: Option, ) -> impl Iterator + '_ { - // TODO: request the best block of each source if necessary + // TODO: could provide more optimized requests by avoiding potentially overlapping requests (e.g. if blocks #4 and #5 are unknown, ask for block #5 with num_blocks=2), but this is complicated because peers aren't obligated to respond with the given number of blocks // List of blocks whose header is known but not its body. let unknown_body_iter = if self.verify_bodies { @@ -856,6 +997,7 @@ impl PendingBlocks { .chain(unknown_header_iter) .filter(move |(unknown_block_height, unknown_block_hash)| { // Cap by `max_requests_per_block`. + // TODO: O(n)? let num_existing_requests = self .blocks_requests .range( @@ -896,6 +1038,7 @@ impl PendingBlocks { .filter(move |source_id| { // Don't start any request towards this source if there's another request // for the same block from the same source. + // TODO: O(n)? !self .blocks_requests .range( diff --git a/src/sync/all_forks/sources.rs b/src/sync/all_forks/sources.rs index 49b370cc94..8183e0186c 100644 --- a/src/sync/all_forks/sources.rs +++ b/src/sync/all_forks/sources.rs @@ -92,6 +92,11 @@ impl AllForksSources { self.sources.is_empty() } + /// Iterates over all sources. + pub fn iter(&'_ self) -> impl Iterator + '_ { + self.sources.keys().copied() + } + /// Returns the number of sources in the data structure. pub fn len(&self) -> usize { self.sources.len() @@ -279,14 +284,22 @@ impl AllForksSources { debug_assert_eq!(_was_in1, _was_in2); } - /// Sets the best block of this source. + /// Registers a new block that the source is aware of and sets it as its best block. + /// + /// If the block height is inferior or equal to the finalized block height, the block itself + /// isn't kept in memory but is still set as the source's best block. /// /// # Panic /// /// Panics if the [`SourceId`] is out of range. /// #[track_caller] - pub fn set_best_block(&mut self, source_id: SourceId, height: u64, hash: [u8; 32]) { + pub fn add_known_block_and_set_best( + &mut self, + source_id: SourceId, + height: u64, + hash: [u8; 32], + ) { self.add_known_block(source_id, height, hash); let source = self.sources.get_mut(&source_id).unwrap(); @@ -296,8 +309,9 @@ impl AllForksSources { /// Returns the current best block of the given source. /// - /// This corresponds either the latest call to [`AllForksSources::set_best_block`], - /// or to the parameter passed to [`AllForksSources::add_source`]. + /// This corresponds either to the latest call to + /// [`AllForksSources::add_known_block_and_set_best`], or to the parameter passed to + /// [`AllForksSources::add_source`]. /// /// # Panic /// @@ -331,9 +345,10 @@ impl AllForksSources { .map(|(_, _, id)| *id) } - /// Returns true if [`AllForksSources::add_known_block`] or [`AllForksSources::set_best_block`] - /// has earlier been called on this source with this height and hash, or if the source was - /// originally created (using [`AllForksSources::add_source`]) with this height and hash. + /// Returns true if [`AllForksSources::add_known_block`] or + /// [`AllForksSources::add_known_block_and_set_best`] has earlier been called on this source + /// with this height and hash, or if the source was originally created (using + /// [`AllForksSources::add_source`]) with this height and hash. /// /// # Panic /// @@ -400,7 +415,7 @@ mod tests { assert_eq!(sources.num_blocks(), 1); assert!(sources.source_knows_non_finalized_block(source1, 12, &[1; 32])); - sources.set_best_block(source1, 13, [2; 32]); + sources.add_known_block_and_set_best(source1, 13, [2; 32]); assert_eq!(sources.num_blocks(), 2); assert!(sources.source_knows_non_finalized_block(source1, 12, &[1; 32])); assert!(sources.source_knows_non_finalized_block(source1, 13, &[2; 32]));