Fix wrong order of parachain blocks at initialization (#2965)

When we subscribe to parachain blocks, the subscription confirmation contains the list of all blocks that already exist. This list of blocks needs to be ordered from parent to child, and the field is actually named `non_finalized_blocks_ancestry_order` ("ancestry order") in order for everyone to get the memo. Unfortunately, the author of the parachains code (me) didn't get the memo, and the parachain blocks aren't properly ordered. This leads to a panic in `runtime_service.rs` because it assumes that the ordering is correct. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
paritytech · Nov 4, 2022 · 77fc2e5 · 77fc2e5
1 parent f2b5ca3
commit 77fc2e5
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 44 deletions.
diff --git a/bin/light-base/src/sync_service/parachain.rs b/bin/light-base/src/sync_service/parachain.rs
@@ -377,15 +377,14 @@ impl<TPlat: Platform> ParachainBackgroundTask<TPlat> {
                         finalized_block_runtime: None,
                         non_finalized_blocks_ancestry_order: {
                             let mut list =
-                                HashMap::<_, super::BlockNotification, _>::with_capacity_and_hasher(
+                                Vec::<([u8; 32], super::BlockNotification)>::with_capacity(
                                     runtime_subscription
                                         .async_tree
                                         .num_input_non_finalized_blocks(),
-                                    fnv::FnvBuildHasher::default(),
                                 );
 
                             for relay_block in
-                                runtime_subscription.async_tree.input_iter_unordered()
+                                runtime_subscription.async_tree.input_iter_ancestry_order()
                             {
                                 let parablock = match relay_block.async_op_user_data {
                                     Some(b) => b.as_ref().unwrap(),
@@ -395,52 +394,58 @@ impl<TPlat: Platform> ParachainBackgroundTask<TPlat> {
                                 let parablock_hash =
                                     header::hash_from_scale_encoded_header(&parablock);
 
-                                match list.entry(parablock_hash) {
-                                    hashbrown::hash_map::Entry::Occupied(entry) => {
-                                        if relay_block.is_output_best {
-                                            entry.into_mut().is_new_best = true;
-                                        }
+                                if let Some((_, entry)) =
+                                    list.iter_mut().find(|(h, _)| *h == parablock_hash)
+                                {
+                                    if relay_block.is_output_best {
+                                        entry.is_new_best = true;
                                     }
-                                    hashbrown::hash_map::Entry::Vacant(entry) => {
-                                        let parent_hash = runtime_subscription
-                                            .async_tree
-                                            .ancestors(relay_block.id)
-                                            .find_map(|idx| {
-                                                let hash = header::hash_from_scale_encoded_header(
-                                                    &runtime_subscription
-                                                        .async_tree
-                                                        .block_async_user_data(idx)
-                                                        .unwrap()
-                                                        .as_ref()
-                                                        .unwrap(),
+                                } else {
+                                    let parent_hash = runtime_subscription
+                                        .async_tree
+                                        .ancestors(relay_block.id)
+                                        .find_map(|idx| {
+                                            let hash = header::hash_from_scale_encoded_header(
+                                                &runtime_subscription
+                                                    .async_tree
+                                                    .block_async_user_data(idx)
+                                                    .unwrap()
+                                                    .as_ref()
+                                                    .unwrap(),
+                                            );
+                                            if hash != parablock_hash {
+                                                Some(hash)
+                                            } else {
+                                                None
+                                            }
+                                        })
+                                        .or_else(|| {
+                                            let finalized_parahash =
+                                                header::hash_from_scale_encoded_header(
+                                                    &finalized_parahead,
                                                 );
-                                                if hash != parablock_hash {
-                                                    Some(hash)
-                                                } else {
-                                                    None
-                                                }
-                                            })
-                                            .or_else(|| {
-                                                let finalized_parahash =
-                                                    header::hash_from_scale_encoded_header(
-                                                        &finalized_parahead,
-                                                    );
-                                                if finalized_parahash != parablock_hash {
-                                                    Some(finalized_parahash)
-                                                } else {
-                                                    None
-                                                }
-                                            });
-
-                                        // `parent_hash` is `None` if the parablock is
-                                        // the same as the finalized parablock.
-                                        if let Some(parent_hash) = parent_hash {
-                                            entry.insert(super::BlockNotification {
+                                            if finalized_parahash != parablock_hash {
+                                                Some(finalized_parahash)
+                                            } else {
+                                                None
+                                            }
+                                        });
+
+                                    // `parent_hash` is `None` if the parablock is
+                                    // the same as the finalized parablock.
+                                    if let Some(parent_hash) = parent_hash {
+                                        debug_assert_eq!(
+                                            list.iter().filter(|(h, _)| *h == parent_hash).count(),
+                                            1
+                                        );
+                                        list.push((
+                                            parablock_hash,
+                                            super::BlockNotification {
                                                 is_new_best: relay_block.is_output_best,
                                                 scale_encoded_header: parablock.clone(),
                                                 parent_hash,
-                                            });
-                                        }
+                                            },
+                                        ));
                                     }
                                 }
                             }

diff --git a/bin/wasm-node/CHANGELOG.md b/bin/wasm-node/CHANGELOG.md
@@ -6,6 +6,7 @@
 
 - On NodeJS, the usage of `hrtime` has been replaced with `performance.now()`. While this doesn't change anything for NodeJS users, Deno users that were importing smoldot through the <https://esm.sh> website will no longer get an error due to Deno's compatibility layer not supporting `hrtime`. As a reminder, smoldot is also published on the Deno/x registry and using <https://esm.sh> is unnecessary. ([#2964](https://github.com/paritytech/smoldot/pull/2964))
 - Fix the `ext_crypto_ecdsa_verify_version_1` and `ext_crypto_ecdsa_verify_prehashed_version_1` host functions mixing their parameters and thus always failing. ([#2955](https://github.com/paritytech/smoldot/pull/2955))
+- Fix an occasional panic in `runtime_service.rs` when adding a parachain. ([#2965](https://github.com/paritytech/smoldot/pull/2965))
 
 ## 0.7.5 - 2022-10-31