From fb272a6fd94b58c6da5a1e474dd5017b13050791 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 16 Jan 2023 17:43:17 +0300 Subject: [PATCH 01/35] Convert `NetworkWorker::poll()` into async `next_action()` --- client/network/src/service.rs | 1103 ++++++++++++++++----------------- client/service/src/lib.rs | 2 +- 2 files changed, 550 insertions(+), 555 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index f943a03f50b38..b760b8715c9d0 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -87,7 +87,6 @@ use std::{ atomic::{AtomicBool, AtomicUsize, Ordering}, Arc, }, - task::Poll, }; pub use behaviour::{InboundFailure, OutboundFailure, ResponseFailure}; @@ -1277,600 +1276,598 @@ where _marker: PhantomData, } -impl Future for NetworkWorker +impl NetworkWorker where B: BlockT + 'static, H: ExHashT, Client: HeaderBackend + 'static, { - type Output = (); - - fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context) -> Poll { - let this = &mut *self; - - // At the time of writing of this comment, due to a high volume of messages, the network - // worker sometimes takes a long time to process the loop below. When that happens, the - // rest of the polling is frozen. In order to avoid negative side-effects caused by this - // freeze, a limit to the number of iterations is enforced below. If the limit is reached, - // the task is interrupted then scheduled again. - // - // This allows for a more even distribution in the time taken by each sub-part of the - // polling. - let mut num_iterations = 0; - loop { - num_iterations += 1; - if num_iterations >= 100 { - cx.waker().wake_by_ref(); - break - } - - // Process the next message coming from the `NetworkService`. - let msg = match this.from_service.poll_next_unpin(cx) { - Poll::Ready(Some(msg)) => msg, - Poll::Ready(None) => return Poll::Ready(()), - Poll::Pending => break, - }; - match msg { - ServiceToWorkerMsg::AnnounceBlock(hash, data) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .announce_block(hash, data), - ServiceToWorkerMsg::GetValue(key) => - this.network_service.behaviour_mut().get_value(key), - ServiceToWorkerMsg::PutValue(key, value) => - this.network_service.behaviour_mut().put_value(key, value), - ServiceToWorkerMsg::SetReservedOnly(reserved_only) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .set_reserved_only(reserved_only), - ServiceToWorkerMsg::SetReserved(peers) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .set_reserved_peers(peers), - ServiceToWorkerMsg::SetPeersetReserved(protocol, peers) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .set_reserved_peerset_peers(protocol, peers), - ServiceToWorkerMsg::AddReserved(peer_id) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .add_reserved_peer(peer_id), - ServiceToWorkerMsg::RemoveReserved(peer_id) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .remove_reserved_peer(peer_id), - ServiceToWorkerMsg::AddSetReserved(protocol, peer_id) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .add_set_reserved_peer(protocol, peer_id), - ServiceToWorkerMsg::RemoveSetReserved(protocol, peer_id) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .remove_set_reserved_peer(protocol, peer_id), - ServiceToWorkerMsg::AddKnownAddress(peer_id, addr) => - this.network_service.behaviour_mut().add_known_address(peer_id, addr), - ServiceToWorkerMsg::AddToPeersSet(protocol, peer_id) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .add_to_peers_set(protocol, peer_id), - ServiceToWorkerMsg::RemoveFromPeersSet(protocol, peer_id) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .remove_from_peers_set(protocol, peer_id), - ServiceToWorkerMsg::EventStream(sender) => this.event_streams.push(sender), - ServiceToWorkerMsg::Request { - target, - protocol, - request, - pending_response, - connect, - } => { - this.network_service.behaviour_mut().send_request( - &target, - &protocol, - request, - pending_response, - connect, - ); - }, - ServiceToWorkerMsg::NetworkStatus { pending_response } => { - let _ = pending_response.send(Ok(this.status())); - }, - ServiceToWorkerMsg::NetworkState { pending_response } => { - let _ = pending_response.send(Ok(this.network_state())); - }, - ServiceToWorkerMsg::DisconnectPeer(who, protocol_name) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .disconnect_peer(&who, protocol_name), - ServiceToWorkerMsg::NewBestBlockImported(hash, number) => this - .network_service - .behaviour_mut() - .user_protocol_mut() - .new_best_block_imported(hash, number), - } - } - - // `num_iterations` serves the same purpose as in the previous loop. - // See the previous loop for explanations. - let mut num_iterations = 0; - loop { - num_iterations += 1; - if num_iterations >= 1000 { - cx.waker().wake_by_ref(); - break + /// Perform one action on the network. + pub async fn next_action(&mut self) { + // Next message from the service, or `future::pending()` if the stream has terminated. + let next_worker_msg = { + let from_service = &mut self.from_service; + + async move { + if let Some(msg) = from_service.next().await { + msg + } else { + loop { + futures::pending!(); + } + } } + }; - // Process the next action coming from the network. - let next_event = this.network_service.select_next_some(); - futures::pin_mut!(next_event); - let poll_value = next_event.poll_unpin(cx); - - match poll_value { - Poll::Pending => break, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::InboundRequest { - protocol, - result, - .. - })) => { - if let Some(metrics) = this.metrics.as_ref() { - match result { - Ok(serve_time) => { - metrics - .requests_in_success_total - .with_label_values(&[&protocol]) - .observe(serve_time.as_secs_f64()); - }, - Err(err) => { - let reason = match err { - ResponseFailure::Network(InboundFailure::Timeout) => "timeout", - ResponseFailure::Network( - InboundFailure::UnsupportedProtocols, - ) => - // `UnsupportedProtocols` is reported for every single - // inbound request whenever a request with an unsupported - // protocol is received. This is not reported in order to - // avoid confusions. - continue, - ResponseFailure::Network(InboundFailure::ResponseOmission) => - "busy-omitted", - ResponseFailure::Network(InboundFailure::ConnectionClosed) => - "connection-closed", - }; + // Next swarm event, or `future::pending()` if the stream has terminated + // (guaranteed to never happen with `Swarm`). + let next_swarm_event = { + let swarm = &mut self.network_service; - metrics - .requests_in_failure_total - .with_label_values(&[&protocol, reason]) - .inc(); - }, - } + async move { + if let Some(event) = swarm.next().await { + event + } else { + loop { + futures::pending!(); } - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::RequestFinished { - protocol, - duration, - result, - .. - })) => - if let Some(metrics) = this.metrics.as_ref() { - match result { - Ok(_) => { - metrics - .requests_out_success_total - .with_label_values(&[&protocol]) - .observe(duration.as_secs_f64()); - }, - Err(err) => { - let reason = match err { - RequestFailure::NotConnected => "not-connected", - RequestFailure::UnknownProtocol => "unknown-protocol", - RequestFailure::Refused => "refused", - RequestFailure::Obsolete => "obsolete", - RequestFailure::Network(OutboundFailure::DialFailure) => - "dial-failure", - RequestFailure::Network(OutboundFailure::Timeout) => "timeout", - RequestFailure::Network(OutboundFailure::ConnectionClosed) => - "connection-closed", - RequestFailure::Network( - OutboundFailure::UnsupportedProtocols, - ) => "unsupported", - }; + } + } + }; - metrics - .requests_out_failure_total - .with_label_values(&[&protocol, reason]) - .inc(); - }, - } + futures::select! { + msg = next_worker_msg.fuse() => { + // Process the next message coming from the `NetworkService`. + match msg { + ServiceToWorkerMsg::AnnounceBlock(hash, data) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .announce_block(hash, data), + ServiceToWorkerMsg::GetValue(key) => + self.network_service.behaviour_mut().get_value(key), + ServiceToWorkerMsg::PutValue(key, value) => + self.network_service.behaviour_mut().put_value(key, value), + ServiceToWorkerMsg::SetReservedOnly(reserved_only) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .set_reserved_only(reserved_only), + ServiceToWorkerMsg::SetReserved(peers) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .set_reserved_peers(peers), + ServiceToWorkerMsg::SetPeersetReserved(protocol, peers) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .set_reserved_peerset_peers(protocol, peers), + ServiceToWorkerMsg::AddReserved(peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .add_reserved_peer(peer_id), + ServiceToWorkerMsg::RemoveReserved(peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .remove_reserved_peer(peer_id), + ServiceToWorkerMsg::AddSetReserved(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .add_set_reserved_peer(protocol, peer_id), + ServiceToWorkerMsg::RemoveSetReserved(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .remove_set_reserved_peer(protocol, peer_id), + ServiceToWorkerMsg::AddKnownAddress(peer_id, addr) => + self.network_service.behaviour_mut().add_known_address(peer_id, addr), + ServiceToWorkerMsg::AddToPeersSet(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .add_to_peers_set(protocol, peer_id), + ServiceToWorkerMsg::RemoveFromPeersSet(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .remove_from_peers_set(protocol, peer_id), + ServiceToWorkerMsg::EventStream(sender) => self.event_streams.push(sender), + ServiceToWorkerMsg::Request { + target, + protocol, + request, + pending_response, + connect, + } => { + self.network_service.behaviour_mut().send_request( + &target, + &protocol, + request, + pending_response, + connect, + ); }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::ReputationChanges { - peer, - changes, - })) => - for change in changes { - this.network_service.behaviour().user_protocol().report_peer(peer, change); + ServiceToWorkerMsg::NetworkStatus { pending_response } => { + let _ = pending_response.send(Ok(self.status())); }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::PeerIdentify { - peer_id, - info: - IdentifyInfo { - protocol_version, - agent_version, - mut listen_addrs, - protocols, - .. - }, - })) => { - if listen_addrs.len() > 30 { - debug!( - target: "sub-libp2p", - "Node {:?} has reported more than 30 addresses; it is identified by {:?} and {:?}", - peer_id, protocol_version, agent_version - ); - listen_addrs.truncate(30); - } - for addr in listen_addrs { - this.network_service - .behaviour_mut() - .add_self_reported_address_to_dht(&peer_id, &protocols, addr); - } - this.network_service + ServiceToWorkerMsg::NetworkState { pending_response } => { + let _ = pending_response.send(Ok(self.network_state())); + }, + ServiceToWorkerMsg::DisconnectPeer(who, protocol_name) => self + .network_service .behaviour_mut() .user_protocol_mut() - .add_default_set_discovered_nodes(iter::once(peer_id)); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::Discovered(peer_id))) => { - this.network_service + .disconnect_peer(&who, protocol_name), + ServiceToWorkerMsg::NewBestBlockImported(hash, number) => self + .network_service .behaviour_mut() .user_protocol_mut() - .add_default_set_discovered_nodes(iter::once(peer_id)); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::RandomKademliaStarted)) => - if let Some(metrics) = this.metrics.as_ref() { - metrics.kademlia_random_queries_total.inc(); + .new_best_block_imported(hash, number), + } + }, + next_event = next_swarm_event.fuse() => { + // Process the next event coming from `Swarm`. + match next_event { + SwarmEvent::Behaviour(BehaviourOut::InboundRequest { + protocol, + result, + .. + }) => { + if let Some(metrics) = self.metrics.as_ref() { + match result { + Ok(serve_time) => { + metrics + .requests_in_success_total + .with_label_values(&[&protocol]) + .observe(serve_time.as_secs_f64()); + }, + Err(err) => { + let reason = match err { + ResponseFailure::Network(InboundFailure::Timeout) => + Some("timeout"), + ResponseFailure::Network( + InboundFailure::UnsupportedProtocols, + ) => + // `UnsupportedProtocols` is reported for every single + // inbound request whenever a request with an unsupported + // protocol is received. This is not reported in order to + // avoid confusions. + None, + ResponseFailure::Network(InboundFailure::ResponseOmission) => + Some("busy-omitted"), + ResponseFailure::Network(InboundFailure::ConnectionClosed) => + Some("connection-closed"), + }; + + if let Some(reason) = reason { + metrics + .requests_in_failure_total + .with_label_values(&[&protocol, reason]) + .inc(); + } + }, + } + } }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::NotificationStreamOpened { - remote, - protocol, - negotiated_fallback, - notifications_sink, - role, - })) => { - if let Some(metrics) = this.metrics.as_ref() { - metrics - .notifications_streams_opened_total - .with_label_values(&[&protocol]) - .inc(); - } - { - let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); - let _previous_value = peers_notifications_sinks - .insert((remote, protocol.clone()), notifications_sink); - debug_assert!(_previous_value.is_none()); - } - this.event_streams.send(Event::NotificationStreamOpened { + SwarmEvent::Behaviour(BehaviourOut::RequestFinished { + protocol, + duration, + result, + .. + }) => + if let Some(metrics) = self.metrics.as_ref() { + match result { + Ok(_) => { + metrics + .requests_out_success_total + .with_label_values(&[&protocol]) + .observe(duration.as_secs_f64()); + }, + Err(err) => { + let reason = match err { + RequestFailure::NotConnected => "not-connected", + RequestFailure::UnknownProtocol => "unknown-protocol", + RequestFailure::Refused => "refused", + RequestFailure::Obsolete => "obsolete", + RequestFailure::Network(OutboundFailure::DialFailure) => + "dial-failure", + RequestFailure::Network(OutboundFailure::Timeout) => "timeout", + RequestFailure::Network(OutboundFailure::ConnectionClosed) => + "connection-closed", + RequestFailure::Network( + OutboundFailure::UnsupportedProtocols, + ) => "unsupported", + }; + + metrics + .requests_out_failure_total + .with_label_values(&[&protocol, reason]) + .inc(); + }, + } + }, + SwarmEvent::Behaviour(BehaviourOut::ReputationChanges { + peer, + changes, + }) => + for change in changes { + self.network_service.behaviour().user_protocol().report_peer(peer, change); + }, + SwarmEvent::Behaviour(BehaviourOut::PeerIdentify { + peer_id, + info: + IdentifyInfo { + protocol_version, + agent_version, + mut listen_addrs, + protocols, + .. + }, + }) => { + if listen_addrs.len() > 30 { + debug!( + target: "sub-libp2p", + "Node {:?} has reported more than 30 addresses; it is identified by {:?} and {:?}", + peer_id, protocol_version, agent_version + ); + listen_addrs.truncate(30); + } + for addr in listen_addrs { + self.network_service + .behaviour_mut() + .add_self_reported_address_to_dht(&peer_id, &protocols, addr); + } + self.network_service + .behaviour_mut() + .user_protocol_mut() + .add_default_set_discovered_nodes(iter::once(peer_id)); + }, + SwarmEvent::Behaviour(BehaviourOut::Discovered(peer_id)) => { + self.network_service + .behaviour_mut() + .user_protocol_mut() + .add_default_set_discovered_nodes(iter::once(peer_id)); + }, + SwarmEvent::Behaviour(BehaviourOut::RandomKademliaStarted) => + if let Some(metrics) = self.metrics.as_ref() { + metrics.kademlia_random_queries_total.inc(); + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationStreamOpened { remote, protocol, negotiated_fallback, + notifications_sink, role, - }); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::NotificationStreamReplaced { - remote, - protocol, - notifications_sink, - })) => { - let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); - if let Some(s) = peers_notifications_sinks.get_mut(&(remote, protocol)) { - *s = notifications_sink; - } else { - error!( - target: "sub-libp2p", - "NotificationStreamReplaced for non-existing substream" - ); - debug_assert!(false); - } + }) => { + if let Some(metrics) = self.metrics.as_ref() { + metrics + .notifications_streams_opened_total + .with_label_values(&[&protocol]) + .inc(); + } + { + let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); + let _previous_value = peers_notifications_sinks + .insert((remote, protocol.clone()), notifications_sink); + debug_assert!(_previous_value.is_none()); + } + self.event_streams.send(Event::NotificationStreamOpened { + remote, + protocol, + negotiated_fallback, + role, + }); + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationStreamReplaced { + remote, + protocol, + notifications_sink, + }) => { + let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); + if let Some(s) = peers_notifications_sinks.get_mut(&(remote, protocol)) { + *s = notifications_sink; + } else { + error!( + target: "sub-libp2p", + "NotificationStreamReplaced for non-existing substream" + ); + debug_assert!(false); + } - // TODO: Notifications might have been lost as a result of the previous - // connection being dropped, and as a result it would be preferable to notify - // the users of this fact by simulating the substream being closed then - // reopened. - // The code below doesn't compile because `role` is unknown. Propagating the - // handshake of the secondary connections is quite an invasive change and - // would conflict with https://github.com/paritytech/substrate/issues/6403. - // Considering that dropping notifications is generally regarded as - // acceptable, this bug is at the moment intentionally left there and is - // intended to be fixed at the same time as - // https://github.com/paritytech/substrate/issues/6403. - // this.event_streams.send(Event::NotificationStreamClosed { - // remote, - // protocol, - // }); - // this.event_streams.send(Event::NotificationStreamOpened { - // remote, - // protocol, - // role, - // }); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::NotificationStreamClosed { - remote, - protocol, - })) => { - if let Some(metrics) = this.metrics.as_ref() { - metrics - .notifications_streams_closed_total - .with_label_values(&[&protocol[..]]) - .inc(); - } - this.event_streams.send(Event::NotificationStreamClosed { + // TODO: Notifications might have been lost as a result of the previous + // connection being dropped, and as a result it would be preferable to notify + // the users of this fact by simulating the substream being closed then + // reopened. + // The code below doesn't compile because `role` is unknown. Propagating the + // handshake of the secondary connections is quite an invasive change and + // would conflict with https://github.com/paritytech/substrate/issues/6403. + // Considering that dropping notifications is generally regarded as + // acceptable, this bug is at the moment intentionally left there and is + // intended to be fixed at the same time as + // https://github.com/paritytech/substrate/issues/6403. + // self.event_streams.send(Event::NotificationStreamClosed { + // remote, + // protocol, + // }); + // self.event_streams.send(Event::NotificationStreamOpened { + // remote, + // protocol, + // role, + // }); + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationStreamClosed { remote, - protocol: protocol.clone(), - }); - { - let mut peers_notifications_sinks = this.peers_notifications_sinks.lock(); - let _previous_value = peers_notifications_sinks.remove(&(remote, protocol)); - debug_assert!(_previous_value.is_some()); - } - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::NotificationsReceived { - remote, - messages, - })) => { - if let Some(metrics) = this.metrics.as_ref() { - for (protocol, message) in &messages { + protocol, + }) => { + if let Some(metrics) = self.metrics.as_ref() { metrics - .notifications_sizes - .with_label_values(&["in", protocol]) - .observe(message.len() as f64); + .notifications_streams_closed_total + .with_label_values(&[&protocol[..]]) + .inc(); + } + self.event_streams.send(Event::NotificationStreamClosed { + remote, + protocol: protocol.clone(), + }); + { + let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); + let _previous_value = peers_notifications_sinks.remove(&(remote, protocol)); + debug_assert!(_previous_value.is_some()); + } + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationsReceived { + remote, + messages, + }) => { + if let Some(metrics) = self.metrics.as_ref() { + for (protocol, message) in &messages { + metrics + .notifications_sizes + .with_label_values(&["in", protocol]) + .observe(message.len() as f64); + } + } + self.event_streams.send(Event::NotificationsReceived { remote, messages }); + }, + SwarmEvent::Behaviour(BehaviourOut::SyncConnected(remote)) => { + self.event_streams.send(Event::SyncConnected { remote }); + }, + SwarmEvent::Behaviour(BehaviourOut::SyncDisconnected(remote)) => { + self.event_streams.send(Event::SyncDisconnected { remote }); + }, + SwarmEvent::Behaviour(BehaviourOut::Dht(event, duration)) => { + if let Some(metrics) = self.metrics.as_ref() { + let query_type = match event { + DhtEvent::ValueFound(_) => "value-found", + DhtEvent::ValueNotFound(_) => "value-not-found", + DhtEvent::ValuePut(_) => "value-put", + DhtEvent::ValuePutFailed(_) => "value-put-failed", + }; + metrics + .kademlia_query_duration + .with_label_values(&[query_type]) + .observe(duration.as_secs_f64()); } - } - this.event_streams.send(Event::NotificationsReceived { remote, messages }); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::SyncConnected(remote))) => { - this.event_streams.send(Event::SyncConnected { remote }); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::SyncDisconnected(remote))) => { - this.event_streams.send(Event::SyncDisconnected { remote }); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::Dht(event, duration))) => { - if let Some(metrics) = this.metrics.as_ref() { - let query_type = match event { - DhtEvent::ValueFound(_) => "value-found", - DhtEvent::ValueNotFound(_) => "value-not-found", - DhtEvent::ValuePut(_) => "value-put", - DhtEvent::ValuePutFailed(_) => "value-put-failed", - }; - metrics - .kademlia_query_duration - .with_label_values(&[query_type]) - .observe(duration.as_secs_f64()); - } - this.event_streams.send(Event::Dht(event)); - }, - Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::None)) => { - // Ignored event from lower layers. - }, - Poll::Ready(SwarmEvent::ConnectionEstablished { - peer_id, - endpoint, - num_established, - concurrent_dial_errors, - }) => { - if let Some(errors) = concurrent_dial_errors { - debug!(target: "sub-libp2p", "Libp2p => Connected({:?}) with errors: {:?}", peer_id, errors); - } else { - debug!(target: "sub-libp2p", "Libp2p => Connected({:?})", peer_id); - } + self.event_streams.send(Event::Dht(event)); + }, + SwarmEvent::Behaviour(BehaviourOut::None) => { + // Ignored event from lower layers. + }, + SwarmEvent::ConnectionEstablished { + peer_id, + endpoint, + num_established, + concurrent_dial_errors, + } => { + if let Some(errors) = concurrent_dial_errors { + debug!(target: "sub-libp2p", "Libp2p => Connected({:?}) with errors: {:?}", peer_id, errors); + } else { + debug!(target: "sub-libp2p", "Libp2p => Connected({:?})", peer_id); + } - if let Some(metrics) = this.metrics.as_ref() { - let direction = match endpoint { - ConnectedPoint::Dialer { .. } => "out", - ConnectedPoint::Listener { .. } => "in", - }; - metrics.connections_opened_total.with_label_values(&[direction]).inc(); + if let Some(metrics) = self.metrics.as_ref() { + let direction = match endpoint { + ConnectedPoint::Dialer { .. } => "out", + ConnectedPoint::Listener { .. } => "in", + }; + metrics.connections_opened_total.with_label_values(&[direction]).inc(); - if num_established.get() == 1 { - metrics.distinct_peers_connections_opened_total.inc(); - } - } - }, - Poll::Ready(SwarmEvent::ConnectionClosed { - peer_id, - cause, - endpoint, - num_established, - }) => { - debug!(target: "sub-libp2p", "Libp2p => Disconnected({:?}, {:?})", peer_id, cause); - if let Some(metrics) = this.metrics.as_ref() { - let direction = match endpoint { - ConnectedPoint::Dialer { .. } => "out", - ConnectedPoint::Listener { .. } => "in", - }; - let reason = match cause { - Some(ConnectionError::IO(_)) => "transport-error", - Some(ConnectionError::Handler(EitherError::A(EitherError::A( - EitherError::B(EitherError::A(PingFailure::Timeout)), - )))) => "ping-timeout", - Some(ConnectionError::Handler(EitherError::A(EitherError::A( - EitherError::A(NotifsHandlerError::SyncNotificationsClogged), - )))) => "sync-notifications-clogged", - Some(ConnectionError::Handler(_)) => "protocol-error", - Some(ConnectionError::KeepAliveTimeout) => "keep-alive-timeout", - None => "actively-closed", - }; - metrics - .connections_closed_total - .with_label_values(&[direction, reason]) - .inc(); - - // `num_established` represents the number of *remaining* connections. - if num_established == 0 { - metrics.distinct_peers_connections_closed_total.inc(); + if num_established.get() == 1 { + metrics.distinct_peers_connections_opened_total.inc(); + } } - } - }, - Poll::Ready(SwarmEvent::NewListenAddr { address, .. }) => { - trace!(target: "sub-libp2p", "Libp2p => NewListenAddr({})", address); - if let Some(metrics) = this.metrics.as_ref() { - metrics.listeners_local_addresses.inc(); - } - }, - Poll::Ready(SwarmEvent::ExpiredListenAddr { address, .. }) => { - info!(target: "sub-libp2p", "📪 No longer listening on {}", address); - if let Some(metrics) = this.metrics.as_ref() { - metrics.listeners_local_addresses.dec(); - } - }, - Poll::Ready(SwarmEvent::OutgoingConnectionError { peer_id, error }) => { - if let Some(peer_id) = peer_id { - trace!( - target: "sub-libp2p", - "Libp2p => Failed to reach {:?}: {}", - peer_id, error, - ); + }, + SwarmEvent::ConnectionClosed { + peer_id, + cause, + endpoint, + num_established, + } => { + debug!(target: "sub-libp2p", "Libp2p => Disconnected({:?}, {:?})", peer_id, cause); + if let Some(metrics) = self.metrics.as_ref() { + let direction = match endpoint { + ConnectedPoint::Dialer { .. } => "out", + ConnectedPoint::Listener { .. } => "in", + }; + let reason = match cause { + Some(ConnectionError::IO(_)) => "transport-error", + Some(ConnectionError::Handler(EitherError::A(EitherError::A( + EitherError::B(EitherError::A(PingFailure::Timeout)), + )))) => "ping-timeout", + Some(ConnectionError::Handler(EitherError::A(EitherError::A( + EitherError::A(NotifsHandlerError::SyncNotificationsClogged), + )))) => "sync-notifications-clogged", + Some(ConnectionError::Handler(_)) => "protocol-error", + Some(ConnectionError::KeepAliveTimeout) => "keep-alive-timeout", + None => "actively-closed", + }; + metrics + .connections_closed_total + .with_label_values(&[direction, reason]) + .inc(); - if this.boot_node_ids.contains(&peer_id) { - if let DialError::WrongPeerId { obtained, endpoint } = &error { - if let ConnectedPoint::Dialer { address, role_override: _ } = - endpoint - { - warn!( - "💔 The bootnode you want to connect to at `{}` provided a different peer ID `{}` than the one you expect `{}`.", - address, - obtained, - peer_id, - ); + // `num_established` represents the number of *remaining* connections. + if num_established == 0 { + metrics.distinct_peers_connections_closed_total.inc(); + } + } + }, + SwarmEvent::NewListenAddr { address, .. } => { + trace!(target: "sub-libp2p", "Libp2p => NewListenAddr({})", address); + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_local_addresses.inc(); + } + }, + SwarmEvent::ExpiredListenAddr { address, .. } => { + info!(target: "sub-libp2p", "📪 No longer listening on {}", address); + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_local_addresses.dec(); + } + }, + SwarmEvent::OutgoingConnectionError { peer_id, error } => { + if let Some(peer_id) = peer_id { + trace!( + target: "sub-libp2p", + "Libp2p => Failed to reach {:?}: {}", + peer_id, error, + ); + + if self.boot_node_ids.contains(&peer_id) { + if let DialError::WrongPeerId { obtained, endpoint } = &error { + if let ConnectedPoint::Dialer { address, role_override: _ } = + endpoint + { + warn!( + "💔 The bootnode you want to connect to at `{}` provided a different peer ID `{}` than the one you expect `{}`.", + address, + obtained, + peer_id, + ); + } } } } - } - if let Some(metrics) = this.metrics.as_ref() { - let reason = match error { - DialError::ConnectionLimit(_) => Some("limit-reached"), - DialError::InvalidPeerId(_) => Some("invalid-peer-id"), - DialError::Transport(_) | DialError::ConnectionIo(_) => - Some("transport-error"), - DialError::Banned | - DialError::LocalPeerId | - DialError::NoAddresses | - DialError::DialPeerConditionFalse(_) | - DialError::WrongPeerId { .. } | - DialError::Aborted => None, // ignore them - }; - if let Some(reason) = reason { - metrics - .pending_connections_errors_total - .with_label_values(&[reason]) - .inc(); + if let Some(metrics) = self.metrics.as_ref() { + let reason = match error { + DialError::ConnectionLimit(_) => Some("limit-reached"), + DialError::InvalidPeerId(_) => Some("invalid-peer-id"), + DialError::Transport(_) | DialError::ConnectionIo(_) => + Some("transport-error"), + DialError::Banned | + DialError::LocalPeerId | + DialError::NoAddresses | + DialError::DialPeerConditionFalse(_) | + DialError::WrongPeerId { .. } | + DialError::Aborted => None, // ignore them + }; + if let Some(reason) = reason { + metrics + .pending_connections_errors_total + .with_label_values(&[reason]) + .inc(); + } } - } - }, - Poll::Ready(SwarmEvent::Dialing(peer_id)) => { - trace!(target: "sub-libp2p", "Libp2p => Dialing({:?})", peer_id) - }, - Poll::Ready(SwarmEvent::IncomingConnection { local_addr, send_back_addr }) => { - trace!(target: "sub-libp2p", "Libp2p => IncomingConnection({},{}))", - local_addr, send_back_addr); - if let Some(metrics) = this.metrics.as_ref() { - metrics.incoming_connections_total.inc(); - } - }, - Poll::Ready(SwarmEvent::IncomingConnectionError { - local_addr, - send_back_addr, - error, - }) => { - debug!( - target: "sub-libp2p", - "Libp2p => IncomingConnectionError({},{}): {}", - local_addr, send_back_addr, error, - ); - if let Some(metrics) = this.metrics.as_ref() { - let reason = match error { - PendingConnectionError::ConnectionLimit(_) => Some("limit-reached"), - PendingConnectionError::WrongPeerId { .. } => Some("invalid-peer-id"), - PendingConnectionError::Transport(_) | - PendingConnectionError::IO(_) => Some("transport-error"), - PendingConnectionError::Aborted => None, // ignore it - }; - - if let Some(reason) = reason { + }, + SwarmEvent::Dialing(peer_id) => { + trace!(target: "sub-libp2p", "Libp2p => Dialing({:?})", peer_id) + }, + SwarmEvent::IncomingConnection { local_addr, send_back_addr } => { + trace!(target: "sub-libp2p", "Libp2p => IncomingConnection({},{}))", + local_addr, send_back_addr); + if let Some(metrics) = self.metrics.as_ref() { + metrics.incoming_connections_total.inc(); + } + }, + SwarmEvent::IncomingConnectionError { + local_addr, + send_back_addr, + error, + } => { + debug!( + target: "sub-libp2p", + "Libp2p => IncomingConnectionError({},{}): {}", + local_addr, send_back_addr, error, + ); + if let Some(metrics) = self.metrics.as_ref() { + let reason = match error { + PendingConnectionError::ConnectionLimit(_) => Some("limit-reached"), + PendingConnectionError::WrongPeerId { .. } => Some("invalid-peer-id"), + PendingConnectionError::Transport(_) | + PendingConnectionError::IO(_) => Some("transport-error"), + PendingConnectionError::Aborted => None, // ignore it + }; + + if let Some(reason) = reason { + metrics + .incoming_connections_errors_total + .with_label_values(&[reason]) + .inc(); + } + } + }, + SwarmEvent::BannedPeer { peer_id, endpoint } => { + debug!( + target: "sub-libp2p", + "Libp2p => BannedPeer({}). Connected via {:?}.", + peer_id, endpoint, + ); + if let Some(metrics) = self.metrics.as_ref() { metrics .incoming_connections_errors_total - .with_label_values(&[reason]) + .with_label_values(&["banned"]) .inc(); } - } - }, - Poll::Ready(SwarmEvent::BannedPeer { peer_id, endpoint }) => { - debug!( - target: "sub-libp2p", - "Libp2p => BannedPeer({}). Connected via {:?}.", - peer_id, endpoint, - ); - if let Some(metrics) = this.metrics.as_ref() { - metrics - .incoming_connections_errors_total - .with_label_values(&["banned"]) - .inc(); - } - }, - Poll::Ready(SwarmEvent::ListenerClosed { reason, addresses, .. }) => { - if let Some(metrics) = this.metrics.as_ref() { - metrics.listeners_local_addresses.sub(addresses.len() as u64); - } - let addrs = - addresses.into_iter().map(|a| a.to_string()).collect::>().join(", "); - match reason { - Ok(()) => error!( - target: "sub-libp2p", - "📪 Libp2p listener ({}) closed gracefully", - addrs - ), - Err(e) => error!( - target: "sub-libp2p", - "📪 Libp2p listener ({}) closed: {}", - addrs, e - ), - } - }, - Poll::Ready(SwarmEvent::ListenerError { error, .. }) => { - debug!(target: "sub-libp2p", "Libp2p => ListenerError: {}", error); - if let Some(metrics) = this.metrics.as_ref() { - metrics.listeners_errors_total.inc(); - } - }, - }; - } + }, + SwarmEvent::ListenerClosed { reason, addresses, .. } => { + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_local_addresses.sub(addresses.len() as u64); + } + let addrs = + addresses.into_iter().map(|a| a.to_string()).collect::>().join(", "); + match reason { + Ok(()) => error!( + target: "sub-libp2p", + "📪 Libp2p listener ({}) closed gracefully", + addrs + ), + Err(e) => error!( + target: "sub-libp2p", + "📪 Libp2p listener ({}) closed: {}", + addrs, e + ), + } + }, + SwarmEvent::ListenerError { error, .. } => { + debug!(target: "sub-libp2p", "Libp2p => ListenerError: {}", error); + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_errors_total.inc(); + } + }, + } + } + }; let num_connected_peers = - this.network_service.behaviour_mut().user_protocol_mut().num_connected_peers(); + self.network_service.behaviour_mut().user_protocol_mut().num_connected_peers(); // Update the variables shared with the `NetworkService`. - this.num_connected.store(num_connected_peers, Ordering::Relaxed); + self.num_connected.store(num_connected_peers, Ordering::Relaxed); { let external_addresses = - Swarm::>::external_addresses(&this.network_service) + Swarm::>::external_addresses(&self.network_service) .map(|r| &r.addr) .cloned() .collect(); - *this.external_addresses.lock() = external_addresses; + *self.external_addresses.lock() = external_addresses; } - let is_major_syncing = this + let is_major_syncing = self .network_service .behaviour_mut() .user_protocol_mut() @@ -1878,10 +1875,10 @@ where .state .is_major_syncing(); - this.is_major_syncing.store(is_major_syncing, Ordering::Relaxed); + self.is_major_syncing.store(is_major_syncing, Ordering::Relaxed); - if let Some(metrics) = this.metrics.as_ref() { - if let Some(buckets) = this.network_service.behaviour_mut().num_entries_per_kbucket() { + if let Some(metrics) = self.metrics.as_ref() { + if let Some(buckets) = self.network_service.behaviour_mut().num_entries_per_kbucket() { for (lower_ilog2_bucket_bound, num_entries) in buckets { metrics .kbuckets_num_nodes @@ -1889,25 +1886,23 @@ where .set(num_entries as u64); } } - if let Some(num_entries) = this.network_service.behaviour_mut().num_kademlia_records() { + if let Some(num_entries) = self.network_service.behaviour_mut().num_kademlia_records() { metrics.kademlia_records_count.set(num_entries as u64); } if let Some(num_entries) = - this.network_service.behaviour_mut().kademlia_records_total_size() + self.network_service.behaviour_mut().kademlia_records_total_size() { metrics.kademlia_records_sizes_total.set(num_entries as u64); } metrics .peerset_num_discovered - .set(this.network_service.behaviour_mut().user_protocol().num_discovered_peers() + .set(self.network_service.behaviour_mut().user_protocol().num_discovered_peers() as u64); metrics.pending_connections.set( - Swarm::network_info(&this.network_service).connection_counters().num_pending() + Swarm::network_info(&self.network_service).connection_counters().num_pending() as u64, ); } - - Poll::Pending } } diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 1529b822ade32..e3b8416808b8c 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -289,7 +289,7 @@ async fn build_network_future< // The network worker has done something. Nothing special to do, but could be // used in the future to perform actions in response of things that happened on // the network. - _ = (&mut network).fuse() => {} + _ = network.next_action().fuse() => {} } } } From 4b5d851ec864f78f9d083a18a618fbe117c896d2 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 18 Jan 2023 17:51:00 +0300 Subject: [PATCH 02/35] Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test` --- Cargo.lock | 11 ++ client/network/test/Cargo.toml | 1 + client/network/test/src/lib.rs | 210 ++++++++++----------- client/network/test/src/sync.rs | 313 +++++++++++++------------------- 4 files changed, 242 insertions(+), 293 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e4e3e1b84625c..8328185442f19 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8635,6 +8635,7 @@ dependencies = [ "substrate-test-runtime", "substrate-test-runtime-client", "tokio", + "tokio-scoped", ] [[package]] @@ -11059,6 +11060,16 @@ dependencies = [ "webpki 0.22.0", ] +[[package]] +name = "tokio-scoped" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e4beb8ba13bc53ac53ce1d52b42f02e5d8060f0f42138862869beb769722b256" +dependencies = [ + "tokio", + "tokio-stream", +] + [[package]] name = "tokio-stream" version = "0.1.11" diff --git a/client/network/test/Cargo.toml b/client/network/test/Cargo.toml index 8368fa278712a..a6f1997f28364 100644 --- a/client/network/test/Cargo.toml +++ b/client/network/test/Cargo.toml @@ -14,6 +14,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] tokio = "1.22.0" +tokio-scoped = "0.2.0" async-trait = "0.1.57" futures = "0.3.21" futures-timer = "3.0.1" diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index b3653ac7c0f88..364f2430653e4 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -22,18 +22,11 @@ mod block_import; #[cfg(test)] mod sync; -use std::{ - collections::HashMap, - marker::PhantomData, - pin::Pin, - sync::Arc, - task::{Context as FutureContext, Poll}, - time::Duration, -}; +use std::{collections::HashMap, marker::PhantomData, pin::Pin, sync::Arc, time::Duration}; use futures::{future::BoxFuture, prelude::*}; use libp2p::{build_multiaddr, PeerId}; -use log::trace; +use log::{error, trace}; use parking_lot::Mutex; use sc_block_builder::{BlockBuilder, BlockBuilderProvider}; use sc_client_api::{ @@ -84,7 +77,7 @@ pub use substrate_test_runtime_client::{ runtime::{Block, Extrinsic, Hash, Transfer}, TestClient, TestClientBuilder, TestClientBuilderExt, }; -use tokio::time::timeout; +use tokio::{task::JoinSet, time::timeout}; type AuthorityId = sp_consensus_babe::AuthorityId; @@ -731,7 +724,7 @@ where { type Verifier: 'static + Verifier; type BlockImport: BlockImport + Clone + Send + Sync + 'static; - type PeerData: Default; + type PeerData: Default + Send; /// This one needs to be implemented! fn make_verifier(&self, client: PeersClient, peer_data: &Self::PeerData) -> Self::Verifier; @@ -743,6 +736,9 @@ where &mut self, closure: F, ); + async fn mut_peers_async(&mut self, async_closure: F) + where + F: FnOnce(&mut Vec>) -> BoxFuture<'_, ()> + Send; /// Get custom block import handle for fresh client, along with peer data. fn make_block_import( @@ -976,114 +972,106 @@ where tokio::spawn(f); } - /// Polls the testnet until all nodes are in sync. - /// - /// Must be executed in a task context. - fn poll_until_sync(&mut self, cx: &mut FutureContext) -> Poll<()> { - self.poll(cx); - - // Return `NotReady` if there's a mismatch in the highest block number. - let mut highest = None; - for peer in self.peers().iter() { - if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { - return Poll::Pending - } - if peer.network.num_sync_requests() != 0 { - return Poll::Pending - } - match (highest, peer.client.info().best_hash) { - (None, b) => highest = Some(b), - (Some(ref a), ref b) if a == b => {}, - (Some(_), _) => return Poll::Pending, + /// Runs the testnet until all nodes are in sync. + async fn run_until_sync_indefinitely(&mut self) { + 'outer: loop { + self.next_action().await; + + // Continue advancing if there's a mismatch in the highest block number. + let mut highest = None; + for peer in self.peers().iter() { + if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { + continue 'outer + } + if peer.network.num_sync_requests() != 0 { + continue 'outer + } + match (highest, peer.client.info().best_hash) { + (None, b) => highest = Some(b), + (Some(ref a), ref b) if a == b => {}, + (Some(_), _) => continue 'outer, + } } + break } - Poll::Ready(()) } - /// Polls the testnet until theres' no activiy of any kind. - /// - /// Must be executed in a task context. - fn poll_until_idle(&mut self, cx: &mut FutureContext) -> Poll<()> { - self.poll(cx); + /// Runs the testnet until theres' no activiy of any kind. + async fn run_until_idle(&mut self) { + 'outer: loop { + self.next_action().await; - for peer in self.peers().iter() { - if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { - return Poll::Pending - } - if peer.network.num_sync_requests() != 0 { - return Poll::Pending + for peer in self.peers().iter() { + if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { + continue 'outer + } + if peer.network.num_sync_requests() != 0 { + continue 'outer + } } - } - Poll::Ready(()) + break + } } - /// Polls the testnet until all peers are connected to each other. - /// - /// Must be executed in a task context. - fn poll_until_connected(&mut self, cx: &mut FutureContext) -> Poll<()> { - self.poll(cx); + /// Runs the testnet until all peers are connected to each other. + async fn run_until_connected(&mut self) { + loop { + self.next_action().await; - let num_peers = self.peers().len(); - if self.peers().iter().all(|p| p.num_peers() == num_peers - 1) { - return Poll::Ready(()) + let num_peers = self.peers().len(); + if self.peers().iter().all(|p| p.num_peers() == num_peers - 1) { + break + } } - - Poll::Pending } - /// Run the network until we are sync'ed. + /// Run the testnet until we are sync'ed (with a timeout). /// - /// Calls `poll_until_sync` repeatedly. + /// Calls `run_until_sync_indefinitely` with a timeout. /// (If we've not synced within 10 mins then panic rather than hang.) async fn run_until_sync(&mut self) { - timeout( - Duration::from_secs(10 * 60), - futures::future::poll_fn::<(), _>(|cx| self.poll_until_sync(cx)), - ) - .await - .expect("sync didn't happen within 10 mins"); - } - - /// Run the network until there are no pending packets. - /// - /// Calls `poll_until_idle` repeatedly with the runtime passed as parameter. - async fn run_until_idle(&mut self) { - futures::future::poll_fn::<(), _>(|cx| self.poll_until_idle(cx)).await; - } - - /// Run the network until all peers are connected to each other. - /// - /// Calls `poll_until_connected` repeatedly with the runtime passed as parameter. - async fn run_until_connected(&mut self) { - futures::future::poll_fn::<(), _>(|cx| self.poll_until_connected(cx)).await; - } - - /// Polls the testnet. Processes all the pending actions. - fn poll(&mut self, cx: &mut FutureContext) { - self.mut_peers(|peers| { - for (i, peer) in peers.iter_mut().enumerate() { - trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); - if let Poll::Ready(()) = peer.network.poll_unpin(cx) { - panic!("NetworkWorker has terminated unexpectedly.") - } - trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); - - // We poll `imported_blocks_stream`. - while let Poll::Ready(Some(notification)) = - peer.imported_blocks_stream.as_mut().poll_next(cx) - { - peer.network.service().announce_block(notification.hash, None); - } - - // We poll `finality_notification_stream`. - while let Poll::Ready(Some(notification)) = - peer.finality_notification_stream.as_mut().poll_next(cx) - { - peer.network.on_block_finalized(notification.hash, notification.header); - } - } - }); + timeout(Duration::from_secs(10 * 60), self.run_until_sync_indefinitely()) + .await + .expect("sync didn't happen within 10 mins"); + } + + /// Advances the testnet. Processes all the pending actions. + async fn next_action(&mut self) { + self.mut_peers_async(|peers| { + Box::pin(async move { + tokio_scoped::scope(|scope| { + for (i, peer) in peers.iter_mut().enumerate() { + scope.spawn({ + async move { + trace!(target: "sync", "-- Next action {}: {}", i, peer.id()); + + timeout(Duration::from_secs(1), peer.network.next_action()).await; + //peer.network.next_action().await; + trace!(target: "sync", "-- Next action complete {}: {}", i, peer.id()); + + // Get next element from `imported_blocks_stream` if it's available. + while let Some(Some(notification)) = + peer.imported_blocks_stream.next().now_or_never() + { + peer.network.service().announce_block(notification.hash, None); + } + + // Get next element from `finality_notification_stream` if it's + // available. + while let Some(Some(notification)) = + peer.finality_notification_stream.next().now_or_never() + { + peer.network + .on_block_finalized(notification.hash, notification.header); + } + } + }); + } + }) + }) + }) + .await; } } @@ -1092,6 +1080,7 @@ pub struct TestNet { peers: Vec>, } +#[async_trait::async_trait] impl TestNetFactory for TestNet { type Verifier = PassThroughVerifier; type PeerData = (); @@ -1123,6 +1112,13 @@ impl TestNetFactory for TestNet { fn mut_peers>)>(&mut self, closure: F) { closure(&mut self.peers); } + + async fn mut_peers_async(&mut self, closure: F) + where + F: FnOnce(&mut Vec>) -> BoxFuture<'_, ()> + Send, + { + closure(&mut self.peers).await + } } pub struct ForceFinalized(PeersClient); @@ -1150,6 +1146,7 @@ impl JustificationImport for ForceFinalized { #[derive(Default)] pub struct JustificationTestNet(TestNet); +#[async_trait::async_trait] impl TestNetFactory for JustificationTestNet { type Verifier = PassThroughVerifier; type PeerData = (); @@ -1174,6 +1171,13 @@ impl TestNetFactory for JustificationTestNet { self.0.mut_peers(closure) } + async fn mut_peers_async(&mut self, closure: F) + where + F: FnOnce(&mut Vec>) -> BoxFuture<'_, ()> + Send, + { + self.0.mut_peers_async(closure).await + } + fn make_block_import( &self, client: PeersClient, diff --git a/client/network/test/src/sync.rs b/client/network/test/src/sync.rs index b629574fe9874..913ec7eb4a971 100644 --- a/client/network/test/src/sync.rs +++ b/client/network/test/src/sync.rs @@ -44,16 +44,15 @@ async fn sync_peers_works() { sp_tracing::try_init_simple(); let mut net = TestNet::new(3); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; for peer in 0..3 { if net.peer(peer).num_peers() != 2 { - return Poll::Pending + continue } } - Poll::Ready(()) - }) - .await; + break + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -69,49 +68,44 @@ async fn sync_cycle_from_offline_to_syncing_to_offline() { // Generate blocks. net.peer(2).push_blocks(100, false); - // Block until all nodes are online and nodes 0 and 1 and major syncing. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + // Run until all nodes are online and nodes 0 and 1 are major syncing. + loop { + net.next_action().await; for peer in 0..3 { // Online if net.peer(peer).is_offline() { - return Poll::Pending + continue } if peer < 2 { // Major syncing. if net.peer(peer).blocks_count() < 100 && !net.peer(peer).is_major_syncing() { - return Poll::Pending + continue } } } - Poll::Ready(()) - }) - .await; + break + } - // Block until all nodes are done syncing. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + // Run until all nodes are done syncing. + loop { + net.next_action().await; for peer in 0..3 { if net.peer(peer).is_major_syncing() { - return Poll::Pending + continue } } - Poll::Ready(()) - }) - .await; + break + } // Now drop nodes 1 and 2, and check that node 0 is offline. net.peers.remove(2); net.peers.remove(1); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if !net.peer(0).is_offline() { - Poll::Pending - } else { - Poll::Ready(()) + loop { + net.next_action().await; + if net.peer(0).is_offline() { + break } - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -126,28 +120,22 @@ async fn syncing_node_not_major_syncing_when_disconnected() { assert!(!net.peer(1).is_major_syncing()); // Check that we switch to major syncing. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if !net.peer(1).is_major_syncing() { - Poll::Pending - } else { - Poll::Ready(()) + loop { + net.next_action().await; + if net.peer(1).is_major_syncing() { + break } - }) - .await; + } // Destroy two nodes, and check that we switch to non-major syncing. net.peers.remove(2); net.peers.remove(0); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if net.peer(0).is_major_syncing() { - Poll::Pending - } else { - Poll::Ready(()) + loop { + net.next_action().await; + if !net.peer(0).is_major_syncing() { + break } - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -233,15 +221,12 @@ async fn sync_no_common_longer_chain_fails() { let mut net = TestNet::new(3); net.peer(0).push_blocks(20, true); net.peer(1).push_blocks(20, false); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if net.peer(0).is_major_syncing() { - Poll::Pending - } else { - Poll::Ready(()) + loop { + net.next_action().await; + if !net.peer(0).is_major_syncing() { + break } - }) - .await; + } let peer1 = &net.peers()[1]; assert!(!net.peers()[0].blockchain_canon_equals(peer1)); } @@ -276,25 +261,24 @@ async fn sync_justifications() { net.peer(1).request_justification(&hashof15, 15); net.peer(1).request_justification(&hashof20, 20); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; for height in (10..21).step_by(5) { if net.peer(0).client().justifications(hashes[height - 1]).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { - return Poll::Pending + continue } if net.peer(1).client().justifications(hashes[height - 1]).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { - return Poll::Pending + continue } } - Poll::Ready(()) - }) - .await; + break + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -317,20 +301,17 @@ async fn sync_justifications_across_forks() { net.peer(1).request_justification(&f1_best, 10); net.peer(1).request_justification(&f2_best, 11); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; if net.peer(0).client().justifications(f1_best).unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) && net.peer(1).client().justifications(f1_best).unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) { - Poll::Ready(()) - } else { - Poll::Pending + break } - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -411,16 +392,13 @@ async fn can_sync_small_non_best_forks() { assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); assert!(net.peer(1).client().header(small_hash).unwrap().is_none()); - // poll until the two nodes connect, otherwise announcing the block will not work - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if net.peer(0).num_peers() == 0 { - Poll::Pending - } else { - Poll::Ready(()) + // Run until the two nodes connect, otherwise announcing the block will not work. + loop { + net.next_action().await; + if net.peer(0).num_peers() != 0 { + break } - }) - .await; + } // synchronization: 0 synced to longer chain and 1 didn't sync to small chain. @@ -433,28 +411,24 @@ async fn can_sync_small_non_best_forks() { // after announcing, peer 1 downloads the block. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); - if net.peer(1).client().header(small_hash).unwrap().is_none() { - return Poll::Pending + if net.peer(1).client().header(small_hash).unwrap().is_some() { + break } - Poll::Ready(()) - }) - .await; + } net.run_until_sync().await; let another_fork = net.peer(0).push_blocks_at(BlockId::Number(35), 2, true).pop().unwrap(); net.peer(0).announce_block(another_fork, None); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if net.peer(1).client().header(another_fork).unwrap().is_none() { - return Poll::Pending + loop { + net.next_action().await; + if net.peer(1).client().header(another_fork).unwrap().is_some() { + break } - Poll::Ready(()) - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -483,15 +457,13 @@ async fn can_sync_forks_ahead_of_the_best_chain() { assert_eq!(net.peer(1).client().info().best_number, 2); // after announcing, peer 1 downloads the block. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; - if net.peer(1).client().header(fork_hash).unwrap().is_none() { - return Poll::Pending + if net.peer(1).client().header(fork_hash).unwrap().is_some() { + break } - Poll::Ready(()) - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -515,16 +487,13 @@ async fn can_sync_explicit_forks() { assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); assert!(net.peer(1).client().header(small_hash).unwrap().is_none()); - // poll until the two nodes connect, otherwise announcing the block will not work - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if net.peer(0).num_peers() == 0 || net.peer(1).num_peers() == 0 { - Poll::Pending - } else { - Poll::Ready(()) + // Run until the two nodes connect, otherwise announcing the block will not work. + loop { + net.next_action().await; + if net.peer(0).num_peers() == 1 && net.peer(1).num_peers() == 1 { + break } - }) - .await; + } // synchronization: 0 synced to longer chain and 1 didn't sync to small chain. @@ -538,16 +507,14 @@ async fn can_sync_explicit_forks() { net.peer(1).set_sync_fork_request(vec![first_peer_id], small_hash, small_number); // peer 1 downloads the block. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); - if net.peer(1).client().header(small_hash).unwrap().is_none() { - return Poll::Pending + if net.peer(1).client().header(small_hash).unwrap().is_some() { + break } - Poll::Ready(()) - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -584,21 +551,13 @@ async fn does_not_sync_announced_old_best_block() { net.peer(1).push_blocks(20, true); net.peer(0).announce_block(old_hash, None); - futures::future::poll_fn::<(), _>(|cx| { - // poll once to import announcement - net.poll(cx); - Poll::Ready(()) - }) - .await; + // Drive network once to import announcement. + net.next_action().await; assert!(!net.peer(1).is_major_syncing()); net.peer(0).announce_block(old_hash_with_parent, None); - futures::future::poll_fn::<(), _>(|cx| { - // poll once to import announcement - net.poll(cx); - Poll::Ready(()) - }) - .await; + // Drive network once to import announcement. + net.next_action().await; assert!(!net.peer(1).is_major_syncing()); } @@ -610,15 +569,12 @@ async fn full_sync_requires_block_body() { net.peer(0).push_headers(1); // Wait for nodes to connect - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if net.peer(0).num_peers() == 0 || net.peer(1).num_peers() == 0 { - Poll::Pending - } else { - Poll::Ready(()) + loop { + net.next_action().await; + if net.peer(0).num_peers() == 1 && net.peer(1).num_peers() == 1 { + break } - }) - .await; + } net.run_until_idle().await; assert_eq!(net.peer(1).client.info().best_number, 0); } @@ -632,15 +588,12 @@ async fn imports_stale_once() { net.peer(0).announce_block(hash, None); net.peer(0).announce_block(hash, None); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; if net.peer(1).client().header(hash).unwrap().is_some() { - Poll::Ready(()) - } else { - Poll::Pending + break } - }) - .await; + } } // given the network with 2 full nodes @@ -829,15 +782,12 @@ async fn sync_to_tip_requires_that_sync_protocol_is_informed_about_best_block() ..Default::default() }); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; if net.peer(2).has_block(block_hash) { - Poll::Ready(()) - } else { - Poll::Pending + break } - }) - .await; + } // However peer 1 should still not have the block. assert!(!net.peer(1).has_block(block_hash)); @@ -914,18 +864,15 @@ async fn block_announce_data_is_propagated() { }); // Wait until peer 1 is connected to both nodes. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; if net.peer(1).num_peers() == 2 && net.peer(0).num_peers() == 1 && net.peer(2).num_peers() == 1 { - Poll::Ready(()) - } else { - Poll::Pending + break } - }) - .await; + } let block_hash = net .peer(0) @@ -1017,18 +964,15 @@ async fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() { .finalize_block(hashof10, Some((*b"FRNK", Vec::new())), true) .unwrap(); - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; - if net.peer(1).client().justifications(hashof10).unwrap() != + if net.peer(1).client().justifications(hashof10).unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) { - return Poll::Pending + break } - - Poll::Ready(()) - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -1045,14 +989,12 @@ async fn syncs_all_forks_from_single_peer() { let branch1 = net.peer(0).push_blocks_at(BlockId::Number(10), 2, true).pop().unwrap(); // Wait till peer 1 starts downloading - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); - if net.peer(1).network().best_seen_block() != Some(12) { - return Poll::Pending + loop { + net.next_action().await; + if net.peer(1).network().best_seen_block() == Some(12) { + break } - Poll::Ready(()) - }) - .await; + } // Peer 0 produces and announces another fork let branch2 = net.peer(0).push_blocks_at(BlockId::Number(10), 2, false).pop().unwrap(); @@ -1140,26 +1082,20 @@ async fn syncs_state() { let hashof60 = hashes[59]; net.peer(1).client().finalize_block(hashof60, Some(just), true).unwrap(); // Wait for state sync. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; if net.peer(1).client.info().finalized_state.is_some() { - Poll::Ready(()) - } else { - Poll::Pending + break } - }) - .await; + } assert!(!net.peer(1).client().has_state_at(&BlockId::Number(64))); // Wait for the rest of the states to be imported. - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; if net.peer(1).client().has_state_at(&BlockId::Number(64)) { - Poll::Ready(()) - } else { - Poll::Pending + break } - }) - .await; + } } } @@ -1238,15 +1174,12 @@ async fn warp_sync() { assert!(net.peer(3).client().has_state_at(&BlockId::Number(64))); // Wait for peer 1 download block history - futures::future::poll_fn::<(), _>(|cx| { - net.poll(cx); + loop { + net.next_action().await; if net.peer(3).has_body(gap_end) && net.peer(3).has_body(target) { - Poll::Ready(()) - } else { - Poll::Pending + break } - }) - .await; + } } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] From 1c87141484b6ac02cc2c137ad437cf64af3614c5 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 23 Jan 2023 17:10:15 +0300 Subject: [PATCH 03/35] Revert "Use `NetworkWorker::next_action` instead of `poll` in `sc-network-test`" This reverts commit 4b5d851ec864f78f9d083a18a618fbe117c896d2. --- Cargo.lock | 11 -- client/network/test/Cargo.toml | 1 - client/network/test/src/lib.rs | 210 +++++++++++---------- client/network/test/src/sync.rs | 313 +++++++++++++++++++------------- 4 files changed, 293 insertions(+), 242 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8328185442f19..e4e3e1b84625c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8635,7 +8635,6 @@ dependencies = [ "substrate-test-runtime", "substrate-test-runtime-client", "tokio", - "tokio-scoped", ] [[package]] @@ -11060,16 +11059,6 @@ dependencies = [ "webpki 0.22.0", ] -[[package]] -name = "tokio-scoped" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e4beb8ba13bc53ac53ce1d52b42f02e5d8060f0f42138862869beb769722b256" -dependencies = [ - "tokio", - "tokio-stream", -] - [[package]] name = "tokio-stream" version = "0.1.11" diff --git a/client/network/test/Cargo.toml b/client/network/test/Cargo.toml index a6f1997f28364..8368fa278712a 100644 --- a/client/network/test/Cargo.toml +++ b/client/network/test/Cargo.toml @@ -14,7 +14,6 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] tokio = "1.22.0" -tokio-scoped = "0.2.0" async-trait = "0.1.57" futures = "0.3.21" futures-timer = "3.0.1" diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 364f2430653e4..b3653ac7c0f88 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -22,11 +22,18 @@ mod block_import; #[cfg(test)] mod sync; -use std::{collections::HashMap, marker::PhantomData, pin::Pin, sync::Arc, time::Duration}; +use std::{ + collections::HashMap, + marker::PhantomData, + pin::Pin, + sync::Arc, + task::{Context as FutureContext, Poll}, + time::Duration, +}; use futures::{future::BoxFuture, prelude::*}; use libp2p::{build_multiaddr, PeerId}; -use log::{error, trace}; +use log::trace; use parking_lot::Mutex; use sc_block_builder::{BlockBuilder, BlockBuilderProvider}; use sc_client_api::{ @@ -77,7 +84,7 @@ pub use substrate_test_runtime_client::{ runtime::{Block, Extrinsic, Hash, Transfer}, TestClient, TestClientBuilder, TestClientBuilderExt, }; -use tokio::{task::JoinSet, time::timeout}; +use tokio::time::timeout; type AuthorityId = sp_consensus_babe::AuthorityId; @@ -724,7 +731,7 @@ where { type Verifier: 'static + Verifier; type BlockImport: BlockImport + Clone + Send + Sync + 'static; - type PeerData: Default + Send; + type PeerData: Default; /// This one needs to be implemented! fn make_verifier(&self, client: PeersClient, peer_data: &Self::PeerData) -> Self::Verifier; @@ -736,9 +743,6 @@ where &mut self, closure: F, ); - async fn mut_peers_async(&mut self, async_closure: F) - where - F: FnOnce(&mut Vec>) -> BoxFuture<'_, ()> + Send; /// Get custom block import handle for fresh client, along with peer data. fn make_block_import( @@ -972,106 +976,114 @@ where tokio::spawn(f); } - /// Runs the testnet until all nodes are in sync. - async fn run_until_sync_indefinitely(&mut self) { - 'outer: loop { - self.next_action().await; - - // Continue advancing if there's a mismatch in the highest block number. - let mut highest = None; - for peer in self.peers().iter() { - if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { - continue 'outer - } - if peer.network.num_sync_requests() != 0 { - continue 'outer - } - match (highest, peer.client.info().best_hash) { - (None, b) => highest = Some(b), - (Some(ref a), ref b) if a == b => {}, - (Some(_), _) => continue 'outer, - } + /// Polls the testnet until all nodes are in sync. + /// + /// Must be executed in a task context. + fn poll_until_sync(&mut self, cx: &mut FutureContext) -> Poll<()> { + self.poll(cx); + + // Return `NotReady` if there's a mismatch in the highest block number. + let mut highest = None; + for peer in self.peers().iter() { + if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { + return Poll::Pending + } + if peer.network.num_sync_requests() != 0 { + return Poll::Pending + } + match (highest, peer.client.info().best_hash) { + (None, b) => highest = Some(b), + (Some(ref a), ref b) if a == b => {}, + (Some(_), _) => return Poll::Pending, } - break } + Poll::Ready(()) } - /// Runs the testnet until theres' no activiy of any kind. - async fn run_until_idle(&mut self) { - 'outer: loop { - self.next_action().await; + /// Polls the testnet until theres' no activiy of any kind. + /// + /// Must be executed in a task context. + fn poll_until_idle(&mut self, cx: &mut FutureContext) -> Poll<()> { + self.poll(cx); - for peer in self.peers().iter() { - if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { - continue 'outer - } - if peer.network.num_sync_requests() != 0 { - continue 'outer - } + for peer in self.peers().iter() { + if peer.is_major_syncing() || peer.network.num_queued_blocks() != 0 { + return Poll::Pending + } + if peer.network.num_sync_requests() != 0 { + return Poll::Pending } - - break } + + Poll::Ready(()) } - /// Runs the testnet until all peers are connected to each other. - async fn run_until_connected(&mut self) { - loop { - self.next_action().await; + /// Polls the testnet until all peers are connected to each other. + /// + /// Must be executed in a task context. + fn poll_until_connected(&mut self, cx: &mut FutureContext) -> Poll<()> { + self.poll(cx); - let num_peers = self.peers().len(); - if self.peers().iter().all(|p| p.num_peers() == num_peers - 1) { - break - } + let num_peers = self.peers().len(); + if self.peers().iter().all(|p| p.num_peers() == num_peers - 1) { + return Poll::Ready(()) } + + Poll::Pending } - /// Run the testnet until we are sync'ed (with a timeout). + /// Run the network until we are sync'ed. /// - /// Calls `run_until_sync_indefinitely` with a timeout. + /// Calls `poll_until_sync` repeatedly. /// (If we've not synced within 10 mins then panic rather than hang.) async fn run_until_sync(&mut self) { - timeout(Duration::from_secs(10 * 60), self.run_until_sync_indefinitely()) - .await - .expect("sync didn't happen within 10 mins"); - } - - /// Advances the testnet. Processes all the pending actions. - async fn next_action(&mut self) { - self.mut_peers_async(|peers| { - Box::pin(async move { - tokio_scoped::scope(|scope| { - for (i, peer) in peers.iter_mut().enumerate() { - scope.spawn({ - async move { - trace!(target: "sync", "-- Next action {}: {}", i, peer.id()); - - timeout(Duration::from_secs(1), peer.network.next_action()).await; - //peer.network.next_action().await; - trace!(target: "sync", "-- Next action complete {}: {}", i, peer.id()); - - // Get next element from `imported_blocks_stream` if it's available. - while let Some(Some(notification)) = - peer.imported_blocks_stream.next().now_or_never() - { - peer.network.service().announce_block(notification.hash, None); - } - - // Get next element from `finality_notification_stream` if it's - // available. - while let Some(Some(notification)) = - peer.finality_notification_stream.next().now_or_never() - { - peer.network - .on_block_finalized(notification.hash, notification.header); - } - } - }); - } - }) - }) - }) - .await; + timeout( + Duration::from_secs(10 * 60), + futures::future::poll_fn::<(), _>(|cx| self.poll_until_sync(cx)), + ) + .await + .expect("sync didn't happen within 10 mins"); + } + + /// Run the network until there are no pending packets. + /// + /// Calls `poll_until_idle` repeatedly with the runtime passed as parameter. + async fn run_until_idle(&mut self) { + futures::future::poll_fn::<(), _>(|cx| self.poll_until_idle(cx)).await; + } + + /// Run the network until all peers are connected to each other. + /// + /// Calls `poll_until_connected` repeatedly with the runtime passed as parameter. + async fn run_until_connected(&mut self) { + futures::future::poll_fn::<(), _>(|cx| self.poll_until_connected(cx)).await; + } + + /// Polls the testnet. Processes all the pending actions. + fn poll(&mut self, cx: &mut FutureContext) { + self.mut_peers(|peers| { + for (i, peer) in peers.iter_mut().enumerate() { + trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); + if let Poll::Ready(()) = peer.network.poll_unpin(cx) { + panic!("NetworkWorker has terminated unexpectedly.") + } + trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); + + // We poll `imported_blocks_stream`. + while let Poll::Ready(Some(notification)) = + peer.imported_blocks_stream.as_mut().poll_next(cx) + { + peer.network.service().announce_block(notification.hash, None); + } + + // We poll `finality_notification_stream`. + while let Poll::Ready(Some(notification)) = + peer.finality_notification_stream.as_mut().poll_next(cx) + { + peer.network.on_block_finalized(notification.hash, notification.header); + } + } + }); } } @@ -1080,7 +1092,6 @@ pub struct TestNet { peers: Vec>, } -#[async_trait::async_trait] impl TestNetFactory for TestNet { type Verifier = PassThroughVerifier; type PeerData = (); @@ -1112,13 +1123,6 @@ impl TestNetFactory for TestNet { fn mut_peers>)>(&mut self, closure: F) { closure(&mut self.peers); } - - async fn mut_peers_async(&mut self, closure: F) - where - F: FnOnce(&mut Vec>) -> BoxFuture<'_, ()> + Send, - { - closure(&mut self.peers).await - } } pub struct ForceFinalized(PeersClient); @@ -1146,7 +1150,6 @@ impl JustificationImport for ForceFinalized { #[derive(Default)] pub struct JustificationTestNet(TestNet); -#[async_trait::async_trait] impl TestNetFactory for JustificationTestNet { type Verifier = PassThroughVerifier; type PeerData = (); @@ -1171,13 +1174,6 @@ impl TestNetFactory for JustificationTestNet { self.0.mut_peers(closure) } - async fn mut_peers_async(&mut self, closure: F) - where - F: FnOnce(&mut Vec>) -> BoxFuture<'_, ()> + Send, - { - self.0.mut_peers_async(closure).await - } - fn make_block_import( &self, client: PeersClient, diff --git a/client/network/test/src/sync.rs b/client/network/test/src/sync.rs index 913ec7eb4a971..b629574fe9874 100644 --- a/client/network/test/src/sync.rs +++ b/client/network/test/src/sync.rs @@ -44,15 +44,16 @@ async fn sync_peers_works() { sp_tracing::try_init_simple(); let mut net = TestNet::new(3); - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); for peer in 0..3 { if net.peer(peer).num_peers() != 2 { - continue + return Poll::Pending } } - break - } + Poll::Ready(()) + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -68,44 +69,49 @@ async fn sync_cycle_from_offline_to_syncing_to_offline() { // Generate blocks. net.peer(2).push_blocks(100, false); - // Run until all nodes are online and nodes 0 and 1 are major syncing. - loop { - net.next_action().await; + // Block until all nodes are online and nodes 0 and 1 and major syncing. + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); for peer in 0..3 { // Online if net.peer(peer).is_offline() { - continue + return Poll::Pending } if peer < 2 { // Major syncing. if net.peer(peer).blocks_count() < 100 && !net.peer(peer).is_major_syncing() { - continue + return Poll::Pending } } } - break - } + Poll::Ready(()) + }) + .await; - // Run until all nodes are done syncing. - loop { - net.next_action().await; + // Block until all nodes are done syncing. + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); for peer in 0..3 { if net.peer(peer).is_major_syncing() { - continue + return Poll::Pending } } - break - } + Poll::Ready(()) + }) + .await; // Now drop nodes 1 and 2, and check that node 0 is offline. net.peers.remove(2); net.peers.remove(1); - loop { - net.next_action().await; - if net.peer(0).is_offline() { - break + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if !net.peer(0).is_offline() { + Poll::Pending + } else { + Poll::Ready(()) } - } + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -120,22 +126,28 @@ async fn syncing_node_not_major_syncing_when_disconnected() { assert!(!net.peer(1).is_major_syncing()); // Check that we switch to major syncing. - loop { - net.next_action().await; - if net.peer(1).is_major_syncing() { - break + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if !net.peer(1).is_major_syncing() { + Poll::Pending + } else { + Poll::Ready(()) } - } + }) + .await; // Destroy two nodes, and check that we switch to non-major syncing. net.peers.remove(2); net.peers.remove(0); - loop { - net.next_action().await; - if !net.peer(0).is_major_syncing() { - break + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if net.peer(0).is_major_syncing() { + Poll::Pending + } else { + Poll::Ready(()) } - } + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -221,12 +233,15 @@ async fn sync_no_common_longer_chain_fails() { let mut net = TestNet::new(3); net.peer(0).push_blocks(20, true); net.peer(1).push_blocks(20, false); - loop { - net.next_action().await; - if !net.peer(0).is_major_syncing() { - break + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if net.peer(0).is_major_syncing() { + Poll::Pending + } else { + Poll::Ready(()) } - } + }) + .await; let peer1 = &net.peers()[1]; assert!(!net.peers()[0].blockchain_canon_equals(peer1)); } @@ -261,24 +276,25 @@ async fn sync_justifications() { net.peer(1).request_justification(&hashof15, 15); net.peer(1).request_justification(&hashof20, 20); - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); for height in (10..21).step_by(5) { if net.peer(0).client().justifications(hashes[height - 1]).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { - continue + return Poll::Pending } if net.peer(1).client().justifications(hashes[height - 1]).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { - continue + return Poll::Pending } } - break - } + Poll::Ready(()) + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -301,17 +317,20 @@ async fn sync_justifications_across_forks() { net.peer(1).request_justification(&f1_best, 10); net.peer(1).request_justification(&f2_best, 11); - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); if net.peer(0).client().justifications(f1_best).unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) && net.peer(1).client().justifications(f1_best).unwrap() == Some(Justifications::from((*b"FRNK", Vec::new()))) { - break + Poll::Ready(()) + } else { + Poll::Pending } - } + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -392,13 +411,16 @@ async fn can_sync_small_non_best_forks() { assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); assert!(net.peer(1).client().header(small_hash).unwrap().is_none()); - // Run until the two nodes connect, otherwise announcing the block will not work. - loop { - net.next_action().await; - if net.peer(0).num_peers() != 0 { - break + // poll until the two nodes connect, otherwise announcing the block will not work + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if net.peer(0).num_peers() == 0 { + Poll::Pending + } else { + Poll::Ready(()) } - } + }) + .await; // synchronization: 0 synced to longer chain and 1 didn't sync to small chain. @@ -411,24 +433,28 @@ async fn can_sync_small_non_best_forks() { // after announcing, peer 1 downloads the block. - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); - if net.peer(1).client().header(small_hash).unwrap().is_some() { - break + if net.peer(1).client().header(small_hash).unwrap().is_none() { + return Poll::Pending } - } + Poll::Ready(()) + }) + .await; net.run_until_sync().await; let another_fork = net.peer(0).push_blocks_at(BlockId::Number(35), 2, true).pop().unwrap(); net.peer(0).announce_block(another_fork, None); - loop { - net.next_action().await; - if net.peer(1).client().header(another_fork).unwrap().is_some() { - break + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if net.peer(1).client().header(another_fork).unwrap().is_none() { + return Poll::Pending } - } + Poll::Ready(()) + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -457,13 +483,15 @@ async fn can_sync_forks_ahead_of_the_best_chain() { assert_eq!(net.peer(1).client().info().best_number, 2); // after announcing, peer 1 downloads the block. - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); - if net.peer(1).client().header(fork_hash).unwrap().is_some() { - break + if net.peer(1).client().header(fork_hash).unwrap().is_none() { + return Poll::Pending } - } + Poll::Ready(()) + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -487,13 +515,16 @@ async fn can_sync_explicit_forks() { assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); assert!(net.peer(1).client().header(small_hash).unwrap().is_none()); - // Run until the two nodes connect, otherwise announcing the block will not work. - loop { - net.next_action().await; - if net.peer(0).num_peers() == 1 && net.peer(1).num_peers() == 1 { - break + // poll until the two nodes connect, otherwise announcing the block will not work + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if net.peer(0).num_peers() == 0 || net.peer(1).num_peers() == 0 { + Poll::Pending + } else { + Poll::Ready(()) } - } + }) + .await; // synchronization: 0 synced to longer chain and 1 didn't sync to small chain. @@ -507,14 +538,16 @@ async fn can_sync_explicit_forks() { net.peer(1).set_sync_fork_request(vec![first_peer_id], small_hash, small_number); // peer 1 downloads the block. - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); assert!(net.peer(0).client().header(small_hash).unwrap().is_some()); - if net.peer(1).client().header(small_hash).unwrap().is_some() { - break + if net.peer(1).client().header(small_hash).unwrap().is_none() { + return Poll::Pending } - } + Poll::Ready(()) + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -551,13 +584,21 @@ async fn does_not_sync_announced_old_best_block() { net.peer(1).push_blocks(20, true); net.peer(0).announce_block(old_hash, None); - // Drive network once to import announcement. - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + // poll once to import announcement + net.poll(cx); + Poll::Ready(()) + }) + .await; assert!(!net.peer(1).is_major_syncing()); net.peer(0).announce_block(old_hash_with_parent, None); - // Drive network once to import announcement. - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + // poll once to import announcement + net.poll(cx); + Poll::Ready(()) + }) + .await; assert!(!net.peer(1).is_major_syncing()); } @@ -569,12 +610,15 @@ async fn full_sync_requires_block_body() { net.peer(0).push_headers(1); // Wait for nodes to connect - loop { - net.next_action().await; - if net.peer(0).num_peers() == 1 && net.peer(1).num_peers() == 1 { - break + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if net.peer(0).num_peers() == 0 || net.peer(1).num_peers() == 0 { + Poll::Pending + } else { + Poll::Ready(()) } - } + }) + .await; net.run_until_idle().await; assert_eq!(net.peer(1).client.info().best_number, 0); } @@ -588,12 +632,15 @@ async fn imports_stale_once() { net.peer(0).announce_block(hash, None); net.peer(0).announce_block(hash, None); - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); if net.peer(1).client().header(hash).unwrap().is_some() { - break + Poll::Ready(()) + } else { + Poll::Pending } - } + }) + .await; } // given the network with 2 full nodes @@ -782,12 +829,15 @@ async fn sync_to_tip_requires_that_sync_protocol_is_informed_about_best_block() ..Default::default() }); - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); if net.peer(2).has_block(block_hash) { - break + Poll::Ready(()) + } else { + Poll::Pending } - } + }) + .await; // However peer 1 should still not have the block. assert!(!net.peer(1).has_block(block_hash)); @@ -864,15 +914,18 @@ async fn block_announce_data_is_propagated() { }); // Wait until peer 1 is connected to both nodes. - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); if net.peer(1).num_peers() == 2 && net.peer(0).num_peers() == 1 && net.peer(2).num_peers() == 1 { - break + Poll::Ready(()) + } else { + Poll::Pending } - } + }) + .await; let block_hash = net .peer(0) @@ -964,15 +1017,18 @@ async fn multiple_requests_are_accepted_as_long_as_they_are_not_fulfilled() { .finalize_block(hashof10, Some((*b"FRNK", Vec::new())), true) .unwrap(); - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); - if net.peer(1).client().justifications(hashof10).unwrap() == + if net.peer(1).client().justifications(hashof10).unwrap() != Some(Justifications::from((*b"FRNK", Vec::new()))) { - break + return Poll::Pending } - } + + Poll::Ready(()) + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -989,12 +1045,14 @@ async fn syncs_all_forks_from_single_peer() { let branch1 = net.peer(0).push_blocks_at(BlockId::Number(10), 2, true).pop().unwrap(); // Wait till peer 1 starts downloading - loop { - net.next_action().await; - if net.peer(1).network().best_seen_block() == Some(12) { - break + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); + if net.peer(1).network().best_seen_block() != Some(12) { + return Poll::Pending } - } + Poll::Ready(()) + }) + .await; // Peer 0 produces and announces another fork let branch2 = net.peer(0).push_blocks_at(BlockId::Number(10), 2, false).pop().unwrap(); @@ -1082,20 +1140,26 @@ async fn syncs_state() { let hashof60 = hashes[59]; net.peer(1).client().finalize_block(hashof60, Some(just), true).unwrap(); // Wait for state sync. - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); if net.peer(1).client.info().finalized_state.is_some() { - break + Poll::Ready(()) + } else { + Poll::Pending } - } + }) + .await; assert!(!net.peer(1).client().has_state_at(&BlockId::Number(64))); // Wait for the rest of the states to be imported. - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); if net.peer(1).client().has_state_at(&BlockId::Number(64)) { - break + Poll::Ready(()) + } else { + Poll::Pending } - } + }) + .await; } } @@ -1174,12 +1238,15 @@ async fn warp_sync() { assert!(net.peer(3).client().has_state_at(&BlockId::Number(64))); // Wait for peer 1 download block history - loop { - net.next_action().await; + futures::future::poll_fn::<(), _>(|cx| { + net.poll(cx); if net.peer(3).has_body(gap_end) && net.peer(3).has_body(target) { - break + Poll::Ready(()) + } else { + Poll::Pending } - } + }) + .await; } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] From dbd8ad910734a84691f8909ca828efe1fc71cbd3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 23 Jan 2023 17:24:45 +0300 Subject: [PATCH 04/35] Fix `sc-network-test` to poll `NetworkWorker::next_action` --- client/network/test/src/lib.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index b3653ac7c0f88..0da727bb61931 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -31,7 +31,7 @@ use std::{ time::Duration, }; -use futures::{future::BoxFuture, prelude::*}; +use futures::{future::BoxFuture, pin_mut, prelude::*}; use libp2p::{build_multiaddr, PeerId}; use log::trace; use parking_lot::Mutex; @@ -1064,8 +1064,12 @@ where self.mut_peers(|peers| { for (i, peer) in peers.iter_mut().enumerate() { trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); - if let Poll::Ready(()) = peer.network.poll_unpin(cx) { - panic!("NetworkWorker has terminated unexpectedly.") + loop { + let net_poll_future = peer.network.next_action(); + pin_mut!(net_poll_future); + if let Poll::Pending = net_poll_future.poll(cx) { + break + } } trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); From e8d0758c17cc7f09bbb7208e548fcde687e9bad3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 23 Jan 2023 18:02:37 +0300 Subject: [PATCH 05/35] Fix `sc_network::service` tests to poll `NetworkWorker::next_action` --- .../network/src/service/tests/chain_sync.rs | 30 ++++--------------- client/network/src/service/tests/mod.rs | 7 +++-- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/client/network/src/service/tests/chain_sync.rs b/client/network/src/service/tests/chain_sync.rs index 9d8463ff190a8..67bab1ce66d60 100644 --- a/client/network/src/service/tests/chain_sync.rs +++ b/client/network/src/service/tests/chain_sync.rs @@ -76,11 +76,7 @@ async fn normal_network_poll_no_peers() { .build(); // poll the network once - futures::future::poll_fn(|cx| { - let _ = network.network().poll_unpin(cx); - Poll::Ready(()) - }) - .await; + let _ = network.network().next_action().now_or_never(); } #[tokio::test] @@ -110,11 +106,7 @@ async fn request_justification() { // send "request justifiction" message and poll the network network.service().request_justification(&hash, number); - futures::future::poll_fn(|cx| { - let _ = network.network().poll_unpin(cx); - Poll::Ready(()) - }) - .await; + let _ = network.network().next_action().now_or_never(); } #[tokio::test] @@ -141,11 +133,7 @@ async fn clear_justification_requests() { // send "request justifiction" message and poll the network network.service().clear_justification_requests(); - futures::future::poll_fn(|cx| { - let _ = network.network().poll_unpin(cx); - Poll::Ready(()) - }) - .await; + let _ = network.network().next_action().now_or_never(); } #[tokio::test] @@ -180,11 +168,7 @@ async fn set_sync_fork_request() { // send "set sync fork request" message and poll the network network.service().set_sync_fork_request(copy_peers, hash, number); - futures::future::poll_fn(|cx| { - let _ = network.network().poll_unpin(cx); - Poll::Ready(()) - }) - .await; + let _ = network.network().next_action().now_or_never(); } #[tokio::test] @@ -225,11 +209,7 @@ async fn on_block_finalized() { // send "set sync fork request" message and poll the network network.network().on_block_finalized(hash, header); - futures::future::poll_fn(|cx| { - let _ = network.network().poll_unpin(cx); - Poll::Ready(()) - }) - .await; + let _ = network.network().next_action().now_or_never(); } // report from mock import queue that importing a justification was not successful diff --git a/client/network/src/service/tests/mod.rs b/client/network/src/service/tests/mod.rs index 9c97a7f73837d..74f9c6e4cf430 100644 --- a/client/network/src/service/tests/mod.rs +++ b/client/network/src/service/tests/mod.rs @@ -76,13 +76,14 @@ impl TestNetwork { pub fn start_network( self, ) -> (Arc, (impl Stream + std::marker::Unpin)) { - let worker = self.network; + let mut worker = self.network; let service = worker.service().clone(); let event_stream = service.event_stream("test"); tokio::spawn(async move { - futures::pin_mut!(worker); - let _ = worker.await; + loop { + worker.next_action().await; + } }); (service, event_stream) From 3071b053dd45b20615bc246d74aba12c267b27c5 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 23 Jan 2023 18:32:44 +0300 Subject: [PATCH 06/35] Fix docs --- client/network/src/service.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index b760b8715c9d0..4a4bd611ab49a 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -19,13 +19,13 @@ //! Main entry point of the sc-network crate. //! //! There are two main structs in this module: [`NetworkWorker`] and [`NetworkService`]. -//! The [`NetworkWorker`] *is* the network and implements the `Future` trait. It must be polled in -//! order for the network to advance. +//! The [`NetworkWorker`] *is* the network. In order for the network to advance +//! the [`NetworkWorker::next_action`] must be called in a loop. //! The [`NetworkService`] is merely a shared version of the [`NetworkWorker`]. You can obtain an //! `Arc` by calling [`NetworkWorker::service`]. //! //! The methods of the [`NetworkService`] are implemented by sending a message over a channel, -//! which is then processed by [`NetworkWorker::poll`]. +//! which is then processed by [`NetworkWorker::next_action`]. use crate::{ behaviour::{self, Behaviour, BehaviourOut}, From d7be660b364c621857dd1c3ea718ddd27b3d3ec3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 23 Jan 2023 19:57:29 +0300 Subject: [PATCH 07/35] kick CI From 78cce1aa25101fd724eeea2fc81f7c6d8e81993a Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 3 Feb 2023 19:23:04 +0300 Subject: [PATCH 08/35] Factor out `next_worker_message()` & `next_swarm_event()` --- client/network/src/service.rs | 1046 ++++++++++++++++----------------- 1 file changed, 509 insertions(+), 537 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 37146e0c71a35..d43c03c761f3c 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -46,8 +46,9 @@ use libp2p::{ multiaddr, ping::Failure as PingFailure, swarm::{ - AddressScore, ConnectionError, ConnectionLimits, DialError, Executor, NetworkBehaviour, - PendingConnectionError, Swarm, SwarmBuilder, SwarmEvent, + AddressScore, ConnectionError, ConnectionHandler, ConnectionLimits, DialError, Executor, + IntoConnectionHandler, NetworkBehaviour, PendingConnectionError, Swarm, SwarmBuilder, + SwarmEvent, }, Multiaddr, PeerId, }; @@ -99,6 +100,12 @@ mod tests; pub use libp2p::identity::{error::DecodingError, Keypair, PublicKey}; use sc_network_common::service::{NetworkBlock, NetworkRequest}; +/// Custom error that can be produced by the [`ConnectionHandler`] of the [`NetworkBehaviour`]. +/// Used as a template parameter of [`SwarmEvent`] below. +type ConnectionHandlerErr = + <<::ConnectionHandler as IntoConnectionHandler> + ::Handler as ConnectionHandler>::Error; + /// Substrate network service. Handles network IO and manages connectivity. pub struct NetworkService { /// Number of peers we're connected to. @@ -1320,541 +1327,8 @@ where }; futures::select! { - msg = next_worker_msg.fuse() => { - // Process the next message coming from the `NetworkService`. - match msg { - ServiceToWorkerMsg::AnnounceBlock(hash, data) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .announce_block(hash, data), - ServiceToWorkerMsg::GetValue(key) => - self.network_service.behaviour_mut().get_value(key), - ServiceToWorkerMsg::PutValue(key, value) => - self.network_service.behaviour_mut().put_value(key, value), - ServiceToWorkerMsg::SetReservedOnly(reserved_only) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .set_reserved_only(reserved_only), - ServiceToWorkerMsg::SetReserved(peers) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .set_reserved_peers(peers), - ServiceToWorkerMsg::SetPeersetReserved(protocol, peers) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .set_reserved_peerset_peers(protocol, peers), - ServiceToWorkerMsg::AddReserved(peer_id) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .add_reserved_peer(peer_id), - ServiceToWorkerMsg::RemoveReserved(peer_id) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .remove_reserved_peer(peer_id), - ServiceToWorkerMsg::AddSetReserved(protocol, peer_id) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .add_set_reserved_peer(protocol, peer_id), - ServiceToWorkerMsg::RemoveSetReserved(protocol, peer_id) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .remove_set_reserved_peer(protocol, peer_id), - ServiceToWorkerMsg::AddKnownAddress(peer_id, addr) => - self.network_service.behaviour_mut().add_known_address(peer_id, addr), - ServiceToWorkerMsg::AddToPeersSet(protocol, peer_id) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .add_to_peers_set(protocol, peer_id), - ServiceToWorkerMsg::RemoveFromPeersSet(protocol, peer_id) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .remove_from_peers_set(protocol, peer_id), - ServiceToWorkerMsg::EventStream(sender) => self.event_streams.push(sender), - ServiceToWorkerMsg::Request { - target, - protocol, - request, - pending_response, - connect, - } => { - self.network_service.behaviour_mut().send_request( - &target, - &protocol, - request, - pending_response, - connect, - ); - }, - ServiceToWorkerMsg::NetworkStatus { pending_response } => { - let _ = pending_response.send(Ok(self.status())); - }, - ServiceToWorkerMsg::NetworkState { pending_response } => { - let _ = pending_response.send(Ok(self.network_state())); - }, - ServiceToWorkerMsg::DisconnectPeer(who, protocol_name) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .disconnect_peer(&who, protocol_name), - ServiceToWorkerMsg::NewBestBlockImported(hash, number) => self - .network_service - .behaviour_mut() - .user_protocol_mut() - .new_best_block_imported(hash, number), - } - }, - next_event = next_swarm_event.fuse() => { - // Process the next event coming from `Swarm`. - match next_event { - SwarmEvent::Behaviour(BehaviourOut::InboundRequest { - protocol, - result, - .. - }) => { - if let Some(metrics) = self.metrics.as_ref() { - match result { - Ok(serve_time) => { - metrics - .requests_in_success_total - .with_label_values(&[&protocol]) - .observe(serve_time.as_secs_f64()); - }, - Err(err) => { - let reason = match err { - ResponseFailure::Network(InboundFailure::Timeout) => - Some("timeout"), - ResponseFailure::Network( - InboundFailure::UnsupportedProtocols, - ) => - // `UnsupportedProtocols` is reported for every single - // inbound request whenever a request with an unsupported - // protocol is received. This is not reported in order to - // avoid confusions. - None, - ResponseFailure::Network(InboundFailure::ResponseOmission) => - Some("busy-omitted"), - ResponseFailure::Network(InboundFailure::ConnectionClosed) => - Some("connection-closed"), - }; - - if let Some(reason) = reason { - metrics - .requests_in_failure_total - .with_label_values(&[&protocol, reason]) - .inc(); - } - }, - } - } - }, - SwarmEvent::Behaviour(BehaviourOut::RequestFinished { - protocol, - duration, - result, - .. - }) => - if let Some(metrics) = self.metrics.as_ref() { - match result { - Ok(_) => { - metrics - .requests_out_success_total - .with_label_values(&[&protocol]) - .observe(duration.as_secs_f64()); - }, - Err(err) => { - let reason = match err { - RequestFailure::NotConnected => "not-connected", - RequestFailure::UnknownProtocol => "unknown-protocol", - RequestFailure::Refused => "refused", - RequestFailure::Obsolete => "obsolete", - RequestFailure::Network(OutboundFailure::DialFailure) => - "dial-failure", - RequestFailure::Network(OutboundFailure::Timeout) => "timeout", - RequestFailure::Network(OutboundFailure::ConnectionClosed) => - "connection-closed", - RequestFailure::Network( - OutboundFailure::UnsupportedProtocols, - ) => "unsupported", - }; - - metrics - .requests_out_failure_total - .with_label_values(&[&protocol, reason]) - .inc(); - }, - } - }, - SwarmEvent::Behaviour(BehaviourOut::ReputationChanges { - peer, - changes, - }) => - for change in changes { - self.network_service.behaviour().user_protocol().report_peer(peer, change); - }, - SwarmEvent::Behaviour(BehaviourOut::PeerIdentify { - peer_id, - info: - IdentifyInfo { - protocol_version, - agent_version, - mut listen_addrs, - protocols, - .. - }, - }) => { - if listen_addrs.len() > 30 { - debug!( - target: "sub-libp2p", - "Node {:?} has reported more than 30 addresses; it is identified by {:?} and {:?}", - peer_id, protocol_version, agent_version - ); - listen_addrs.truncate(30); - } - for addr in listen_addrs { - self.network_service - .behaviour_mut() - .add_self_reported_address_to_dht(&peer_id, &protocols, addr); - } - self.network_service - .behaviour_mut() - .user_protocol_mut() - .add_default_set_discovered_nodes(iter::once(peer_id)); - }, - SwarmEvent::Behaviour(BehaviourOut::Discovered(peer_id)) => { - self.network_service - .behaviour_mut() - .user_protocol_mut() - .add_default_set_discovered_nodes(iter::once(peer_id)); - }, - SwarmEvent::Behaviour(BehaviourOut::RandomKademliaStarted) => - if let Some(metrics) = self.metrics.as_ref() { - metrics.kademlia_random_queries_total.inc(); - }, - SwarmEvent::Behaviour(BehaviourOut::NotificationStreamOpened { - remote, - protocol, - negotiated_fallback, - notifications_sink, - role, - }) => { - if let Some(metrics) = self.metrics.as_ref() { - metrics - .notifications_streams_opened_total - .with_label_values(&[&protocol]) - .inc(); - } - { - let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); - let _previous_value = peers_notifications_sinks - .insert((remote, protocol.clone()), notifications_sink); - debug_assert!(_previous_value.is_none()); - } - self.event_streams.send(Event::NotificationStreamOpened { - remote, - protocol, - negotiated_fallback, - role, - }); - }, - SwarmEvent::Behaviour(BehaviourOut::NotificationStreamReplaced { - remote, - protocol, - notifications_sink, - }) => { - let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); - if let Some(s) = peers_notifications_sinks.get_mut(&(remote, protocol)) { - *s = notifications_sink; - } else { - error!( - target: "sub-libp2p", - "NotificationStreamReplaced for non-existing substream" - ); - debug_assert!(false); - } - - // TODO: Notifications might have been lost as a result of the previous - // connection being dropped, and as a result it would be preferable to notify - // the users of this fact by simulating the substream being closed then - // reopened. - // The code below doesn't compile because `role` is unknown. Propagating the - // handshake of the secondary connections is quite an invasive change and - // would conflict with https://github.com/paritytech/substrate/issues/6403. - // Considering that dropping notifications is generally regarded as - // acceptable, this bug is at the moment intentionally left there and is - // intended to be fixed at the same time as - // https://github.com/paritytech/substrate/issues/6403. - // self.event_streams.send(Event::NotificationStreamClosed { - // remote, - // protocol, - // }); - // self.event_streams.send(Event::NotificationStreamOpened { - // remote, - // protocol, - // role, - // }); - }, - SwarmEvent::Behaviour(BehaviourOut::NotificationStreamClosed { - remote, - protocol, - }) => { - if let Some(metrics) = self.metrics.as_ref() { - metrics - .notifications_streams_closed_total - .with_label_values(&[&protocol[..]]) - .inc(); - } - self.event_streams.send(Event::NotificationStreamClosed { - remote, - protocol: protocol.clone(), - }); - { - let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); - let _previous_value = peers_notifications_sinks.remove(&(remote, protocol)); - debug_assert!(_previous_value.is_some()); - } - }, - SwarmEvent::Behaviour(BehaviourOut::NotificationsReceived { - remote, - messages, - }) => { - if let Some(metrics) = self.metrics.as_ref() { - for (protocol, message) in &messages { - metrics - .notifications_sizes - .with_label_values(&["in", protocol]) - .observe(message.len() as f64); - } - } - self.event_streams.send(Event::NotificationsReceived { remote, messages }); - }, - SwarmEvent::Behaviour(BehaviourOut::SyncConnected(remote)) => { - self.event_streams.send(Event::SyncConnected { remote }); - }, - SwarmEvent::Behaviour(BehaviourOut::SyncDisconnected(remote)) => { - self.event_streams.send(Event::SyncDisconnected { remote }); - }, - SwarmEvent::Behaviour(BehaviourOut::Dht(event, duration)) => { - if let Some(metrics) = self.metrics.as_ref() { - let query_type = match event { - DhtEvent::ValueFound(_) => "value-found", - DhtEvent::ValueNotFound(_) => "value-not-found", - DhtEvent::ValuePut(_) => "value-put", - DhtEvent::ValuePutFailed(_) => "value-put-failed", - }; - metrics - .kademlia_query_duration - .with_label_values(&[query_type]) - .observe(duration.as_secs_f64()); - } - - self.event_streams.send(Event::Dht(event)); - }, - SwarmEvent::Behaviour(BehaviourOut::None) => { - // Ignored event from lower layers. - }, - SwarmEvent::ConnectionEstablished { - peer_id, - endpoint, - num_established, - concurrent_dial_errors, - } => { - if let Some(errors) = concurrent_dial_errors { - debug!(target: "sub-libp2p", "Libp2p => Connected({:?}) with errors: {:?}", peer_id, errors); - } else { - debug!(target: "sub-libp2p", "Libp2p => Connected({:?})", peer_id); - } - - if let Some(metrics) = self.metrics.as_ref() { - let direction = match endpoint { - ConnectedPoint::Dialer { .. } => "out", - ConnectedPoint::Listener { .. } => "in", - }; - metrics.connections_opened_total.with_label_values(&[direction]).inc(); - - if num_established.get() == 1 { - metrics.distinct_peers_connections_opened_total.inc(); - } - } - }, - SwarmEvent::ConnectionClosed { - peer_id, - cause, - endpoint, - num_established, - } => { - debug!(target: "sub-libp2p", "Libp2p => Disconnected({:?}, {:?})", peer_id, cause); - if let Some(metrics) = self.metrics.as_ref() { - let direction = match endpoint { - ConnectedPoint::Dialer { .. } => "out", - ConnectedPoint::Listener { .. } => "in", - }; - let reason = match cause { - Some(ConnectionError::IO(_)) => "transport-error", - Some(ConnectionError::Handler(EitherError::A(EitherError::A( - EitherError::B(EitherError::A(PingFailure::Timeout)), - )))) => "ping-timeout", - Some(ConnectionError::Handler(EitherError::A(EitherError::A( - EitherError::A(NotifsHandlerError::SyncNotificationsClogged), - )))) => "sync-notifications-clogged", - Some(ConnectionError::Handler(_)) => "protocol-error", - Some(ConnectionError::KeepAliveTimeout) => "keep-alive-timeout", - None => "actively-closed", - }; - metrics - .connections_closed_total - .with_label_values(&[direction, reason]) - .inc(); - - // `num_established` represents the number of *remaining* connections. - if num_established == 0 { - metrics.distinct_peers_connections_closed_total.inc(); - } - } - }, - SwarmEvent::NewListenAddr { address, .. } => { - trace!(target: "sub-libp2p", "Libp2p => NewListenAddr({})", address); - if let Some(metrics) = self.metrics.as_ref() { - metrics.listeners_local_addresses.inc(); - } - }, - SwarmEvent::ExpiredListenAddr { address, .. } => { - info!(target: "sub-libp2p", "📪 No longer listening on {}", address); - if let Some(metrics) = self.metrics.as_ref() { - metrics.listeners_local_addresses.dec(); - } - }, - SwarmEvent::OutgoingConnectionError { peer_id, error } => { - if let Some(peer_id) = peer_id { - trace!( - target: "sub-libp2p", - "Libp2p => Failed to reach {:?}: {}", - peer_id, error, - ); - - if self.boot_node_ids.contains(&peer_id) { - if let DialError::WrongPeerId { obtained, endpoint } = &error { - if let ConnectedPoint::Dialer { address, role_override: _ } = - endpoint - { - warn!( - "💔 The bootnode you want to connect to at `{}` provided a different peer ID `{}` than the one you expect `{}`.", - address, - obtained, - peer_id, - ); - } - } - } - } - - if let Some(metrics) = self.metrics.as_ref() { - let reason = match error { - DialError::ConnectionLimit(_) => Some("limit-reached"), - DialError::InvalidPeerId(_) => Some("invalid-peer-id"), - DialError::Transport(_) | DialError::ConnectionIo(_) => - Some("transport-error"), - DialError::Banned | - DialError::LocalPeerId | - DialError::NoAddresses | - DialError::DialPeerConditionFalse(_) | - DialError::WrongPeerId { .. } | - DialError::Aborted => None, // ignore them - }; - if let Some(reason) = reason { - metrics - .pending_connections_errors_total - .with_label_values(&[reason]) - .inc(); - } - } - }, - SwarmEvent::Dialing(peer_id) => { - trace!(target: "sub-libp2p", "Libp2p => Dialing({:?})", peer_id) - }, - SwarmEvent::IncomingConnection { local_addr, send_back_addr } => { - trace!(target: "sub-libp2p", "Libp2p => IncomingConnection({},{}))", - local_addr, send_back_addr); - if let Some(metrics) = self.metrics.as_ref() { - metrics.incoming_connections_total.inc(); - } - }, - SwarmEvent::IncomingConnectionError { - local_addr, - send_back_addr, - error, - } => { - debug!( - target: "sub-libp2p", - "Libp2p => IncomingConnectionError({},{}): {}", - local_addr, send_back_addr, error, - ); - if let Some(metrics) = self.metrics.as_ref() { - let reason = match error { - PendingConnectionError::ConnectionLimit(_) => Some("limit-reached"), - PendingConnectionError::WrongPeerId { .. } => Some("invalid-peer-id"), - PendingConnectionError::Transport(_) | - PendingConnectionError::IO(_) => Some("transport-error"), - PendingConnectionError::Aborted => None, // ignore it - }; - - if let Some(reason) = reason { - metrics - .incoming_connections_errors_total - .with_label_values(&[reason]) - .inc(); - } - } - }, - SwarmEvent::BannedPeer { peer_id, endpoint } => { - debug!( - target: "sub-libp2p", - "Libp2p => BannedPeer({}). Connected via {:?}.", - peer_id, endpoint, - ); - if let Some(metrics) = self.metrics.as_ref() { - metrics - .incoming_connections_errors_total - .with_label_values(&["banned"]) - .inc(); - } - }, - SwarmEvent::ListenerClosed { reason, addresses, .. } => { - if let Some(metrics) = self.metrics.as_ref() { - metrics.listeners_local_addresses.sub(addresses.len() as u64); - } - let addrs = - addresses.into_iter().map(|a| a.to_string()).collect::>().join(", "); - match reason { - Ok(()) => error!( - target: "sub-libp2p", - "📪 Libp2p listener ({}) closed gracefully", - addrs - ), - Err(e) => error!( - target: "sub-libp2p", - "📪 Libp2p listener ({}) closed: {}", - addrs, e - ), - } - }, - SwarmEvent::ListenerError { error, .. } => { - debug!(target: "sub-libp2p", "Libp2p => ListenerError: {}", error); - if let Some(metrics) = self.metrics.as_ref() { - metrics.listeners_errors_total.inc(); - } - }, - } - } + msg = next_worker_msg.fuse() => self.handle_worker_message(msg), + next_event = next_swarm_event.fuse() => self.handle_swarm_event(next_event), }; let num_connected_peers = @@ -1908,6 +1382,504 @@ where ); } } + + /// Process the next message coming from the `NetworkService`. + fn handle_worker_message(&mut self, msg: ServiceToWorkerMsg) { + match msg { + ServiceToWorkerMsg::AnnounceBlock(hash, data) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .announce_block(hash, data), + ServiceToWorkerMsg::GetValue(key) => + self.network_service.behaviour_mut().get_value(key), + ServiceToWorkerMsg::PutValue(key, value) => + self.network_service.behaviour_mut().put_value(key, value), + ServiceToWorkerMsg::SetReservedOnly(reserved_only) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .set_reserved_only(reserved_only), + ServiceToWorkerMsg::SetReserved(peers) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .set_reserved_peers(peers), + ServiceToWorkerMsg::SetPeersetReserved(protocol, peers) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .set_reserved_peerset_peers(protocol, peers), + ServiceToWorkerMsg::AddReserved(peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .add_reserved_peer(peer_id), + ServiceToWorkerMsg::RemoveReserved(peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .remove_reserved_peer(peer_id), + ServiceToWorkerMsg::AddSetReserved(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .add_set_reserved_peer(protocol, peer_id), + ServiceToWorkerMsg::RemoveSetReserved(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .remove_set_reserved_peer(protocol, peer_id), + ServiceToWorkerMsg::AddKnownAddress(peer_id, addr) => + self.network_service.behaviour_mut().add_known_address(peer_id, addr), + ServiceToWorkerMsg::AddToPeersSet(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .add_to_peers_set(protocol, peer_id), + ServiceToWorkerMsg::RemoveFromPeersSet(protocol, peer_id) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .remove_from_peers_set(protocol, peer_id), + ServiceToWorkerMsg::EventStream(sender) => self.event_streams.push(sender), + ServiceToWorkerMsg::Request { + target, + protocol, + request, + pending_response, + connect, + } => { + self.network_service.behaviour_mut().send_request( + &target, + &protocol, + request, + pending_response, + connect, + ); + }, + ServiceToWorkerMsg::NetworkStatus { pending_response } => { + let _ = pending_response.send(Ok(self.status())); + }, + ServiceToWorkerMsg::NetworkState { pending_response } => { + let _ = pending_response.send(Ok(self.network_state())); + }, + ServiceToWorkerMsg::DisconnectPeer(who, protocol_name) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .disconnect_peer(&who, protocol_name), + ServiceToWorkerMsg::NewBestBlockImported(hash, number) => self + .network_service + .behaviour_mut() + .user_protocol_mut() + .new_best_block_imported(hash, number), + } + } + + /// Process the next event coming from `Swarm`. + fn handle_swarm_event( + &mut self, + event: SwarmEvent>>, + ) { + match event { + SwarmEvent::Behaviour(BehaviourOut::InboundRequest { protocol, result, .. }) => { + if let Some(metrics) = self.metrics.as_ref() { + match result { + Ok(serve_time) => { + metrics + .requests_in_success_total + .with_label_values(&[&protocol]) + .observe(serve_time.as_secs_f64()); + }, + Err(err) => { + let reason = match err { + ResponseFailure::Network(InboundFailure::Timeout) => + Some("timeout"), + ResponseFailure::Network(InboundFailure::UnsupportedProtocols) => + // `UnsupportedProtocols` is reported for every single + // inbound request whenever a request with an unsupported + // protocol is received. This is not reported in order to + // avoid confusions. + None, + ResponseFailure::Network(InboundFailure::ResponseOmission) => + Some("busy-omitted"), + ResponseFailure::Network(InboundFailure::ConnectionClosed) => + Some("connection-closed"), + }; + + if let Some(reason) = reason { + metrics + .requests_in_failure_total + .with_label_values(&[&protocol, reason]) + .inc(); + } + }, + } + } + }, + SwarmEvent::Behaviour(BehaviourOut::RequestFinished { + protocol, + duration, + result, + .. + }) => + if let Some(metrics) = self.metrics.as_ref() { + match result { + Ok(_) => { + metrics + .requests_out_success_total + .with_label_values(&[&protocol]) + .observe(duration.as_secs_f64()); + }, + Err(err) => { + let reason = match err { + RequestFailure::NotConnected => "not-connected", + RequestFailure::UnknownProtocol => "unknown-protocol", + RequestFailure::Refused => "refused", + RequestFailure::Obsolete => "obsolete", + RequestFailure::Network(OutboundFailure::DialFailure) => + "dial-failure", + RequestFailure::Network(OutboundFailure::Timeout) => "timeout", + RequestFailure::Network(OutboundFailure::ConnectionClosed) => + "connection-closed", + RequestFailure::Network(OutboundFailure::UnsupportedProtocols) => + "unsupported", + }; + + metrics + .requests_out_failure_total + .with_label_values(&[&protocol, reason]) + .inc(); + }, + } + }, + SwarmEvent::Behaviour(BehaviourOut::ReputationChanges { peer, changes }) => + for change in changes { + self.network_service.behaviour().user_protocol().report_peer(peer, change); + }, + SwarmEvent::Behaviour(BehaviourOut::PeerIdentify { + peer_id, + info: + IdentifyInfo { + protocol_version, agent_version, mut listen_addrs, protocols, .. + }, + }) => { + if listen_addrs.len() > 30 { + debug!( + target: "sub-libp2p", + "Node {:?} has reported more than 30 addresses; it is identified by {:?} and {:?}", + peer_id, protocol_version, agent_version + ); + listen_addrs.truncate(30); + } + for addr in listen_addrs { + self.network_service + .behaviour_mut() + .add_self_reported_address_to_dht(&peer_id, &protocols, addr); + } + self.network_service + .behaviour_mut() + .user_protocol_mut() + .add_default_set_discovered_nodes(iter::once(peer_id)); + }, + SwarmEvent::Behaviour(BehaviourOut::Discovered(peer_id)) => { + self.network_service + .behaviour_mut() + .user_protocol_mut() + .add_default_set_discovered_nodes(iter::once(peer_id)); + }, + SwarmEvent::Behaviour(BehaviourOut::RandomKademliaStarted) => + if let Some(metrics) = self.metrics.as_ref() { + metrics.kademlia_random_queries_total.inc(); + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationStreamOpened { + remote, + protocol, + negotiated_fallback, + notifications_sink, + role, + }) => { + if let Some(metrics) = self.metrics.as_ref() { + metrics + .notifications_streams_opened_total + .with_label_values(&[&protocol]) + .inc(); + } + { + let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); + let _previous_value = peers_notifications_sinks + .insert((remote, protocol.clone()), notifications_sink); + debug_assert!(_previous_value.is_none()); + } + self.event_streams.send(Event::NotificationStreamOpened { + remote, + protocol, + negotiated_fallback, + role, + }); + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationStreamReplaced { + remote, + protocol, + notifications_sink, + }) => { + let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); + if let Some(s) = peers_notifications_sinks.get_mut(&(remote, protocol)) { + *s = notifications_sink; + } else { + error!( + target: "sub-libp2p", + "NotificationStreamReplaced for non-existing substream" + ); + debug_assert!(false); + } + + // TODO: Notifications might have been lost as a result of the previous + // connection being dropped, and as a result it would be preferable to notify + // the users of this fact by simulating the substream being closed then + // reopened. + // The code below doesn't compile because `role` is unknown. Propagating the + // handshake of the secondary connections is quite an invasive change and + // would conflict with https://github.com/paritytech/substrate/issues/6403. + // Considering that dropping notifications is generally regarded as + // acceptable, this bug is at the moment intentionally left there and is + // intended to be fixed at the same time as + // https://github.com/paritytech/substrate/issues/6403. + // self.event_streams.send(Event::NotificationStreamClosed { + // remote, + // protocol, + // }); + // self.event_streams.send(Event::NotificationStreamOpened { + // remote, + // protocol, + // role, + // }); + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationStreamClosed { remote, protocol }) => { + if let Some(metrics) = self.metrics.as_ref() { + metrics + .notifications_streams_closed_total + .with_label_values(&[&protocol[..]]) + .inc(); + } + self.event_streams + .send(Event::NotificationStreamClosed { remote, protocol: protocol.clone() }); + { + let mut peers_notifications_sinks = self.peers_notifications_sinks.lock(); + let _previous_value = peers_notifications_sinks.remove(&(remote, protocol)); + debug_assert!(_previous_value.is_some()); + } + }, + SwarmEvent::Behaviour(BehaviourOut::NotificationsReceived { remote, messages }) => { + if let Some(metrics) = self.metrics.as_ref() { + for (protocol, message) in &messages { + metrics + .notifications_sizes + .with_label_values(&["in", protocol]) + .observe(message.len() as f64); + } + } + self.event_streams.send(Event::NotificationsReceived { remote, messages }); + }, + SwarmEvent::Behaviour(BehaviourOut::SyncConnected(remote)) => { + self.event_streams.send(Event::SyncConnected { remote }); + }, + SwarmEvent::Behaviour(BehaviourOut::SyncDisconnected(remote)) => { + self.event_streams.send(Event::SyncDisconnected { remote }); + }, + SwarmEvent::Behaviour(BehaviourOut::Dht(event, duration)) => { + if let Some(metrics) = self.metrics.as_ref() { + let query_type = match event { + DhtEvent::ValueFound(_) => "value-found", + DhtEvent::ValueNotFound(_) => "value-not-found", + DhtEvent::ValuePut(_) => "value-put", + DhtEvent::ValuePutFailed(_) => "value-put-failed", + }; + metrics + .kademlia_query_duration + .with_label_values(&[query_type]) + .observe(duration.as_secs_f64()); + } + + self.event_streams.send(Event::Dht(event)); + }, + SwarmEvent::Behaviour(BehaviourOut::None) => { + // Ignored event from lower layers. + }, + SwarmEvent::ConnectionEstablished { + peer_id, + endpoint, + num_established, + concurrent_dial_errors, + } => { + if let Some(errors) = concurrent_dial_errors { + debug!(target: "sub-libp2p", "Libp2p => Connected({:?}) with errors: {:?}", peer_id, errors); + } else { + debug!(target: "sub-libp2p", "Libp2p => Connected({:?})", peer_id); + } + + if let Some(metrics) = self.metrics.as_ref() { + let direction = match endpoint { + ConnectedPoint::Dialer { .. } => "out", + ConnectedPoint::Listener { .. } => "in", + }; + metrics.connections_opened_total.with_label_values(&[direction]).inc(); + + if num_established.get() == 1 { + metrics.distinct_peers_connections_opened_total.inc(); + } + } + }, + SwarmEvent::ConnectionClosed { peer_id, cause, endpoint, num_established } => { + debug!(target: "sub-libp2p", "Libp2p => Disconnected({:?}, {:?})", peer_id, cause); + if let Some(metrics) = self.metrics.as_ref() { + let direction = match endpoint { + ConnectedPoint::Dialer { .. } => "out", + ConnectedPoint::Listener { .. } => "in", + }; + let reason = match cause { + Some(ConnectionError::IO(_)) => "transport-error", + Some(ConnectionError::Handler(EitherError::A(EitherError::A( + EitherError::B(EitherError::A(PingFailure::Timeout)), + )))) => "ping-timeout", + Some(ConnectionError::Handler(EitherError::A(EitherError::A( + EitherError::A(NotifsHandlerError::SyncNotificationsClogged), + )))) => "sync-notifications-clogged", + Some(ConnectionError::Handler(_)) => "protocol-error", + Some(ConnectionError::KeepAliveTimeout) => "keep-alive-timeout", + None => "actively-closed", + }; + metrics.connections_closed_total.with_label_values(&[direction, reason]).inc(); + + // `num_established` represents the number of *remaining* connections. + if num_established == 0 { + metrics.distinct_peers_connections_closed_total.inc(); + } + } + }, + SwarmEvent::NewListenAddr { address, .. } => { + trace!(target: "sub-libp2p", "Libp2p => NewListenAddr({})", address); + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_local_addresses.inc(); + } + }, + SwarmEvent::ExpiredListenAddr { address, .. } => { + info!(target: "sub-libp2p", "📪 No longer listening on {}", address); + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_local_addresses.dec(); + } + }, + SwarmEvent::OutgoingConnectionError { peer_id, error } => { + if let Some(peer_id) = peer_id { + trace!( + target: "sub-libp2p", + "Libp2p => Failed to reach {:?}: {}", + peer_id, error, + ); + + if self.boot_node_ids.contains(&peer_id) { + if let DialError::WrongPeerId { obtained, endpoint } = &error { + if let ConnectedPoint::Dialer { address, role_override: _ } = endpoint { + warn!( + "💔 The bootnode you want to connect to at `{}` provided a different peer ID `{}` than the one you expect `{}`.", + address, + obtained, + peer_id, + ); + } + } + } + } + + if let Some(metrics) = self.metrics.as_ref() { + let reason = match error { + DialError::ConnectionLimit(_) => Some("limit-reached"), + DialError::InvalidPeerId(_) => Some("invalid-peer-id"), + DialError::Transport(_) | DialError::ConnectionIo(_) => + Some("transport-error"), + DialError::Banned | + DialError::LocalPeerId | + DialError::NoAddresses | + DialError::DialPeerConditionFalse(_) | + DialError::WrongPeerId { .. } | + DialError::Aborted => None, // ignore them + }; + if let Some(reason) = reason { + metrics.pending_connections_errors_total.with_label_values(&[reason]).inc(); + } + } + }, + SwarmEvent::Dialing(peer_id) => { + trace!(target: "sub-libp2p", "Libp2p => Dialing({:?})", peer_id) + }, + SwarmEvent::IncomingConnection { local_addr, send_back_addr } => { + trace!(target: "sub-libp2p", "Libp2p => IncomingConnection({},{}))", + local_addr, send_back_addr); + if let Some(metrics) = self.metrics.as_ref() { + metrics.incoming_connections_total.inc(); + } + }, + SwarmEvent::IncomingConnectionError { local_addr, send_back_addr, error } => { + debug!( + target: "sub-libp2p", + "Libp2p => IncomingConnectionError({},{}): {}", + local_addr, send_back_addr, error, + ); + if let Some(metrics) = self.metrics.as_ref() { + let reason = match error { + PendingConnectionError::ConnectionLimit(_) => Some("limit-reached"), + PendingConnectionError::WrongPeerId { .. } => Some("invalid-peer-id"), + PendingConnectionError::Transport(_) | PendingConnectionError::IO(_) => + Some("transport-error"), + PendingConnectionError::Aborted => None, // ignore it + }; + + if let Some(reason) = reason { + metrics + .incoming_connections_errors_total + .with_label_values(&[reason]) + .inc(); + } + } + }, + SwarmEvent::BannedPeer { peer_id, endpoint } => { + debug!( + target: "sub-libp2p", + "Libp2p => BannedPeer({}). Connected via {:?}.", + peer_id, endpoint, + ); + if let Some(metrics) = self.metrics.as_ref() { + metrics.incoming_connections_errors_total.with_label_values(&["banned"]).inc(); + } + }, + SwarmEvent::ListenerClosed { reason, addresses, .. } => { + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_local_addresses.sub(addresses.len() as u64); + } + let addrs = + addresses.into_iter().map(|a| a.to_string()).collect::>().join(", "); + match reason { + Ok(()) => error!( + target: "sub-libp2p", + "📪 Libp2p listener ({}) closed gracefully", + addrs + ), + Err(e) => error!( + target: "sub-libp2p", + "📪 Libp2p listener ({}) closed: {}", + addrs, e + ), + } + }, + SwarmEvent::ListenerError { error, .. } => { + debug!(target: "sub-libp2p", "Libp2p => ListenerError: {}", error); + if let Some(metrics) = self.metrics.as_ref() { + metrics.listeners_errors_total.inc(); + } + }, + } + } } impl Unpin for NetworkWorker From c9f75f7e063a6eee5569307021f0d69c3c0a4fda Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 6 Feb 2023 14:42:10 +0300 Subject: [PATCH 09/35] Error handling: replace `futures::pending!()` with `expect()` --- client/network/src/service.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index d43c03c761f3c..8a2e4375eaa21 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1310,19 +1310,12 @@ where } }; - // Next swarm event, or `future::pending()` if the stream has terminated - // (guaranteed to never happen with `Swarm`). + // Next swarm event. let next_swarm_event = { let swarm = &mut self.network_service; async move { - if let Some(event) = swarm.next().await { - event - } else { - loop { - futures::pending!(); - } - } + swarm.next().await.expect("`Swarm` event stream guaranteed to never terminate.") } }; From 4a85d1f58aa60699874cf1cd12f5acd71f92d37f Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 6 Feb 2023 15:11:45 +0300 Subject: [PATCH 10/35] Simplify stream polling in `select!` --- client/network/src/service.rs | 37 ++++++++++++----------------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 8a2e4375eaa21..55de8b62a5fea 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1295,33 +1295,20 @@ where { /// Perform one action on the network. pub async fn next_action(&mut self) { - // Next message from the service, or `future::pending()` if the stream has terminated. - let next_worker_msg = { - let from_service = &mut self.from_service; - - async move { - if let Some(msg) = from_service.next().await { - msg + futures::select! { + // Next message from the service. + msg = self.from_service.next().fuse() => { + if let Some(msg) = msg { + self.handle_worker_message(msg); } else { - loop { - futures::pending!(); - } + return } - } - }; - - // Next swarm event. - let next_swarm_event = { - let swarm = &mut self.network_service; - - async move { - swarm.next().await.expect("`Swarm` event stream guaranteed to never terminate.") - } - }; - - futures::select! { - msg = next_worker_msg.fuse() => self.handle_worker_message(msg), - next_event = next_swarm_event.fuse() => self.handle_swarm_event(next_event), + }, + // Next event from `Swarm`. + event = self.network_service.next().fuse() => { + let event = event.expect("`Swarm` event stream guaranteed to never terminate."); + self.handle_swarm_event(event); + }, }; let num_connected_peers = From c165185fbc2123baef0a9753b40a40ec5f2c3448 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 6 Feb 2023 15:37:07 +0300 Subject: [PATCH 11/35] Replace `NetworkWorker::next_action()` with `run()` --- client/network/src/service.rs | 11 +++++++++-- client/network/src/service/tests/chain_sync.rs | 10 +++++----- client/network/src/service/tests/mod.rs | 4 +--- client/network/test/src/lib.rs | 2 +- client/service/src/lib.rs | 6 ++---- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 55de8b62a5fea..9eaab19ad04d8 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1293,15 +1293,20 @@ where H: ExHashT, Client: HeaderBackend + 'static, { + /// Run the network. + pub async fn run(&mut self) { + while self.next_action().await {} + } + /// Perform one action on the network. - pub async fn next_action(&mut self) { + async fn next_action(&mut self) -> bool { futures::select! { // Next message from the service. msg = self.from_service.next().fuse() => { if let Some(msg) = msg { self.handle_worker_message(msg); } else { - return + return false } }, // Next event from `Swarm`. @@ -1361,6 +1366,8 @@ where as u64, ); } + + true } /// Process the next message coming from the `NetworkService`. diff --git a/client/network/src/service/tests/chain_sync.rs b/client/network/src/service/tests/chain_sync.rs index 67bab1ce66d60..ec79d21e3af33 100644 --- a/client/network/src/service/tests/chain_sync.rs +++ b/client/network/src/service/tests/chain_sync.rs @@ -76,7 +76,7 @@ async fn normal_network_poll_no_peers() { .build(); // poll the network once - let _ = network.network().next_action().now_or_never(); + let _ = network.network().run().now_or_never(); } #[tokio::test] @@ -106,7 +106,7 @@ async fn request_justification() { // send "request justifiction" message and poll the network network.service().request_justification(&hash, number); - let _ = network.network().next_action().now_or_never(); + let _ = network.network().run().now_or_never(); } #[tokio::test] @@ -133,7 +133,7 @@ async fn clear_justification_requests() { // send "request justifiction" message and poll the network network.service().clear_justification_requests(); - let _ = network.network().next_action().now_or_never(); + let _ = network.network().run().now_or_never(); } #[tokio::test] @@ -168,7 +168,7 @@ async fn set_sync_fork_request() { // send "set sync fork request" message and poll the network network.service().set_sync_fork_request(copy_peers, hash, number); - let _ = network.network().next_action().now_or_never(); + let _ = network.network().run().now_or_never(); } #[tokio::test] @@ -209,7 +209,7 @@ async fn on_block_finalized() { // send "set sync fork request" message and poll the network network.network().on_block_finalized(hash, header); - let _ = network.network().next_action().now_or_never(); + let _ = network.network().run().now_or_never(); } // report from mock import queue that importing a justification was not successful diff --git a/client/network/src/service/tests/mod.rs b/client/network/src/service/tests/mod.rs index 74f9c6e4cf430..94fbf3208fe96 100644 --- a/client/network/src/service/tests/mod.rs +++ b/client/network/src/service/tests/mod.rs @@ -81,9 +81,7 @@ impl TestNetwork { let event_stream = service.event_stream("test"); tokio::spawn(async move { - loop { - worker.next_action().await; - } + worker.run().await; }); (service, event_stream) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 0da727bb61931..9b3634bdff134 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1065,7 +1065,7 @@ where for (i, peer) in peers.iter_mut().enumerate() { trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); loop { - let net_poll_future = peer.network.next_action(); + let net_poll_future = peer.network.run(); pin_mut!(net_poll_future); if let Poll::Pending = net_poll_future.poll(cx) { break diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index e3b8416808b8c..5dd7fd2975f9d 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -286,10 +286,8 @@ async fn build_network_future< } } - // The network worker has done something. Nothing special to do, but could be - // used in the future to perform actions in response of things that happened on - // the network. - _ = network.next_action().fuse() => {} + // Drive the network. Shut down the network future if `NetworkWorker` has terminated. + _ = network.run().fuse() => return, } } } From 019ec35f728276c8949ead9b024a5ae6efa97256 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 6 Feb 2023 19:08:16 +0300 Subject: [PATCH 12/35] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/network/src/service.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 3ff59e25e8916..56af9f94cdf0e 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1302,7 +1302,7 @@ where async fn next_action(&mut self) -> bool { futures::select! { // Next message from the service. - msg = self.from_service.next().fuse() => { + msg = self.from_service.next() => { if let Some(msg) = msg { self.handle_worker_message(msg); } else { @@ -1310,8 +1310,7 @@ where } }, // Next event from `Swarm`. - event = self.network_service.next().fuse() => { - let event = event.expect("`Swarm` event stream guaranteed to never terminate."); + event = self.network_service.select_next_some() => { self.handle_swarm_event(event); }, }; From d38b2b7103c9f609b2b4bdd8a91c3071bdbbc79a Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 6 Feb 2023 19:10:18 +0300 Subject: [PATCH 13/35] minor: comment --- client/network/src/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 56af9f94cdf0e..304e51f832ea5 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1309,7 +1309,7 @@ where return false } }, - // Next event from `Swarm`. + // Next event from `Swarm` (the stream guaranteed to never terminate). event = self.network_service.select_next_some() => { self.handle_swarm_event(event); }, From dd4980dae54cf3d71591584e32b046c1bd1f67fd Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 7 Feb 2023 13:14:11 +0300 Subject: [PATCH 14/35] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/network/src/service.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 304e51f832ea5..69b646f2fc0ec 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1299,6 +1299,8 @@ where } /// Perform one action on the network. + /// + /// Returns `false` when the worker should be shutdown. async fn next_action(&mut self) -> bool { futures::select! { // Next message from the service. From 403dded4cd672b260e997a40139f28c0d3bb0e58 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 7 Feb 2023 20:22:18 +0300 Subject: [PATCH 15/35] Print debug log when network future is shut down --- client/service/src/lib.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 5dd7fd2975f9d..d1261d119de98 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -175,7 +175,10 @@ async fn build_network_future< Some(n) => n, // If this stream is shut down, that means the client has shut down, and the // most appropriate thing to do for the network future is to shut down too. - None => return, + None => { + debug!("Block import stream has terminated, shutting down the network future."); + return + }, }; if announce_imported_blocks { @@ -287,7 +290,10 @@ async fn build_network_future< } // Drive the network. Shut down the network future if `NetworkWorker` has terminated. - _ = network.run().fuse() => return, + _ = network.run().fuse() => { + debug!("`NetworkWorker` has terminated, shutting down the network future."); + return + }, } } } From 5a63842af39948c7193887c232aa45265c68f136 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 10 Feb 2023 21:16:12 +0300 Subject: [PATCH 16/35] Evaluate `NetworkWorker::run()` future once before the loop --- client/network/common/src/service.rs | 25 ++- client/network/src/service.rs | 82 ++++++++- client/network/test/src/lib.rs | 6 +- client/service/src/builder.rs | 22 ++- client/service/src/lib.rs | 244 ++++++++++++++++----------- 5 files changed, 255 insertions(+), 124 deletions(-) diff --git a/client/network/common/src/service.rs b/client/network/common/src/service.rs index 54d254eac384f..71425d219188e 100644 --- a/client/network/common/src/service.rs +++ b/client/network/common/src/service.rs @@ -180,13 +180,13 @@ pub trait NetworkPeers { /// purposes. fn deny_unreserved_peers(&self); - /// Adds a `PeerId` and its `Multiaddr` as reserved. + /// Adds a `PeerId` and its `Multiaddr` as reserved for a sync protocol (default peer set). /// /// Returns an `Err` if the given string is not a valid multiaddress /// or contains an invalid peer ID (which includes the local peer ID). fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String>; - /// Removes a `PeerId` from the list of reserved peers. + /// Removes a `PeerId` from the list of reserved peers for a sync protocol (default peer set). fn remove_reserved_peer(&self, peer_id: PeerId); /// Sets the reserved set of a protocol to the given set of peers. @@ -359,6 +359,9 @@ pub trait NetworkStateInfo { /// Returns the local external addresses. fn external_addresses(&self) -> Vec; + /// Returns the listening addresses (without trailing `/p2p/` with our `PeerId`). + fn listen_addresses(&self) -> Vec; + /// Returns the local Peer ID. fn local_peer_id(&self) -> PeerId; } @@ -372,6 +375,10 @@ where T::external_addresses(self) } + fn listen_addresses(&self) -> Vec { + T::listen_addresses(self) + } + fn local_peer_id(&self) -> PeerId { T::local_peer_id(self) } @@ -605,7 +612,7 @@ where } /// Provides ability to announce blocks to the network. -pub trait NetworkBlock { +pub trait NetworkBlock { /// Make sure an important block is propagated to peers. /// /// In chain-based consensus, we often need to make sure non-best forks are @@ -614,12 +621,16 @@ pub trait NetworkBlock { /// Inform the network service about new best imported block. fn new_best_block_imported(&self, hash: BlockHash, number: BlockNumber); + + /// Notify the network service (and underlying `NetworkSync`) about a finalized block. + fn on_block_finalized(&self, hash: BlockHash, header: BlockHeader); } -impl NetworkBlock for Arc +impl NetworkBlock + for Arc where T: ?Sized, - T: NetworkBlock, + T: NetworkBlock, { fn announce_block(&self, hash: BlockHash, data: Option>) { T::announce_block(self, hash, data) @@ -628,4 +639,8 @@ where fn new_best_block_imported(&self, hash: BlockHash, number: BlockNumber) { T::new_best_block_imported(self, hash, number) } + + fn on_block_finalized(&self, hash: BlockHash, header: BlockHeader) { + T::on_block_finalized(self, hash, header) + } } diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 69b646f2fc0ec..2b1bad0203f64 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -112,8 +112,12 @@ pub struct NetworkService { num_connected: Arc, /// The local external addresses. external_addresses: Arc>>, + /// Listen addresses. Do **NOT** include a trailing `/p2p/` with our `PeerId`. + listen_addresses: Arc>>, /// Are we actively catching up with the chain? is_major_syncing: Arc, + /// Best seen block number. + best_seen_block: Arc>>>, /// Local copy of the `PeerId` of the local node. local_peer_id: PeerId, /// The `KeyPair` that defines the `PeerId` of the local node. @@ -440,13 +444,17 @@ where } let external_addresses = Arc::new(Mutex::new(Vec::new())); + let listen_addresses = Arc::new(Mutex::new(Vec::new())); let peers_notifications_sinks = Arc::new(Mutex::new(HashMap::new())); + let best_seen_block = Arc::new(Mutex::new(None)); let service = Arc::new(NetworkService { bandwidth, external_addresses: external_addresses.clone(), + listen_addresses: listen_addresses.clone(), num_connected: num_connected.clone(), is_major_syncing: is_major_syncing.clone(), + best_seen_block: best_seen_block.clone(), peerset: peerset_handle, local_peer_id, local_identity, @@ -461,8 +469,10 @@ where Ok(NetworkWorker { external_addresses, + listen_addresses, num_connected, is_major_syncing, + best_seen_block, network_service: swarm, service, from_service, @@ -717,6 +727,35 @@ impl NetworkService { } } + /// Get currently connected peers. + pub async fn peers_debug_info(&self) -> Result)>, ()> { + let (tx, rx) = oneshot::channel(); + + let _ = self + .to_worker + .unbounded_send(ServiceToWorkerMsg::PeersDebugInfo { pending_response: tx }); + + // The channel can only be closed if the network worker no longer exists. + rx.await.map_err(|_| ()) + } + + /// Get the list of reserved peers. + pub async fn reserved_peers(&self) -> Result, ()> { + let (tx, rx) = oneshot::channel(); + + let _ = self + .to_worker + .unbounded_send(ServiceToWorkerMsg::ReservedPeers { pending_response: tx }); + + // The channel can only be closed if the network worker no longer exists. + rx.await.map_err(|_| ()) + } + + /// Get target sync block number. + pub fn best_seen_block(&self) -> Option> { + self.best_seen_block.lock().clone() + } + /// Utility function to extract `PeerId` from each `Multiaddr` for peer set updates. /// /// Returns an `Err` if one of the given addresses is invalid or contains an @@ -780,6 +819,11 @@ where self.external_addresses.lock().clone() } + /// Returns the listener addresses (without trailing `/p2p/` with our `PeerId`). + fn listen_addresses(&self) -> Vec { + self.listen_addresses.lock().clone() + } + /// Returns the local Peer ID. fn local_peer_id(&self) -> PeerId { self.local_peer_id @@ -1133,7 +1177,7 @@ where } } -impl NetworkBlock> for NetworkService +impl NetworkBlock, B::Header> for NetworkService where B: BlockT + 'static, H: ExHashT, @@ -1147,6 +1191,10 @@ where .to_worker .unbounded_send(ServiceToWorkerMsg::NewBestBlockImported(hash, number)); } + + fn on_block_finalized(&self, hash: B::Hash, header: B::Header) { + let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::BlockFinalized(hash, header)); + } } /// A `NotificationSender` allows for sending notifications to a peer with a chosen protocol. @@ -1249,6 +1297,13 @@ enum ServiceToWorkerMsg { }, DisconnectPeer(PeerId, ProtocolName), NewBestBlockImported(B::Hash, NumberFor), + BlockFinalized(B::Hash, B::Header), + PeersDebugInfo { + pending_response: oneshot::Sender)>>, + }, + ReservedPeers { + pending_response: oneshot::Sender>, + }, } /// Main network worker. Must be polled in order for the network to advance. @@ -1264,9 +1319,13 @@ where /// Updated by the `NetworkWorker` and loaded by the `NetworkService`. external_addresses: Arc>>, /// Updated by the `NetworkWorker` and loaded by the `NetworkService`. + listen_addresses: Arc>>, + /// Updated by the `NetworkWorker` and loaded by the `NetworkService`. num_connected: Arc, /// Updated by the `NetworkWorker` and loaded by the `NetworkService`. is_major_syncing: Arc, + /// Best seen block. + best_seen_block: Arc>>>, /// The network service that can be extracted and shared through the codebase. service: Arc>, /// The *actual* network. @@ -1324,11 +1383,15 @@ where self.num_connected.store(num_connected_peers, Ordering::Relaxed); { let external_addresses = - Swarm::>::external_addresses(&self.network_service) - .map(|r| &r.addr) - .cloned() - .collect(); + self.network_service.external_addresses().map(|r| &r.addr).cloned().collect(); *self.external_addresses.lock() = external_addresses; + + let listen_addresses = + self.network_service.listeners().map(ToOwned::to_owned).collect(); + *self.listen_addresses.lock() = listen_addresses; + + let best_seen_block = self.best_seen_block(); + *self.best_seen_block.lock() = best_seen_block; } let is_major_syncing = self @@ -1462,6 +1525,15 @@ where .behaviour_mut() .user_protocol_mut() .new_best_block_imported(hash, number), + ServiceToWorkerMsg::BlockFinalized(hash, header) => + self.on_block_finalized(hash, header), + ServiceToWorkerMsg::PeersDebugInfo { pending_response } => { + let _ = pending_response.send(self.peers_debug_info()); + }, + ServiceToWorkerMsg::ReservedPeers { pending_response } => { + let _ = + pending_response.send(self.reserved_peers().map(ToOwned::to_owned).collect()); + }, } } diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 9b3634bdff134..4df9ab6b8457f 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1064,12 +1064,10 @@ where self.mut_peers(|peers| { for (i, peer) in peers.iter_mut().enumerate() { trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); - loop { + { let net_poll_future = peer.network.run(); pin_mut!(net_poll_future); - if let Poll::Pending = net_poll_future.poll(cx) { - break - } + let _ = net_poll_future.poll(cx); } trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 0b09f550ce338..e1ad4022e8dc6 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -17,7 +17,7 @@ // along with this program. If not, see . use crate::{ - build_network_future, + build_network_future, build_system_rpc_future, client::{Client, ClientConfig}, config::{Configuration, KeystoreConfig, PrometheusConfig}, error::Error, @@ -965,16 +965,20 @@ where spawn_handle.spawn("import-queue", None, import_queue.run(Box::new(chain_sync_service))); let (system_rpc_tx, system_rpc_rx) = tracing_unbounded("mpsc_system_rpc", 10_000); - - let future = build_network_future( - config.role.clone(), - network_mut, - client, - system_rpc_rx, - has_bootnodes, - config.announce_block, + spawn_handle.spawn( + "system-rpc-handler", + Some("networking"), + build_system_rpc_future( + config.role.clone(), + network_mut.service().clone(), + client.clone(), + system_rpc_rx, + has_bootnodes, + ), ); + let future = build_network_future(network_mut, client, config.announce_block); + // TODO: Normally, one is supposed to pass a list of notifications protocols supported by the // node through the `NetworkConfiguration` struct. But because this function doesn't know in // advance which components, such as GrandPa or Polkadot, will be plugged on top of the diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index d1261d119de98..b3e352645d944 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -37,12 +37,15 @@ mod task_manager; use std::{collections::HashMap, net::SocketAddr}; use codec::{Decode, Encode}; -use futures::{channel::mpsc, FutureExt, StreamExt}; +use futures::{channel::mpsc, pin_mut, FutureExt, StreamExt}; use jsonrpsee::{core::Error as JsonRpseeError, RpcModule}; use log::{debug, error, warn}; use sc_client_api::{blockchain::HeaderBackend, BlockBackend, BlockchainEvents, ProofProvider}; -use sc_network::PeerId; -use sc_network_common::{config::MultiaddrWithPeerId, service::NetworkBlock}; +use sc_network::{NetworkStateInfo, PeerId}; +use sc_network_common::{ + config::MultiaddrWithPeerId, + service::{NetworkBlock, NetworkPeers}, +}; use sc_utils::mpsc::TracingUnboundedReceiver; use sp_blockchain::HeaderMetadata; use sp_consensus::SyncOracle; @@ -137,9 +140,7 @@ pub struct PartialComponents @@ -152,21 +153,20 @@ async fn build_network_future< + 'static, H: sc_network_common::ExHashT, >( - role: Role, mut network: sc_network::NetworkWorker, client: Arc, - mut rpc_rx: TracingUnboundedReceiver>, - should_have_peers: bool, announce_imported_blocks: bool, ) { let mut imported_blocks_stream = client.import_notification_stream().fuse(); - // Current best block at initialization, to report to the RPC layer. - let starting_block = client.info().best_number; - // Stream of finalized blocks reported by the client. let mut finality_notification_stream = client.finality_notification_stream().fuse(); + let network_service = network.service().clone(); + + let network_run = network.run().fuse(); + pin_mut!(network_run); + loop { futures::select! { // List of blocks that the client has imported. @@ -182,11 +182,11 @@ async fn build_network_future< }; if announce_imported_blocks { - network.service().announce_block(notification.hash, None); + network_service.announce_block(notification.hash, None); } if notification.is_new_best { - network.service().new_best_block_imported( + network_service.new_best_block_imported( notification.hash, *notification.header.number(), ); @@ -195,107 +195,149 @@ async fn build_network_future< // List of blocks that the client has finalized. notification = finality_notification_stream.select_next_some() => { - network.on_block_finalized(notification.hash, notification.header); + network_service.on_block_finalized(notification.hash, notification.header); } - // Answer incoming RPC requests. - request = rpc_rx.select_next_some() => { - match request { - sc_rpc::system::Request::Health(sender) => { - let _ = sender.send(sc_rpc::system::Health { - peers: network.peers_debug_info().len(), - is_syncing: network.service().is_major_syncing(), - should_have_peers, - }); - }, - sc_rpc::system::Request::LocalPeerId(sender) => { - let _ = sender.send(network.local_peer_id().to_base58()); - }, - sc_rpc::system::Request::LocalListenAddresses(sender) => { - let peer_id = (*network.local_peer_id()).into(); - let p2p_proto_suffix = sc_network::multiaddr::Protocol::P2p(peer_id); - let addresses = network.listen_addresses() - .map(|addr| addr.clone().with(p2p_proto_suffix.clone()).to_string()) - .collect(); - let _ = sender.send(addresses); - }, - sc_rpc::system::Request::Peers(sender) => { - let _ = sender.send(network.peers_debug_info().into_iter().map(|(peer_id, p)| - sc_rpc::system::PeerInfo { + // Drive the network. Shut down the network future if `NetworkWorker` has terminated. + _ = network_run => { + debug!("`NetworkWorker` has terminated, shutting down the network future."); + return + } + } + } +} + +/// Builds a future that processes system RPC requests. +async fn build_system_rpc_future< + B: BlockT, + C: BlockchainEvents + + HeaderBackend + + BlockBackend + + HeaderMetadata + + ProofProvider + + Send + + Sync + + 'static, + H: sc_network_common::ExHashT, +>( + role: Role, + network_service: Arc>, + client: Arc, + mut rpc_rx: TracingUnboundedReceiver>, + should_have_peers: bool, +) { + // Current best block at initialization, to report to the RPC layer. + let starting_block = client.info().best_number; + + loop { + // Answer incoming RPC requests. + match rpc_rx.select_next_some().await { + sc_rpc::system::Request::Health(sender) => { + let peers = network_service.peers_debug_info().await; + if let Ok(peers) = peers { + let _ = sender.send(sc_rpc::system::Health { + peers: peers.len(), + is_syncing: network_service.is_major_syncing(), + should_have_peers, + }); + } else { + break + } + }, + sc_rpc::system::Request::LocalPeerId(sender) => { + let _ = sender.send(network_service.local_peer_id().to_base58()); + }, + sc_rpc::system::Request::LocalListenAddresses(sender) => { + let peer_id = network_service.local_peer_id().into(); + let p2p_proto_suffix = sc_network::multiaddr::Protocol::P2p(peer_id); + let addresses = network_service + .listen_addresses() + .iter() + .map(|addr| addr.clone().with(p2p_proto_suffix.clone()).to_string()) + .collect(); + let _ = sender.send(addresses); + }, + sc_rpc::system::Request::Peers(sender) => { + let peers = network_service.peers_debug_info().await; + if let Ok(peers) = peers { + let _ = sender.send( + peers + .into_iter() + .map(|(peer_id, p)| sc_rpc::system::PeerInfo { peer_id: peer_id.to_base58(), roles: format!("{:?}", p.roles), best_hash: p.best_hash, best_number: p.best_number, - } - ).collect()); - } - sc_rpc::system::Request::NetworkState(sender) => { - if let Ok(network_state) = serde_json::to_value(&network.network_state()) { - let _ = sender.send(network_state); - } - } - sc_rpc::system::Request::NetworkAddReservedPeer(peer_addr, sender) => { - let result = match MultiaddrWithPeerId::try_from(peer_addr) { - Ok(peer) => { - network.add_reserved_peer(peer) - }, - Err(err) => { - Err(err.to_string()) - }, - }; - let x = result.map_err(sc_rpc::system::error::Error::MalformattedPeerArg); - let _ = sender.send(x); - } - sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender) => { - let _ = match peer_id.parse::() { - Ok(peer_id) => { - network.remove_reserved_peer(peer_id); - sender.send(Ok(())) - } - Err(e) => sender.send(Err(sc_rpc::system::error::Error::MalformattedPeerArg( - e.to_string(), - ))), - }; - } - sc_rpc::system::Request::NetworkReservedPeers(sender) => { - let reserved_peers = network.reserved_peers(); - let reserved_peers = reserved_peers - .map(|peer_id| peer_id.to_base58()) - .collect(); - - let _ = sender.send(reserved_peers); + }) + .collect(), + ); + } else { + break + } + }, + sc_rpc::system::Request::NetworkState(sender) => { + let network_state = network_service.network_state().await; + if let Ok(network_state) = network_state { + if let Ok(network_state) = serde_json::to_value(network_state) { + let _ = sender.send(network_state); } - sc_rpc::system::Request::NodeRoles(sender) => { - use sc_rpc::system::NodeRole; - - let node_role = match role { - Role::Authority { .. } => NodeRole::Authority, - Role::Full => NodeRole::Full, - }; + } else { + break + } + }, + sc_rpc::system::Request::NetworkAddReservedPeer(peer_addr, sender) => { + let result = match MultiaddrWithPeerId::try_from(peer_addr) { + Ok(peer) => network_service.add_reserved_peer(peer), + Err(err) => Err(err.to_string()), + }; + let x = result.map_err(sc_rpc::system::error::Error::MalformattedPeerArg); + let _ = sender.send(x); + }, + sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender) => { + let _ = match peer_id.parse::() { + Ok(peer_id) => { + network_service.remove_reserved_peer(peer_id); + sender.send(Ok(())) + }, + Err(e) => sender.send(Err(sc_rpc::system::error::Error::MalformattedPeerArg( + e.to_string(), + ))), + }; + }, + sc_rpc::system::Request::NetworkReservedPeers(sender) => { + let reserved_peers = network_service.reserved_peers().await; + if let Ok(reserved_peers) = reserved_peers { + let reserved_peers = + reserved_peers.iter().map(|peer_id| peer_id.to_base58()).collect(); + let _ = sender.send(reserved_peers); + } else { + break + } + }, + sc_rpc::system::Request::NodeRoles(sender) => { + use sc_rpc::system::NodeRole; - let _ = sender.send(vec![node_role]); - } - sc_rpc::system::Request::SyncState(sender) => { - use sc_rpc::system::SyncState; + let node_role = match role { + Role::Authority { .. } => NodeRole::Authority, + Role::Full => NodeRole::Full, + }; - let best_number = client.info().best_number; + let _ = sender.send(vec![node_role]); + }, + sc_rpc::system::Request::SyncState(sender) => { + use sc_rpc::system::SyncState; - let _ = sender.send(SyncState { - starting_block, - current_block: best_number, - highest_block: network.best_seen_block().unwrap_or(best_number), - }); - } - } - } + let best_number = client.info().best_number; - // Drive the network. Shut down the network future if `NetworkWorker` has terminated. - _ = network.run().fuse() => { - debug!("`NetworkWorker` has terminated, shutting down the network future."); - return + let _ = sender.send(SyncState { + starting_block, + current_block: best_number, + highest_block: network_service.best_seen_block().unwrap_or(best_number), + }); }, } } + debug!("`NetworkWorker` has terminated, shutting down the system RPC future.`"); } // Wrapper for HTTP and WS servers that makes sure they are properly shut down. From 8e1db2c9a30772085c6a71be9aa887fa03182080 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Sat, 11 Feb 2023 13:41:22 +0300 Subject: [PATCH 17/35] Fix client code to match new `NetworkService` interfaces --- client/authority-discovery/src/worker/tests.rs | 4 ++++ client/finality-grandpa/src/communication/mod.rs | 4 ++-- client/finality-grandpa/src/communication/tests.rs | 8 ++++++-- client/network-gossip/src/bridge.rs | 12 +++++++++++- client/network-gossip/src/lib.rs | 7 +++++-- client/network-gossip/src/state_machine.rs | 12 +++++++++++- client/network/test/src/lib.rs | 2 +- client/offchain/src/api.rs | 4 ++++ client/offchain/src/lib.rs | 4 ++++ 9 files changed, 48 insertions(+), 9 deletions(-) diff --git a/client/authority-discovery/src/worker/tests.rs b/client/authority-discovery/src/worker/tests.rs index ce55728a1bfa6..7f3d113a83448 100644 --- a/client/authority-discovery/src/worker/tests.rs +++ b/client/authority-discovery/src/worker/tests.rs @@ -184,6 +184,10 @@ impl NetworkStateInfo for TestNetwork { fn external_addresses(&self) -> Vec { self.external_addresses.clone() } + + fn listen_addresses(&self) -> Vec { + self.external_addresses.clone() + } } struct TestSigner<'a> { diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index e7e3c12989c96..af625e172ad8e 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -167,7 +167,7 @@ const TELEMETRY_VOTERS_LIMIT: usize = 10; /// well as the ability to set a fork sync request for a particular block. pub trait Network: NetworkSyncForkRequest> - + NetworkBlock> + + NetworkBlock, Block::Header> + GossipNetwork + Clone + Send @@ -179,7 +179,7 @@ impl Network for T where Block: BlockT, T: NetworkSyncForkRequest> - + NetworkBlock> + + NetworkBlock, Block::Header> + GossipNetwork + Clone + Send diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 839b2d52be651..9174120f6ff79 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -35,7 +35,7 @@ use sc_network_common::{ }, }; use sc_network_gossip::Validator; -use sc_network_test::{Block, Hash}; +use sc_network_test::{Block, Hash, Header}; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnboundedSender}; use sp_finality_grandpa::AuthorityList; use sp_keyring::Ed25519Keyring; @@ -155,7 +155,7 @@ impl NetworkNotification for TestNetwork { } } -impl NetworkBlock> for TestNetwork { +impl NetworkBlock, Header> for TestNetwork { fn announce_block(&self, hash: Hash, _data: Option>) { let _ = self.sender.unbounded_send(Event::Announce(hash)); } @@ -163,6 +163,10 @@ impl NetworkBlock> for TestNetwork { fn new_best_block_imported(&self, _hash: Hash, _number: NumberFor) { unimplemented!(); } + + fn on_block_finalized(&self, _hash: Hash, _header: Header) { + unimplemented!(); + } } impl NetworkSyncForkRequest> for TestNetwork { diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 5d7fd96c13e57..41b87f5a549e5 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -435,7 +435,9 @@ mod tests { } } - impl NetworkBlock<::Hash, NumberFor> for TestNetwork { + impl NetworkBlock<::Hash, NumberFor, ::Header> + for TestNetwork + { fn announce_block(&self, _hash: ::Hash, _data: Option>) { unimplemented!(); } @@ -447,6 +449,14 @@ mod tests { ) { unimplemented!(); } + + fn on_block_finalized( + &self, + _hash: ::Hash, + _header: ::Header, + ) { + unimplemented!(); + } } struct AllowAll; diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index 1c8fb8ba05ce7..20a737818b8a4 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -81,7 +81,10 @@ mod validator; /// Abstraction over a network. pub trait Network: - NetworkPeers + NetworkEventStream + NetworkNotification + NetworkBlock> + NetworkPeers + + NetworkEventStream + + NetworkNotification + + NetworkBlock, B::Header> { fn add_set_reserved(&self, who: PeerId, protocol: ProtocolName) { let addr = @@ -97,6 +100,6 @@ impl Network for T where T: NetworkPeers + NetworkEventStream + NetworkNotification - + NetworkBlock> + + NetworkBlock, B::Header> { } diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index 001f2c6136a00..fcd0ca5a7e3e6 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -677,7 +677,9 @@ mod tests { } } - impl NetworkBlock<::Hash, NumberFor> for NoOpNetwork { + impl NetworkBlock<::Hash, NumberFor, ::Header> + for NoOpNetwork + { fn announce_block(&self, _hash: ::Hash, _data: Option>) { unimplemented!(); } @@ -689,6 +691,14 @@ mod tests { ) { unimplemented!(); } + + fn on_block_finalized( + &self, + _hash: ::Hash, + _header: ::Header, + ) { + unimplemented!(); + } } #[test] diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 4df9ab6b8457f..d680b03b3a4cd 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -81,7 +81,7 @@ use sp_runtime::{ }; use substrate_test_runtime_client::AccountKeyring; pub use substrate_test_runtime_client::{ - runtime::{Block, Extrinsic, Hash, Transfer}, + runtime::{Block, Extrinsic, Hash, Header, Transfer}, TestClient, TestClientBuilder, TestClientBuilderExt, }; use tokio::time::timeout; diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs index 1301ce9fd9627..cd9f5c8f6feb9 100644 --- a/client/offchain/src/api.rs +++ b/client/offchain/src/api.rs @@ -419,6 +419,10 @@ mod tests { fn local_peer_id(&self) -> PeerId { PeerId::random() } + + fn listen_addresses(&self) -> Vec { + Vec::new() + } } fn offchain_api() -> (Api, AsyncApi) { diff --git a/client/offchain/src/lib.rs b/client/offchain/src/lib.rs index 6b28d3f8a48e9..6cf5838a46bed 100644 --- a/client/offchain/src/lib.rs +++ b/client/offchain/src/lib.rs @@ -270,6 +270,10 @@ mod tests { fn local_peer_id(&self) -> PeerId { PeerId::random() } + + fn listen_addresses(&self) -> Vec { + Vec::new() + } } impl NetworkPeers for TestNetwork { From faec2e9350a22d02e348bb23bc56a4df3d53cf01 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Sat, 11 Feb 2023 14:52:55 +0300 Subject: [PATCH 18/35] Make clippy happy --- client/network/src/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 2b1bad0203f64..0b987f1e48332 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -753,7 +753,7 @@ impl NetworkService { /// Get target sync block number. pub fn best_seen_block(&self) -> Option> { - self.best_seen_block.lock().clone() + *self.best_seen_block.lock() } /// Utility function to extract `PeerId` from each `Multiaddr` for peer set updates. From 9fa646d0ed613e5f8623d3d37d1d59ec0a535850 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 17:53:32 +0300 Subject: [PATCH 19/35] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/network/src/service/tests/mod.rs | 6 +++--- client/network/test/src/lib.rs | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/client/network/src/service/tests/mod.rs b/client/network/src/service/tests/mod.rs index 94fbf3208fe96..6f35a19aeb9d3 100644 --- a/client/network/src/service/tests/mod.rs +++ b/client/network/src/service/tests/mod.rs @@ -80,9 +80,9 @@ impl TestNetwork { let service = worker.service().clone(); let event_stream = service.event_stream("test"); - tokio::spawn(async move { - worker.run().await; - }); + tokio::spawn( + worker.run() + ); (service, event_stream) } diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index d680b03b3a4cd..77a83d10aeee8 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1065,9 +1065,8 @@ where for (i, peer) in peers.iter_mut().enumerate() { trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); { - let net_poll_future = peer.network.run(); - pin_mut!(net_poll_future); - let _ = net_poll_future.poll(cx); + peer.network.next_action().poll(cx); + } trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); From 9c5b610bbedc30c67d9882779d3ddddb490d69e8 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 17:54:38 +0300 Subject: [PATCH 20/35] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- client/network/src/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 0b987f1e48332..a8bce42841258 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1353,7 +1353,7 @@ where Client: HeaderBackend + 'static, { /// Run the network. - pub async fn run(&mut self) { + pub async fn run(mut self) { while self.next_action().await {} } From f3249376c8c3ff3e5ea9f0cf2867109f7b50df39 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 18:25:04 +0300 Subject: [PATCH 21/35] Revert "Apply suggestions from code review" This reverts commit 9fa646d0ed613e5f8623d3d37d1d59ec0a535850. --- client/network/test/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 77a83d10aeee8..d680b03b3a4cd 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1065,8 +1065,9 @@ where for (i, peer) in peers.iter_mut().enumerate() { trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); { - peer.network.next_action().poll(cx); - + let net_poll_future = peer.network.run(); + pin_mut!(net_poll_future); + let _ = net_poll_future.poll(cx); } trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); From 9856cc479839c529c633a2fa0ddc4a89d91f60bd Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 18:55:48 +0300 Subject: [PATCH 22/35] Make `NetworkWorker::run()` consume `self` --- client/network/src/service.rs | 3 ++- client/network/src/service/tests/chain_sync.rs | 16 ++++++++++------ client/network/src/service/tests/mod.rs | 8 ++++---- client/network/test/src/lib.rs | 2 +- client/service/src/lib.rs | 2 +- 5 files changed, 18 insertions(+), 13 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index a8bce42841258..9a6c2406ee1d5 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -1360,7 +1360,8 @@ where /// Perform one action on the network. /// /// Returns `false` when the worker should be shutdown. - async fn next_action(&mut self) -> bool { + /// Use in tests only. + pub async fn next_action(&mut self) -> bool { futures::select! { // Next message from the service. msg = self.from_service.next() => { diff --git a/client/network/src/service/tests/chain_sync.rs b/client/network/src/service/tests/chain_sync.rs index ec79d21e3af33..9bcf807f9b782 100644 --- a/client/network/src/service/tests/chain_sync.rs +++ b/client/network/src/service/tests/chain_sync.rs @@ -75,8 +75,8 @@ async fn normal_network_poll_no_peers() { .with_chain_sync((chain_sync, chain_sync_service)) .build(); - // poll the network once - let _ = network.network().run().now_or_never(); + // perform one action on network + let _ = network.network().next_action(); } #[tokio::test] @@ -106,7 +106,8 @@ async fn request_justification() { // send "request justifiction" message and poll the network network.service().request_justification(&hash, number); - let _ = network.network().run().now_or_never(); + // perform one action on network + let _ = network.network().next_action(); } #[tokio::test] @@ -133,7 +134,8 @@ async fn clear_justification_requests() { // send "request justifiction" message and poll the network network.service().clear_justification_requests(); - let _ = network.network().run().now_or_never(); + // perform one action on network + let _ = network.network().next_action(); } #[tokio::test] @@ -168,7 +170,8 @@ async fn set_sync_fork_request() { // send "set sync fork request" message and poll the network network.service().set_sync_fork_request(copy_peers, hash, number); - let _ = network.network().run().now_or_never(); + // perform one action on network + let _ = network.network().next_action(); } #[tokio::test] @@ -209,7 +212,8 @@ async fn on_block_finalized() { // send "set sync fork request" message and poll the network network.network().on_block_finalized(hash, header); - let _ = network.network().run().now_or_never(); + // perform one action on network + let _ = network.network().next_action(); } // report from mock import queue that importing a justification was not successful diff --git a/client/network/src/service/tests/mod.rs b/client/network/src/service/tests/mod.rs index 6f35a19aeb9d3..b4e15f4bc2671 100644 --- a/client/network/src/service/tests/mod.rs +++ b/client/network/src/service/tests/mod.rs @@ -76,13 +76,13 @@ impl TestNetwork { pub fn start_network( self, ) -> (Arc, (impl Stream + std::marker::Unpin)) { - let mut worker = self.network; + let worker = self.network; let service = worker.service().clone(); let event_stream = service.event_stream("test"); - tokio::spawn( - worker.run() - ); + tokio::spawn(async move { + worker.run().await; + }); (service, event_stream) } diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index d680b03b3a4cd..213b859de0146 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1065,7 +1065,7 @@ where for (i, peer) in peers.iter_mut().enumerate() { trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); { - let net_poll_future = peer.network.run(); + let net_poll_future = peer.network.next_action(); pin_mut!(net_poll_future); let _ = net_poll_future.poll(cx); } diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index b3e352645d944..22c6e88499c33 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -153,7 +153,7 @@ async fn build_network_future< + 'static, H: sc_network_common::ExHashT, >( - mut network: sc_network::NetworkWorker, + network: sc_network::NetworkWorker, client: Arc, announce_imported_blocks: bool, ) { From f86ec123305d4171beba9c3ffb73a49a75a80c86 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 19:02:21 +0300 Subject: [PATCH 23/35] Terminate system RPC future if RPC rx stream has terminated. --- client/service/src/lib.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 22c6e88499c33..0ea3605b2cbcb 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -231,8 +231,8 @@ async fn build_system_rpc_future< loop { // Answer incoming RPC requests. - match rpc_rx.select_next_some().await { - sc_rpc::system::Request::Health(sender) => { + match rpc_rx.next().await { + Some(sc_rpc::system::Request::Health(sender)) => { let peers = network_service.peers_debug_info().await; if let Ok(peers) = peers { let _ = sender.send(sc_rpc::system::Health { @@ -244,10 +244,10 @@ async fn build_system_rpc_future< break } }, - sc_rpc::system::Request::LocalPeerId(sender) => { + Some(sc_rpc::system::Request::LocalPeerId(sender)) => { let _ = sender.send(network_service.local_peer_id().to_base58()); }, - sc_rpc::system::Request::LocalListenAddresses(sender) => { + Some(sc_rpc::system::Request::LocalListenAddresses(sender)) => { let peer_id = network_service.local_peer_id().into(); let p2p_proto_suffix = sc_network::multiaddr::Protocol::P2p(peer_id); let addresses = network_service @@ -257,7 +257,7 @@ async fn build_system_rpc_future< .collect(); let _ = sender.send(addresses); }, - sc_rpc::system::Request::Peers(sender) => { + Some(sc_rpc::system::Request::Peers(sender)) => { let peers = network_service.peers_debug_info().await; if let Ok(peers) = peers { let _ = sender.send( @@ -275,7 +275,7 @@ async fn build_system_rpc_future< break } }, - sc_rpc::system::Request::NetworkState(sender) => { + Some(sc_rpc::system::Request::NetworkState(sender)) => { let network_state = network_service.network_state().await; if let Ok(network_state) = network_state { if let Ok(network_state) = serde_json::to_value(network_state) { @@ -285,7 +285,7 @@ async fn build_system_rpc_future< break } }, - sc_rpc::system::Request::NetworkAddReservedPeer(peer_addr, sender) => { + Some(sc_rpc::system::Request::NetworkAddReservedPeer(peer_addr, sender)) => { let result = match MultiaddrWithPeerId::try_from(peer_addr) { Ok(peer) => network_service.add_reserved_peer(peer), Err(err) => Err(err.to_string()), @@ -293,7 +293,7 @@ async fn build_system_rpc_future< let x = result.map_err(sc_rpc::system::error::Error::MalformattedPeerArg); let _ = sender.send(x); }, - sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender) => { + Some(sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender)) => { let _ = match peer_id.parse::() { Ok(peer_id) => { network_service.remove_reserved_peer(peer_id); @@ -304,7 +304,7 @@ async fn build_system_rpc_future< ))), }; }, - sc_rpc::system::Request::NetworkReservedPeers(sender) => { + Some(sc_rpc::system::Request::NetworkReservedPeers(sender)) => { let reserved_peers = network_service.reserved_peers().await; if let Ok(reserved_peers) = reserved_peers { let reserved_peers = @@ -314,7 +314,7 @@ async fn build_system_rpc_future< break } }, - sc_rpc::system::Request::NodeRoles(sender) => { + Some(sc_rpc::system::Request::NodeRoles(sender)) => { use sc_rpc::system::NodeRole; let node_role = match role { @@ -324,7 +324,7 @@ async fn build_system_rpc_future< let _ = sender.send(vec![node_role]); }, - sc_rpc::system::Request::SyncState(sender) => { + Some(sc_rpc::system::Request::SyncState(sender)) => { use sc_rpc::system::SyncState; let best_number = client.info().best_number; @@ -335,6 +335,8 @@ async fn build_system_rpc_future< highest_block: network_service.best_seen_block().unwrap_or(best_number), }); }, + None => + debug!("RPC requests stream has terminated, shutting down the system RPC future.`"), } } debug!("`NetworkWorker` has terminated, shutting down the system RPC future.`"); From 9557a42fac308e2bfe271a4f3d1b66c5b21d9a6a Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 19:08:17 +0300 Subject: [PATCH 24/35] Rewrite with let-else --- client/service/src/lib.rs | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 0ea3605b2cbcb..fac99b47049a2 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -231,8 +231,13 @@ async fn build_system_rpc_future< loop { // Answer incoming RPC requests. - match rpc_rx.next().await { - Some(sc_rpc::system::Request::Health(sender)) => { + let Some(req) = rpc_rx.next().await else { + debug!("RPC requests stream has terminated, shutting down the system RPC future.`"); + return; + }; + + match req { + sc_rpc::system::Request::Health(sender) => { let peers = network_service.peers_debug_info().await; if let Ok(peers) = peers { let _ = sender.send(sc_rpc::system::Health { @@ -244,10 +249,10 @@ async fn build_system_rpc_future< break } }, - Some(sc_rpc::system::Request::LocalPeerId(sender)) => { + sc_rpc::system::Request::LocalPeerId(sender) => { let _ = sender.send(network_service.local_peer_id().to_base58()); }, - Some(sc_rpc::system::Request::LocalListenAddresses(sender)) => { + sc_rpc::system::Request::LocalListenAddresses(sender) => { let peer_id = network_service.local_peer_id().into(); let p2p_proto_suffix = sc_network::multiaddr::Protocol::P2p(peer_id); let addresses = network_service @@ -257,7 +262,7 @@ async fn build_system_rpc_future< .collect(); let _ = sender.send(addresses); }, - Some(sc_rpc::system::Request::Peers(sender)) => { + sc_rpc::system::Request::Peers(sender) => { let peers = network_service.peers_debug_info().await; if let Ok(peers) = peers { let _ = sender.send( @@ -275,7 +280,7 @@ async fn build_system_rpc_future< break } }, - Some(sc_rpc::system::Request::NetworkState(sender)) => { + sc_rpc::system::Request::NetworkState(sender) => { let network_state = network_service.network_state().await; if let Ok(network_state) = network_state { if let Ok(network_state) = serde_json::to_value(network_state) { @@ -285,7 +290,7 @@ async fn build_system_rpc_future< break } }, - Some(sc_rpc::system::Request::NetworkAddReservedPeer(peer_addr, sender)) => { + sc_rpc::system::Request::NetworkAddReservedPeer(peer_addr, sender) => { let result = match MultiaddrWithPeerId::try_from(peer_addr) { Ok(peer) => network_service.add_reserved_peer(peer), Err(err) => Err(err.to_string()), @@ -293,7 +298,7 @@ async fn build_system_rpc_future< let x = result.map_err(sc_rpc::system::error::Error::MalformattedPeerArg); let _ = sender.send(x); }, - Some(sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender)) => { + sc_rpc::system::Request::NetworkRemoveReservedPeer(peer_id, sender) => { let _ = match peer_id.parse::() { Ok(peer_id) => { network_service.remove_reserved_peer(peer_id); @@ -304,7 +309,7 @@ async fn build_system_rpc_future< ))), }; }, - Some(sc_rpc::system::Request::NetworkReservedPeers(sender)) => { + sc_rpc::system::Request::NetworkReservedPeers(sender) => { let reserved_peers = network_service.reserved_peers().await; if let Ok(reserved_peers) = reserved_peers { let reserved_peers = @@ -314,7 +319,7 @@ async fn build_system_rpc_future< break } }, - Some(sc_rpc::system::Request::NodeRoles(sender)) => { + sc_rpc::system::Request::NodeRoles(sender) => { use sc_rpc::system::NodeRole; let node_role = match role { @@ -324,7 +329,7 @@ async fn build_system_rpc_future< let _ = sender.send(vec![node_role]); }, - Some(sc_rpc::system::Request::SyncState(sender)) => { + sc_rpc::system::Request::SyncState(sender) => { use sc_rpc::system::SyncState; let best_number = client.info().best_number; @@ -335,8 +340,6 @@ async fn build_system_rpc_future< highest_block: network_service.best_seen_block().unwrap_or(best_number), }); }, - None => - debug!("RPC requests stream has terminated, shutting down the system RPC future.`"), } } debug!("`NetworkWorker` has terminated, shutting down the system RPC future.`"); From 533cc041e015545c110a937cd753f0e2fe6769ab Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 19:11:57 +0300 Subject: [PATCH 25/35] Fix comments --- client/network/src/service.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 9a6c2406ee1d5..5bf36dd061c38 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -727,7 +727,9 @@ impl NetworkService { } } - /// Get currently connected peers. + /// Get connected peers debug information. + /// + /// Returns an error if the `NetworkWorker` is no longer running. pub async fn peers_debug_info(&self) -> Result)>, ()> { let (tx, rx) = oneshot::channel(); @@ -740,6 +742,8 @@ impl NetworkService { } /// Get the list of reserved peers. + /// + /// Returns an error if the `NetworkWorker` is no longer running. pub async fn reserved_peers(&self) -> Result, ()> { let (tx, rx) = oneshot::channel(); From e669c6a0440df1f0c56944fa952b7ec9fcc20f69 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 20:55:32 +0300 Subject: [PATCH 26/35] Get `best_seen_block` and call `on_block_finalized` via `ChainSync` instead of `NetworkService` --- .../finality-grandpa/src/communication/mod.rs | 4 ++-- .../src/communication/tests.rs | 8 ++----- client/network-gossip/src/bridge.rs | 10 +------- client/network-gossip/src/lib.rs | 4 ++-- client/network-gossip/src/state_machine.rs | 10 +------- client/network/common/src/service.rs | 13 +++------- client/network/src/lib.rs | 2 +- client/network/src/service.rs | 24 +------------------ client/network/sync/src/lib.rs | 6 +++++ client/network/sync/src/service/chain_sync.rs | 20 +++++++++++++++- client/service/src/builder.rs | 5 ++-- client/service/src/lib.rs | 16 +++++++++---- 12 files changed, 53 insertions(+), 69 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index af625e172ad8e..e7e3c12989c96 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -167,7 +167,7 @@ const TELEMETRY_VOTERS_LIMIT: usize = 10; /// well as the ability to set a fork sync request for a particular block. pub trait Network: NetworkSyncForkRequest> - + NetworkBlock, Block::Header> + + NetworkBlock> + GossipNetwork + Clone + Send @@ -179,7 +179,7 @@ impl Network for T where Block: BlockT, T: NetworkSyncForkRequest> - + NetworkBlock, Block::Header> + + NetworkBlock> + GossipNetwork + Clone + Send diff --git a/client/finality-grandpa/src/communication/tests.rs b/client/finality-grandpa/src/communication/tests.rs index 9174120f6ff79..839b2d52be651 100644 --- a/client/finality-grandpa/src/communication/tests.rs +++ b/client/finality-grandpa/src/communication/tests.rs @@ -35,7 +35,7 @@ use sc_network_common::{ }, }; use sc_network_gossip::Validator; -use sc_network_test::{Block, Hash, Header}; +use sc_network_test::{Block, Hash}; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnboundedSender}; use sp_finality_grandpa::AuthorityList; use sp_keyring::Ed25519Keyring; @@ -155,7 +155,7 @@ impl NetworkNotification for TestNetwork { } } -impl NetworkBlock, Header> for TestNetwork { +impl NetworkBlock> for TestNetwork { fn announce_block(&self, hash: Hash, _data: Option>) { let _ = self.sender.unbounded_send(Event::Announce(hash)); } @@ -163,10 +163,6 @@ impl NetworkBlock, Header> for TestNetwork { fn new_best_block_imported(&self, _hash: Hash, _number: NumberFor) { unimplemented!(); } - - fn on_block_finalized(&self, _hash: Hash, _header: Header) { - unimplemented!(); - } } impl NetworkSyncForkRequest> for TestNetwork { diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 41b87f5a549e5..77a4512b0d8ad 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -435,7 +435,7 @@ mod tests { } } - impl NetworkBlock<::Hash, NumberFor, ::Header> + impl NetworkBlock<::Hash, NumberFor> for TestNetwork { fn announce_block(&self, _hash: ::Hash, _data: Option>) { @@ -449,14 +449,6 @@ mod tests { ) { unimplemented!(); } - - fn on_block_finalized( - &self, - _hash: ::Hash, - _header: ::Header, - ) { - unimplemented!(); - } } struct AllowAll; diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index 20a737818b8a4..f357c50d566c6 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -84,7 +84,7 @@ pub trait Network: NetworkPeers + NetworkEventStream + NetworkNotification - + NetworkBlock, B::Header> + + NetworkBlock> { fn add_set_reserved(&self, who: PeerId, protocol: ProtocolName) { let addr = @@ -100,6 +100,6 @@ impl Network for T where T: NetworkPeers + NetworkEventStream + NetworkNotification - + NetworkBlock, B::Header> + + NetworkBlock> { } diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index fcd0ca5a7e3e6..8cc0ecca01ea2 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -677,7 +677,7 @@ mod tests { } } - impl NetworkBlock<::Hash, NumberFor, ::Header> + impl NetworkBlock<::Hash, NumberFor> for NoOpNetwork { fn announce_block(&self, _hash: ::Hash, _data: Option>) { @@ -691,14 +691,6 @@ mod tests { ) { unimplemented!(); } - - fn on_block_finalized( - &self, - _hash: ::Hash, - _header: ::Header, - ) { - unimplemented!(); - } } #[test] diff --git a/client/network/common/src/service.rs b/client/network/common/src/service.rs index 71425d219188e..89331a5b74fb6 100644 --- a/client/network/common/src/service.rs +++ b/client/network/common/src/service.rs @@ -612,7 +612,7 @@ where } /// Provides ability to announce blocks to the network. -pub trait NetworkBlock { +pub trait NetworkBlock { /// Make sure an important block is propagated to peers. /// /// In chain-based consensus, we often need to make sure non-best forks are @@ -621,16 +621,13 @@ pub trait NetworkBlock { /// Inform the network service about new best imported block. fn new_best_block_imported(&self, hash: BlockHash, number: BlockNumber); - - /// Notify the network service (and underlying `NetworkSync`) about a finalized block. - fn on_block_finalized(&self, hash: BlockHash, header: BlockHeader); } -impl NetworkBlock +impl NetworkBlock for Arc where T: ?Sized, - T: NetworkBlock, + T: NetworkBlock, { fn announce_block(&self, hash: BlockHash, data: Option>) { T::announce_block(self, hash, data) @@ -639,8 +636,4 @@ where fn new_best_block_imported(&self, hash: BlockHash, number: BlockNumber) { T::new_best_block_imported(self, hash, number) } - - fn on_block_finalized(&self, hash: BlockHash, header: BlockHeader) { - T::on_block_finalized(self, hash, header) - } } diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index f185458e0dace..65052306afe65 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -273,7 +273,7 @@ pub use sc_network_common::{ }, sync::{ warp::{WarpSyncPhase, WarpSyncProgress}, - StateDownloadProgress, SyncState, + StateDownloadProgress, SyncState, }, }; pub use service::{ diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 5bf36dd061c38..30ea2ad245caf 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -116,8 +116,6 @@ pub struct NetworkService { listen_addresses: Arc>>, /// Are we actively catching up with the chain? is_major_syncing: Arc, - /// Best seen block number. - best_seen_block: Arc>>>, /// Local copy of the `PeerId` of the local node. local_peer_id: PeerId, /// The `KeyPair` that defines the `PeerId` of the local node. @@ -446,7 +444,6 @@ where let external_addresses = Arc::new(Mutex::new(Vec::new())); let listen_addresses = Arc::new(Mutex::new(Vec::new())); let peers_notifications_sinks = Arc::new(Mutex::new(HashMap::new())); - let best_seen_block = Arc::new(Mutex::new(None)); let service = Arc::new(NetworkService { bandwidth, @@ -454,7 +451,6 @@ where listen_addresses: listen_addresses.clone(), num_connected: num_connected.clone(), is_major_syncing: is_major_syncing.clone(), - best_seen_block: best_seen_block.clone(), peerset: peerset_handle, local_peer_id, local_identity, @@ -472,7 +468,6 @@ where listen_addresses, num_connected, is_major_syncing, - best_seen_block, network_service: swarm, service, from_service, @@ -755,11 +750,6 @@ impl NetworkService { rx.await.map_err(|_| ()) } - /// Get target sync block number. - pub fn best_seen_block(&self) -> Option> { - *self.best_seen_block.lock() - } - /// Utility function to extract `PeerId` from each `Multiaddr` for peer set updates. /// /// Returns an `Err` if one of the given addresses is invalid or contains an @@ -1181,7 +1171,7 @@ where } } -impl NetworkBlock, B::Header> for NetworkService +impl NetworkBlock> for NetworkService where B: BlockT + 'static, H: ExHashT, @@ -1195,10 +1185,6 @@ where .to_worker .unbounded_send(ServiceToWorkerMsg::NewBestBlockImported(hash, number)); } - - fn on_block_finalized(&self, hash: B::Hash, header: B::Header) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::BlockFinalized(hash, header)); - } } /// A `NotificationSender` allows for sending notifications to a peer with a chosen protocol. @@ -1301,7 +1287,6 @@ enum ServiceToWorkerMsg { }, DisconnectPeer(PeerId, ProtocolName), NewBestBlockImported(B::Hash, NumberFor), - BlockFinalized(B::Hash, B::Header), PeersDebugInfo { pending_response: oneshot::Sender)>>, }, @@ -1328,8 +1313,6 @@ where num_connected: Arc, /// Updated by the `NetworkWorker` and loaded by the `NetworkService`. is_major_syncing: Arc, - /// Best seen block. - best_seen_block: Arc>>>, /// The network service that can be extracted and shared through the codebase. service: Arc>, /// The *actual* network. @@ -1394,9 +1377,6 @@ where let listen_addresses = self.network_service.listeners().map(ToOwned::to_owned).collect(); *self.listen_addresses.lock() = listen_addresses; - - let best_seen_block = self.best_seen_block(); - *self.best_seen_block.lock() = best_seen_block; } let is_major_syncing = self @@ -1530,8 +1510,6 @@ where .behaviour_mut() .user_protocol_mut() .new_best_block_imported(hash, number), - ServiceToWorkerMsg::BlockFinalized(hash, header) => - self.on_block_finalized(hash, header), ServiceToWorkerMsg::PeersDebugInfo { pending_response } => { let _ = pending_response.send(self.peers_debug_info()); }, diff --git a/client/network/sync/src/lib.rs b/client/network/sync/src/lib.rs index 312fc6f5b7947..4976f6c7c3588 100644 --- a/client/network/sync/src/lib.rs +++ b/client/network/sync/src/lib.rs @@ -1357,6 +1357,12 @@ where ); } }, + ToServiceCommand::BlockFinalized(hash, number) => { + self.on_block_finalized(&hash, number); + }, + ToServiceCommand::Status { pending_response } => { + let _ = pending_response.send(self.status()); + } } } self.process_outbound_requests(); diff --git a/client/network/sync/src/service/chain_sync.rs b/client/network/sync/src/service/chain_sync.rs index 50ded5b643dea..99cba9262cf5c 100644 --- a/client/network/sync/src/service/chain_sync.rs +++ b/client/network/sync/src/service/chain_sync.rs @@ -16,9 +16,10 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . +use futures::channel::oneshot; use libp2p::PeerId; use sc_consensus::{BlockImportError, BlockImportStatus, JustificationSyncLink, Link}; -use sc_network_common::service::NetworkSyncForkRequest; +use sc_network_common::{service::NetworkSyncForkRequest, sync::SyncStatus}; use sc_utils::mpsc::TracingUnboundedSender; use sp_runtime::traits::{Block as BlockT, NumberFor}; @@ -34,6 +35,8 @@ pub enum ToServiceCommand { Vec<(Result>, BlockImportError>, B::Hash)>, ), JustificationImported(PeerId, B::Hash, NumberFor, bool), + BlockFinalized(B::Hash, NumberFor), + Status { pending_response: oneshot::Sender> }, } /// Handle for communicating with `ChainSync` asynchronously @@ -47,6 +50,21 @@ impl ChainSyncInterfaceHandle { pub fn new(tx: TracingUnboundedSender>) -> Self { Self { tx } } + + /// Notify ChainSync about finalized block + pub fn on_block_finalized(&self, hash: B::Hash, number: NumberFor) { + let _ = self.tx.unbounded_send(ToServiceCommand::BlockFinalized(hash, number)); + } + + /// Get sync status + /// + /// Returns an error if `ChainSync` has terminated. + pub async fn status(&self) -> Result, ()> { + let (tx, rx) = oneshot::channel(); + let _ = self.tx.unbounded_send(ToServiceCommand::Status { pending_response: tx }); + + rx.await.map_err(|_| ()) + } } impl NetworkSyncForkRequest> diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index e1ad4022e8dc6..73a728437c46f 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -962,7 +962,7 @@ where Some("networking"), chain_sync_network_provider.run(network.clone()), ); - spawn_handle.spawn("import-queue", None, import_queue.run(Box::new(chain_sync_service))); + spawn_handle.spawn("import-queue", None, import_queue.run(Box::new(chain_sync_service.clone()))); let (system_rpc_tx, system_rpc_rx) = tracing_unbounded("mpsc_system_rpc", 10_000); spawn_handle.spawn( @@ -971,13 +971,14 @@ where build_system_rpc_future( config.role.clone(), network_mut.service().clone(), + chain_sync_service.clone(), client.clone(), system_rpc_rx, has_bootnodes, ), ); - let future = build_network_future(network_mut, client, config.announce_block); + let future = build_network_future(network_mut, client, chain_sync_service, config.announce_block); // TODO: Normally, one is supposed to pass a list of notifications protocols supported by the // node through the `NetworkConfiguration` struct. But because this function doesn't know in diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index fac99b47049a2..d280be962fb4e 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -46,6 +46,7 @@ use sc_network_common::{ config::MultiaddrWithPeerId, service::{NetworkBlock, NetworkPeers}, }; +use sc_network_sync::service::chain_sync::ChainSyncInterfaceHandle; use sc_utils::mpsc::TracingUnboundedReceiver; use sp_blockchain::HeaderMetadata; use sp_consensus::SyncOracle; @@ -155,6 +156,7 @@ async fn build_network_future< >( network: sc_network::NetworkWorker, client: Arc, + chain_sync_service: ChainSyncInterfaceHandle, announce_imported_blocks: bool, ) { let mut imported_blocks_stream = client.import_notification_stream().fuse(); @@ -195,7 +197,7 @@ async fn build_network_future< // List of blocks that the client has finalized. notification = finality_notification_stream.select_next_some() => { - network_service.on_block_finalized(notification.hash, notification.header); + chain_sync_service.on_block_finalized(notification.hash, notification.header.number().clone()); } // Drive the network. Shut down the network future if `NetworkWorker` has terminated. @@ -222,6 +224,7 @@ async fn build_system_rpc_future< >( role: Role, network_service: Arc>, + chain_sync_service: ChainSyncInterfaceHandle, client: Arc, mut rpc_rx: TracingUnboundedReceiver>, should_have_peers: bool, @@ -232,7 +235,7 @@ async fn build_system_rpc_future< loop { // Answer incoming RPC requests. let Some(req) = rpc_rx.next().await else { - debug!("RPC requests stream has terminated, shutting down the system RPC future.`"); + debug!("RPC requests stream has terminated, shutting down the system RPC future."); return; }; @@ -334,15 +337,20 @@ async fn build_system_rpc_future< let best_number = client.info().best_number; + let Ok(status) = chain_sync_service.status().await else { + debug!("`ChainSync` has terminated, shutting down the system RPC future."); + return + }; + let _ = sender.send(SyncState { starting_block, current_block: best_number, - highest_block: network_service.best_seen_block().unwrap_or(best_number), + highest_block: status.best_seen_block.unwrap_or(best_number), }); }, } } - debug!("`NetworkWorker` has terminated, shutting down the system RPC future.`"); + debug!("`NetworkWorker` has terminated, shutting down the system RPC future."); } // Wrapper for HTTP and WS servers that makes sure they are properly shut down. From 6ed5d48dce41918ed347eefd730f2ad66e6d99ce Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 20:59:15 +0300 Subject: [PATCH 27/35] rustfmt --- client/network-gossip/src/bridge.rs | 4 +--- client/network-gossip/src/lib.rs | 5 +---- client/network-gossip/src/state_machine.rs | 4 +--- client/network/common/src/service.rs | 3 +-- client/network/src/lib.rs | 2 +- client/network/sync/src/lib.rs | 2 +- client/network/sync/src/service/chain_sync.rs | 6 ++++-- client/service/src/builder.rs | 9 +++++++-- 8 files changed, 17 insertions(+), 18 deletions(-) diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 77a4512b0d8ad..5d7fd96c13e57 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -435,9 +435,7 @@ mod tests { } } - impl NetworkBlock<::Hash, NumberFor> - for TestNetwork - { + impl NetworkBlock<::Hash, NumberFor> for TestNetwork { fn announce_block(&self, _hash: ::Hash, _data: Option>) { unimplemented!(); } diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index f357c50d566c6..1c8fb8ba05ce7 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -81,10 +81,7 @@ mod validator; /// Abstraction over a network. pub trait Network: - NetworkPeers - + NetworkEventStream - + NetworkNotification - + NetworkBlock> + NetworkPeers + NetworkEventStream + NetworkNotification + NetworkBlock> { fn add_set_reserved(&self, who: PeerId, protocol: ProtocolName) { let addr = diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index 8cc0ecca01ea2..001f2c6136a00 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -677,9 +677,7 @@ mod tests { } } - impl NetworkBlock<::Hash, NumberFor> - for NoOpNetwork - { + impl NetworkBlock<::Hash, NumberFor> for NoOpNetwork { fn announce_block(&self, _hash: ::Hash, _data: Option>) { unimplemented!(); } diff --git a/client/network/common/src/service.rs b/client/network/common/src/service.rs index 89331a5b74fb6..f0f307800202f 100644 --- a/client/network/common/src/service.rs +++ b/client/network/common/src/service.rs @@ -623,8 +623,7 @@ pub trait NetworkBlock { fn new_best_block_imported(&self, hash: BlockHash, number: BlockNumber); } -impl NetworkBlock - for Arc +impl NetworkBlock for Arc where T: ?Sized, T: NetworkBlock, diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index 65052306afe65..f185458e0dace 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -273,7 +273,7 @@ pub use sc_network_common::{ }, sync::{ warp::{WarpSyncPhase, WarpSyncProgress}, - StateDownloadProgress, SyncState, + StateDownloadProgress, SyncState, }, }; pub use service::{ diff --git a/client/network/sync/src/lib.rs b/client/network/sync/src/lib.rs index 4976f6c7c3588..4cbc2da75c71e 100644 --- a/client/network/sync/src/lib.rs +++ b/client/network/sync/src/lib.rs @@ -1362,7 +1362,7 @@ where }, ToServiceCommand::Status { pending_response } => { let _ = pending_response.send(self.status()); - } + }, } } self.process_outbound_requests(); diff --git a/client/network/sync/src/service/chain_sync.rs b/client/network/sync/src/service/chain_sync.rs index 99cba9262cf5c..824303ec09d70 100644 --- a/client/network/sync/src/service/chain_sync.rs +++ b/client/network/sync/src/service/chain_sync.rs @@ -36,7 +36,9 @@ pub enum ToServiceCommand { ), JustificationImported(PeerId, B::Hash, NumberFor, bool), BlockFinalized(B::Hash, NumberFor), - Status { pending_response: oneshot::Sender> }, + Status { + pending_response: oneshot::Sender>, + }, } /// Handle for communicating with `ChainSync` asynchronously @@ -57,7 +59,7 @@ impl ChainSyncInterfaceHandle { } /// Get sync status - /// + /// /// Returns an error if `ChainSync` has terminated. pub async fn status(&self) -> Result, ()> { let (tx, rx) = oneshot::channel(); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 73a728437c46f..a18d23d10ba3b 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -962,7 +962,11 @@ where Some("networking"), chain_sync_network_provider.run(network.clone()), ); - spawn_handle.spawn("import-queue", None, import_queue.run(Box::new(chain_sync_service.clone()))); + spawn_handle.spawn( + "import-queue", + None, + import_queue.run(Box::new(chain_sync_service.clone())), + ); let (system_rpc_tx, system_rpc_rx) = tracing_unbounded("mpsc_system_rpc", 10_000); spawn_handle.spawn( @@ -978,7 +982,8 @@ where ), ); - let future = build_network_future(network_mut, client, chain_sync_service, config.announce_block); + let future = + build_network_future(network_mut, client, chain_sync_service, config.announce_block); // TODO: Normally, one is supposed to pass a list of notifications protocols supported by the // node through the `NetworkConfiguration` struct. But because this function doesn't know in From ab1ced4aafbea1ceacd7e9443817ef79ff0ef46f Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 13 Feb 2023 21:10:37 +0300 Subject: [PATCH 28/35] make clippy happy --- client/service/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index d280be962fb4e..708f43926f846 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -197,7 +197,7 @@ async fn build_network_future< // List of blocks that the client has finalized. notification = finality_notification_stream.select_next_some() => { - chain_sync_service.on_block_finalized(notification.hash, notification.header.number().clone()); + chain_sync_service.on_block_finalized(notification.hash, *notification.header.number()); } // Drive the network. Shut down the network future if `NetworkWorker` has terminated. From 4be62c7f9bd2a91faad2c579fbff0773415a4d0c Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 14 Feb 2023 12:07:00 +0300 Subject: [PATCH 29/35] Tests: schedule wake if `next_action()` returned true --- client/network/test/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 213b859de0146..8e294adb09b58 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1067,7 +1067,9 @@ where { let net_poll_future = peer.network.next_action(); pin_mut!(net_poll_future); - let _ = net_poll_future.poll(cx); + if let Poll::Ready(true) = net_poll_future.poll(cx) { + cx.waker().wake_by_ref(); + } } trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); From 5e90af05f8198e22213c9ffcc93a616640ead759 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 14 Feb 2023 12:10:11 +0300 Subject: [PATCH 30/35] minor: comment --- client/network/test/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 8e294adb09b58..e90c539dd4dfd 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1068,6 +1068,7 @@ where let net_poll_future = peer.network.next_action(); pin_mut!(net_poll_future); if let Poll::Ready(true) = net_poll_future.poll(cx) { + // Schedule wake if `next_action()` indicated that it must be called again. cx.waker().wake_by_ref(); } } From 113a8888441eb25fa872cd74a533679d99820ad0 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 14 Feb 2023 18:33:57 +0300 Subject: [PATCH 31/35] minor: fix `NetworkWorker` rustdoc --- client/network/src/service.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 30ea2ad245caf..19dc12620866a 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -19,8 +19,8 @@ //! Main entry point of the sc-network crate. //! //! There are two main structs in this module: [`NetworkWorker`] and [`NetworkService`]. -//! The [`NetworkWorker`] *is* the network. In order for the network to advance -//! the [`NetworkWorker::next_action`] must be called in a loop. +//! The [`NetworkWorker`] *is* the network. Network is driven by `NetworkWorker::run()` future that +//! terminates only when all instances of the control handles [`NetworkService`] were dropped. //! The [`NetworkService`] is merely a shared version of the [`NetworkWorker`]. You can obtain an //! `Arc` by calling [`NetworkWorker::service`]. //! From ac1452ce579bc5eb78b1f51620cfa90806a8bbe3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 14 Feb 2023 18:35:06 +0300 Subject: [PATCH 32/35] minor: amend the rustdoc --- client/network/src/service.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 19dc12620866a..3c8856eaf8eff 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -19,7 +19,7 @@ //! Main entry point of the sc-network crate. //! //! There are two main structs in this module: [`NetworkWorker`] and [`NetworkService`]. -//! The [`NetworkWorker`] *is* the network. Network is driven by `NetworkWorker::run()` future that +//! The [`NetworkWorker`] *is* the network. Network is driven by [`NetworkWorker::run`] future that //! terminates only when all instances of the control handles [`NetworkService`] were dropped. //! The [`NetworkService`] is merely a shared version of the [`NetworkWorker`]. You can obtain an //! `Arc` by calling [`NetworkWorker::service`]. From 5fc3831bec4a1008ea84421aef6127c53d57578f Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 15 Feb 2023 15:48:49 +0300 Subject: [PATCH 33/35] Fix bug that caused `on_demand_beefy_justification_sync` test to hang --- client/network/test/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index e90c539dd4dfd..344f9d7d749db 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -1064,12 +1064,15 @@ where self.mut_peers(|peers| { for (i, peer) in peers.iter_mut().enumerate() { trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); - { + loop { + // The code below is not quite correct, because we are polling a different + // instance of the future every time. But as long as `NetworkWorker::next_action()` + // contains just streams polling not interleaved with other `.await`s, dropping + // the future and recreating it works the same as polling a single instance. let net_poll_future = peer.network.next_action(); pin_mut!(net_poll_future); - if let Poll::Ready(true) = net_poll_future.poll(cx) { - // Schedule wake if `next_action()` indicated that it must be called again. - cx.waker().wake_by_ref(); + if let Poll::Pending = net_poll_future.poll(cx) { + break } } trace!(target: "sync", "-- Polling complete {}: {}", i, peer.id()); From 9df7d0ede90972725b469cf3455b801fba3df784 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 15 Feb 2023 15:52:21 +0300 Subject: [PATCH 34/35] rustfmt --- client/network/test/src/lib.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index a9b5c11e29ca6..c47e3c86f5c0e 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -31,7 +31,7 @@ use std::{ time::Duration, }; -use futures::{channel::oneshot, pin_mut, future::BoxFuture, prelude::*}; +use futures::{channel::oneshot, future::BoxFuture, pin_mut, prelude::*}; use libp2p::{build_multiaddr, PeerId}; use log::trace; use parking_lot::Mutex; @@ -1080,9 +1080,10 @@ where trace!(target: "sync", "-- Polling {}: {}", i, peer.id()); loop { // The code below is not quite correct, because we are polling a different - // instance of the future every time. But as long as `NetworkWorker::next_action()` - // contains just streams polling not interleaved with other `.await`s, dropping - // the future and recreating it works the same as polling a single instance. + // instance of the future every time. But as long as + // `NetworkWorker::next_action()` contains just streams polling not interleaved + // with other `.await`s, dropping the future and recreating it works the same as + // polling a single instance. let net_poll_future = peer.network.next_action(); pin_mut!(net_poll_future); if let Poll::Pending = net_poll_future.poll(cx) { From 812c316514dad4795249ce47d098f041be775377 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 17 Feb 2023 18:39:37 +0300 Subject: [PATCH 35/35] Apply review suggestions --- client/network/src/service/tests/chain_sync.rs | 10 +++++----- client/network/src/service/tests/mod.rs | 4 +--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/client/network/src/service/tests/chain_sync.rs b/client/network/src/service/tests/chain_sync.rs index 9bcf807f9b782..1ae432fd4c235 100644 --- a/client/network/src/service/tests/chain_sync.rs +++ b/client/network/src/service/tests/chain_sync.rs @@ -76,7 +76,7 @@ async fn normal_network_poll_no_peers() { .build(); // perform one action on network - let _ = network.network().next_action(); + let _ = network.network().next_action().await; } #[tokio::test] @@ -107,7 +107,7 @@ async fn request_justification() { network.service().request_justification(&hash, number); // perform one action on network - let _ = network.network().next_action(); + let _ = network.network().next_action().await; } #[tokio::test] @@ -135,7 +135,7 @@ async fn clear_justification_requests() { network.service().clear_justification_requests(); // perform one action on network - let _ = network.network().next_action(); + let _ = network.network().next_action().await; } #[tokio::test] @@ -171,7 +171,7 @@ async fn set_sync_fork_request() { network.service().set_sync_fork_request(copy_peers, hash, number); // perform one action on network - let _ = network.network().next_action(); + let _ = network.network().next_action().await; } #[tokio::test] @@ -213,7 +213,7 @@ async fn on_block_finalized() { network.network().on_block_finalized(hash, header); // perform one action on network - let _ = network.network().next_action(); + let _ = network.network().next_action().await; } // report from mock import queue that importing a justification was not successful diff --git a/client/network/src/service/tests/mod.rs b/client/network/src/service/tests/mod.rs index b4e15f4bc2671..3ce139ff03a33 100644 --- a/client/network/src/service/tests/mod.rs +++ b/client/network/src/service/tests/mod.rs @@ -80,9 +80,7 @@ impl TestNetwork { let service = worker.service().clone(); let event_stream = service.event_stream("test"); - tokio::spawn(async move { - worker.run().await; - }); + tokio::spawn(worker.run()); (service, event_stream) }