From bf57a2e92d04771d68ef8b52855de2c4a6563ef0 Mon Sep 17 00:00:00 2001 From: Nazar Mokrynskyi Date: Fri, 21 Oct 2022 16:38:53 +0300 Subject: [PATCH] Actually fix major sync detection (#12114) * Actually fix major sync detection * Introduce `SyncState::Importing` state * Add target to SyncState enum variants and add `is_major_syncing` method on it * Remove unnecessary duplicated `best_seen_block` from `SyncState` struct * Revert "Remove unnecessary duplicated `best_seen_block` from `SyncState` struct" This reverts commit bb8abd458c939881c049f69d59f3acba47c97c5c. * Add missing `websocket` feature to `libp2p` Co-authored-by: parity-processbot <> --- substrate/client/informant/src/display.rs | 63 +++++++++---------- substrate/client/network/Cargo.toml | 2 +- .../client/network/common/src/service.rs | 2 +- substrate/client/network/common/src/sync.rs | 15 ++++- substrate/client/network/src/service.rs | 14 +++-- substrate/client/network/sync/src/lib.rs | 37 ++++++----- 6 files changed, 71 insertions(+), 62 deletions(-) diff --git a/substrate/client/informant/src/display.rs b/substrate/client/informant/src/display.rs index 0441011b0e..3d585a9985 100644 --- a/substrate/client/informant/src/display.rs +++ b/substrate/client/informant/src/display.rs @@ -93,42 +93,37 @@ impl InformantDisplay { (diff_bytes_inbound, diff_bytes_outbound) }; - let (level, status, target) = match ( - net_status.sync_state, - net_status.best_seen_block, - net_status.state_sync, - net_status.warp_sync, - ) { - ( - _, - _, - _, - Some(WarpSyncProgress { phase: WarpSyncPhase::DownloadingBlocks(n), .. }), - ) => ("⏩", "Block history".into(), format!(", #{}", n)), - (_, _, _, Some(warp)) => ( - "⏩", - "Warping".into(), - format!( - ", {}, {:.2} Mib", - warp.phase, - (warp.total_bytes as f32) / (1024f32 * 1024f32) + let (level, status, target) = + match (net_status.sync_state, net_status.state_sync, net_status.warp_sync) { + ( + _, + _, + Some(WarpSyncProgress { phase: WarpSyncPhase::DownloadingBlocks(n), .. }), + ) => ("⏩", "Block history".into(), format!(", #{}", n)), + (_, _, Some(warp)) => ( + "⏩", + "Warping".into(), + format!( + ", {}, {:.2} Mib", + warp.phase, + (warp.total_bytes as f32) / (1024f32 * 1024f32) + ), ), - ), - (_, _, Some(state), _) => ( - "⚙️ ", - "Downloading state".into(), - format!( - ", {}%, {:.2} Mib", - state.percentage, - (state.size as f32) / (1024f32 * 1024f32) + (_, Some(state), _) => ( + "⚙️ ", + "Downloading state".into(), + format!( + ", {}%, {:.2} Mib", + state.percentage, + (state.size as f32) / (1024f32 * 1024f32) + ), ), - ), - (SyncState::Idle, _, _, _) => ("💤", "Idle".into(), "".into()), - (SyncState::Downloading, None, _, _) => - ("⚙️ ", format!("Preparing{}", speed), "".into()), - (SyncState::Downloading, Some(n), None, _) => - ("⚙️ ", format!("Syncing{}", speed), format!(", target=#{}", n)), - }; + (SyncState::Idle, _, _) => ("💤", "Idle".into(), "".into()), + (SyncState::Downloading { target }, _, _) => + ("⚙️ ", format!("Syncing{}", speed), format!(", target=#{target}")), + (SyncState::Importing { target }, _, _) => + ("⚙️ ", format!("Preparing{}", speed), format!(", target=#{target}")), + }; if self.format.enable_color { info!( diff --git a/substrate/client/network/Cargo.toml b/substrate/client/network/Cargo.toml index 9c2833ec00..81b684af43 100644 --- a/substrate/client/network/Cargo.toml +++ b/substrate/client/network/Cargo.toml @@ -26,7 +26,7 @@ fnv = "1.0.6" futures = "0.3.21" futures-timer = "3.0.2" ip_network = "0.4.1" -libp2p = { version = "0.49.0", features = ["async-std", "dns", "identify", "kad", "mdns-async-io", "mplex", "noise", "ping", "tcp", "yamux"] } +libp2p = { version = "0.49.0", features = ["async-std", "dns", "identify", "kad", "mdns-async-io", "mplex", "noise", "ping", "tcp", "yamux", "websocket"] } linked_hash_set = "0.1.3" linked-hash-map = "0.5.4" log = "0.4.17" diff --git a/substrate/client/network/common/src/service.rs b/substrate/client/network/common/src/service.rs index aa4967ba51..54d254eac3 100644 --- a/substrate/client/network/common/src/service.rs +++ b/substrate/client/network/common/src/service.rs @@ -98,7 +98,7 @@ where #[derive(Clone)] pub struct NetworkStatus { /// Current global sync state. - pub sync_state: SyncState, + pub sync_state: SyncState>, /// Target sync block number. pub best_seen_block: Option>, /// Number of peers participating in syncing. diff --git a/substrate/client/network/common/src/sync.rs b/substrate/client/network/common/src/sync.rs index aa761e66eb..dd216b2a52 100644 --- a/substrate/client/network/common/src/sync.rs +++ b/substrate/client/network/common/src/sync.rs @@ -44,11 +44,20 @@ pub struct PeerInfo { /// Reported sync state. #[derive(Clone, Eq, PartialEq, Debug)] -pub enum SyncState { +pub enum SyncState { /// Initial sync is complete, keep-up sync is active. Idle, /// Actively catching up with the chain. - Downloading, + Downloading { target: BlockNumber }, + /// All blocks are downloaded and are being imported. + Importing { target: BlockNumber }, +} + +impl SyncState { + /// Are we actively catching up with the chain? + pub fn is_major_syncing(&self) -> bool { + !matches!(self, SyncState::Idle) + } } /// Reported state download progress. @@ -64,7 +73,7 @@ pub struct StateDownloadProgress { #[derive(Clone)] pub struct SyncStatus { /// Current global sync state. - pub state: SyncState, + pub state: SyncState>, /// Target sync block number. pub best_seen_block: Option>, /// Number of peers participating in syncing. diff --git a/substrate/client/network/src/service.rs b/substrate/client/network/src/service.rs index 8dfd46f24a..89dc2ff440 100644 --- a/substrate/client/network/src/service.rs +++ b/substrate/client/network/src/service.rs @@ -70,7 +70,7 @@ use sc_network_common::{ NotificationSender as NotificationSenderT, NotificationSenderError, NotificationSenderReady as NotificationSenderReadyT, Signature, SigningError, }, - sync::{SyncState, SyncStatus}, + sync::SyncStatus, ExHashT, }; use sc_peerset::PeersetHandle; @@ -1997,11 +1997,13 @@ where *this.external_addresses.lock() = external_addresses; } - let is_major_syncing = - match this.network_service.behaviour_mut().user_protocol_mut().sync_state().state { - SyncState::Idle => false, - SyncState::Downloading => true, - }; + let is_major_syncing = this + .network_service + .behaviour_mut() + .user_protocol_mut() + .sync_state() + .state + .is_major_syncing(); this.is_major_syncing.store(is_major_syncing, Ordering::Relaxed); diff --git a/substrate/client/network/sync/src/lib.rs b/substrate/client/network/sync/src/lib.rs index 76d7d624be..63f63f7188 100644 --- a/substrate/client/network/sync/src/lib.rs +++ b/substrate/client/network/sync/src/lib.rs @@ -410,13 +410,21 @@ where /// Returns the current sync status. fn status(&self) -> SyncStatus { - let best_seen = self.best_seen(); - let sync_state = if let Some(n) = best_seen { + let median_seen = self.median_seen(); + let best_seen_block = + median_seen.and_then(|median| (median > self.best_queued_number).then_some(median)); + let sync_state = if let Some(target) = median_seen { // A chain is classified as downloading if the provided best block is - // more than `MAJOR_SYNC_BLOCKS` behind the best block. + // more than `MAJOR_SYNC_BLOCKS` behind the best block or as importing + // if the same can be said about queued blocks. let best_block = self.client.info().best_number; - if n > best_block && n - best_block > MAJOR_SYNC_BLOCKS.into() { - SyncState::Downloading + if target > best_block && target - best_block > MAJOR_SYNC_BLOCKS.into() { + // If target is not queued, we're downloading, otherwise importing. + if target > self.best_queued_number { + SyncState::Downloading { target } + } else { + SyncState::Importing { target } + } } else { SyncState::Idle } @@ -437,7 +445,7 @@ where SyncStatus { state: sync_state, - best_seen_block: best_seen, + best_seen_block, num_peers: self.peers.len() as u32, queued_blocks: self.queue_blocks.len() as u32, state_sync: self.state_sync.as_ref().map(|s| s.progress()), @@ -693,7 +701,7 @@ where trace!(target: "sync", "Too many blocks in the queue."); return Box::new(std::iter::empty()) } - let major_sync = self.status().state == SyncState::Downloading; + let is_major_syncing = self.status().state.is_major_syncing(); let attrs = self.required_block_attributes(); let blocks = &mut self.blocks; let fork_targets = &mut self.fork_targets; @@ -703,7 +711,7 @@ where let client = &self.client; let queue = &self.queue_blocks; let allowed_requests = self.allowed_requests.take(); - let max_parallel = if major_sync { 1 } else { self.max_parallel_downloads }; + let max_parallel = if is_major_syncing { 1 } else { self.max_parallel_downloads }; let gap_sync = &mut self.gap_sync; let iter = self.peers.iter_mut().filter_map(move |(&id, peer)| { if !peer.state.is_available() || !allowed_requests.contains(&id) { @@ -1797,8 +1805,8 @@ where Ok((sync, Box::new(ChainSyncInterfaceHandle::new(tx)))) } - /// Returns the best seen block number if we don't have that block yet, `None` otherwise. - fn best_seen(&self) -> Option> { + /// Returns the median seen block number. + fn median_seen(&self) -> Option> { let mut best_seens = self.peers.values().map(|p| p.best_number).collect::>(); if best_seens.is_empty() { @@ -1807,12 +1815,7 @@ where let middle = best_seens.len() / 2; // Not the "perfect median" when we have an even number of peers. - let median = *best_seens.select_nth_unstable(middle).1; - if median > self.best_queued_number { - Some(median) - } else { - None - } + Some(*best_seens.select_nth_unstable(middle).1) } } @@ -1854,7 +1857,7 @@ where ); } - let origin = if !gap && self.status().state != SyncState::Downloading { + let origin = if !gap && !self.status().state.is_major_syncing() { BlockOrigin::NetworkBroadcast } else { BlockOrigin::NetworkInitialSync