diff --git a/prdoc/pr_5396.prdoc b/prdoc/pr_5396.prdoc new file mode 100644 index 000000000000..d78e9ac46932 --- /dev/null +++ b/prdoc/pr_5396.prdoc @@ -0,0 +1,13 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Simplify `SyncingEngine::new()` + +doc: + - audience: Node Dev + description: | + Tiny changes to simplify the internal implemenation of API `SyncingEngine::new()` to prevent panics while fetching the genesis hash and to eliminate unnecessary allocation for reserved peers. + +crates: +- name: sc-network-sync + bump: none diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs index 4a57d61df457..4b6ccb085834 100644 --- a/substrate/client/network/sync/src/engine.rs +++ b/substrate/client/network/sync/src/engine.rs @@ -318,8 +318,7 @@ where if net_config.network_config.max_blocks_per_request > MAX_BLOCKS_IN_RESPONSE as u32 { log::info!( target: LOG_TARGET, - "clamping maximum blocks per request to {}", - MAX_BLOCKS_IN_RESPONSE, + "clamping maximum blocks per request to {MAX_BLOCKS_IN_RESPONSE}", ); MAX_BLOCKS_IN_RESPONSE as u32 } else { @@ -340,12 +339,7 @@ where imp_p.insert(reserved.peer_id); } for config in net_config.notification_protocols() { - let peer_ids = config - .set_config() - .reserved_nodes - .iter() - .map(|info| info.peer_id) - .collect::>(); + let peer_ids = config.set_config().reserved_nodes.iter().map(|info| info.peer_id); imp_p.extend(peer_ids); } @@ -379,18 +373,16 @@ where total.saturating_sub(net_config.network_config.default_peers_set_num_full) as usize }; + let info = client.info(); + let (block_announce_config, notification_service) = Self::get_block_announce_proto_config::( protocol_id, fork_id, roles, - client.info().best_number, - client.info().best_hash, - client - .block_hash(Zero::zero()) - .ok() - .flatten() - .expect("Genesis block exists; qed"), + info.best_number, + info.best_hash, + info.genesis_hash, &net_config.network_config.default_peers_set, network_metrics, Arc::clone(&peer_store_handle), @@ -403,11 +395,6 @@ where let (tx, service_rx) = tracing_unbounded("mpsc_chain_sync", 100_000); let num_connected = Arc::new(AtomicUsize::new(0)); let is_major_syncing = Arc::new(AtomicBool::new(false)); - let genesis_hash = client - .block_hash(0u32.into()) - .ok() - .flatten() - .expect("Genesis block exists; qed"); // `default_peers_set.in_peers` contains an unspecified amount of light peers so the number // of full inbound peers must be calculated from the total full peer count @@ -436,7 +423,7 @@ where num_connected: num_connected.clone(), is_major_syncing: is_major_syncing.clone(), service_rx, - genesis_hash, + genesis_hash: info.genesis_hash, important_peers, default_peers_set_no_slot_connected_peers: HashSet::new(), boot_node_ids, @@ -534,7 +521,7 @@ where "Received block announce from disconnected peer {peer_id}", ); debug_assert!(false); - return + return; }, }; peer.known_blocks.insert(hash); @@ -559,17 +546,17 @@ where Ok(Some(header)) => header, Ok(None) => { log::warn!(target: LOG_TARGET, "Trying to announce unknown block: {hash}"); - return + return; }, Err(e) => { log::warn!(target: LOG_TARGET, "Error reading block header {hash}: {e}"); - return + return; }, }; // don't announce genesis block since it will be ignored if header.number().is_zero() { - return + return; } let is_best = self.client.info().best_hash == hash; @@ -623,7 +610,7 @@ where target: LOG_TARGET, "Terminating `SyncingEngine` due to fatal error: {e:?}.", ); - return + return; } } } @@ -825,12 +812,12 @@ where target: LOG_TARGET, "received notification from {peer} who had been earlier refused by `SyncingEngine`", ); - return + return; } let Ok(announce) = BlockAnnounce::decode(&mut notification.as_ref()) else { log::warn!(target: LOG_TARGET, "failed to decode block announce"); - return + return; }; self.push_block_announce_validation(peer, announce); @@ -844,7 +831,7 @@ where fn on_sync_peer_disconnected(&mut self, peer_id: PeerId) { let Some(info) = self.peers.remove(&peer_id) else { log::debug!(target: LOG_TARGET, "{peer_id} does not exist in `SyncingEngine`"); - return + return; }; if let Some(metrics) = &self.metrics { metrics.peers.dec(); @@ -918,7 +905,7 @@ where ); } - return Err(true) + return Err(true); } Ok(handshake) @@ -953,7 +940,7 @@ where "Called `validate_connection()` with already connected peer {peer_id}", ); debug_assert!(false); - return Err(false) + return Err(false); } let no_slot_peer = self.default_peers_set_no_slot_peers.contains(&peer_id); @@ -966,7 +953,7 @@ where this_peer_reserved_slot { log::debug!(target: LOG_TARGET, "Too many full nodes, rejecting {peer_id}"); - return Err(false) + return Err(false); } // make sure to accept no more than `--in-peers` many full nodes @@ -976,7 +963,7 @@ where self.num_in_peers == self.max_in_peers { log::debug!(target: LOG_TARGET, "All inbound slots have been consumed, rejecting {peer_id}"); - return Err(false) + return Err(false); } // make sure that all slots are not occupied by light peers @@ -987,7 +974,7 @@ where (self.peers.len() - self.strategy.num_peers()) >= self.default_peers_set_num_light { log::debug!(target: LOG_TARGET, "Too many light nodes, rejecting {peer_id}"); - return Err(false) + return Err(false); } Ok(handshake) @@ -1049,7 +1036,7 @@ where if !self.peers.contains_key(&peer_id) { trace!(target: LOG_TARGET, "Cannot send block request to unknown peer {peer_id}"); debug_assert!(false); - return + return; } let downloader = self.block_downloader.clone(); @@ -1071,7 +1058,7 @@ where if !self.peers.contains_key(&peer_id) { trace!(target: LOG_TARGET, "Cannot send state request to unknown peer {peer_id}"); debug_assert!(false); - return + return; } let (tx, rx) = oneshot::channel(); @@ -1106,7 +1093,7 @@ where if !self.peers.contains_key(&peer_id) { trace!(target: LOG_TARGET, "Cannot send warp proof request to unknown peer {peer_id}"); debug_assert!(false); - return + return; } let (tx, rx) = oneshot::channel(); @@ -1169,7 +1156,7 @@ where peer_id, self.block_announce_protocol_name.clone(), ); - return + return; }, Err(BlockResponseError::ExtractionFailed(e)) => { debug!( @@ -1179,7 +1166,7 @@ where e ); self.network_service.report_peer(peer_id, rep::BAD_MESSAGE); - return + return; }, } }, @@ -1196,7 +1183,7 @@ where peer_id, self.block_announce_protocol_name.clone(), ); - return + return; }, };