Fix wrong Grandpa warp sync block being reported (#3044)

Fix #3015 We now properly report the block of the fragments which we've downloaded, as opposed to the block whose chain information we have.
paritytech · Nov 28, 2022 · ab489e4 · ab489e4
1 parent 5387ad0
commit ab489e4
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 25 deletions.
diff --git a/bin/light-base/src/sync_service/standalone.rs b/bin/light-base/src/sync_service/standalone.rs
@@ -316,17 +316,21 @@ pub(super) async fn start_standalone_chain<TPlat: Platform>(
             () = &mut task.warp_sync_taking_long_time_warning => {
                 match task.sync.status() {
                     all::Status::Sync => {},
-                    all::Status::WarpSyncFragments { source: None } => {
-                        log::warn!(target: &task.log_target, "GrandPa warp sync idle");
+                    all::Status::WarpSyncFragments { source: None, finalized_block_hash, finalized_block_number } => {
+                        log::warn!(
+                            target: &task.log_target,
+                            "GrandPa warp sync idle at block #{} (0x{})",
+                            finalized_block_number,
+                            HashDisplay(&finalized_block_hash),
+                        );
                     }
-                    all::Status::WarpSyncFragments { source: Some((_, (peer_id, _))) } |
-                    all::Status::WarpSyncChainInformation { source: (_, (peer_id, _)) } => {
-                        let finalized_block = task.sync.finalized_block_header();
+                    all::Status::WarpSyncFragments { source: Some((_, (peer_id, _))), finalized_block_hash, finalized_block_number } |
+                    all::Status::WarpSyncChainInformation { source: (_, (peer_id, _)), finalized_block_hash, finalized_block_number } => {
                         log::warn!(
                             target: &task.log_target,
                             "GrandPa warp sync in progress. Block: #{} (0x{}). Peer attempt: {}.",
-                            finalized_block.number,
-                            HashDisplay(&finalized_block.hash(task.sync.block_number_bytes())),
+                            finalized_block_number,
+                            HashDisplay(&finalized_block_hash),
                             peer_id
                         );
                     },

diff --git a/bin/wasm-node/CHANGELOG.md b/bin/wasm-node/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 ### Fixed
 
+- Fix wrong block being reported in the logs when printing the status of the Grandpa warp syncing, giving the impression that the warp syncing wasn't advancing. ([#3044](https://github.com/paritytech/smoldot/pull/3044))
 - Fix panic introduced in v0.7.8 when verifying a Merkle proof of a trie related to a chain whose `state_version` is equal to `1`. ([#3043](https://github.com/paritytech/smoldot/pull/3043))
 
 ## 0.7.8 - 2022-11-23

diff --git a/src/sync/all.rs b/src/sync/all.rs
@@ -132,12 +132,28 @@ pub enum Status<'a, TSrc> {
     WarpSyncFragments {
         /// Source from which the fragments are currently being downloaded, if any.
         source: Option<(SourceId, &'a TSrc)>,
+        /// Hash of the highest block that is proven to be finalized.
+        ///
+        /// This isn't necessarily the same block as returned by
+        /// [`AllSync::as_chain_information`], as this function first has to download extra
+        /// information compared to just the finalized block.
+        finalized_block_hash: [u8; 32],
+        /// Height of the block indicated by [`Status::ChainInformation::finalized_block_hash`].
+        finalized_block_number: u64,
     },
     /// Warp syncing algorithm has reached the head of the finalized chain and is downloading and
     /// building the chain information.
     WarpSyncChainInformation {
         /// Source from which the chain information is being downloaded.
         source: (SourceId, &'a TSrc),
+        /// Hash of the highest block that is proven to be finalized.
+        ///
+        /// This isn't necessarily the same block as returned by
+        /// [`AllSync::as_chain_information`], as this function first has to download extra
+        /// information compared to just the finalized block.
+        finalized_block_hash: [u8; 32],
+        /// Height of the block indicated by [`Status::ChainInformation::finalized_block_hash`].
+        finalized_block_number: u64,
     },
 }
 
@@ -233,18 +249,32 @@ impl<TRq, TSrc, TBl> AllSync<TRq, TSrc, TBl> {
         match &self.inner {
             AllSyncInner::AllForks(_) => Status::Sync,
             AllSyncInner::GrandpaWarpSync { inner: sync } => match sync.status() {
-                warp_sync::Status::Fragments { source: None } => {
-                    Status::WarpSyncFragments { source: None }
-                }
+                warp_sync::Status::Fragments {
+                    source: None,
+                    finalized_block_hash,
+                    finalized_block_number,
+                } => Status::WarpSyncFragments {
+                    source: None,
+                    finalized_block_hash,
+                    finalized_block_number,
+                },
                 warp_sync::Status::Fragments {
                     source: Some((_, user_data)),
+                    finalized_block_hash,
+                    finalized_block_number,
                 } => Status::WarpSyncFragments {
                     source: Some((user_data.outer_source_id, &user_data.user_data)),
+                    finalized_block_hash,
+                    finalized_block_number,
                 },
                 warp_sync::Status::ChainInformation {
                     source: (_, user_data),
+                    finalized_block_hash,
+                    finalized_block_number,
                 } => Status::WarpSyncChainInformation {
                     source: (user_data.outer_source_id, &user_data.user_data),
+                    finalized_block_hash,
+                    finalized_block_number,
                 },
             },
             AllSyncInner::Optimistic { .. } => Status::Sync, // TODO: right now we don't differentiate between AllForks and Optimistic, as they're kind of similar anyway

diff --git a/src/sync/warp_sync.rs b/src/sync/warp_sync.rs
@@ -346,12 +346,28 @@ pub enum Status<'a, TSrc> {
     Fragments {
         /// Source from which the fragments are currently being downloaded, if any.
         source: Option<(SourceId, &'a TSrc)>,
+        /// Hash of the highest block that is proven to be finalized.
+        ///
+        /// This isn't necessarily the same block as returned by
+        /// [`InProgressWarpSync::as_chain_information`], as this function first has to download
+        /// extra information compared to just the finalized block.
+        finalized_block_hash: [u8; 32],
+        /// Height of the block indicated by [`Status::ChainInformation::finalized_block_hash`].
+        finalized_block_number: u64,
     },
     /// Warp syncing algorithm has reached the head of the finalized chain and is downloading and
     /// building the chain information.
     ChainInformation {
         /// Source from which the chain information is being downloaded.
         source: (SourceId, &'a TSrc),
+        /// Hash of the highest block that is proven to be finalized.
+        ///
+        /// This isn't necessarily the same block as returned by
+        /// [`InProgressWarpSync::as_chain_information`], as this function first has to download
+        /// extra information compared to just the finalized block.
+        finalized_block_hash: [u8; 32],
+        /// Height of the block indicated by [`Status::ChainInformation::finalized_block_hash`].
+        finalized_block_number: u64,
     },
 }
 
@@ -377,21 +393,22 @@ impl<TSrc, TRq> InProgressWarpSync<TSrc, TRq> {
             Phase::DownloadFragments {
                 ref previous_verifier_values,
             } => {
-                let start_block_hash = match previous_verifier_values.as_ref() {
-                    Some((header, _)) => header.hash(self.block_number_bytes),
-                    None => self
-                        .start_chain_information
-                        .as_ref()
-                        .finalized_block_header
-                        .hash(self.block_number_bytes),
+                let (finalized_block_hash, finalized_block_number) = match previous_verifier_values
+                    .as_ref()
+                {
+                    Some((header, _)) => (header.hash(self.block_number_bytes), header.number),
+                    None => {
+                        let header = self.start_chain_information.as_ref().finalized_block_header;
+                        (header.hash(self.block_number_bytes), header.number)
+                    }
                 };
 
                 let source_id =
                     self.in_progress_requests
                         .iter()
                         .find_map(|(_, (source_id, _, rq))| match rq {
                             RequestDetail::WarpSyncRequest { block_hash }
-                                if *block_hash == start_block_hash =>
+                                if *block_hash == finalized_block_hash =>
                             {
                                 Some(*source_id)
                             }
@@ -400,28 +417,50 @@ impl<TSrc, TRq> InProgressWarpSync<TSrc, TRq> {
 
                 Status::Fragments {
                     source: source_id.map(|id| (id, &self.sources[id.0].user_data)),
+                    finalized_block_hash,
+                    finalized_block_number,
                 }
             }
             Phase::PendingVerify {
-                downloaded_source, ..
-            } => Status::Fragments {
-                source: Some((
-                    downloaded_source,
-                    &self.sources[downloaded_source.0].user_data,
-                )),
-            },
+                downloaded_source,
+                ref previous_verifier_values,
+                ..
+            } => {
+                let (finalized_block_hash, finalized_block_number) = match previous_verifier_values
+                    .as_ref()
+                {
+                    Some((header, _)) => (header.hash(self.block_number_bytes), header.number),
+                    None => {
+                        let header = self.start_chain_information.as_ref().finalized_block_header;
+                        (header.hash(self.block_number_bytes), header.number)
+                    }
+                };
+
+                Status::Fragments {
+                    source: Some((
+                        downloaded_source,
+                        &self.sources[downloaded_source.0].user_data,
+                    )),
+                    finalized_block_hash,
+                    finalized_block_number,
+                }
+            }
             Phase::RuntimeDownload {
                 warp_sync_source_id,
+                ref header,
                 ..
             }
             | Phase::ChainInformationDownload {
                 warp_sync_source_id,
+                ref header,
                 ..
             } => Status::ChainInformation {
                 source: (
                     warp_sync_source_id,
                     &self.sources[warp_sync_source_id.0].user_data,
                 ),
+                finalized_block_hash: header.hash(self.block_number_bytes),
+                finalized_block_number: header.number,
             },
         }
     }