Improvements to all_forks syncing

bin/light-base/src/sync_service/parachain.rs

@@ -482,7 +482,7 @@ pub(super) async fn start_parachain<TPlat: Platform>(
                             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);
                             }
                         },
                         _ => {

src/sync/all.rs

@@ -2218,6 +2218,7 @@ impl<TRq> Shared<TRq> {
             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

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<TBannedBlocksIter> {
     /// 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<TBl, TRq, TSrc> {
@@ -171,6 +178,9 @@ struct Inner<TRq, TSrc> {
     /// These justifications came with a block header that has been successfully verified in the
     /// past.
     pending_justifications_verify: vec::IntoIter<([u8; 4], Vec<u8>)>,
+
+    /// Same value as [`Config::banned_blocks`].
+    banned_blocks: hashbrown::HashSet<[u8; 32], fnv::FnvBuildHasher>,
 }
 
 struct PendingBlock {
@@ -186,7 +196,7 @@ struct Block<TBl> {
 
 impl<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
     /// Initializes a new [`AllForksSync`].
-    pub fn new(config: Config) -> Self {
+    pub fn new(config: Config<impl Iterator<Item = [u8; 32]>>) -> Self {
         let finalized_block_height = config
             .chain_information
             .as_ref()
@@ -207,9 +217,9 @@ impl<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
                     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<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
         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<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
                     justifications: Vec::new(),
                 },
             );
+
+            if self.inner.banned_blocks.contains(&best_block_hash) {
+                self.inner
+                    .blocks
+                    .set_unverified_block_bad(best_block_number, &best_block_hash);
+            }
         }
 
         source_id
@@ -604,7 +620,7 @@ impl<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
             // 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_source_known_block(
                 source_id,
                 requested_block_height,
                 &requested_block_hash,
@@ -751,19 +767,21 @@ impl<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
         // 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_source_known_block_and_set_best(
+                source_id,
+                header.number,
+                *header_hash,
+            );
         } else {
             self.inner
                 .blocks
-                .add_known_block(source_id, header.number, *header_hash);
+                .add_source_known_block(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);
+            .add_source_known_block(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
@@ -785,7 +803,11 @@ impl<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
         // 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,
@@ -806,15 +828,36 @@ impl<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
                         .collect::<Vec<_>>(),
                 },
             );
+
+            if self.inner.banned_blocks.contains(header_hash) {
+                self.inner
+                    .blocks
+                    .set_unverified_block_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,
@@ -824,7 +867,7 @@ impl<TBl, TRq, TSrc> AllForksSync<TBl, TRq, TSrc> {
             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 :-/
             }
@@ -1056,7 +1099,7 @@ impl<TBl, TRq, TSrc> HeaderVerify<TBl, TRq, TSrc> {
             .parent
             .inner
             .blocks
-            .block_user_data(
+            .unverified_block_user_data(
                 self.block_to_verify.block_number,
                 &self.block_to_verify.block_hash,
             )
@@ -1096,13 +1139,17 @@ impl<TBl, TRq, TSrc> HeaderVerify<TBl, TRq, TSrc> {
 
         // 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.set_unverified_block_bad(
                 self.block_to_verify.block_number,
                 &self.block_to_verify.block_hash,
             );

src/sync/all_forks/disjoint.rs

@@ -219,6 +219,8 @@ impl<TBl> DisjointBlocks<TBl> {
     /// # 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<TBl> DisjointBlocks<TBl> {
         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<TBl> DisjointBlocks<TBl> {
             (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<bool> {
+        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<bool> {
+        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<TBl: fmt::Debug> fmt::Debug for DisjointBlocks<TBl> {