From 6e38b7ed8361aa32b9ee98398234478047c1b69e Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 6 Jun 2023 13:14:10 +0300 Subject: [PATCH 01/37] Move bootnodes from individual `SetConfig`s to `PeersetConfig` --- client/network/src/peerset.rs | 11 ++----- client/network/src/protocol.rs | 4 +-- .../src/protocol/notifications/behaviour.rs | 6 ++-- .../src/protocol/notifications/tests.rs | 14 +++----- client/network/src/protocol_controller.rs | 33 ++----------------- client/network/src/request_responses.rs | 2 +- client/network/test/src/peerset.rs | 14 ++++---- 7 files changed, 23 insertions(+), 61 deletions(-) diff --git a/client/network/src/peerset.rs b/client/network/src/peerset.rs index fd57175dd77cd..af9eddff44d68 100644 --- a/client/network/src/peerset.rs +++ b/client/network/src/peerset.rs @@ -192,6 +192,8 @@ impl From for IncomingIndex { /// Configuration to pass when creating the peer set manager. #[derive(Debug)] pub struct PeersetConfig { + /// Bootnodes. + pub bootnodes: Vec, /// List of sets of nodes the peerset manages. pub sets: Vec, } @@ -205,12 +207,6 @@ pub struct SetConfig { /// Maximum number of outgoing links to peers. pub out_peers: u32, - /// List of bootstrap nodes to initialize the set with. - /// - /// > **Note**: Keep in mind that the networking has to know an address for these nodes, - /// > otherwise it will not be able to connect to them. - pub bootnodes: Vec, - /// Lists of nodes we should always be connected to. /// /// > **Note**: Keep in mind that the networking has to know an address for these nodes, @@ -244,8 +240,7 @@ pub struct Peerset { impl Peerset { /// Builds a new peerset from the given configuration. pub fn from_config(config: PeersetConfig) -> (Peerset, PeersetHandle) { - let default_set_config = &config.sets[0]; - let peer_store = PeerStore::new(default_set_config.bootnodes.clone()); + let peer_store = PeerStore::new(config.bootnodes); let (to_notifications, from_controllers) = tracing_unbounded("mpsc_protocol_controllers_to_notifications", 10_000); diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index e57bc3e520be4..abe2ea65c88c7 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -131,7 +131,6 @@ impl Protocol { sets.push(crate::peerset::SetConfig { in_peers: network_config.default_peers_set.in_peers, out_peers: network_config.default_peers_set.out_peers, - bootnodes, reserved_nodes: default_sets_reserved.clone(), reserved_only: network_config.default_peers_set.non_reserved_mode == NonReservedPeerMode::Deny, @@ -150,13 +149,12 @@ impl Protocol { sets.push(crate::peerset::SetConfig { in_peers: set_cfg.set_config.in_peers, out_peers: set_cfg.set_config.out_peers, - bootnodes: Vec::new(), reserved_nodes, reserved_only, }); } - crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { sets }) + crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { bootnodes, sets }) }; let behaviour = { diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 0619e72d65e93..a029385b48174 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -2159,12 +2159,14 @@ mod tests { sets.push(crate::peerset::SetConfig { in_peers: 25, out_peers: 25, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }); - crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { sets }) + crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { + bootnodes: Vec::new(), + sets, + }) }; ( diff --git a/client/network/src/protocol/notifications/tests.rs b/client/network/src/protocol/notifications/tests.rs index 0c2eb89262f93..27e3c4968f292 100644 --- a/client/network/src/protocol/notifications/tests.rs +++ b/client/network/src/protocol/notifications/tests.rs @@ -67,18 +67,14 @@ fn build_nodes() -> (Swarm, Swarm) { let (peerset, handle) = crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { + bootnodes: if index == 0 { + keypairs.iter().skip(1).map(|keypair| keypair.public().to_peer_id()).collect() + } else { + vec![] + }, sets: vec![crate::peerset::SetConfig { in_peers: 25, out_peers: 25, - bootnodes: if index == 0 { - keypairs - .iter() - .skip(1) - .map(|keypair| keypair.public().to_peer_id()) - .collect() - } else { - vec![] - }, reserved_nodes: Default::default(), reserved_only: false, }], diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index 72373974a44ff..919d15753cfe5 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -775,7 +775,6 @@ mod tests { let config = SetConfig { in_peers: 0, out_peers: 0, - bootnodes: Vec::new(), reserved_nodes: std::iter::once(reserved1).collect(), reserved_only: true, }; @@ -838,7 +837,6 @@ mod tests { let config = SetConfig { in_peers: 0, out_peers: 0, - bootnodes: Vec::new(), reserved_nodes: std::iter::once(reserved1).collect(), reserved_only: true, }; @@ -890,7 +888,6 @@ mod tests { let config = SetConfig { in_peers: 0, out_peers: 0, - bootnodes: Vec::new(), reserved_nodes: std::iter::once(reserved1).collect(), reserved_only: true, }; @@ -950,7 +947,6 @@ mod tests { in_peers: 0, // Less slots than candidates. out_peers: 2, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -998,13 +994,8 @@ mod tests { let outgoing_candidates = vec![regular1, regular2]; let reserved_nodes = [reserved1, reserved2].iter().cloned().collect(); - let config = SetConfig { - in_peers: 10, - out_peers: 10, - bootnodes: Vec::new(), - reserved_nodes, - reserved_only: false, - }; + let config = + SetConfig { in_peers: 10, out_peers: 10, reserved_nodes, reserved_only: false }; let (tx, mut rx) = tracing_unbounded("mpsc_test_to_notifications", 100); let mut peer_store = MockPeerStoreHandle::new(); @@ -1043,7 +1034,6 @@ mod tests { in_peers: 0, // Less slots than candidates. out_peers: 2, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1114,7 +1104,6 @@ mod tests { in_peers: 0, // Make sure we have slots available. out_peers: 2, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: true, }; @@ -1141,7 +1130,6 @@ mod tests { // Make sure we have slots available. in_peers: 2, out_peers: 0, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: true, }; @@ -1179,7 +1167,6 @@ mod tests { in_peers: 0, // Make sure we have slots available. out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: true, }; @@ -1226,7 +1213,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), reserved_only: false, }; @@ -1287,7 +1273,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), reserved_only: false, }; @@ -1320,7 +1305,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), reserved_only: true, }; @@ -1367,7 +1351,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: [peer1, peer2].iter().cloned().collect(), reserved_only: false, }; @@ -1414,7 +1397,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1457,7 +1439,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1517,7 +1498,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), reserved_only: false, }; @@ -1574,7 +1554,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1627,7 +1606,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), reserved_only: false, }; @@ -1688,7 +1666,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1741,7 +1718,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1795,7 +1771,6 @@ mod tests { let config = SetConfig { in_peers: 1, out_peers: 1, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1849,7 +1824,6 @@ mod tests { let config = SetConfig { in_peers: 1, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1880,7 +1854,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: HashSet::new(), reserved_only: false, }; @@ -1906,7 +1879,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: std::iter::once(reserved1).collect(), reserved_only: false, }; @@ -1933,7 +1905,6 @@ mod tests { let config = SetConfig { in_peers: 10, out_peers: 10, - bootnodes: Vec::new(), reserved_nodes: std::iter::once(reserved1).collect(), reserved_only: false, }; diff --git a/client/network/src/request_responses.rs b/client/network/src/request_responses.rs index 44e6f588ab520..1bd857fa8828c 100644 --- a/client/network/src/request_responses.rs +++ b/client/network/src/request_responses.rs @@ -1073,10 +1073,10 @@ mod tests { .boxed(); let config = PeersetConfig { + bootnodes: vec![], sets: vec![SetConfig { in_peers: u32::max_value(), out_peers: u32::max_value(), - bootnodes: vec![], reserved_nodes: Default::default(), reserved_only: false, }], diff --git a/client/network/test/src/peerset.rs b/client/network/test/src/peerset.rs index 855d2339eda12..0be8595d6506f 100644 --- a/client/network/test/src/peerset.rs +++ b/client/network/test/src/peerset.rs @@ -130,14 +130,14 @@ fn test_once() { let mut reserved_nodes = HashSet::::new(); let (mut peerset, peerset_handle) = Peerset::from_config(PeersetConfig { + bootnodes: (0..Uniform::new_inclusive(0, 4).sample(&mut rng)) + .map(|_| { + let id = PeerId::random(); + known_nodes.insert(id, State::Disconnected); + id + }) + .collect(), sets: vec![SetConfig { - bootnodes: (0..Uniform::new_inclusive(0, 4).sample(&mut rng)) - .map(|_| { - let id = PeerId::random(); - known_nodes.insert(id, State::Disconnected); - id - }) - .collect(), reserved_nodes: { (0..Uniform::new_inclusive(0, 2).sample(&mut rng)) .map(|_| { From dae075dacf1372751d8ce4cc17b368cffa0026a0 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 6 Jun 2023 15:12:51 +0300 Subject: [PATCH 02/37] Move `SetId` & `SetConfig` from `peerset` to `protocol_controller` --- client/network/src/peerset.rs | 64 ++------ client/network/src/protocol.rs | 22 ++- .../src/protocol/notifications/behaviour.rs | 149 +++++++++--------- .../src/protocol/notifications/tests.rs | 21 +-- client/network/src/protocol_controller.rs | 107 +++++++++---- client/network/src/request_responses.rs | 7 +- 6 files changed, 187 insertions(+), 183 deletions(-) diff --git a/client/network/src/peerset.rs b/client/network/src/peerset.rs index af9eddff44d68..c11917fcb45ae 100644 --- a/client/network/src/peerset.rs +++ b/client/network/src/peerset.rs @@ -34,7 +34,7 @@ use crate::{ peer_store::{PeerStore, PeerStoreHandle, PeerStoreProvider}, - protocol_controller::{ProtocolController, ProtocolHandle}, + protocol_controller::{ProtoSetConfig, ProtocolController, ProtocolHandle, SetId}, }; use futures::{ @@ -68,33 +68,6 @@ enum Action { PeerReputation(PeerId, oneshot::Sender), } -/// Identifier of a set in the peerset. -/// -/// Can be constructed using the `From` trait implementation based on the index of the set -/// within [`PeersetConfig::sets`]. For example, the first element of [`PeersetConfig::sets`] is -/// later referred to with `SetId::from(0)`. It is intended that the code responsible for building -/// the [`PeersetConfig`] is also responsible for constructing the [`SetId`]s. -#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct SetId(usize); - -impl SetId { - pub const fn from(id: usize) -> Self { - Self(id) - } -} - -impl From for SetId { - fn from(id: usize) -> Self { - Self(id) - } -} - -impl From for usize { - fn from(id: SetId) -> Self { - id.0 - } -} - /// Shared handle to the peer set manager (PSM). Distributed around the code. #[derive(Debug, Clone)] pub struct PeersetHandle { @@ -195,26 +168,7 @@ pub struct PeersetConfig { /// Bootnodes. pub bootnodes: Vec, /// List of sets of nodes the peerset manages. - pub sets: Vec, -} - -/// Configuration for a single set of nodes. -#[derive(Debug)] -pub struct SetConfig { - /// Maximum number of ingoing links to peers. - pub in_peers: u32, - - /// Maximum number of outgoing links to peers. - pub out_peers: u32, - - /// Lists of nodes we should always be connected to. - /// - /// > **Note**: Keep in mind that the networking has to know an address for these nodes, - /// > otherwise it will not be able to connect to them. - pub reserved_nodes: HashSet, - - /// If true, we only accept nodes in [`SetConfig::reserved_nodes`]. - pub reserved_only: bool, + pub sets: Vec, } /// Side of the peer set manager owned by the network. In other words, the "receiving" side. @@ -283,7 +237,7 @@ impl Peerset { /// Returns the list of reserved peers. pub fn reserved_peers(&self, set_id: SetId, pending_response: oneshot::Sender>) { - self.protocol_handles[set_id.0].reserved_peers(pending_response); + self.protocol_handles[usize::from(set_id)].reserved_peers(pending_response); } /// Indicate that we received an incoming connection. Must be answered either with @@ -296,7 +250,7 @@ impl Peerset { // message to the output channel with a `PeerId`, and that `incoming` gets called with the same // `PeerId` before that message has been read by the user. In this situation we must not answer. pub fn incoming(&mut self, set_id: SetId, peer_id: PeerId, index: IncomingIndex) { - self.protocol_handles[set_id.0].incoming_connection(peer_id, index); + self.protocol_handles[usize::from(set_id)].incoming_connection(peer_id, index); } /// Indicate that we dropped an active connection with a peer, or that we failed to connect. @@ -304,7 +258,7 @@ impl Peerset { /// Must only be called after the PSM has either generated a `Connect` message with this /// `PeerId`, or accepted an incoming connection with this `PeerId`. pub fn dropped(&mut self, set_id: SetId, peer_id: PeerId, _reason: DropReason) { - self.protocol_handles[set_id.0].dropped(peer_id); + self.protocol_handles[usize::from(set_id)].dropped(peer_id); } /// Produces a JSON object containing the state of the peerset manager, for debugging purposes. @@ -340,13 +294,13 @@ impl Stream for Peerset { if let Some(action) = action { match action { Action::AddReservedPeer(set_id, peer_id) => - self.protocol_handles[set_id.0].add_reserved_peer(peer_id), + self.protocol_handles[usize::from(set_id)].add_reserved_peer(peer_id), Action::RemoveReservedPeer(set_id, peer_id) => - self.protocol_handles[set_id.0].remove_reserved_peer(peer_id), + self.protocol_handles[usize::from(set_id)].remove_reserved_peer(peer_id), Action::SetReservedPeers(set_id, peer_ids) => - self.protocol_handles[set_id.0].set_reserved_peers(peer_ids), + self.protocol_handles[usize::from(set_id)].set_reserved_peers(peer_ids), Action::SetReservedOnly(set_id, reserved_only) => - self.protocol_handles[set_id.0].set_reserved_only(reserved_only), + self.protocol_handles[usize::from(set_id)].set_reserved_only(reserved_only), Action::ReportPeer(peer_id, score_diff) => self.peer_store_handle.report_peer(peer_id, score_diff), Action::AddKnownPeer(peer_id) => self.peer_store_handle.add_known_peer(peer_id), diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index abe2ea65c88c7..885262fb6d5b7 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -19,6 +19,7 @@ use crate::{ config::{self, NonReservedPeerMode}, error, + protocol_controller::{ProtoSetConfig, SetId}, types::ProtocolName, ReputationChange, }; @@ -62,7 +63,7 @@ pub mod message; pub(crate) const BLOCK_ANNOUNCES_TRANSACTIONS_SUBSTREAM_SIZE: u64 = 16 * 1024 * 1024; /// Identifier of the peerset for the block announces protocol. -const HARDCODED_PEERSETS_SYNC: crate::peerset::SetId = crate::peerset::SetId::from(0); +const HARDCODED_PEERSETS_SYNC: SetId = SetId::from(0); /// Number of hardcoded peersets (the constants right above). Any set whose identifier is equal or /// superior to this value corresponds to a user-defined protocol. const NUM_HARDCODED_PEERSETS: usize = 1; @@ -90,7 +91,7 @@ pub struct Protocol { /// event to the outer layers, we also shouldn't propagate this "substream closed" event. To /// solve this, an entry is added to this map whenever an invalid handshake is received. /// Entries are removed when the corresponding "substream closed" is later received. - bad_handshake_substreams: HashSet<(PeerId, crate::peerset::SetId)>, + bad_handshake_substreams: HashSet<(PeerId, SetId)>, /// Connected peers. peers: HashMap, sync_substream_validations: FuturesUnordered, @@ -128,7 +129,7 @@ impl Protocol { } // Set number 0 is used for block announces. - sets.push(crate::peerset::SetConfig { + sets.push(ProtoSetConfig { in_peers: network_config.default_peers_set.in_peers, out_peers: network_config.default_peers_set.out_peers, reserved_nodes: default_sets_reserved.clone(), @@ -146,7 +147,7 @@ impl Protocol { let reserved_only = set_cfg.set_config.non_reserved_mode == NonReservedPeerMode::Deny; - sets.push(crate::peerset::SetConfig { + sets.push(ProtoSetConfig { in_peers: set_cfg.set_config.in_peers, out_peers: set_cfg.set_config.out_peers, reserved_nodes, @@ -209,7 +210,7 @@ impl Protocol { pub fn disconnect_peer(&mut self, peer_id: &PeerId, protocol_name: ProtocolName) { if let Some(position) = self.notification_protocols.iter().position(|p| *p == protocol_name) { - self.behaviour.disconnect_peer(peer_id, crate::peerset::SetId::from(position)); + self.behaviour.disconnect_peer(peer_id, SetId::from(position)); self.peers.remove(peer_id); } else { warn!(target: "sub-libp2p", "disconnect_peer() with invalid protocol name") @@ -234,8 +235,7 @@ impl Protocol { /// Set handshake for the notification protocol. pub fn set_notification_handshake(&mut self, protocol: ProtocolName, handshake: Vec) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.behaviour - .set_notif_protocol_handshake(crate::peerset::SetId::from(index), handshake); + self.behaviour.set_notif_protocol_handshake(SetId::from(index), handshake); } else { error!( target: "sub-libp2p", @@ -273,8 +273,7 @@ impl Protocol { /// Sets the list of reserved peers for the given protocol/peerset. pub fn set_reserved_peerset_peers(&self, protocol: ProtocolName, peers: HashSet) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.peerset_handle - .set_reserved_peers(crate::peerset::SetId::from(index), peers); + self.peerset_handle.set_reserved_peers(SetId::from(index), peers); } else { error!( target: "sub-libp2p", @@ -287,8 +286,7 @@ impl Protocol { /// Removes a `PeerId` from the list of reserved peers. pub fn remove_set_reserved_peer(&self, protocol: ProtocolName, peer: PeerId) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.peerset_handle - .remove_reserved_peer(crate::peerset::SetId::from(index), peer); + self.peerset_handle.remove_reserved_peer(SetId::from(index), peer); } else { error!( target: "sub-libp2p", @@ -301,7 +299,7 @@ impl Protocol { /// Adds a `PeerId` to the list of reserved peers. pub fn add_set_reserved_peer(&self, protocol: ProtocolName, peer: PeerId) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.peerset_handle.add_reserved_peer(crate::peerset::SetId::from(index), peer); + self.peerset_handle.add_reserved_peer(SetId::from(index), peer); } else { error!( target: "sub-libp2p", diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index a029385b48174..fa94b61e57d59 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -21,6 +21,7 @@ use crate::{ protocol::notifications::handler::{ self, NotificationsSink, NotifsHandler, NotifsHandlerIn, NotifsHandlerOut, }, + protocol_controller::SetId, types::ProtocolName, }; @@ -111,7 +112,7 @@ pub struct Notifications { peerset: crate::peerset::Peerset, /// List of peers in our state. - peers: FnvHashMap<(PeerId, crate::peerset::SetId), PeerState>, + peers: FnvHashMap<(PeerId, SetId), PeerState>, /// The elements in `peers` occasionally contain `Delay` objects that we would normally have /// to be polled one by one. In order to avoid doing so, as an optimization, every `Delay` is @@ -120,9 +121,8 @@ pub struct Notifications { /// /// By design, we never remove elements from this list. Elements are removed only when the /// `Delay` triggers. As such, this stream may produce obsolete elements. - delays: stream::FuturesUnordered< - Pin + Send>>, - >, + delays: + stream::FuturesUnordered + Send>>>, /// [`DelayId`] to assign to the next delay. next_delay_id: DelayId, @@ -296,7 +296,7 @@ struct IncomingPeer { /// Id of the remote peer of the incoming substream. peer_id: PeerId, /// Id of the set the incoming substream would belong to. - set_id: crate::peerset::SetId, + set_id: SetId, /// If true, this "incoming" still corresponds to an actual connection. If false, then the /// connection corresponding to it has been closed or replaced already. alive: bool, @@ -312,7 +312,7 @@ pub enum NotificationsOut { /// Id of the peer we are connected to. peer_id: PeerId, /// Peerset set ID the substream is tied to. - set_id: crate::peerset::SetId, + set_id: SetId, /// If `Some`, a fallback protocol name has been used rather the main protocol name. /// Always matches one of the fallback names passed at initialization. negotiated_fallback: Option, @@ -332,7 +332,7 @@ pub enum NotificationsOut { /// Id of the peer we are connected to. peer_id: PeerId, /// Peerset set ID the substream is tied to. - set_id: crate::peerset::SetId, + set_id: SetId, /// Replacement for the previous [`NotificationsSink`]. notifications_sink: NotificationsSink, }, @@ -343,7 +343,7 @@ pub enum NotificationsOut { /// Id of the peer we were connected to. peer_id: PeerId, /// Peerset set ID the substream was tied to. - set_id: crate::peerset::SetId, + set_id: SetId, }, /// Receives a message on a custom protocol substream. @@ -353,7 +353,7 @@ pub enum NotificationsOut { /// Id of the peer the message came from. peer_id: PeerId, /// Peerset set ID the substream is tied to. - set_id: crate::peerset::SetId, + set_id: SetId, /// Message that has been received. message: BytesMut, }, @@ -391,7 +391,7 @@ impl Notifications { /// Modifies the handshake of the given notifications protocol. pub fn set_notif_protocol_handshake( &mut self, - set_id: crate::peerset::SetId, + set_id: SetId, handshake_message: impl Into>, ) { if let Some(p) = self.notif_protocols.get_mut(usize::from(set_id)) { @@ -413,18 +413,18 @@ impl Notifications { } /// Returns true if we have an open substream to the given peer. - pub fn is_open(&self, peer_id: &PeerId, set_id: crate::peerset::SetId) -> bool { + pub fn is_open(&self, peer_id: &PeerId, set_id: SetId) -> bool { self.peers.get(&(*peer_id, set_id)).map(|p| p.is_open()).unwrap_or(false) } /// Disconnects the given peer if we are connected to it. - pub fn disconnect_peer(&mut self, peer_id: &PeerId, set_id: crate::peerset::SetId) { + pub fn disconnect_peer(&mut self, peer_id: &PeerId, set_id: SetId) { trace!(target: "sub-libp2p", "External API => Disconnect({}, {:?})", peer_id, set_id); self.disconnect_peer_inner(peer_id, set_id); } /// Inner implementation of `disconnect_peer`. - fn disconnect_peer_inner(&mut self, peer_id: &PeerId, set_id: crate::peerset::SetId) { + fn disconnect_peer_inner(&mut self, peer_id: &PeerId, set_id: SetId) { let mut entry = if let Entry::Occupied(entry) = self.peers.entry((*peer_id, set_id)) { entry } else { @@ -539,11 +539,7 @@ impl Notifications { } /// Returns the list of reserved peers. - pub fn reserved_peers( - &self, - set_id: crate::peerset::SetId, - pending_response: oneshot::Sender>, - ) { + pub fn reserved_peers(&self, set_id: SetId, pending_response: oneshot::Sender>) { self.peerset.reserved_peers(set_id, pending_response); } @@ -553,7 +549,7 @@ impl Notifications { } /// Function that is called when the peerset wants us to connect to a peer. - fn peerset_report_connect(&mut self, peer_id: PeerId, set_id: crate::peerset::SetId) { + fn peerset_report_connect(&mut self, peer_id: PeerId, set_id: SetId) { // If `PeerId` is unknown to us, insert an entry, start dialing, and return early. let mut occ_entry = match self.peers.entry((peer_id, set_id)) { Entry::Occupied(entry) => entry, @@ -731,7 +727,7 @@ impl Notifications { } /// Function that is called when the peerset wants us to disconnect from a peer. - fn peerset_report_disconnect(&mut self, peer_id: PeerId, set_id: crate::peerset::SetId) { + fn peerset_report_disconnect(&mut self, peer_id: PeerId, set_id: SetId) { let mut entry = match self.peers.entry((peer_id, set_id)) { Entry::Occupied(entry) => entry, Entry::Vacant(entry) => { @@ -1059,7 +1055,7 @@ impl NetworkBehaviour for Notifications { connection_id, .. }) => { - for set_id in (0..self.notif_protocols.len()).map(crate::peerset::SetId::from) { + for set_id in (0..self.notif_protocols.len()).map(SetId::from) { match self.peers.entry((peer_id, set_id)).or_insert(PeerState::Poisoned) { // Requested | PendingRequest => Enabled st @ &mut PeerState::Requested | @@ -1113,7 +1109,7 @@ impl NetworkBehaviour for Notifications { } }, FromSwarm::ConnectionClosed(ConnectionClosed { peer_id, connection_id, .. }) => { - for set_id in (0..self.notif_protocols.len()).map(crate::peerset::SetId::from) { + for set_id in (0..self.notif_protocols.len()).map(SetId::from) { let mut entry = if let Entry::Occupied(entry) = self.peers.entry((peer_id, set_id)) { @@ -1406,7 +1402,7 @@ impl NetworkBehaviour for Notifications { if let Some(peer_id) = peer_id { trace!(target: "sub-libp2p", "Libp2p => Dial failure for {:?}", peer_id); - for set_id in (0..self.notif_protocols.len()).map(crate::peerset::SetId::from) { + for set_id in (0..self.notif_protocols.len()).map(SetId::from) { if let Entry::Occupied(mut entry) = self.peers.entry((peer_id, set_id)) { match mem::replace(entry.get_mut(), PeerState::Poisoned) { // The peer is not in our list. @@ -1485,7 +1481,7 @@ impl NetworkBehaviour for Notifications { ) { match event { NotifsHandlerOut::OpenDesiredByRemote { protocol_index } => { - let set_id = crate::peerset::SetId::from(protocol_index); + let set_id = SetId::from(protocol_index); trace!(target: "sub-libp2p", "Handler({:?}, {:?}]) => OpenDesiredByRemote({:?})", @@ -1675,7 +1671,7 @@ impl NetworkBehaviour for Notifications { }, NotifsHandlerOut::CloseDesired { protocol_index } => { - let set_id = crate::peerset::SetId::from(protocol_index); + let set_id = SetId::from(protocol_index); trace!(target: "sub-libp2p", "Handler({}, {:?}) => CloseDesired({:?})", @@ -1775,7 +1771,7 @@ impl NetworkBehaviour for Notifications { }, NotifsHandlerOut::CloseResult { protocol_index } => { - let set_id = crate::peerset::SetId::from(protocol_index); + let set_id = SetId::from(protocol_index); trace!(target: "sub-libp2p", "Handler({}, {:?}) => CloseResult({:?})", @@ -1814,7 +1810,7 @@ impl NetworkBehaviour for Notifications { notifications_sink, .. } => { - let set_id = crate::peerset::SetId::from(protocol_index); + let set_id = SetId::from(protocol_index); trace!(target: "sub-libp2p", "Handler({}, {:?}) => OpenResultOk({:?})", peer_id, connection_id, set_id); @@ -1880,7 +1876,7 @@ impl NetworkBehaviour for Notifications { }, NotifsHandlerOut::OpenResultErr { protocol_index } => { - let set_id = crate::peerset::SetId::from(protocol_index); + let set_id = SetId::from(protocol_index); trace!(target: "sub-libp2p", "Handler({:?}, {:?}) => OpenResultErr({:?})", peer_id, connection_id, set_id); @@ -1969,7 +1965,7 @@ impl NetworkBehaviour for Notifications { }, NotifsHandlerOut::Notification { protocol_index, message } => { - let set_id = crate::peerset::SetId::from(protocol_index); + let set_id = SetId::from(protocol_index); if self.is_open(&peer_id, set_id) { trace!( target: "sub-libp2p", @@ -2105,7 +2101,10 @@ impl NetworkBehaviour for Notifications { #[allow(deprecated)] mod tests { use super::*; - use crate::{peerset::IncomingIndex, protocol::notifications::handler::tests::*}; + use crate::{ + peerset::IncomingIndex, protocol::notifications::handler::tests::*, + protocol_controller::ProtoSetConfig, + }; use libp2p::swarm::AddressRecord; use std::{collections::HashSet, iter}; @@ -2156,7 +2155,7 @@ mod tests { let (peerset, peerset_handle) = { let mut sets = Vec::with_capacity(1); - sets.push(crate::peerset::SetConfig { + sets.push(ProtoSetConfig { in_peers: 25, out_peers: 25, reserved_nodes: HashSet::new(), @@ -2355,7 +2354,7 @@ mod tests { #[test] fn peerset_report_connect_backoff() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -2422,7 +2421,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2456,7 +2455,7 @@ mod tests { #[test] fn peerset_disconnect_disable_pending_enable() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -2505,7 +2504,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2540,7 +2539,7 @@ mod tests { fn peerset_disconnect_requested() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); // Set peer into `Requested` state. notif.peerset_report_connect(peer, set_id); @@ -2554,7 +2553,7 @@ mod tests { #[test] fn peerset_disconnect_pending_request() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -2609,7 +2608,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2657,7 +2656,7 @@ mod tests { let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2711,7 +2710,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2745,7 +2744,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2791,7 +2790,7 @@ mod tests { let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let conn1 = ConnectionId::new_unchecked(1); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2859,7 +2858,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -2914,7 +2913,7 @@ mod tests { let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -3007,7 +3006,7 @@ mod tests { fn dial_failure_for_requested_peer() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); // Set peer into `Requested` state. notif.peerset_report_connect(peer, set_id); @@ -3031,7 +3030,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -3078,7 +3077,7 @@ mod tests { #[test] fn peerset_report_connect_backoff_expired() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -3127,7 +3126,7 @@ mod tests { fn peerset_report_disconnect_disabled() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), @@ -3152,7 +3151,7 @@ mod tests { #[test] fn peerset_report_disconnect_backoff() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -3198,7 +3197,7 @@ mod tests { #[test] fn peer_is_backed_off_if_both_connections_get_closed_while_peer_is_disabled_with_back_off() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); @@ -3272,7 +3271,7 @@ mod tests { fn inject_connection_closed_incoming_with_backoff() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), @@ -3325,7 +3324,7 @@ mod tests { let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -3380,7 +3379,7 @@ mod tests { let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -3438,7 +3437,7 @@ mod tests { let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -3503,7 +3502,7 @@ mod tests { #[test] fn inject_dial_failure_for_pending_request() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -3567,7 +3566,7 @@ mod tests { fn peerstate_incoming_open_desired_by_remote() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); let connected = ConnectedPoint::Listener { @@ -3621,7 +3620,7 @@ mod tests { async fn remove_backoff_peer_after_timeout() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), @@ -3700,7 +3699,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -3818,7 +3817,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -3866,7 +3865,7 @@ mod tests { #[cfg(debug_assertions)] fn peerset_report_connect_with_disabled_pending_enable_peer() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -3913,7 +3912,7 @@ mod tests { fn peerset_report_connect_with_requested_peer() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); // Set peer into `Requested` state. notif.peerset_report_connect(peer, set_id); @@ -3928,7 +3927,7 @@ mod tests { #[cfg(debug_assertions)] fn peerset_report_connect_with_pending_requested() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -3986,7 +3985,7 @@ mod tests { fn peerset_report_connect_with_incoming_peer() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), @@ -4021,7 +4020,7 @@ mod tests { fn peerset_report_disconnect_with_incoming_peer() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), @@ -4058,7 +4057,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -4099,7 +4098,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -4167,7 +4166,7 @@ mod tests { fn disconnect_non_existent_peer() { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); notif.peerset_report_disconnect(peer, set_id); @@ -4200,7 +4199,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -4238,7 +4237,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -4278,7 +4277,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -4319,7 +4318,7 @@ mod tests { #[cfg(debug_assertions)] fn inject_non_existent_connection_closed_for_disabled_peer() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -4354,7 +4353,7 @@ mod tests { #[cfg(debug_assertions)] fn inject_non_existent_connection_closed_for_disabled_pending_enable() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -4407,7 +4406,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -4451,7 +4450,7 @@ mod tests { let (mut notif, _peerset) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), send_back_addr: Multiaddr::empty(), @@ -4496,7 +4495,7 @@ mod tests { #[cfg(debug_assertions)] fn inject_connection_closed_for_backoff_peer() { let (mut notif, _peerset) = development_notifs(); - let set_id = crate::peerset::SetId::from(0); + let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { diff --git a/client/network/src/protocol/notifications/tests.rs b/client/network/src/protocol/notifications/tests.rs index 27e3c4968f292..9495ecdb4afaf 100644 --- a/client/network/src/protocol/notifications/tests.rs +++ b/client/network/src/protocol/notifications/tests.rs @@ -18,7 +18,10 @@ #![cfg(test)] -use crate::protocol::notifications::{Notifications, NotificationsOut, ProtocolConfig}; +use crate::{ + protocol::notifications::{Notifications, NotificationsOut, ProtocolConfig}, + protocol_controller::{ProtoSetConfig, SetId}, +}; use futures::prelude::*; use libp2p::{ @@ -72,7 +75,7 @@ fn build_nodes() -> (Swarm, Swarm) { } else { vec![] }, - sets: vec![crate::peerset::SetConfig { + sets: vec![ProtoSetConfig { in_peers: 25, out_peers: 25, reserved_nodes: Default::default(), @@ -268,10 +271,9 @@ fn reconnect_after_disconnect() { ServiceState::NotConnected => { service1_state = ServiceState::FirstConnec; if service2_state == ServiceState::FirstConnec { - service1.behaviour_mut().disconnect_peer( - Swarm::local_peer_id(&service2), - crate::peerset::SetId::from(0), - ); + service1 + .behaviour_mut() + .disconnect_peer(Swarm::local_peer_id(&service2), SetId::from(0)); } }, ServiceState::Disconnected => service1_state = ServiceState::ConnectedAgain, @@ -291,10 +293,9 @@ fn reconnect_after_disconnect() { ServiceState::NotConnected => { service2_state = ServiceState::FirstConnec; if service1_state == ServiceState::FirstConnec { - service1.behaviour_mut().disconnect_peer( - Swarm::local_peer_id(&service2), - crate::peerset::SetId::from(0), - ); + service1 + .behaviour_mut() + .disconnect_peer(Swarm::local_peer_id(&service2), SetId::from(0)); } }, ServiceState::Disconnected => service2_state = ServiceState::ConnectedAgain, diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index 919d15753cfe5..03db878d50059 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -54,12 +54,61 @@ use wasm_timer::Delay; use crate::{ peer_store::PeerStoreProvider, - peerset::{IncomingIndex, Message, SetConfig, SetId}, + peerset::{IncomingIndex, Message}, }; /// Log target for this file. pub const LOG_TARGET: &str = "peerset"; +/// `Notifications` protocol identifier. For historical reasons it's called `SetId`, because it +/// used to refer to a set of peers in a peerset for this protocol. +/// +/// Can be constructed using the `From` trait implementation based on the index of the set +/// within [`PeersetConfig::sets`]. For example, the first element of [`PeersetConfig::sets`] is +/// later referred to with `SetId::from(0)`. It is intended that the code responsible for building +/// the [`PeersetConfig`] is also responsible for constructing the [`SetId`]s. +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct SetId(usize); + +impl SetId { + pub const fn from(id: usize) -> Self { + Self(id) + } +} + +impl From for SetId { + fn from(id: usize) -> Self { + Self(id) + } +} + +impl From for usize { + fn from(id: SetId) -> Self { + id.0 + } +} + +/// Configuration for a set of nodes for a specific protocol. +// +// TODO: merge this with [`crate::config::SetConfig`]. +#[derive(Debug)] +pub struct ProtoSetConfig { + /// Maximum number of ingoing links to peers. + pub in_peers: u32, + + /// Maximum number of outgoing links to peers. + pub out_peers: u32, + + /// Lists of nodes we should always be connected to. + /// + /// > **Note**: Keep in mind that the networking has to know an address for these nodes, + /// > otherwise it will not be able to connect to them. + pub reserved_nodes: HashSet, + + /// If true, we only accept nodes in [`SetConfig::reserved_nodes`]. + pub reserved_only: bool, +} + /// External API actions. #[derive(Debug)] enum Action { @@ -217,7 +266,7 @@ impl ProtocolController { /// Construct new [`ProtocolController`]. pub fn new( set_id: SetId, - config: SetConfig, + config: ProtoSetConfig, to_notifications: TracingUnboundedSender, peer_store: Box, ) -> (ProtocolHandle, ProtocolController) { @@ -742,10 +791,10 @@ impl ProtocolController { #[cfg(test)] mod tests { - use super::{Direction, PeerState, ProtocolController, ProtocolHandle}; + use super::{Direction, PeerState, ProtoSetConfig, ProtocolController, ProtocolHandle, SetId}; use crate::{ peer_store::PeerStoreProvider, - peerset::{IncomingIndex, Message, SetConfig, SetId}, + peerset::{IncomingIndex, Message}, ReputationChange, }; use libp2p::PeerId; @@ -772,7 +821,7 @@ mod tests { let reserved2 = PeerId::random(); // Add first reserved node via config. - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 0, out_peers: 0, reserved_nodes: std::iter::once(reserved1).collect(), @@ -834,7 +883,7 @@ mod tests { let reserved2 = PeerId::random(); // Add first reserved node via config. - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 0, out_peers: 0, reserved_nodes: std::iter::once(reserved1).collect(), @@ -885,7 +934,7 @@ mod tests { let reserved2 = PeerId::random(); // Add first reserved node via config. - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 0, out_peers: 0, reserved_nodes: std::iter::once(reserved1).collect(), @@ -943,7 +992,7 @@ mod tests { let peer2 = PeerId::random(); let candidates = vec![peer1, peer2]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 0, // Less slots than candidates. out_peers: 2, @@ -995,7 +1044,7 @@ mod tests { let reserved_nodes = [reserved1, reserved2].iter().cloned().collect(); let config = - SetConfig { in_peers: 10, out_peers: 10, reserved_nodes, reserved_only: false }; + ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes, reserved_only: false }; let (tx, mut rx) = tracing_unbounded("mpsc_test_to_notifications", 100); let mut peer_store = MockPeerStoreHandle::new(); @@ -1030,7 +1079,7 @@ mod tests { let candidates1 = vec![peer1, peer2]; let candidates2 = vec![peer3]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 0, // Less slots than candidates. out_peers: 2, @@ -1100,7 +1149,7 @@ mod tests { #[test] fn in_reserved_only_mode_no_peers_are_requested_from_peer_store_and_connected() { - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 0, // Make sure we have slots available. out_peers: 2, @@ -1126,7 +1175,7 @@ mod tests { #[test] fn in_reserved_only_mode_no_regular_peers_are_accepted() { - let config = SetConfig { + let config = ProtoSetConfig { // Make sure we have slots available. in_peers: 2, out_peers: 0, @@ -1163,7 +1212,7 @@ mod tests { let peer2 = PeerId::random(); let candidates = vec![peer1, peer2]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 0, // Make sure we have slots available. out_peers: 10, @@ -1210,7 +1259,7 @@ mod tests { let regular2 = PeerId::random(); let outgoing_candidates = vec![regular1]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), @@ -1270,7 +1319,7 @@ mod tests { let reserved1 = PeerId::random(); let reserved2 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), @@ -1302,7 +1351,7 @@ mod tests { let reserved1 = PeerId::random(); let reserved2 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), @@ -1348,7 +1397,7 @@ mod tests { let peer1 = PeerId::random(); let peer2 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: [peer1, peer2].iter().cloned().collect(), @@ -1394,7 +1443,7 @@ mod tests { let peer2 = PeerId::random(); let outgoing_candidates = vec![peer1]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: HashSet::new(), @@ -1436,7 +1485,7 @@ mod tests { let peer2 = PeerId::random(); let outgoing_candidates = vec![peer1]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: HashSet::new(), @@ -1495,7 +1544,7 @@ mod tests { let reserved1 = PeerId::random(); let reserved2 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), @@ -1551,7 +1600,7 @@ mod tests { let peer2 = PeerId::random(); let outgoing_candidates = vec![peer1]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: HashSet::new(), @@ -1603,7 +1652,7 @@ mod tests { let reserved1 = PeerId::random(); let reserved2 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: [reserved1, reserved2].iter().cloned().collect(), @@ -1663,7 +1712,7 @@ mod tests { let regular2 = PeerId::random(); let outgoing_candidates = vec![regular1]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: HashSet::new(), @@ -1715,7 +1764,7 @@ mod tests { let regular2 = PeerId::random(); let outgoing_candidates = vec![regular1]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: HashSet::new(), @@ -1768,7 +1817,7 @@ mod tests { let regular2 = PeerId::random(); let outgoing_candidates = vec![regular1]; - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 1, out_peers: 1, reserved_nodes: HashSet::new(), @@ -1821,7 +1870,7 @@ mod tests { let peer1 = PeerId::random(); let peer2 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 1, out_peers: 10, reserved_nodes: HashSet::new(), @@ -1851,7 +1900,7 @@ mod tests { fn banned_regular_incoming_node_is_rejected() { let peer1 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: HashSet::new(), @@ -1876,7 +1925,7 @@ mod tests { fn banned_reserved_incoming_node_is_rejected() { let reserved1 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: std::iter::once(reserved1).collect(), @@ -1902,7 +1951,7 @@ mod tests { fn we_dont_connect_to_banned_reserved_node() { let reserved1 = PeerId::random(); - let config = SetConfig { + let config = ProtoSetConfig { in_peers: 10, out_peers: 10, reserved_nodes: std::iter::once(reserved1).collect(), diff --git a/client/network/src/request_responses.rs b/client/network/src/request_responses.rs index 1bd857fa8828c..af3cc991e0780 100644 --- a/client/network/src/request_responses.rs +++ b/client/network/src/request_responses.rs @@ -1040,7 +1040,10 @@ impl Codec for GenericCodec { mod tests { use super::*; - use crate::peerset::{Peerset, PeersetConfig, SetConfig}; + use crate::{ + peerset::{Peerset, PeersetConfig}, + protocol_controller::ProtoSetConfig, + }; use futures::{channel::oneshot, executor::LocalPool, task::Spawn}; use libp2p::{ core::{ @@ -1074,7 +1077,7 @@ mod tests { let config = PeersetConfig { bootnodes: vec![], - sets: vec![SetConfig { + sets: vec![ProtoSetConfig { in_peers: u32::max_value(), out_peers: u32::max_value(), reserved_nodes: Default::default(), From 5a4f6a4784f82ad316a3204c231d85216f421972 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 6 Jun 2023 15:56:23 +0300 Subject: [PATCH 03/37] Remove unused `DropReason` --- client/network/src/peerset.rs | 12 +----------- .../src/protocol/notifications/behaviour.rs | 19 +++++++++---------- client/network/test/src/peerset.rs | 4 ++-- 3 files changed, 12 insertions(+), 23 deletions(-) diff --git a/client/network/src/peerset.rs b/client/network/src/peerset.rs index c11917fcb45ae..018338d65cbbc 100644 --- a/client/network/src/peerset.rs +++ b/client/network/src/peerset.rs @@ -257,7 +257,7 @@ impl Peerset { /// /// Must only be called after the PSM has either generated a `Connect` message with this /// `PeerId`, or accepted an incoming connection with this `PeerId`. - pub fn dropped(&mut self, set_id: SetId, peer_id: PeerId, _reason: DropReason) { + pub fn dropped(&mut self, set_id: SetId, peer_id: PeerId) { self.protocol_handles[usize::from(set_id)].dropped(peer_id); } @@ -331,13 +331,3 @@ impl Stream for Peerset { Poll::Pending } } - -/// Reason for calling [`Peerset::dropped`]. -#[derive(Debug)] -pub enum DropReason { - /// Substream or connection has been closed for an unknown reason. - Unknown, - /// Substream or connection has been explicitly refused by the target. In other words, the - /// peer doesn't actually belong to this set. - Refused, -} diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index fa94b61e57d59..12fe3db46e26f 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -17,7 +17,6 @@ // along with this program. If not, see . use crate::{ - peerset::DropReason, protocol::notifications::handler::{ self, NotificationsSink, NotifsHandler, NotifsHandlerIn, NotifsHandlerOut, }, @@ -441,7 +440,7 @@ impl Notifications { // DisabledPendingEnable => Disabled. PeerState::DisabledPendingEnable { connections, timer_deadline, timer: _ } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, *peer_id, DropReason::Unknown); + self.peerset.dropped(set_id, *peer_id); *entry.into_mut() = PeerState::Disabled { connections, backoff_until: Some(timer_deadline) } }, @@ -451,7 +450,7 @@ impl Notifications { // If relevant, the external API is instantly notified. PeerState::Enabled { mut connections } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, *peer_id, DropReason::Unknown); + self.peerset.dropped(set_id, *peer_id); if connections.iter().any(|(_, s)| matches!(s, ConnectionState::Open(_))) { trace!(target: "sub-libp2p", "External API <= Closed({}, {:?})", peer_id, set_id); @@ -853,7 +852,7 @@ impl Notifications { _ => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", incoming.peer_id, incoming.set_id); - self.peerset.dropped(incoming.set_id, incoming.peer_id, DropReason::Unknown); + self.peerset.dropped(incoming.set_id, incoming.peer_id); }, } return @@ -1191,7 +1190,7 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id, DropReason::Unknown); + self.peerset.dropped(set_id, peer_id); *entry.get_mut() = PeerState::Backoff { timer, timer_deadline }; } else { *entry.get_mut() = PeerState::DisabledPendingEnable { @@ -1345,7 +1344,7 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id, DropReason::Unknown); + self.peerset.dropped(set_id, peer_id); let ban_dur = Uniform::new(5, 10).sample(&mut rand::thread_rng()); let delay_id = self.next_delay_id; @@ -1367,7 +1366,7 @@ impl NetworkBehaviour for Notifications { matches!(s, ConnectionState::Opening | ConnectionState::Open(_)) }) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id, DropReason::Unknown); + self.peerset.dropped(set_id, peer_id); *entry.get_mut() = PeerState::Disabled { connections, backoff_until: None }; @@ -1415,7 +1414,7 @@ impl NetworkBehaviour for Notifications { st @ PeerState::Requested | st @ PeerState::PendingRequest { .. } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id, DropReason::Unknown); + self.peerset.dropped(set_id, peer_id); let now = Instant::now(); let ban_duration = match st { @@ -1743,7 +1742,7 @@ impl NetworkBehaviour for Notifications { .any(|(_, s)| matches!(s, ConnectionState::Opening)) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id, DropReason::Refused); + self.peerset.dropped(set_id, peer_id); *entry.into_mut() = PeerState::Disabled { connections, backoff_until: None }; } else { @@ -1917,7 +1916,7 @@ impl NetworkBehaviour for Notifications { matches!(s, ConnectionState::Opening | ConnectionState::Open(_)) }) { trace!(target: "sub-libp2p", "PSM <= Dropped({:?})", peer_id); - self.peerset.dropped(set_id, peer_id, DropReason::Refused); + self.peerset.dropped(set_id, peer_id); let ban_dur = Uniform::new(5, 10).sample(&mut rand::thread_rng()); *entry.into_mut() = PeerState::Disabled { diff --git a/client/network/test/src/peerset.rs b/client/network/test/src/peerset.rs index 0be8595d6506f..9ef8f3d627420 100644 --- a/client/network/test/src/peerset.rs +++ b/client/network/test/src/peerset.rs @@ -23,7 +23,7 @@ use rand::{ seq::IteratorRandom, }; use sc_peerset::{ - DropReason, IncomingIndex, Message, Peerset, PeersetConfig, ReputationChange, SetConfig, SetId, + IncomingIndex, Message, Peerset, PeersetConfig, ReputationChange, SetConfig, SetId, }; use std::{ collections::{HashMap, HashSet}, @@ -322,7 +322,7 @@ fn test_once() { last_state = Some(*state); *state = State::Disconnected; - peerset.dropped(SetId::from(0), id, DropReason::Unknown); + peerset.dropped(SetId::from(0), id); current_peer = Some(id); current_event = Some(Event::Disconnected); From 7acf0e3efc1b6aee75aeb0734a86e8d71c583330 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 6 Jun 2023 16:05:42 +0300 Subject: [PATCH 04/37] Move `Message` & `IncomingIndex` from `peerset` to `protocol_controller` --- client/network/src/peerset.rs | 41 ++------------- .../src/protocol/notifications/behaviour.rs | 48 +++++++++--------- client/network/src/protocol_controller.rs | 50 +++++++++++++++---- 3 files changed, 67 insertions(+), 72 deletions(-) diff --git a/client/network/src/peerset.rs b/client/network/src/peerset.rs index 018338d65cbbc..4f15140badf31 100644 --- a/client/network/src/peerset.rs +++ b/client/network/src/peerset.rs @@ -34,7 +34,9 @@ use crate::{ peer_store::{PeerStore, PeerStoreHandle, PeerStoreProvider}, - protocol_controller::{ProtoSetConfig, ProtocolController, ProtocolHandle, SetId}, + protocol_controller::{ + IncomingIndex, Message, ProtoSetConfig, ProtocolController, ProtocolHandle, SetId, + }, }; use futures::{ @@ -125,43 +127,6 @@ impl PeersetHandle { } } -/// Message that can be sent by the peer set manager (PSM). -#[derive(Debug, PartialEq)] -pub enum Message { - /// Request to open a connection to the given peer. From the point of view of the PSM, we are - /// immediately connected. - Connect { - /// Set id to connect on. - set_id: SetId, - /// Peer to connect to. - peer_id: PeerId, - }, - - /// Drop the connection to the given peer, or cancel the connection attempt after a `Connect`. - Drop { - /// Set id to disconnect on. - set_id: SetId, - /// Peer to disconnect from. - peer_id: PeerId, - }, - - /// Equivalent to `Connect` for the peer corresponding to this incoming index. - Accept(IncomingIndex), - - /// Equivalent to `Drop` for the peer corresponding to this incoming index. - Reject(IncomingIndex), -} - -/// Opaque identifier for an incoming connection. Allocated by the network. -#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct IncomingIndex(pub u64); - -impl From for IncomingIndex { - fn from(val: u64) -> Self { - Self(val) - } -} - /// Configuration to pass when creating the peer set manager. #[derive(Debug)] pub struct PeersetConfig { diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 12fe3db46e26f..47b34c451cab2 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -20,7 +20,7 @@ use crate::{ protocol::notifications::handler::{ self, NotificationsSink, NotifsHandler, NotifsHandlerIn, NotifsHandlerOut, }, - protocol_controller::SetId, + protocol_controller::{IncomingIndex, Message, SetId}, types::ProtocolName, }; @@ -132,7 +132,7 @@ pub struct Notifications { /// We generate indices to identify incoming connections. This is the next value for the index /// to use when a connection is incoming. - next_incoming_index: crate::peerset::IncomingIndex, + next_incoming_index: IncomingIndex, /// Events to produce from `poll()`. events: VecDeque>, @@ -231,7 +231,7 @@ enum PeerState { backoff_until: Option, /// Incoming index tracking this connection. - incoming_index: crate::peerset::IncomingIndex, + incoming_index: IncomingIndex, /// List of connections with this peer, and their state. connections: SmallVec<[(ConnectionId, ConnectionState); crate::MAX_CONNECTIONS_PER_PEER]>, @@ -300,7 +300,7 @@ struct IncomingPeer { /// connection corresponding to it has been closed or replaced already. alive: bool, /// Id that the we sent to the peerset. - incoming_id: crate::peerset::IncomingIndex, + incoming_id: IncomingIndex, } /// Event that can be emitted by the `Notifications`. @@ -382,7 +382,7 @@ impl Notifications { delays: Default::default(), next_delay_id: DelayId(0), incoming: SmallVec::new(), - next_incoming_index: crate::peerset::IncomingIndex(0), + next_incoming_index: IncomingIndex(0), events: VecDeque::new(), } } @@ -834,7 +834,7 @@ impl Notifications { /// Function that is called when the peerset wants us to accept a connection /// request from a peer. - fn peerset_report_accept(&mut self, index: crate::peerset::IncomingIndex) { + fn peerset_report_accept(&mut self, index: IncomingIndex) { let incoming = if let Some(pos) = self.incoming.iter().position(|i| i.incoming_id == index) { self.incoming.remove(pos) @@ -920,7 +920,7 @@ impl Notifications { } /// Function that is called when the peerset wants us to reject an incoming peer. - fn peerset_report_reject(&mut self, index: crate::peerset::IncomingIndex) { + fn peerset_report_reject(&mut self, index: IncomingIndex) { let incoming = if let Some(pos) = self.incoming.iter().position(|i| i.incoming_id == index) { self.incoming.remove(pos) @@ -2010,16 +2010,16 @@ impl NetworkBehaviour for Notifications { // Note that the peerset is a *best effort* crate, and we have to use defensive programming. loop { match futures::Stream::poll_next(Pin::new(&mut self.peerset), cx) { - Poll::Ready(Some(crate::peerset::Message::Accept(index))) => { + Poll::Ready(Some(Message::Accept(index))) => { self.peerset_report_accept(index); }, - Poll::Ready(Some(crate::peerset::Message::Reject(index))) => { + Poll::Ready(Some(Message::Reject(index))) => { self.peerset_report_reject(index); }, - Poll::Ready(Some(crate::peerset::Message::Connect { peer_id, set_id, .. })) => { + Poll::Ready(Some(Message::Connect { peer_id, set_id, .. })) => { self.peerset_report_connect(peer_id, set_id); }, - Poll::Ready(Some(crate::peerset::Message::Drop { peer_id, set_id, .. })) => { + Poll::Ready(Some(Message::Drop { peer_id, set_id, .. })) => { self.peerset_report_disconnect(peer_id, set_id); }, Poll::Ready(None) => { @@ -2101,8 +2101,8 @@ impl NetworkBehaviour for Notifications { mod tests { use super::*; use crate::{ - peerset::IncomingIndex, protocol::notifications::handler::tests::*, - protocol_controller::ProtoSetConfig, + protocol::notifications::handler::tests::*, + protocol_controller::{IncomingIndex, ProtoSetConfig}, }; use libp2p::swarm::AddressRecord; use std::{collections::HashSet, iter}; @@ -2310,7 +2310,7 @@ mod tests { assert!(std::matches!( notif.incoming.pop(), - Some(IncomingPeer { alive: true, incoming_id: crate::peerset::IncomingIndex(0), .. }), + Some(IncomingPeer { alive: true, incoming_id: IncomingIndex(0), .. }), )); } @@ -2634,17 +2634,17 @@ mod tests { assert!(std::matches!( notif.incoming[0], - IncomingPeer { alive: true, incoming_id: crate::peerset::IncomingIndex(0), .. }, + IncomingPeer { alive: true, incoming_id: IncomingIndex(0), .. }, )); notif.disconnect_peer(&peer, set_id); assert!(std::matches!(notif.peers.get(&(peer, set_id)), Some(&PeerState::Disabled { .. }))); assert!(std::matches!( notif.incoming[0], - IncomingPeer { alive: false, incoming_id: crate::peerset::IncomingIndex(0), .. }, + IncomingPeer { alive: false, incoming_id: IncomingIndex(0), .. }, )); - notif.peerset_report_accept(crate::peerset::IncomingIndex(0)); + notif.peerset_report_accept(IncomingIndex(0)); assert_eq!(notif.incoming.len(), 0); assert!(std::matches!(notif.peers.get(&(peer, set_id)), Some(PeerState::Disabled { .. }))); } @@ -2779,7 +2779,7 @@ mod tests { assert!(notif.peers.get(&(peer, set_id)).is_none()); assert!(std::matches!( notif.incoming[0], - IncomingPeer { alive: false, incoming_id: crate::peerset::IncomingIndex(0), .. }, + IncomingPeer { alive: false, incoming_id: IncomingIndex(0), .. }, )); } @@ -2885,7 +2885,7 @@ mod tests { // We rely on the implementation detail that incoming indices are counted // from 0 to not mock the `Peerset`. - notif.peerset_report_accept(crate::peerset::IncomingIndex(0)); + notif.peerset_report_accept(IncomingIndex(0)); assert!(std::matches!(notif.peers.get(&(peer, set_id)), Some(&PeerState::Enabled { .. }))); // open new substream @@ -4083,11 +4083,11 @@ mod tests { assert!(std::matches!( notif.incoming[0], - IncomingPeer { alive: true, incoming_id: crate::peerset::IncomingIndex(0), .. }, + IncomingPeer { alive: true, incoming_id: IncomingIndex(0), .. }, )); notif.peers.remove(&(peer, set_id)); - notif.peerset_report_accept(crate::peerset::IncomingIndex(0)); + notif.peerset_report_accept(IncomingIndex(0)); } #[test] @@ -4125,7 +4125,7 @@ mod tests { assert!(std::matches!( notif.incoming[0], - IncomingPeer { alive: true, incoming_id: crate::peerset::IncomingIndex(0), .. }, + IncomingPeer { alive: true, incoming_id: IncomingIndex(0), .. }, )); notif.peerset_report_connect(peer, set_id); @@ -4136,7 +4136,7 @@ mod tests { assert!(std::matches!(notif.peers.get(&(peer, set_id)), Some(&PeerState::Enabled { .. }))); notif.incoming[0].alive = true; - notif.peerset_report_accept(crate::peerset::IncomingIndex(0)); + notif.peerset_report_accept(IncomingIndex(0)); } #[test] @@ -4262,7 +4262,7 @@ mod tests { assert!(std::matches!(notif.peers.get(&(peer, set_id)), Some(&PeerState::Incoming { .. }))); assert!(std::matches!( notif.incoming[0], - IncomingPeer { alive: true, incoming_id: crate::peerset::IncomingIndex(0), .. }, + IncomingPeer { alive: true, incoming_id: IncomingIndex(0), .. }, )); notif.peers.remove(&(peer, set_id)); diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index 03db878d50059..30c0624ddc548 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -52,10 +52,7 @@ use std::{ }; use wasm_timer::Delay; -use crate::{ - peer_store::PeerStoreProvider, - peerset::{IncomingIndex, Message}, -}; +use crate::peer_store::PeerStoreProvider; /// Log target for this file. pub const LOG_TARGET: &str = "peerset"; @@ -109,6 +106,43 @@ pub struct ProtoSetConfig { pub reserved_only: bool, } +/// Message that is sent by [`ProtocolController`] to `Notifications`. +#[derive(Debug, PartialEq)] +pub enum Message { + /// Request to open a connection to the given peer. From the point of view of the PSM, we are + /// immediately connected. + Connect { + /// Set id to connect on. + set_id: SetId, + /// Peer to connect to. + peer_id: PeerId, + }, + + /// Drop the connection to the given peer, or cancel the connection attempt after a `Connect`. + Drop { + /// Set id to disconnect on. + set_id: SetId, + /// Peer to disconnect from. + peer_id: PeerId, + }, + + /// Equivalent to `Connect` for the peer corresponding to this incoming index. + Accept(IncomingIndex), + + /// Equivalent to `Drop` for the peer corresponding to this incoming index. + Reject(IncomingIndex), +} + +/// Opaque identifier for an incoming connection. Allocated by the network. +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct IncomingIndex(pub u64); + +impl From for IncomingIndex { + fn from(val: u64) -> Self { + Self(val) + } +} + /// External API actions. #[derive(Debug)] enum Action { @@ -791,12 +825,8 @@ impl ProtocolController { #[cfg(test)] mod tests { - use super::{Direction, PeerState, ProtoSetConfig, ProtocolController, ProtocolHandle, SetId}; - use crate::{ - peer_store::PeerStoreProvider, - peerset::{IncomingIndex, Message}, - ReputationChange, - }; + use super::*; + use crate::{peer_store::PeerStoreProvider, ReputationChange}; use libp2p::PeerId; use sc_utils::mpsc::{tracing_unbounded, TryRecvError}; use std::collections::HashSet; From 037ead47dc5e87588a3af06b832400042074bfe0 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 6 Jun 2023 16:22:07 +0300 Subject: [PATCH 05/37] Restore running fuzz test --- client/network/src/lib.rs | 4 ++-- client/network/src/peerset.rs | 1 + client/network/src/protocol_controller.rs | 1 + .../network/test/src/{peerset.rs => fuzz.rs} | 18 ++++++++++-------- client/network/test/src/lib.rs | 2 ++ 5 files changed, 16 insertions(+), 10 deletions(-) rename client/network/test/src/{peerset.rs => fuzz.rs} (97%) diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index 9f528b8bec38c..b1f93759ab694 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -244,9 +244,7 @@ mod behaviour; mod peer_store; -mod peerset; mod protocol; -mod protocol_controller; mod service; pub mod config; @@ -255,6 +253,8 @@ pub mod error; pub mod event; pub mod network_state; pub mod peer_info; +pub mod peerset; +pub mod protocol_controller; pub mod request_responses; pub mod transport; pub mod types; diff --git a/client/network/src/peerset.rs b/client/network/src/peerset.rs index 4f15140badf31..4fe504b99774c 100644 --- a/client/network/src/peerset.rs +++ b/client/network/src/peerset.rs @@ -57,6 +57,7 @@ use std::{ use libp2p::PeerId; +/// Log target for this file. pub const LOG_TARGET: &str = "peerset"; #[derive(Debug)] diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index 30c0624ddc548..4881042d7cc8d 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -68,6 +68,7 @@ pub const LOG_TARGET: &str = "peerset"; pub struct SetId(usize); impl SetId { + /// Const conversion function for initialization of hardcoded peerset indices. pub const fn from(id: usize) -> Self { Self(id) } diff --git a/client/network/test/src/peerset.rs b/client/network/test/src/fuzz.rs similarity index 97% rename from client/network/test/src/peerset.rs rename to client/network/test/src/fuzz.rs index 9ef8f3d627420..baf38dba7b24d 100644 --- a/client/network/test/src/peerset.rs +++ b/client/network/test/src/fuzz.rs @@ -17,13 +17,15 @@ // along with this program. If not, see . use futures::prelude::*; -use libp2p_identity::PeerId; +use libp2p::PeerId; use rand::{ distributions::{Distribution, Uniform, WeightedIndex}, seq::IteratorRandom, }; -use sc_peerset::{ - IncomingIndex, Message, Peerset, PeersetConfig, ReputationChange, SetConfig, SetId, +use sc_network::{ + peerset::{Peerset, PeersetConfig}, + protocol_controller::{IncomingIndex, Message, ProtoSetConfig, SetId}, + ReputationChange, }; use std::{ collections::{HashMap, HashSet}, @@ -83,16 +85,16 @@ fn discard_incoming_index(state: State) -> BareState { } } -#[test] -fn run() { +#[tokio::test] +async fn run() { sp_tracing::try_init_simple(); for _ in 0..50 { - test_once(); + test_once().await; } } -fn test_once() { +async fn test_once() { // Allowed events that can be received in a specific state. let allowed_events: HashMap> = [ ( @@ -137,7 +139,7 @@ fn test_once() { id }) .collect(), - sets: vec![SetConfig { + sets: vec![ProtoSetConfig { reserved_nodes: { (0..Uniform::new_inclusive(0, 2).sample(&mut rng)) .map(|_| { diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index a9ff38e4ea608..9f0010f545527 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -20,6 +20,8 @@ #[cfg(test)] mod block_import; #[cfg(test)] +mod fuzz; +#[cfg(test)] mod service; #[cfg(test)] mod sync; From efba474f4e016152764ac1a2e4ee59d5d056415f Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 6 Jun 2023 17:39:57 +0300 Subject: [PATCH 06/37] Get rid of `Peerset` in `fuzz` test --- client/network/src/lib.rs | 2 +- client/network/src/peer_store.rs | 6 + client/network/test/src/fuzz.rs | 514 ++++++++++++++++--------------- 3 files changed, 275 insertions(+), 247 deletions(-) diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index b1f93759ab694..ad5ffa2dffae5 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -243,7 +243,6 @@ //! More precise usage details are still being worked on and will likely change in the future. mod behaviour; -mod peer_store; mod protocol; mod service; @@ -253,6 +252,7 @@ pub mod error; pub mod event; pub mod network_state; pub mod peer_info; +pub mod peer_store; pub mod peerset; pub mod protocol_controller; pub mod request_responses; diff --git a/client/network/src/peer_store.rs b/client/network/src/peer_store.rs index 59886c335784b..2f3d4a1fd1a0b 100644 --- a/client/network/src/peer_store.rs +++ b/client/network/src/peer_store.rs @@ -16,6 +16,9 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . +//! [`PeerStore`] manages peer reputations and provides connection candidates to +//! [`crate::protocol_controller::ProtocolController`]. + use libp2p::PeerId; use log::trace; use parking_lot::Mutex; @@ -49,6 +52,7 @@ const INVERSE_DECREMENT: i32 = 50; /// remove it, once the reputation value reaches 0. const FORGET_AFTER: Duration = Duration::from_secs(3600); +/// Trait providing peer reputation management and connection candidates. pub trait PeerStoreProvider: Debug + Send { /// Check whether the peer is banned. fn is_banned(&self, peer_id: &PeerId) -> bool; @@ -69,6 +73,7 @@ pub trait PeerStoreProvider: Debug + Send { fn outgoing_candidates(&self, count: usize, ignored: HashSet<&PeerId>) -> Vec; } +/// Actual implementation of peer reputations and connection candidates provider. #[derive(Debug, Clone)] pub struct PeerStoreHandle { inner: Arc>, @@ -289,6 +294,7 @@ impl PeerStoreInner { } } +/// Worker part of [`PeerStoreHandle`] #[derive(Debug)] pub struct PeerStore { inner: Arc>, diff --git a/client/network/test/src/fuzz.rs b/client/network/test/src/fuzz.rs index baf38dba7b24d..1487862062548 100644 --- a/client/network/test/src/fuzz.rs +++ b/client/network/test/src/fuzz.rs @@ -16,6 +16,9 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . +//! Fuzz test emulates network events and peer connection handling by `ProtocolController` +//! and `PeerStore` to discover possible inconsistencies in peer management. + use futures::prelude::*; use libp2p::PeerId; use rand::{ @@ -23,13 +26,13 @@ use rand::{ seq::IteratorRandom, }; use sc_network::{ - peerset::{Peerset, PeersetConfig}, - protocol_controller::{IncomingIndex, Message, ProtoSetConfig, SetId}, + peer_store::{PeerStore, PeerStoreProvider}, + protocol_controller::{IncomingIndex, Message, ProtoSetConfig, ProtocolController, SetId}, ReputationChange, }; +use sc_utils::mpsc::tracing_unbounded; use std::{ collections::{HashMap, HashSet}, - pin::Pin, task::Poll, }; @@ -131,15 +134,23 @@ async fn test_once() { // Nodes that we have reserved. Always a subset of `known_nodes`. let mut reserved_nodes = HashSet::::new(); - let (mut peerset, peerset_handle) = Peerset::from_config(PeersetConfig { - bootnodes: (0..Uniform::new_inclusive(0, 4).sample(&mut rng)) - .map(|_| { - let id = PeerId::random(); - known_nodes.insert(id, State::Disconnected); - id - }) - .collect(), - sets: vec![ProtoSetConfig { + // Bootnodes for `PeerStore` initialization. + let bootnodes = (0..Uniform::new_inclusive(0, 4).sample(&mut rng)) + .map(|_| { + let id = PeerId::random(); + known_nodes.insert(id, State::Disconnected); + id + }) + .collect(); + + let peer_store = PeerStore::new(bootnodes); + let mut peer_store_handle = peer_store.handle(); + + let (to_notifications, mut from_controller) = + tracing_unbounded("test_to_notifications", 10_000); + let (protocol_handle, protocol_controller) = ProtocolController::new( + SetId::from(0), + ProtoSetConfig { reserved_nodes: { (0..Uniform::new_inclusive(0, 2).sample(&mut rng)) .map(|_| { @@ -153,252 +164,263 @@ async fn test_once() { in_peers: Uniform::new_inclusive(0, 25).sample(&mut rng), out_peers: Uniform::new_inclusive(0, 25).sample(&mut rng), reserved_only: Uniform::new_inclusive(0, 10).sample(&mut rng) == 0, - }], - }); - - let new_id = PeerId::random(); - known_nodes.insert(new_id, State::Disconnected); - peerset_handle.add_known_peer(new_id); - - futures::executor::block_on(futures::future::poll_fn(move |cx| { - // List of nodes the user of `peerset` assumes it's connected to. Always a subset of - // `known_nodes`. - let mut connected_nodes = HashSet::::new(); - // List of nodes the user of `peerset` called `incoming` with and that haven't been - // accepted or rejected yet. - let mut incoming_nodes = HashMap::::new(); - // Next id for incoming connections. - let mut next_incoming_id = IncomingIndex(0); - - // Perform a certain number of actions while checking that the state is consistent. If we - // reach the end of the loop, the run has succeeded. - // Note that with the ACKing and event postponing mechanism in `ProtocolController` - // the test time grows quadratically with the number of iterations below. - for _ in 0..2500 { - // Peer we are working with. - let mut current_peer = None; - // Current event for event bigrams validation. - let mut current_event = None; - // Last peer state for allowed event validation. - let mut last_state = None; - - // Each of these weights corresponds to an action that we may perform. - let action_weights = [150, 90, 90, 30, 30, 1, 1, 4, 4]; - match WeightedIndex::new(&action_weights).unwrap().sample(&mut rng) { - // If we generate 0, poll the peerset. - 0 => match Stream::poll_next(Pin::new(&mut peerset), cx) { - Poll::Ready(Some(Message::Connect { peer_id, .. })) => { - log::info!("PSM: connecting to peer {}", peer_id); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - if matches!(*state, State::Incoming(_)) { - log::info!( - "Awaiting incoming response, ignoring obsolete Connect from PSM for peer {}", - peer_id, - ); - continue - } - - last_state = Some(*state); - - if *state != State::Inbound { - *state = State::Outbound; - } - - if !connected_nodes.insert(peer_id) { - log::info!("Request to connect to an already connected node {peer_id}"); - } - - current_peer = Some(peer_id); - current_event = Some(Event::PsmConnect); - }, - Poll::Ready(Some(Message::Drop { peer_id, .. })) => { - log::info!("PSM: dropping peer {}", peer_id); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - if matches!(*state, State::Incoming(_)) { - log::info!( - "Awaiting incoming response, ignoring obsolete Drop from PSM for peer {}", - peer_id, - ); - continue - } - - last_state = Some(*state); - *state = State::Disconnected; - - if !connected_nodes.remove(&peer_id) { - log::info!("Ignoring attempt to drop a disconnected peer {}", peer_id); - } - - current_peer = Some(peer_id); - current_event = Some(Event::PsmDrop); - }, - Poll::Ready(Some(Message::Accept(n))) => { - log::info!("PSM: accepting index {}", n.0); - - let peer_id = incoming_nodes.remove(&n).unwrap(); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - match *state { - State::Incoming(incoming_index) => - if n.0 < incoming_index.0 { - log::info!( - "Ignoring obsolete Accept for {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - continue - } else if n.0 > incoming_index.0 { - panic!( - "Received {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - }, - _ => {}, - } - - last_state = Some(*state); - *state = State::Inbound; - - assert!(connected_nodes.insert(peer_id)); - - current_peer = Some(peer_id); - current_event = Some(Event::PsmAccept); - }, - Poll::Ready(Some(Message::Reject(n))) => { - log::info!("PSM: rejecting index {}", n.0); - - let peer_id = incoming_nodes.remove(&n).unwrap(); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - match *state { - State::Incoming(incoming_index) => - if n.0 < incoming_index.0 { - log::info!( - "Ignoring obsolete Reject for {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - continue - } else if n.0 > incoming_index.0 { - panic!( - "Received {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - }, - _ => {}, - } - - last_state = Some(*state); - *state = State::Disconnected; - - assert!(!connected_nodes.contains(&peer_id)); - - current_peer = Some(peer_id); - current_event = Some(Event::PsmReject); - }, - Poll::Ready(None) => panic!(), - Poll::Pending => {}, - }, + }, + to_notifications, + Box::new(peer_store_handle.clone()), + ); + + let mut peer_store_future = peer_store.run().boxed(); + let mut protocol_controller_future = protocol_controller.run().boxed(); + + // List of nodes the user of `peerset` assumes it's connected to. Always a subset of + // `known_nodes`. + let mut connected_nodes = HashSet::::new(); + // List of nodes the user of `peerset` called `incoming` with and that haven't been + // accepted or rejected yet. + let mut incoming_nodes = HashMap::::new(); + // Next id for incoming connections. + let mut next_incoming_id = IncomingIndex(0); + + // Perform a certain number of actions while checking that the state is consistent. If we + // reach the end of the loop, the run has succeeded. + // Note that with the ACKing and event postponing mechanism in `ProtocolController` + // the test time grows quadratically with the number of iterations below. + for _ in 0..2500 { + // Peer we are working with. + let mut current_peer = None; + // Current event for state transition validation. + let mut current_event = None; + // Last peer state for allowed event validation. + let mut last_state = None; + + // Each of these weights corresponds to an action that we may perform. + let action_weights = [150, 90, 90, 30, 30, 1, 1, 4, 4]; + + // We make sure to poll `PeerStore` and `ProtocolController` every time we try to grab + // a message from `from_controller`. This speeds the test up 10x compared to spawning + // `PeerStore` and `ProtocolController` runners as separate tasks. + let next_message = futures::future::poll_fn(|cx| { + let _ = peer_store_future.poll_unpin(cx); + let _ = protocol_controller_future.poll_unpin(cx); + + if let Poll::Ready(Some(msg)) = from_controller.poll_next_unpin(cx) { + Poll::Ready(Some(msg)) + } else { + Poll::Pending + } + }); + + match WeightedIndex::new(&action_weights).unwrap().sample(&mut rng) { + // If we generate 0, try to grab the next message from `ProtocolController`. + 0 => match futures::poll!(next_message) { + Poll::Ready(Some(Message::Connect { peer_id, .. })) => { + log::info!("PSM: connecting to peer {}", peer_id); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + if matches!(*state, State::Incoming(_)) { + log::info!( + "Awaiting incoming response, ignoring obsolete Connect from PSM for peer {}", + peer_id, + ); + continue + } + + last_state = Some(*state); + + if *state != State::Inbound { + *state = State::Outbound; + } + + if !connected_nodes.insert(peer_id) { + log::info!("Request to connect to an already connected node {peer_id}"); + } - // If we generate 1, discover a new node. - 1 => { - let new_id = PeerId::random(); - known_nodes.insert(new_id, State::Disconnected); - peerset_handle.add_known_peer(new_id); + current_peer = Some(peer_id); + current_event = Some(Event::PsmConnect); }, + Poll::Ready(Some(Message::Drop { peer_id, .. })) => { + log::info!("PSM: dropping peer {}", peer_id); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + if matches!(*state, State::Incoming(_)) { + log::info!( + "Awaiting incoming response, ignoring obsolete Drop from PSM for peer {}", + peer_id, + ); + continue + } + + last_state = Some(*state); + *state = State::Disconnected; - // If we generate 2, adjust a random reputation. - 2 => - if let Some(id) = known_nodes.keys().choose(&mut rng) { - let val = Uniform::new_inclusive(i32::MIN, i32::MAX).sample(&mut rng); - peerset_handle.report_peer(*id, ReputationChange::new(val, "")); - }, - - // If we generate 3, disconnect from a random node. - 3 => - if let Some(id) = connected_nodes.iter().choose(&mut rng).cloned() { - log::info!("Disconnected from {}", id); - connected_nodes.remove(&id); - - let state = known_nodes.get_mut(&id).unwrap(); - last_state = Some(*state); - *state = State::Disconnected; - - peerset.dropped(SetId::from(0), id); - - current_peer = Some(id); - current_event = Some(Event::Disconnected); - }, - - // If we generate 4, connect to a random node. - 4 => { - if let Some(id) = known_nodes - .keys() - .filter(|n| { - incoming_nodes.values().all(|m| m != *n) && - !connected_nodes.contains(*n) - }) - .choose(&mut rng) - .cloned() - { - log::info!("Incoming connection from {}, index {}", id, next_incoming_id.0); - peerset.incoming(SetId::from(0), id, next_incoming_id); - incoming_nodes.insert(next_incoming_id, id); - - let state = known_nodes.get_mut(&id).unwrap(); - last_state = Some(*state); - *state = State::Incoming(next_incoming_id); - - next_incoming_id.0 += 1; - - current_peer = Some(id); - current_event = Some(Event::Incoming); + if !connected_nodes.remove(&peer_id) { + log::info!("Ignoring attempt to drop a disconnected peer {}", peer_id); } + + current_peer = Some(peer_id); + current_event = Some(Event::PsmDrop); }, + Poll::Ready(Some(Message::Accept(n))) => { + log::info!("PSM: accepting index {}", n.0); + + let peer_id = incoming_nodes.remove(&n).unwrap(); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + match *state { + State::Incoming(incoming_index) => + if n.0 < incoming_index.0 { + log::info!( + "Ignoring obsolete Accept for {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + continue + } else if n.0 > incoming_index.0 { + panic!( + "Received {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + }, + _ => {}, + } + + last_state = Some(*state); + *state = State::Inbound; + + assert!(connected_nodes.insert(peer_id)); - // 5 and 6 are the reserved-only mode. - 5 => { - log::info!("Set reserved only"); - peerset_handle.set_reserved_only(SetId::from(0), true); + current_peer = Some(peer_id); + current_event = Some(Event::PsmAccept); }, - 6 => { - log::info!("Unset reserved only"); - peerset_handle.set_reserved_only(SetId::from(0), false); + Poll::Ready(Some(Message::Reject(n))) => { + log::info!("PSM: rejecting index {}", n.0); + + let peer_id = incoming_nodes.remove(&n).unwrap(); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + match *state { + State::Incoming(incoming_index) => + if n.0 < incoming_index.0 { + log::info!( + "Ignoring obsolete Reject for {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + continue + } else if n.0 > incoming_index.0 { + panic!( + "Received {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + }, + _ => {}, + } + + last_state = Some(*state); + *state = State::Disconnected; + + assert!(!connected_nodes.contains(&peer_id)); + + current_peer = Some(peer_id); + current_event = Some(Event::PsmReject); }, + Poll::Ready(None) => panic!(), + Poll::Pending => {}, + }, - // 7 and 8 are about switching a random node in or out of reserved mode. - 7 => { - if let Some(id) = - known_nodes.keys().filter(|n| !reserved_nodes.contains(*n)).choose(&mut rng) - { - log::info!("Add reserved: {}", id); - peerset_handle.add_reserved_peer(SetId::from(0), *id); - reserved_nodes.insert(*id); - } + // If we generate 1, discover a new node. + 1 => { + let new_id = PeerId::random(); + known_nodes.insert(new_id, State::Disconnected); + peer_store_handle.add_known_peer(new_id); + }, + + // If we generate 2, adjust a random reputation. + 2 => + if let Some(id) = known_nodes.keys().choose(&mut rng) { + let val = Uniform::new_inclusive(i32::MIN, i32::MAX).sample(&mut rng); + peer_store_handle.report_peer(*id, ReputationChange::new(val, "")); + }, + + // If we generate 3, disconnect from a random node. + 3 => + if let Some(id) = connected_nodes.iter().choose(&mut rng).cloned() { + log::info!("Disconnected from {}", id); + connected_nodes.remove(&id); + + let state = known_nodes.get_mut(&id).unwrap(); + last_state = Some(*state); + *state = State::Disconnected; + + protocol_handle.dropped(id); + + current_peer = Some(id); + current_event = Some(Event::Disconnected); }, - 8 => - if let Some(id) = reserved_nodes.iter().choose(&mut rng).cloned() { - log::info!("Remove reserved: {}", id); - reserved_nodes.remove(&id); - peerset_handle.remove_reserved_peer(SetId::from(0), id); - }, - - _ => unreachable!(), - } - // Validate event bigrams and state transitions. - if let Some(peer_id) = current_peer { - let event = current_event.unwrap(); - let last_state = discard_incoming_index(last_state.unwrap()); - if !allowed_events.get(&last_state).unwrap().contains(&event) { - panic!( - "Invalid state transition: {:?} x {:?} for peer {}", - last_state, event, peer_id, - ); + // If we generate 4, connect to a random node. + 4 => { + if let Some(id) = known_nodes + .keys() + .filter(|n| { + incoming_nodes.values().all(|m| m != *n) && !connected_nodes.contains(*n) + }) + .choose(&mut rng) + .cloned() + { + log::info!("Incoming connection from {}, index {}", id, next_incoming_id.0); + protocol_handle.incoming_connection(id, next_incoming_id); + incoming_nodes.insert(next_incoming_id, id); + + let state = known_nodes.get_mut(&id).unwrap(); + last_state = Some(*state); + *state = State::Incoming(next_incoming_id); + + next_incoming_id.0 += 1; + + current_peer = Some(id); + current_event = Some(Event::Incoming); } - } + }, + + // 5 and 6 are the reserved-only mode. + 5 => { + log::info!("Set reserved only"); + protocol_handle.set_reserved_only(true); + }, + 6 => { + log::info!("Unset reserved only"); + protocol_handle.set_reserved_only(false); + }, + + // 7 and 8 are about switching a random node in or out of reserved mode. + 7 => { + if let Some(id) = + known_nodes.keys().filter(|n| !reserved_nodes.contains(*n)).choose(&mut rng) + { + log::info!("Add reserved: {}", id); + protocol_handle.add_reserved_peer(*id); + reserved_nodes.insert(*id); + } + }, + 8 => + if let Some(id) = reserved_nodes.iter().choose(&mut rng).cloned() { + log::info!("Remove reserved: {}", id); + reserved_nodes.remove(&id); + protocol_handle.remove_reserved_peer(id); + }, + + _ => unreachable!(), } - Poll::Ready(()) - })); + // Validate state transitions. + if let Some(peer_id) = current_peer { + let event = current_event.unwrap(); + let last_state = discard_incoming_index(last_state.unwrap()); + if !allowed_events.get(&last_state).unwrap().contains(&event) { + panic!( + "Invalid state transition: {:?} x {:?} for peer {}", + last_state, event, peer_id, + ); + } + } + } } From 911d26f95c2ab6fec4c62a1acfbc09bce957317e Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 7 Jun 2023 11:40:01 +0300 Subject: [PATCH 07/37] Spawn runners instead of manual polling in `fuzz` test --- client/network/test/src/fuzz.rs | 460 ++++++++++++++++---------------- 1 file changed, 226 insertions(+), 234 deletions(-) diff --git a/client/network/test/src/fuzz.rs b/client/network/test/src/fuzz.rs index 1487862062548..2e288accd80bc 100644 --- a/client/network/test/src/fuzz.rs +++ b/client/network/test/src/fuzz.rs @@ -31,10 +31,7 @@ use sc_network::{ ReputationChange, }; use sc_utils::mpsc::tracing_unbounded; -use std::{ - collections::{HashMap, HashSet}, - task::Poll, -}; +use std::collections::{HashMap, HashSet}; /// Peer events as observed by `Notifications` / fuzz test. #[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)] @@ -169,8 +166,8 @@ async fn test_once() { Box::new(peer_store_handle.clone()), ); - let mut peer_store_future = peer_store.run().boxed(); - let mut protocol_controller_future = protocol_controller.run().boxed(); + tokio::spawn(peer_store.run()); + tokio::spawn(protocol_controller.run()); // List of nodes the user of `peerset` assumes it's connected to. Always a subset of // `known_nodes`. @@ -181,246 +178,241 @@ async fn test_once() { // Next id for incoming connections. let mut next_incoming_id = IncomingIndex(0); - // Perform a certain number of actions while checking that the state is consistent. If we - // reach the end of the loop, the run has succeeded. - // Note that with the ACKing and event postponing mechanism in `ProtocolController` - // the test time grows quadratically with the number of iterations below. - for _ in 0..2500 { - // Peer we are working with. - let mut current_peer = None; - // Current event for state transition validation. - let mut current_event = None; - // Last peer state for allowed event validation. - let mut last_state = None; - - // Each of these weights corresponds to an action that we may perform. - let action_weights = [150, 90, 90, 30, 30, 1, 1, 4, 4]; - - // We make sure to poll `PeerStore` and `ProtocolController` every time we try to grab - // a message from `from_controller`. This speeds the test up 10x compared to spawning - // `PeerStore` and `ProtocolController` runners as separate tasks. - let next_message = futures::future::poll_fn(|cx| { - let _ = peer_store_future.poll_unpin(cx); - let _ = protocol_controller_future.poll_unpin(cx); - - if let Poll::Ready(Some(msg)) = from_controller.poll_next_unpin(cx) { - Poll::Ready(Some(msg)) - } else { - Poll::Pending - } - }); - - match WeightedIndex::new(&action_weights).unwrap().sample(&mut rng) { - // If we generate 0, try to grab the next message from `ProtocolController`. - 0 => match futures::poll!(next_message) { - Poll::Ready(Some(Message::Connect { peer_id, .. })) => { - log::info!("PSM: connecting to peer {}", peer_id); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - if matches!(*state, State::Incoming(_)) { - log::info!( - "Awaiting incoming response, ignoring obsolete Connect from PSM for peer {}", - peer_id, - ); - continue - } - - last_state = Some(*state); - - if *state != State::Inbound { - *state = State::Outbound; - } - - if !connected_nodes.insert(peer_id) { - log::info!("Request to connect to an already connected node {peer_id}"); - } - - current_peer = Some(peer_id); - current_event = Some(Event::PsmConnect); + // The loop below is effectively synchronous, so for `PeerStore` & `ProtocolController` + // runners, spawned above, to advance, we use `spawn_blocking`. + let _ = tokio::task::spawn_blocking(move || { + // PRNG to use in `spawn_blocking` context. + let mut rng = rand::thread_rng(); + + // Perform a certain number of actions while checking that the state is consistent. If we + // reach the end of the loop, the run has succeeded. + // Note that with the ACKing and event postponing mechanism in `ProtocolController` + // the test time grows quadratically with the number of iterations below. + for _ in 0..2500 { + // Peer we are working with. + let mut current_peer = None; + // Current event for state transition validation. + let mut current_event = None; + // Last peer state for allowed event validation. + let mut last_state = None; + + // Each of these weights corresponds to an action that we may perform. + let action_weights = [150, 90, 90, 30, 30, 1, 1, 4, 4]; + + match WeightedIndex::new(&action_weights).unwrap().sample(&mut rng) { + // If we generate 0, try to grab the next message from `ProtocolController`. + 0 => match from_controller.next().now_or_never() { + Some(Some(Message::Connect { peer_id, .. })) => { + log::info!("PSM: connecting to peer {}", peer_id); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + if matches!(*state, State::Incoming(_)) { + log::info!( + "Awaiting incoming response, ignoring obsolete Connect from PSM for peer {}", + peer_id, + ); + continue + } + + last_state = Some(*state); + + if *state != State::Inbound { + *state = State::Outbound; + } + + if !connected_nodes.insert(peer_id) { + log::info!("Request to connect to an already connected node {peer_id}"); + } + + current_peer = Some(peer_id); + current_event = Some(Event::PsmConnect); + }, + Some(Some(Message::Drop { peer_id, .. })) => { + log::info!("PSM: dropping peer {}", peer_id); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + if matches!(*state, State::Incoming(_)) { + log::info!( + "Awaiting incoming response, ignoring obsolete Drop from PSM for peer {}", + peer_id, + ); + continue + } + + last_state = Some(*state); + *state = State::Disconnected; + + if !connected_nodes.remove(&peer_id) { + log::info!("Ignoring attempt to drop a disconnected peer {}", peer_id); + } + + current_peer = Some(peer_id); + current_event = Some(Event::PsmDrop); + }, + Some(Some(Message::Accept(n))) => { + log::info!("PSM: accepting index {}", n.0); + + let peer_id = incoming_nodes.remove(&n).unwrap(); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + match *state { + State::Incoming(incoming_index) => + if n.0 < incoming_index.0 { + log::info!( + "Ignoring obsolete Accept for {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + continue + } else if n.0 > incoming_index.0 { + panic!( + "Received {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + }, + _ => {}, + } + + last_state = Some(*state); + *state = State::Inbound; + + assert!(connected_nodes.insert(peer_id)); + + current_peer = Some(peer_id); + current_event = Some(Event::PsmAccept); + }, + Some(Some(Message::Reject(n))) => { + log::info!("PSM: rejecting index {}", n.0); + + let peer_id = incoming_nodes.remove(&n).unwrap(); + + let state = known_nodes.get_mut(&peer_id).unwrap(); + match *state { + State::Incoming(incoming_index) => + if n.0 < incoming_index.0 { + log::info!( + "Ignoring obsolete Reject for {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + continue + } else if n.0 > incoming_index.0 { + panic!( + "Received {:?} while awaiting {:?} for peer {}", + n, incoming_index, peer_id, + ); + }, + _ => {}, + } + + last_state = Some(*state); + *state = State::Disconnected; + + assert!(!connected_nodes.contains(&peer_id)); + + current_peer = Some(peer_id); + current_event = Some(Event::PsmReject); + }, + Some(None) => panic!(), + None => {}, }, - Poll::Ready(Some(Message::Drop { peer_id, .. })) => { - log::info!("PSM: dropping peer {}", peer_id); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - if matches!(*state, State::Incoming(_)) { - log::info!( - "Awaiting incoming response, ignoring obsolete Drop from PSM for peer {}", - peer_id, - ); - continue - } - - last_state = Some(*state); - *state = State::Disconnected; - if !connected_nodes.remove(&peer_id) { - log::info!("Ignoring attempt to drop a disconnected peer {}", peer_id); - } - - current_peer = Some(peer_id); - current_event = Some(Event::PsmDrop); + // If we generate 1, discover a new node. + 1 => { + let new_id = PeerId::random(); + known_nodes.insert(new_id, State::Disconnected); + peer_store_handle.add_known_peer(new_id); }, - Poll::Ready(Some(Message::Accept(n))) => { - log::info!("PSM: accepting index {}", n.0); - - let peer_id = incoming_nodes.remove(&n).unwrap(); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - match *state { - State::Incoming(incoming_index) => - if n.0 < incoming_index.0 { - log::info!( - "Ignoring obsolete Accept for {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - continue - } else if n.0 > incoming_index.0 { - panic!( - "Received {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - }, - _ => {}, - } - - last_state = Some(*state); - *state = State::Inbound; - assert!(connected_nodes.insert(peer_id)); - - current_peer = Some(peer_id); - current_event = Some(Event::PsmAccept); - }, - Poll::Ready(Some(Message::Reject(n))) => { - log::info!("PSM: rejecting index {}", n.0); - - let peer_id = incoming_nodes.remove(&n).unwrap(); - - let state = known_nodes.get_mut(&peer_id).unwrap(); - match *state { - State::Incoming(incoming_index) => - if n.0 < incoming_index.0 { - log::info!( - "Ignoring obsolete Reject for {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - continue - } else if n.0 > incoming_index.0 { - panic!( - "Received {:?} while awaiting {:?} for peer {}", - n, incoming_index, peer_id, - ); - }, - _ => {}, + // If we generate 2, adjust a random reputation. + 2 => + if let Some(id) = known_nodes.keys().choose(&mut rng) { + let val = Uniform::new_inclusive(i32::MIN, i32::MAX).sample(&mut rng); + peer_store_handle.report_peer(*id, ReputationChange::new(val, "")); + }, + + // If we generate 3, disconnect from a random node. + 3 => + if let Some(id) = connected_nodes.iter().choose(&mut rng).cloned() { + log::info!("Disconnected from {}", id); + connected_nodes.remove(&id); + + let state = known_nodes.get_mut(&id).unwrap(); + last_state = Some(*state); + *state = State::Disconnected; + + protocol_handle.dropped(id); + + current_peer = Some(id); + current_event = Some(Event::Disconnected); + }, + + // If we generate 4, connect to a random node. + 4 => { + if let Some(id) = known_nodes + .keys() + .filter(|n| { + incoming_nodes.values().all(|m| m != *n) && + !connected_nodes.contains(*n) + }) + .choose(&mut rng) + .cloned() + { + log::info!("Incoming connection from {}, index {}", id, next_incoming_id.0); + protocol_handle.incoming_connection(id, next_incoming_id); + incoming_nodes.insert(next_incoming_id, id); + + let state = known_nodes.get_mut(&id).unwrap(); + last_state = Some(*state); + *state = State::Incoming(next_incoming_id); + + next_incoming_id.0 += 1; + + current_peer = Some(id); + current_event = Some(Event::Incoming); } - - last_state = Some(*state); - *state = State::Disconnected; - - assert!(!connected_nodes.contains(&peer_id)); - - current_peer = Some(peer_id); - current_event = Some(Event::PsmReject); }, - Poll::Ready(None) => panic!(), - Poll::Pending => {}, - }, - - // If we generate 1, discover a new node. - 1 => { - let new_id = PeerId::random(); - known_nodes.insert(new_id, State::Disconnected); - peer_store_handle.add_known_peer(new_id); - }, - // If we generate 2, adjust a random reputation. - 2 => - if let Some(id) = known_nodes.keys().choose(&mut rng) { - let val = Uniform::new_inclusive(i32::MIN, i32::MAX).sample(&mut rng); - peer_store_handle.report_peer(*id, ReputationChange::new(val, "")); + // 5 and 6 are the reserved-only mode. + 5 => { + log::info!("Set reserved only"); + protocol_handle.set_reserved_only(true); }, - - // If we generate 3, disconnect from a random node. - 3 => - if let Some(id) = connected_nodes.iter().choose(&mut rng).cloned() { - log::info!("Disconnected from {}", id); - connected_nodes.remove(&id); - - let state = known_nodes.get_mut(&id).unwrap(); - last_state = Some(*state); - *state = State::Disconnected; - - protocol_handle.dropped(id); - - current_peer = Some(id); - current_event = Some(Event::Disconnected); + 6 => { + log::info!("Unset reserved only"); + protocol_handle.set_reserved_only(false); }, - // If we generate 4, connect to a random node. - 4 => { - if let Some(id) = known_nodes - .keys() - .filter(|n| { - incoming_nodes.values().all(|m| m != *n) && !connected_nodes.contains(*n) - }) - .choose(&mut rng) - .cloned() - { - log::info!("Incoming connection from {}, index {}", id, next_incoming_id.0); - protocol_handle.incoming_connection(id, next_incoming_id); - incoming_nodes.insert(next_incoming_id, id); - - let state = known_nodes.get_mut(&id).unwrap(); - last_state = Some(*state); - *state = State::Incoming(next_incoming_id); - - next_incoming_id.0 += 1; - - current_peer = Some(id); - current_event = Some(Event::Incoming); - } - }, - - // 5 and 6 are the reserved-only mode. - 5 => { - log::info!("Set reserved only"); - protocol_handle.set_reserved_only(true); - }, - 6 => { - log::info!("Unset reserved only"); - protocol_handle.set_reserved_only(false); - }, - - // 7 and 8 are about switching a random node in or out of reserved mode. - 7 => { - if let Some(id) = - known_nodes.keys().filter(|n| !reserved_nodes.contains(*n)).choose(&mut rng) - { - log::info!("Add reserved: {}", id); - protocol_handle.add_reserved_peer(*id); - reserved_nodes.insert(*id); - } - }, - 8 => - if let Some(id) = reserved_nodes.iter().choose(&mut rng).cloned() { - log::info!("Remove reserved: {}", id); - reserved_nodes.remove(&id); - protocol_handle.remove_reserved_peer(id); + // 7 and 8 are about switching a random node in or out of reserved mode. + 7 => { + if let Some(id) = + known_nodes.keys().filter(|n| !reserved_nodes.contains(*n)).choose(&mut rng) + { + log::info!("Add reserved: {}", id); + protocol_handle.add_reserved_peer(*id); + reserved_nodes.insert(*id); + } }, + 8 => + if let Some(id) = reserved_nodes.iter().choose(&mut rng).cloned() { + log::info!("Remove reserved: {}", id); + reserved_nodes.remove(&id); + protocol_handle.remove_reserved_peer(id); + }, + + _ => unreachable!(), + } - _ => unreachable!(), - } - - // Validate state transitions. - if let Some(peer_id) = current_peer { - let event = current_event.unwrap(); - let last_state = discard_incoming_index(last_state.unwrap()); - if !allowed_events.get(&last_state).unwrap().contains(&event) { - panic!( - "Invalid state transition: {:?} x {:?} for peer {}", - last_state, event, peer_id, - ); + // Validate state transitions. + if let Some(peer_id) = current_peer { + let event = current_event.unwrap(); + let last_state = discard_incoming_index(last_state.unwrap()); + if !allowed_events.get(&last_state).unwrap().contains(&event) { + panic!( + "Invalid state transition: {:?} x {:?} for peer {}", + last_state, event, peer_id, + ); + } } } - } + }) + .await; } From 25b63cd7013ec0eee270bb2703b57eaee7b06bcd Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 7 Jun 2023 18:25:49 +0300 Subject: [PATCH 08/37] Migrate `Protocol` from `Peerset` to `PeerStore` & `ProtocolController` --- client/network/src/protocol.rs | 103 ++++++++++----------------------- 1 file changed, 32 insertions(+), 71 deletions(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 885262fb6d5b7..c0d5cb6a1c11a 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -17,9 +17,9 @@ // along with this program. If not, see . use crate::{ - config::{self, NonReservedPeerMode}, - error, - protocol_controller::{ProtoSetConfig, SetId}, + config, error, + peer_store::{PeerStoreHandle, PeerStoreProvider}, + protocol_controller::{self, SetId}, types::ProtocolName, ReputationChange, }; @@ -80,7 +80,11 @@ type PendingSyncSubstreamValidation = // Lock must always be taken in order declared here. pub struct Protocol { /// Used to report reputation changes. - peerset_handle: crate::peerset::PeersetHandle, + peer_store_handle: PeerStoreHandle, + /// Used to modify reserved peers. + protocol_controller_handles: Vec, + /// Shortcut for sync protocol controller handle. + sync_protocol_controller_handle: protocol_controller::ProtocolHandle, /// Handles opening the unique substream and sending and receiving raw messages. behaviour: Notifications, /// List of notifications protocols that have been registered. @@ -106,61 +110,13 @@ impl Protocol { network_config: &config::NetworkConfiguration, notification_protocols: Vec, block_announces_protocol: config::NonDefaultSetConfig, + peer_store_handle: PeerStoreHandle, + protocol_controller_handles: Vec, tx: TracingUnboundedSender>, - ) -> error::Result<(Self, crate::peerset::PeersetHandle, Vec<(PeerId, Multiaddr)>)> { - let mut known_addresses = Vec::new(); - - let (peerset, peerset_handle) = { - let mut sets = - Vec::with_capacity(NUM_HARDCODED_PEERSETS + notification_protocols.len()); - - let mut default_sets_reserved = HashSet::new(); - for reserved in network_config.default_peers_set.reserved_nodes.iter() { - default_sets_reserved.insert(reserved.peer_id); - - if !reserved.multiaddr.is_empty() { - known_addresses.push((reserved.peer_id, reserved.multiaddr.clone())); - } - } - - let mut bootnodes = Vec::with_capacity(network_config.boot_nodes.len()); - for bootnode in network_config.boot_nodes.iter() { - bootnodes.push(bootnode.peer_id); - } - - // Set number 0 is used for block announces. - sets.push(ProtoSetConfig { - in_peers: network_config.default_peers_set.in_peers, - out_peers: network_config.default_peers_set.out_peers, - reserved_nodes: default_sets_reserved.clone(), - reserved_only: network_config.default_peers_set.non_reserved_mode == - NonReservedPeerMode::Deny, - }); - - for set_cfg in ¬ification_protocols { - let mut reserved_nodes = HashSet::new(); - for reserved in set_cfg.set_config.reserved_nodes.iter() { - reserved_nodes.insert(reserved.peer_id); - known_addresses.push((reserved.peer_id, reserved.multiaddr.clone())); - } - - let reserved_only = - set_cfg.set_config.non_reserved_mode == NonReservedPeerMode::Deny; - - sets.push(ProtoSetConfig { - in_peers: set_cfg.set_config.in_peers, - out_peers: set_cfg.set_config.out_peers, - reserved_nodes, - reserved_only, - }); - } - - crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { bootnodes, sets }) - }; - + ) -> error::Result { let behaviour = { Notifications::new( - peerset, + protocol_controller_handles.clone(), // NOTE: Block announcement protocol is still very much hardcoded into `Protocol`. // This protocol must be the first notification protocol given to // `Notifications` @@ -179,8 +135,13 @@ impl Protocol { ) }; + let sync_protocol_controller_handle = + protocol_controller_handles[usize::from(HARDCODED_PEERSETS_SYNC)].clone(); + let protocol = Self { - peerset_handle: peerset_handle.clone(), + peer_store_handle, + protocol_controller_handles, + sync_protocol_controller_handle, behaviour, notification_protocols: iter::once(block_announces_protocol.notifications_protocol) .chain(notification_protocols.iter().map(|s| s.notifications_protocol.clone())) @@ -193,7 +154,7 @@ impl Protocol { _marker: Default::default(), }; - Ok((protocol, peerset_handle, known_addresses)) + Ok(protocol) } /// Returns the list of all the peers we have an open channel to. @@ -229,7 +190,7 @@ impl Protocol { /// Adjusts the reputation of a node. pub fn report_peer(&self, who: PeerId, reputation: ReputationChange) { - self.peerset_handle.report_peer(who, reputation) + self.peer_store_handle.report_peer(who, reputation) } /// Set handshake for the notification protocol. @@ -247,33 +208,33 @@ impl Protocol { /// Set whether the syncing peers set is in reserved-only mode. pub fn set_reserved_only(&self, reserved_only: bool) { - self.peerset_handle.set_reserved_only(HARDCODED_PEERSETS_SYNC, reserved_only); + self.sync_protocol_controller_handle.set_reserved_only(reserved_only); } /// Removes a `PeerId` from the list of reserved peers for syncing purposes. pub fn remove_reserved_peer(&self, peer: PeerId) { - self.peerset_handle.remove_reserved_peer(HARDCODED_PEERSETS_SYNC, peer); + self.sync_protocol_controller_handle.remove_reserved_peer(peer); } /// Returns the list of reserved peers. pub fn reserved_peers(&self, pending_response: oneshot::Sender>) { - self.behaviour.reserved_peers(HARDCODED_PEERSETS_SYNC, pending_response); + self.sync_protocol_controller_handle.reserved_peers(pending_response); } /// Adds a `PeerId` to the list of reserved peers for syncing purposes. pub fn add_reserved_peer(&self, peer: PeerId) { - self.peerset_handle.add_reserved_peer(HARDCODED_PEERSETS_SYNC, peer); + self.sync_protocol_controller_handle.add_reserved_peer(peer); } /// Sets the list of reserved peers for syncing purposes. pub fn set_reserved_peers(&self, peers: HashSet) { - self.peerset_handle.set_reserved_peers(HARDCODED_PEERSETS_SYNC, peers); + self.sync_protocol_controller_handle.set_reserved_peers(peers); } /// Sets the list of reserved peers for the given protocol/peerset. pub fn set_reserved_peerset_peers(&self, protocol: ProtocolName, peers: HashSet) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.peerset_handle.set_reserved_peers(SetId::from(index), peers); + self.protocol_controller_handles[index].set_reserved_peers(peers); } else { error!( target: "sub-libp2p", @@ -286,7 +247,7 @@ impl Protocol { /// Removes a `PeerId` from the list of reserved peers. pub fn remove_set_reserved_peer(&self, protocol: ProtocolName, peer: PeerId) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.peerset_handle.remove_reserved_peer(SetId::from(index), peer); + self.protocol_controller_handles[index].remove_reserved_peer(peer); } else { error!( target: "sub-libp2p", @@ -299,7 +260,7 @@ impl Protocol { /// Adds a `PeerId` to the list of reserved peers. pub fn add_set_reserved_peer(&self, protocol: ProtocolName, peer: PeerId) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.peerset_handle.add_reserved_peer(SetId::from(index), peer); + self.protocol_controller_handles[index].add_reserved_peer(peer); } else { error!( target: "sub-libp2p", @@ -315,7 +276,7 @@ impl Protocol { pub fn add_known_peer(&mut self, peer_id: PeerId) { // TODO: get rid of this function and call `Peerset`/`PeerStore` directly // from `NetworkWorker`. - self.peerset_handle.add_known_peer(peer_id); + self.peer_store_handle.add_known_peer(peer_id); } } @@ -493,7 +454,7 @@ impl NetworkBehaviour for Protocol { peer_id, msg, ); - self.peerset_handle.report_peer(peer_id, rep::BAD_MESSAGE); + self.peer_store_handle.report_peer(peer_id, rep::BAD_MESSAGE); CustomMessageOutcome::None }, Err(err) => { @@ -534,7 +495,7 @@ impl NetworkBehaviour for Protocol { err, err2, ); - self.peerset_handle.report_peer(peer_id, rep::BAD_MESSAGE); + self.peer_store_handle.report_peer(peer_id, rep::BAD_MESSAGE); CustomMessageOutcome::None }, } @@ -571,7 +532,7 @@ impl NetworkBehaviour for Protocol { debug!(target: "sync", "Failed to parse remote handshake: {}", err); self.bad_handshake_substreams.insert((peer_id, set_id)); self.behaviour.disconnect_peer(&peer_id, set_id); - self.peerset_handle.report_peer(peer_id, rep::BAD_MESSAGE); + self.peer_store_handle.report_peer(peer_id, rep::BAD_MESSAGE); self.peers.remove(&peer_id); CustomMessageOutcome::None }, From 1fbff994eb782a617710799ceae420e256b8cc05 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 7 Jun 2023 18:26:46 +0300 Subject: [PATCH 09/37] Migrate `NetworkService` from `Peerset` to `PeerStore` & `ProtocolController` --- client/network/src/behaviour.rs | 6 +- client/network/src/config.rs | 5 ++ client/network/src/request_responses.rs | 10 ++- client/network/src/service.rs | 111 ++++++++++++++++++++---- 4 files changed, 109 insertions(+), 23 deletions(-) diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index a4c19c47c3a80..98c20a4132822 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -20,7 +20,7 @@ use crate::{ discovery::{DiscoveryBehaviour, DiscoveryConfig, DiscoveryOut}, event::DhtEvent, peer_info, - peerset::PeersetHandle, + peer_store::PeerStoreHandle, protocol::{CustomMessageOutcome, NotificationsSink, Protocol}, request_responses::{self, IfDisconnected, ProtocolConfig, RequestFailure}, types::ProtocolName, @@ -170,7 +170,7 @@ impl Behaviour { local_public_key: PublicKey, disco_config: DiscoveryConfig, request_response_protocols: Vec, - peerset: PeersetHandle, + peer_store_handle: PeerStoreHandle, ) -> Result { Ok(Self { substrate, @@ -178,7 +178,7 @@ impl Behaviour { discovery: disco_config.finish(), request_responses: request_responses::RequestResponsesBehaviour::new( request_response_protocols.into_iter(), - peerset, + peer_store_handle, )?, }) } diff --git a/client/network/src/config.rs b/client/network/src/config.rs index 17ca8335653de..6c2a6c51ee5dc 100644 --- a/client/network/src/config.rs +++ b/client/network/src/config.rs @@ -273,6 +273,11 @@ impl NonReservedPeerMode { _ => None, } } + + /// If we are in "reserved-only" peer mode. + pub fn is_reserved_only(&self) -> bool { + matches!(self, NonReservedPeerMode::Deny) + } } /// Sync operation mode. diff --git a/client/network/src/request_responses.rs b/client/network/src/request_responses.rs index af3cc991e0780..791aa3fc81a42 100644 --- a/client/network/src/request_responses.rs +++ b/client/network/src/request_responses.rs @@ -35,7 +35,9 @@ //! is used to handle incoming requests. use crate::{ - peer_store::BANNED_THRESHOLD, peerset::PeersetHandle, types::ProtocolName, ReputationChange, + peer_store::{PeerStoreHandle, BANNED_THRESHOLD}, + types::ProtocolName, + ReputationChange, }; use futures::{channel::oneshot, prelude::*}; @@ -279,7 +281,7 @@ pub struct RequestResponsesBehaviour { send_feedback: HashMap>, /// Primarily used to get a reputation of a node. - peerset: PeersetHandle, + peer_store: PeerStoreHandle, /// Pending message request, holds `MessageRequest` as a Future state to poll it /// until we get a response from `Peerset` @@ -317,7 +319,7 @@ impl RequestResponsesBehaviour { /// the same protocol is passed twice. pub fn new( list: impl Iterator, - peerset: PeersetHandle, + peer_store: PeerStoreHandle, ) -> Result { let mut protocols = HashMap::new(); for protocol in list { @@ -354,7 +356,7 @@ impl RequestResponsesBehaviour { pending_responses: Default::default(), pending_responses_arrival_time: Default::default(), send_feedback: Default::default(), - peerset, + peer_store, message_request: None, }) } diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 62ff5ae42466b..3a6926fe1b93a 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -36,8 +36,9 @@ use crate::{ network_state::{ NetworkState, NotConnectedPeer as NetworkStateNotConnectedPeer, Peer as NetworkStatePeer, }, - peerset::PeersetHandle, + peer_store::{PeerStore, PeerStoreHandle, PeerStoreProvider}, protocol::{self, NotifsHandlerError, Protocol, Ready}, + protocol_controller::{ProtoSetConfig, ProtocolController, SetId}, request_responses::{IfDisconnected, RequestFailure}, service::{ signature::{Signature, SigningError}, @@ -115,9 +116,6 @@ pub struct NetworkService { local_identity: Keypair, /// Bandwidth logging system. Can be queried to know the average bandwidth consumed. bandwidth: Arc, - /// Peerset manager (PSM); manages the reputation of nodes and indicates the network which - /// nodes it should be connected to or not. - peerset: PeersetHandle, /// Channel that sends messages to the actual worker. to_worker: TracingUnboundedSender, /// For each peer and protocol combination, an object that allows sending notifications to @@ -262,24 +260,92 @@ where ) }; - let (protocol, peerset_handle, mut known_addresses) = Protocol::new( + let peer_store = PeerStore::new( + network_config.boot_nodes.iter().map(|bootnode| bootnode.peer_id).collect(), + ); + let peer_store_handle = peer_store.handle(); + + // Spawn `PeerStore` runner. + (params.executor)(peer_store.run().boxed()); + + let (to_notifications, from_controllers) = + tracing_unbounded("mpsc_protocol_controllers_to_notifications", 10_000); + + // We must prepend a hardcoded default peer set to notification protocols. + let all_peer_sets_iter = iter::once(&network_config.default_peers_set) + .chain(notification_protocols.iter().map(|protocol| &protocol.set_config)); + + let (protocol_handles, protocol_controllers): (Vec<_>, Vec<_>) = all_peer_sets_iter + .enumerate() + .map(|(set_id, set_config)| { + let proto_set_config = ProtoSetConfig { + in_peers: set_config.in_peers, + out_peers: set_config.out_peers, + reserved_nodes: set_config + .reserved_nodes + .iter() + .map(|node| node.peer_id) + .collect(), + reserved_only: set_config.non_reserved_mode.is_reserved_only(), + }; + + ProtocolController::new( + SetId::from(set_id), + proto_set_config, + to_notifications.clone(), + Box::new(peer_store_handle.clone()), + ) + }) + .unzip(); + + // Spawn `ProtocolController` runners. + protocol_controllers + .into_iter() + .for_each(|controller| (params.executor)(controller.run().boxed())); + + let protocol = Protocol::new( From::from(¶ms.role), &network_config, notification_protocols, params.block_announce_config, + peer_store_handle, + protocol_handles, params.tx, )?; - // List of multiaddresses that we know in the network. - let mut boot_node_ids = HashSet::new(); + let known_addresses = { + // Collect all reserved nodes and bootnodes addresses. + let mut addresses: Vec<_> = network_config + .default_peers_set + .reserved_nodes + .iter() + .map(|reserved| (reserved.peer_id, reserved.multiaddr.clone())) + .chain( + notification_protocols + .iter() + .map(|protocol| { + protocol + .set_config + .reserved_nodes + .iter() + .map(|reserved| (reserved.peer_id, reserved.multiaddr.clone())) + }) + .flatten(), + ) + .chain( + network_config + .boot_nodes + .iter() + .map(|bootnode| (bootnode.peer_id, bootnode.multiaddr.clone())), + ) + .collect(); - // Process the bootnodes. - for bootnode in network_config.boot_nodes.iter() { - boot_node_ids.insert(bootnode.peer_id); - known_addresses.push((bootnode.peer_id, bootnode.multiaddr.clone())); - } + // Remove possible duplicates. + addresses.sort(); + addresses.dedup(); - let boot_node_ids = Arc::new(boot_node_ids); + addresses + }; // Check for duplicate bootnodes. network_config.boot_nodes.iter().try_for_each(|bootnode| { @@ -299,6 +365,14 @@ where } })?; + let boot_node_ids = Arc::new( + network_config + .boot_nodes + .iter() + .map(|bootnode| bootnode.peer_id) + .collect::>(), + ); + let num_connected = Arc::new(AtomicUsize::new(0)); // Build the swarm. @@ -346,7 +420,7 @@ where local_public, discovery_config, request_response_protocols, - peerset_handle.clone(), + peer_store_handle.clone(), ); match result { @@ -426,7 +500,6 @@ where external_addresses: external_addresses.clone(), listen_addresses: listen_addresses.clone(), num_connected: num_connected.clone(), - peerset: peerset_handle, local_peer_id, local_identity, to_worker, @@ -449,6 +522,7 @@ where peers_notifications_sinks, metrics, boot_node_ids, + peer_store_handle, _marker: Default::default(), _block: Default::default(), }) @@ -788,7 +862,7 @@ where } fn report_peer(&self, who: PeerId, cost_benefit: ReputationChange) { - self.peerset.report_peer(who, cost_benefit); + let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::ReportPeer(who, cost_benefit)); } fn disconnect_peer(&self, who: PeerId, protocol: ProtocolName) { @@ -1090,6 +1164,7 @@ enum ServiceToWorkerMsg { GetValue(KademliaKey), PutValue(KademliaKey, Vec), AddKnownAddress(PeerId, Multiaddr), + ReportPeer(PeerId, ReputationChange), SetReservedOnly(bool), AddReserved(PeerId), RemoveReserved(PeerId), @@ -1148,6 +1223,8 @@ where /// For each peer and protocol combination, an object that allows sending notifications to /// that peer. Shared with the [`NetworkService`]. peers_notifications_sinks: Arc>>, + /// Peer reputation store handle. + peer_store_handle: PeerStoreHandle, /// Marker to pin the `H` generic. Serves no purpose except to not break backwards /// compatibility. _marker: PhantomData, @@ -1273,6 +1350,8 @@ where .remove_set_reserved_peer(protocol, peer_id), ServiceToWorkerMsg::AddKnownAddress(peer_id, addr) => self.network_service.behaviour_mut().add_known_address(peer_id, addr), + ServiceToWorkerMsg::ReportPeer(peer_id, reputation_change) => + self.peer_store_handle.report_peer(peer_id, reputation_change), ServiceToWorkerMsg::EventStream(sender) => self.event_streams.push(sender), ServiceToWorkerMsg::Request { target, From 8923fb511da93b2ce2953e9abb0f09a9662212b5 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 13:25:24 +0300 Subject: [PATCH 10/37] Migrate `Notifications` from `Peerset` to `ProtocolController`s --- client/network/src/peerset.rs | 8 -- client/network/src/protocol.rs | 6 +- .../src/protocol/notifications/behaviour.rs | 78 ++++++++++++------- client/network/src/service.rs | 3 +- 4 files changed, 54 insertions(+), 41 deletions(-) diff --git a/client/network/src/peerset.rs b/client/network/src/peerset.rs index 4fe504b99774c..58ffb27db8a04 100644 --- a/client/network/src/peerset.rs +++ b/client/network/src/peerset.rs @@ -48,7 +48,6 @@ use futures::{ use log::debug; use sc_network_common::types::ReputationChange; use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnboundedSender}; -use serde_json::json; use std::{ collections::HashSet, pin::Pin, @@ -227,13 +226,6 @@ impl Peerset { self.protocol_handles[usize::from(set_id)].dropped(peer_id); } - /// Produces a JSON object containing the state of the peerset manager, for debugging purposes. - pub fn debug_info(&mut self) -> serde_json::Value { - // TODO: Check what info we can include here. - // Issue reference: https://github.com/paritytech/substrate/issues/14160. - json!("unimplemented") - } - /// Returns the number of peers that we have discovered. pub fn num_discovered_peers(&self) -> usize { self.peer_store_handle.num_known_peers() diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index c0d5cb6a1c11a..4ced8373865c6 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -38,7 +38,7 @@ use libp2p::{ use log::{debug, error, warn}; use sc_network_common::{role::Roles, sync::message::BlockAnnouncesHandshake}; -use sc_utils::mpsc::TracingUnboundedSender; +use sc_utils::mpsc::{TracingUnboundedReceiver, TracingUnboundedSender}; use sp_runtime::traits::Block as BlockT; use std::{ @@ -112,11 +112,13 @@ impl Protocol { block_announces_protocol: config::NonDefaultSetConfig, peer_store_handle: PeerStoreHandle, protocol_controller_handles: Vec, + from_protocol_controllers: TracingUnboundedReceiver, tx: TracingUnboundedSender>, ) -> error::Result { let behaviour = { Notifications::new( protocol_controller_handles.clone(), + from_protocol_controllers, // NOTE: Block announcement protocol is still very much hardcoded into `Protocol`. // This protocol must be the first notification protocol given to // `Notifications` @@ -164,7 +166,7 @@ impl Protocol { /// Returns the number of discovered nodes that we keep in memory. pub fn num_discovered_peers(&self) -> usize { - self.behaviour.num_discovered_peers() + self.peer_store_handle.num_known_peers() } /// Disconnects the given peer if we are connected to it. diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 47b34c451cab2..49041a3ba10e7 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -20,7 +20,7 @@ use crate::{ protocol::notifications::handler::{ self, NotificationsSink, NotifsHandler, NotifsHandlerIn, NotifsHandlerOut, }, - protocol_controller::{IncomingIndex, Message, SetId}, + protocol_controller::{self, IncomingIndex, Message, SetId}, types::ProtocolName, }; @@ -39,6 +39,7 @@ use libp2p::{ use log::{debug, error, info, trace, warn}; use parking_lot::RwLock; use rand::distributions::{Distribution as _, Uniform}; +use sc_utils::mpsc::TracingUnboundedReceiver; use smallvec::SmallVec; use std::{ cmp, @@ -107,8 +108,11 @@ pub struct Notifications { /// Notification protocols. Entries never change after initialization. notif_protocols: Vec, + /// Protocol controllers are responsible for peer connections management. + protocol_controller_handles: Vec, + /// Receiver for instructions about who to connect to or disconnect from. - peerset: crate::peerset::Peerset, + from_protocol_controllers: TracingUnboundedReceiver, /// List of peers in our state. peers: FnvHashMap<(PeerId, SetId), PeerState>, @@ -361,7 +365,8 @@ pub enum NotificationsOut { impl Notifications { /// Creates a `CustomProtos`. pub fn new( - peerset: crate::peerset::Peerset, + protocol_controller_handles: Vec, + from_protocol_controllers: TracingUnboundedReceiver, notif_protocols: impl Iterator, ) -> Self { let notif_protocols = notif_protocols @@ -377,7 +382,8 @@ impl Notifications { Self { notif_protocols, - peerset, + protocol_controller_handles, + from_protocol_controllers, peers: FnvHashMap::default(), delays: Default::default(), next_delay_id: DelayId(0), @@ -401,11 +407,6 @@ impl Notifications { } } - /// Returns the number of discovered nodes that we keep in memory. - pub fn num_discovered_peers(&self) -> usize { - self.peerset.num_discovered_peers() - } - /// Returns the list of all the peers we have an open channel to. pub fn open_peers(&self) -> impl Iterator { self.peers.iter().filter(|(_, state)| state.is_open()).map(|((id, _), _)| id) @@ -440,7 +441,7 @@ impl Notifications { // DisabledPendingEnable => Disabled. PeerState::DisabledPendingEnable { connections, timer_deadline, timer: _ } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, *peer_id); + self.report_peer_dropped(set_id, *peer_id); *entry.into_mut() = PeerState::Disabled { connections, backoff_until: Some(timer_deadline) } }, @@ -450,7 +451,7 @@ impl Notifications { // If relevant, the external API is instantly notified. PeerState::Enabled { mut connections } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, *peer_id); + self.report_peer_dropped(set_id, *peer_id); if connections.iter().any(|(_, s)| matches!(s, ConnectionState::Open(_))) { trace!(target: "sub-libp2p", "External API <= Closed({}, {:?})", peer_id, set_id); @@ -539,12 +540,25 @@ impl Notifications { /// Returns the list of reserved peers. pub fn reserved_peers(&self, set_id: SetId, pending_response: oneshot::Sender>) { - self.peerset.reserved_peers(set_id, pending_response); + self.protocol_controller_handles[usize::from(set_id)].reserved_peers(pending_response); } /// Returns the state of the peerset manager, for debugging purposes. pub fn peerset_debug_info(&mut self) -> serde_json::Value { - self.peerset.debug_info() + // TODO: Check what info we can include here. + // Issue reference: https://github.com/paritytech/substrate/issues/14160. + serde_json::json!("unimplemented") + } + + /// Report to `ProtocolController` that peer has dropped. + fn report_peer_dropped(&self, set_id: SetId, peer_id: PeerId) { + self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); + } + + /// Request incoming connection approval from `ProtocolController`. + fn request_incoming(&self, set_id: SetId, peer_id: PeerId, incoming_index: IncomingIndex) { + self.protocol_controller_handles[usize::from(set_id)] + .incoming_connection(peer_id, incoming_index); } /// Function that is called when the peerset wants us to connect to a peer. @@ -852,7 +866,7 @@ impl Notifications { _ => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", incoming.peer_id, incoming.set_id); - self.peerset.dropped(incoming.set_id, incoming.peer_id); + self.report_peer_dropped(incoming.set_id, incoming.peer_id); }, } return @@ -1190,7 +1204,7 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id); + self.report_peer_dropped(set_id, peer_id); *entry.get_mut() = PeerState::Backoff { timer, timer_deadline }; } else { *entry.get_mut() = PeerState::DisabledPendingEnable { @@ -1344,7 +1358,7 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id); + self.report_peer_dropped(set_id, peer_id); let ban_dur = Uniform::new(5, 10).sample(&mut rand::thread_rng()); let delay_id = self.next_delay_id; @@ -1366,7 +1380,7 @@ impl NetworkBehaviour for Notifications { matches!(s, ConnectionState::Opening | ConnectionState::Open(_)) }) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id); + self.report_peer_dropped(set_id, peer_id); *entry.get_mut() = PeerState::Disabled { connections, backoff_until: None }; @@ -1414,7 +1428,7 @@ impl NetworkBehaviour for Notifications { st @ PeerState::Requested | st @ PeerState::PendingRequest { .. } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id); + self.report_peer_dropped(set_id, peer_id); let now = Instant::now(); let ban_duration = match st { @@ -1586,7 +1600,7 @@ impl NetworkBehaviour for Notifications { trace!(target: "sub-libp2p", "PSM <= Incoming({}, {:?}).", peer_id, incoming_id); - self.peerset.incoming(set_id, peer_id, incoming_id); + self.request_incoming(set_id, peer_id, incoming_id); self.incoming.push(IncomingPeer { peer_id, set_id, @@ -1742,7 +1756,7 @@ impl NetworkBehaviour for Notifications { .any(|(_, s)| matches!(s, ConnectionState::Opening)) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.peerset.dropped(set_id, peer_id); + self.report_peer_dropped(set_id, peer_id); *entry.into_mut() = PeerState::Disabled { connections, backoff_until: None }; } else { @@ -1916,7 +1930,7 @@ impl NetworkBehaviour for Notifications { matches!(s, ConnectionState::Opening | ConnectionState::Open(_)) }) { trace!(target: "sub-libp2p", "PSM <= Dropped({:?})", peer_id); - self.peerset.dropped(set_id, peer_id); + self.report_peer_dropped(set_id, peer_id); let ban_dur = Uniform::new(5, 10).sample(&mut rand::thread_rng()); *entry.into_mut() = PeerState::Disabled { @@ -2009,24 +2023,27 @@ impl NetworkBehaviour for Notifications { // Poll for instructions from the peerset. // Note that the peerset is a *best effort* crate, and we have to use defensive programming. loop { - match futures::Stream::poll_next(Pin::new(&mut self.peerset), cx) { - Poll::Ready(Some(Message::Accept(index))) => { + match self.from_protocol_controllers.next().now_or_never() { + Some(Some(Message::Accept(index))) => { self.peerset_report_accept(index); }, - Poll::Ready(Some(Message::Reject(index))) => { + Some(Some(Message::Reject(index))) => { self.peerset_report_reject(index); }, - Poll::Ready(Some(Message::Connect { peer_id, set_id, .. })) => { + Some(Some(Message::Connect { peer_id, set_id, .. })) => { self.peerset_report_connect(peer_id, set_id); }, - Poll::Ready(Some(Message::Drop { peer_id, set_id, .. })) => { + Some(Some(Message::Drop { peer_id, set_id, .. })) => { self.peerset_report_disconnect(peer_id, set_id); }, - Poll::Ready(None) => { - error!(target: "sub-libp2p", "Peerset receiver stream has returned None"); + Some(None) => { + error!( + target: "sub-libp2p", + "Protocol controllers receiver stream has returned None", + ); break }, - Poll::Pending => break, + None => break, } } @@ -2974,7 +2991,8 @@ mod tests { // check peer information assert_eq!(notif.open_peers().collect::>(), vec![&peer],); - assert_eq!(notif.num_discovered_peers(), 0usize); + //assert_eq!(notif.num_discovered_peers(), 0usize); + todo!("^"); // close the other connection and verify that notification replacement event is emitted notif.on_swarm_event(FromSwarm::ConnectionClosed( diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 3a6926fe1b93a..cda5446fd5bad 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -268,7 +268,7 @@ where // Spawn `PeerStore` runner. (params.executor)(peer_store.run().boxed()); - let (to_notifications, from_controllers) = + let (to_notifications, from_protocol_controllers) = tracing_unbounded("mpsc_protocol_controllers_to_notifications", 10_000); // We must prepend a hardcoded default peer set to notification protocols. @@ -310,6 +310,7 @@ where params.block_announce_config, peer_store_handle, protocol_handles, + from_protocol_controllers, params.tx, )?; From 1350193a2e6cfb4a57a6f54c4a6902e5b8f598fb Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 15:16:49 +0300 Subject: [PATCH 11/37] Migrate `Notifications` tests from `Peerset` to `ProtocolController` --- .../src/protocol/notifications/behaviour.rs | 172 ++++++++++-------- 1 file changed, 101 insertions(+), 71 deletions(-) diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 49041a3ba10e7..d7d77288af241 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -2118,10 +2118,12 @@ impl NetworkBehaviour for Notifications { mod tests { use super::*; use crate::{ + peer_store::PeerStoreProvider, protocol::notifications::handler::tests::*, - protocol_controller::{IncomingIndex, ProtoSetConfig}, + protocol_controller::{IncomingIndex, Message, ProtoSetConfig, ProtocolHandle}, }; use libp2p::swarm::AddressRecord; + use sc_utuls::mpsc::tracing_unbounded; use std::{collections::HashSet, iter}; impl PartialEq for ConnectionState { @@ -2167,26 +2169,54 @@ mod tests { } } - fn development_notifs() -> (Notifications, crate::peerset::PeersetHandle) { - let (peerset, peerset_handle) = { - let mut sets = Vec::with_capacity(1); + struct MockPeerStore {} - sets.push(ProtoSetConfig { + impl PeerStoreProvider for MockPeerStore { + fn is_banned(&self, peer_id: &PeerId) -> bool { + unimplemented!() + } + + fn register_protocol(&self, protocol_handle: ProtocolHandle) { + unimplemented!() + } + + fn report_disconnect(&mut self, peer_id: PeerId) { + unimplemented!() + } + + fn report_peer(&mut self, peer_id: PeerId, change: ReputationChange) { + unimplemented!() + } + + fn peer_reputation(&self, peer_id: &PeerId) -> i32 { + unimplemented!() + } + + fn outgoing_candidates(&self, count: usize, ignored: HashSet<&PeerId>) -> Vec { + unimplemented!() + } + } + + fn development_notifs() -> (Notifications, ProtocolController) { + let (to_notifications, from_controller) = + tracing_unbounded("test_controller_to_notifications", 10_000); + + let (controller, handle) = ProtocolController::new( + SetId::from(0), + ProtoSetConfig { in_peers: 25, out_peers: 25, reserved_nodes: HashSet::new(), reserved_only: false, - }); - - crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { - bootnodes: Vec::new(), - sets, - }) - }; + }, + to_notifications, + MockPeerStore {}, + ); ( Notifications::new( - peerset, + vec![handle], + from_controller, iter::once(ProtocolConfig { name: "/foo".into(), fallback_names: Vec::new(), @@ -2194,13 +2224,13 @@ mod tests { max_notification_size: u64::MAX, }), ), - peerset_handle, + controller, ) } #[test] fn update_handshake() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let inner = notif.notif_protocols.get_mut(0).unwrap().handshake.read().clone(); assert_eq!(inner, vec![1, 2, 3, 4]); @@ -2215,14 +2245,14 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn update_unknown_handshake() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); notif.set_notif_protocol_handshake(1337.into(), vec![5, 6, 7, 8]); } #[test] fn disconnect_backoff_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); notif.peers.insert( @@ -2239,7 +2269,7 @@ mod tests { #[test] fn disconnect_pending_request() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); notif.peers.insert( @@ -2256,7 +2286,7 @@ mod tests { #[test] fn disconnect_requested_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); notif.peers.insert((peer, 0.into()), PeerState::Requested); @@ -2267,7 +2297,7 @@ mod tests { #[test] fn disconnect_disabled_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); notif.peers.insert( (peer, 0.into()), @@ -2283,7 +2313,7 @@ mod tests { #[test] fn remote_opens_connection_and_substream() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -2332,8 +2362,8 @@ mod tests { } #[tokio::test] - async fn disconnect_remote_substream_before_handled_by_peerset() { - let (mut notif, _peerset) = development_notifs(); + async fn disconnect_remote_substream_before_handled_by_controller() { + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { @@ -2369,7 +2399,7 @@ mod tests { #[test] fn peerset_report_connect_backoff() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -2434,7 +2464,7 @@ mod tests { #[test] fn peerset_connect_incoming() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -2470,7 +2500,7 @@ mod tests { #[test] fn peerset_disconnect_disable_pending_enable() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -2517,7 +2547,7 @@ mod tests { #[test] fn peerset_disconnect_enabled() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -2553,7 +2583,7 @@ mod tests { #[test] fn peerset_disconnect_requested() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); @@ -2568,7 +2598,7 @@ mod tests { #[test] fn peerset_disconnect_pending_request() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -2621,7 +2651,7 @@ mod tests { #[test] fn peerset_accept_peer_not_alive() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -2668,7 +2698,7 @@ mod tests { #[test] fn secondary_connection_peer_state_incoming() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); @@ -2723,7 +2753,7 @@ mod tests { #[test] fn close_connection_for_disabled_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -2757,7 +2787,7 @@ mod tests { #[test] fn close_connection_for_incoming_peer_one_connection() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -2802,7 +2832,7 @@ mod tests { #[test] fn close_connection_for_incoming_peer_two_connections() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let conn1 = ConnectionId::new_unchecked(1); @@ -2871,7 +2901,7 @@ mod tests { #[test] fn connection_and_substream_open() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -2925,7 +2955,7 @@ mod tests { #[test] fn connection_closed_sink_replaced() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); @@ -3021,7 +3051,7 @@ mod tests { #[test] fn dial_failure_for_requested_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); @@ -3044,7 +3074,7 @@ mod tests { #[tokio::test] async fn write_notification() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -3093,7 +3123,7 @@ mod tests { #[test] fn peerset_report_connect_backoff_expired() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -3141,7 +3171,7 @@ mod tests { #[test] fn peerset_report_disconnect_disabled() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); @@ -3167,7 +3197,7 @@ mod tests { #[test] fn peerset_report_disconnect_backoff() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -3213,7 +3243,7 @@ mod tests { #[test] fn peer_is_backed_off_if_both_connections_get_closed_while_peer_is_disabled_with_back_off() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); @@ -3286,7 +3316,7 @@ mod tests { #[test] fn inject_connection_closed_incoming_with_backoff() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); @@ -3337,7 +3367,7 @@ mod tests { #[test] fn two_connections_inactive_connection_gets_closed_peer_state_is_still_incoming() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); @@ -3392,7 +3422,7 @@ mod tests { #[test] fn two_connections_active_connection_gets_closed_peer_state_is_disabled() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); @@ -3450,7 +3480,7 @@ mod tests { #[test] fn inject_connection_closed_for_active_connection() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn1 = ConnectionId::new_unchecked(0); let conn2 = ConnectionId::new_unchecked(1); @@ -3518,7 +3548,7 @@ mod tests { #[test] fn inject_dial_failure_for_pending_request() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -3581,7 +3611,7 @@ mod tests { #[test] fn peerstate_incoming_open_desired_by_remote() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); let conn1 = ConnectionId::new_unchecked(0); @@ -3635,7 +3665,7 @@ mod tests { #[tokio::test] async fn remove_backoff_peer_after_timeout() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); @@ -3713,7 +3743,7 @@ mod tests { #[tokio::test] async fn reschedule_disabled_pending_enable_when_connection_not_closed() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -3831,7 +3861,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn peerset_report_connect_with_enabled_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -3881,7 +3911,7 @@ mod tests { #[test] #[cfg(debug_assertions)] fn peerset_report_connect_with_disabled_pending_enable_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -3927,7 +3957,7 @@ mod tests { #[test] #[cfg(debug_assertions)] fn peerset_report_connect_with_requested_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); @@ -3943,7 +3973,7 @@ mod tests { #[test] #[cfg(debug_assertions)] fn peerset_report_connect_with_pending_requested() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -4000,7 +4030,7 @@ mod tests { #[test] #[cfg(debug_assertions)] fn peerset_report_connect_with_incoming_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); @@ -4035,7 +4065,7 @@ mod tests { #[test] #[cfg(debug_assertions)] fn peerset_report_disconnect_with_incoming_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); let conn = ConnectionId::new_unchecked(0); @@ -4071,7 +4101,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn peerset_report_accept_incoming_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -4112,7 +4142,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn peerset_report_accept_not_incoming_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -4161,7 +4191,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn inject_connection_closed_non_existent_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let endpoint = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), @@ -4181,7 +4211,7 @@ mod tests { #[test] fn disconnect_non_existent_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let set_id = SetId::from(0); @@ -4193,7 +4223,7 @@ mod tests { #[test] fn accept_non_existent_connection() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); notif.peerset_report_accept(0.into()); @@ -4203,7 +4233,7 @@ mod tests { #[test] fn reject_non_existent_connection() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); notif.peerset_report_reject(0.into()); @@ -4213,7 +4243,7 @@ mod tests { #[test] fn reject_non_active_connection() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -4251,7 +4281,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn reject_non_existent_peer_but_alive_connection() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -4291,7 +4321,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn inject_non_existent_connection_closed_for_incoming_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -4334,7 +4364,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn inject_non_existent_connection_closed_for_disabled_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -4369,7 +4399,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn inject_non_existent_connection_closed_for_disabled_pending_enable() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -4420,7 +4450,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn inject_connection_closed_for_incoming_peer_state_mismatch() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -4464,7 +4494,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn inject_connection_closed_for_enabled_state_mismatch() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); let set_id = SetId::from(0); @@ -4511,7 +4541,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn inject_connection_closed_for_backoff_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let set_id = SetId::from(0); let peer = PeerId::random(); let conn = ConnectionId::new_unchecked(0); @@ -4565,7 +4595,7 @@ mod tests { #[should_panic] #[cfg(debug_assertions)] fn open_result_ok_non_existent_peer() { - let (mut notif, _peerset) = development_notifs(); + let (mut notif, _controller) = development_notifs(); let conn = ConnectionId::new_unchecked(0); let connected = ConnectedPoint::Listener { local_addr: Multiaddr::empty(), From e0cae7e2c4d161404c14684790d0f3d3a2a65589 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 16:16:38 +0300 Subject: [PATCH 12/37] Fix compilation of `NetworkService` & `Protocol` --- client/network/src/protocol.rs | 6 +----- client/network/src/service.rs | 10 ++++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 4ced8373865c6..5010739e752f2 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -64,9 +64,6 @@ pub(crate) const BLOCK_ANNOUNCES_TRANSACTIONS_SUBSTREAM_SIZE: u64 = 16 * 1024 * /// Identifier of the peerset for the block announces protocol. const HARDCODED_PEERSETS_SYNC: SetId = SetId::from(0); -/// Number of hardcoded peersets (the constants right above). Any set whose identifier is equal or -/// superior to this value corresponds to a user-defined protocol. -const NUM_HARDCODED_PEERSETS: usize = 1; mod rep { use crate::ReputationChange as Rep; @@ -107,7 +104,6 @@ impl Protocol { /// Create a new instance. pub fn new( roles: Roles, - network_config: &config::NetworkConfiguration, notification_protocols: Vec, block_announces_protocol: config::NonDefaultSetConfig, peer_store_handle: PeerStoreHandle, @@ -191,7 +187,7 @@ impl Protocol { } /// Adjusts the reputation of a node. - pub fn report_peer(&self, who: PeerId, reputation: ReputationChange) { + pub fn report_peer(&mut self, who: PeerId, reputation: ReputationChange) { self.peer_store_handle.report_peer(who, reputation) } diff --git a/client/network/src/service.rs b/client/network/src/service.rs index cda5446fd5bad..6ca4480cdf89c 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -305,10 +305,9 @@ where let protocol = Protocol::new( From::from(¶ms.role), - &network_config, - notification_protocols, + notification_protocols.clone(), params.block_announce_config, - peer_store_handle, + peer_store_handle.clone(), protocol_handles, from_protocol_controllers, params.tx, @@ -1467,7 +1466,10 @@ where }, SwarmEvent::Behaviour(BehaviourOut::ReputationChanges { peer, changes }) => { for change in changes { - self.network_service.behaviour().user_protocol().report_peer(peer, change); + self.network_service + .behaviour_mut() + .user_protocol_mut() + .report_peer(peer, change); } }, SwarmEvent::Behaviour(BehaviourOut::PeerIdentify { From 10ec8b4977339b7bb1b0c582938d47f33110d43b Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 16:18:30 +0300 Subject: [PATCH 13/37] Fix borrowing issues in `Notifications` --- .../src/protocol/notifications/behaviour.rs | 49 +++++++------------ 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index d7d77288af241..ae8a0a28ac93f 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -26,7 +26,7 @@ use crate::{ use bytes::BytesMut; use fnv::FnvHashMap; -use futures::{channel::oneshot, prelude::*}; +use futures::prelude::*; use libp2p::{ core::{ConnectedPoint, Endpoint, Multiaddr}, swarm::{ @@ -441,7 +441,7 @@ impl Notifications { // DisabledPendingEnable => Disabled. PeerState::DisabledPendingEnable { connections, timer_deadline, timer: _ } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.report_peer_dropped(set_id, *peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(*peer_id); *entry.into_mut() = PeerState::Disabled { connections, backoff_until: Some(timer_deadline) } }, @@ -451,7 +451,7 @@ impl Notifications { // If relevant, the external API is instantly notified. PeerState::Enabled { mut connections } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.report_peer_dropped(set_id, *peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(*peer_id); if connections.iter().any(|(_, s)| matches!(s, ConnectionState::Open(_))) { trace!(target: "sub-libp2p", "External API <= Closed({}, {:?})", peer_id, set_id); @@ -538,11 +538,6 @@ impl Notifications { } } - /// Returns the list of reserved peers. - pub fn reserved_peers(&self, set_id: SetId, pending_response: oneshot::Sender>) { - self.protocol_controller_handles[usize::from(set_id)].reserved_peers(pending_response); - } - /// Returns the state of the peerset manager, for debugging purposes. pub fn peerset_debug_info(&mut self) -> serde_json::Value { // TODO: Check what info we can include here. @@ -550,17 +545,6 @@ impl Notifications { serde_json::json!("unimplemented") } - /// Report to `ProtocolController` that peer has dropped. - fn report_peer_dropped(&self, set_id: SetId, peer_id: PeerId) { - self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); - } - - /// Request incoming connection approval from `ProtocolController`. - fn request_incoming(&self, set_id: SetId, peer_id: PeerId, incoming_index: IncomingIndex) { - self.protocol_controller_handles[usize::from(set_id)] - .incoming_connection(peer_id, incoming_index); - } - /// Function that is called when the peerset wants us to connect to a peer. fn peerset_report_connect(&mut self, peer_id: PeerId, set_id: SetId) { // If `PeerId` is unknown to us, insert an entry, start dialing, and return early. @@ -866,7 +850,7 @@ impl Notifications { _ => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", incoming.peer_id, incoming.set_id); - self.report_peer_dropped(incoming.set_id, incoming.peer_id); + self.protocol_controller_handles[usize::from(incoming.set_id)].dropped(incoming.peer_id); }, } return @@ -1204,7 +1188,7 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.report_peer_dropped(set_id, peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); *entry.get_mut() = PeerState::Backoff { timer, timer_deadline }; } else { *entry.get_mut() = PeerState::DisabledPendingEnable { @@ -1358,7 +1342,7 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.report_peer_dropped(set_id, peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); let ban_dur = Uniform::new(5, 10).sample(&mut rand::thread_rng()); let delay_id = self.next_delay_id; @@ -1380,7 +1364,7 @@ impl NetworkBehaviour for Notifications { matches!(s, ConnectionState::Opening | ConnectionState::Open(_)) }) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.report_peer_dropped(set_id, peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); *entry.get_mut() = PeerState::Disabled { connections, backoff_until: None }; @@ -1428,7 +1412,7 @@ impl NetworkBehaviour for Notifications { st @ PeerState::Requested | st @ PeerState::PendingRequest { .. } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.report_peer_dropped(set_id, peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); let now = Instant::now(); let ban_duration = match st { @@ -1600,7 +1584,8 @@ impl NetworkBehaviour for Notifications { trace!(target: "sub-libp2p", "PSM <= Incoming({}, {:?}).", peer_id, incoming_id); - self.request_incoming(set_id, peer_id, incoming_id); + self.protocol_controller_handles[usize::from(set_id)] + .incoming_connection(peer_id, incoming_id); self.incoming.push(IncomingPeer { peer_id, set_id, @@ -1756,7 +1741,7 @@ impl NetworkBehaviour for Notifications { .any(|(_, s)| matches!(s, ConnectionState::Opening)) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.report_peer_dropped(set_id, peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); *entry.into_mut() = PeerState::Disabled { connections, backoff_until: None }; } else { @@ -1930,7 +1915,7 @@ impl NetworkBehaviour for Notifications { matches!(s, ConnectionState::Opening | ConnectionState::Open(_)) }) { trace!(target: "sub-libp2p", "PSM <= Dropped({:?})", peer_id); - self.report_peer_dropped(set_id, peer_id); + self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); let ban_dur = Uniform::new(5, 10).sample(&mut rand::thread_rng()); *entry.into_mut() = PeerState::Disabled { @@ -2120,10 +2105,11 @@ mod tests { use crate::{ peer_store::PeerStoreProvider, protocol::notifications::handler::tests::*, - protocol_controller::{IncomingIndex, Message, ProtoSetConfig, ProtocolHandle}, + protocol_controller::{IncomingIndex, ProtoSetConfig, ProtocolHandle, ProtocolController}, + ReputationChange, }; use libp2p::swarm::AddressRecord; - use sc_utuls::mpsc::tracing_unbounded; + use sc_utils::mpsc::tracing_unbounded; use std::{collections::HashSet, iter}; impl PartialEq for ConnectionState { @@ -2169,6 +2155,7 @@ mod tests { } } + #[derive(Debug)] struct MockPeerStore {} impl PeerStoreProvider for MockPeerStore { @@ -2201,7 +2188,7 @@ mod tests { let (to_notifications, from_controller) = tracing_unbounded("test_controller_to_notifications", 10_000); - let (controller, handle) = ProtocolController::new( + let (handle, controller) = ProtocolController::new( SetId::from(0), ProtoSetConfig { in_peers: 25, @@ -2210,7 +2197,7 @@ mod tests { reserved_only: false, }, to_notifications, - MockPeerStore {}, + Box::new(MockPeerStore {}), ); ( From b92ed075f76191a462f27fb2361576133081fcad Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 16:19:18 +0300 Subject: [PATCH 14/37] Migrate `RequestResponse`from `Peerset` to `PeerStore` --- client/network/src/request_responses.rs | 179 +++++++----------------- 1 file changed, 52 insertions(+), 127 deletions(-) diff --git a/client/network/src/request_responses.rs b/client/network/src/request_responses.rs index 791aa3fc81a42..2cd8d15d09111 100644 --- a/client/network/src/request_responses.rs +++ b/client/network/src/request_responses.rs @@ -35,7 +35,7 @@ //! is used to handle incoming requests. use crate::{ - peer_store::{PeerStoreHandle, BANNED_THRESHOLD}, + peer_store::{PeerStoreHandle, PeerStoreProvider, BANNED_THRESHOLD}, types::ProtocolName, ReputationChange, }; @@ -282,27 +282,6 @@ pub struct RequestResponsesBehaviour { /// Primarily used to get a reputation of a node. peer_store: PeerStoreHandle, - - /// Pending message request, holds `MessageRequest` as a Future state to poll it - /// until we get a response from `Peerset` - message_request: Option, -} - -// This is a state of processing incoming request Message. -// The main reason of this struct is to hold `get_peer_reputation` as a Future state. -struct MessageRequest { - peer: PeerId, - request_id: RequestId, - request: Vec, - channel: ResponseChannel, ()>>, - protocol: ProtocolName, - // A builder used for building responses for incoming requests. Note that we use - // `async_channel` and not `mpsc` on purpose, because `mpsc::channel` allocates an extra - // message slot for every cloned `Sender` and this breaks a back-pressure mechanism. - resp_builder: Option>, - // Once we get incoming request we save all params, create an async call to Peerset - // to get the reputation of the peer. - get_peer_reputation: Pin> + Send>>, } /// Generated by the response builder and waiting to be processed. @@ -357,7 +336,6 @@ impl RequestResponsesBehaviour { pending_responses_arrival_time: Default::default(), send_feedback: Default::default(), peer_store, - message_request: None, }) } @@ -572,94 +550,6 @@ impl NetworkBehaviour for RequestResponsesBehaviour { params: &mut impl PollParameters, ) -> Poll>> { 'poll_all: loop { - if let Some(message_request) = self.message_request.take() { - // Now we can can poll `MessageRequest` until we get the reputation - - let MessageRequest { - peer, - request_id, - request, - channel, - protocol, - resp_builder, - mut get_peer_reputation, - } = message_request; - - let reputation = Future::poll(Pin::new(&mut get_peer_reputation), cx); - match reputation { - Poll::Pending => { - // Save the state to poll it again next time. - - self.message_request = Some(MessageRequest { - peer, - request_id, - request, - channel, - protocol, - resp_builder, - get_peer_reputation, - }); - return Poll::Pending - }, - Poll::Ready(reputation) => { - // Once we get the reputation we can continue processing the request. - - let reputation = reputation.expect( - "The channel can only be closed if the peerset no longer exists; qed", - ); - - if reputation < BANNED_THRESHOLD { - log::debug!( - target: "sub-libp2p", - "Cannot handle requests from a node with a low reputation {}: {}", - peer, - reputation, - ); - continue 'poll_all - } - - let (tx, rx) = oneshot::channel(); - - // Submit the request to the "response builder" passed by the user at - // initialization. - if let Some(resp_builder) = resp_builder { - // If the response builder is too busy, silently drop `tx`. This - // will be reported by the corresponding request-response [`Behaviour`] - // through an `InboundFailure::Omission` event. - // Note that we use `async_channel::bounded` and not `mpsc::channel` - // because the latter allocates an extra slot for every cloned sender. - let _ = resp_builder.try_send(IncomingRequest { - peer, - payload: request, - pending_response: tx, - }); - } else { - debug_assert!(false, "Received message on outbound-only protocol."); - } - - self.pending_responses.push(Box::pin(async move { - // The `tx` created above can be dropped if we are not capable of - // processing this request, which is reflected as a - // `InboundFailure::Omission` event. - if let Ok(response) = rx.await { - Some(RequestProcessingOutcome { - peer, - request_id, - protocol, - inner_channel: channel, - response, - }) - } else { - None - } - })); - - // This `continue` makes sure that `pending_responses` gets polled - // after we have added the new element. - continue 'poll_all - }, - } - } // Poll to see if any response is ready to be sent back. while let Poll::Ready(Some(outcome)) = self.pending_responses.poll_next_unpin(cx) { let RequestProcessingOutcome { @@ -704,7 +594,7 @@ impl NetworkBehaviour for RequestResponsesBehaviour { // Poll request-responses protocols. for (protocol, (behaviour, resp_builder)) in &mut self.protocols { - while let Poll::Ready(ev) = behaviour.poll(cx, params) { + 'poll_protocol: while let Poll::Ready(ev) = behaviour.poll(cx, params) { let ev = match ev { // Main events we are interested in. ToSwarm::GenerateEvent(ev) => ev, @@ -740,23 +630,58 @@ impl NetworkBehaviour for RequestResponsesBehaviour { self.pending_responses_arrival_time .insert((protocol.clone(), request_id).into(), Instant::now()); - let get_peer_reputation = self.peerset.clone().peer_reputation(peer); - let get_peer_reputation = Box::pin(get_peer_reputation); + let reputation = self.peer_store.peer_reputation(&peer); - // Save the Future-like state with params to poll `get_peer_reputation` - // and to continue processing the request once we get the reputation of - // the peer. - self.message_request = Some(MessageRequest { - peer, - request_id, - request, - channel, - protocol: protocol.clone(), - resp_builder: resp_builder.clone(), - get_peer_reputation, - }); + if reputation < BANNED_THRESHOLD { + log::debug!( + target: "sub-libp2p", + "Cannot handle requests from a node with a low reputation {}: {}", + peer, + reputation, + ); + continue 'poll_protocol + } - // This `continue` makes sure that `message_request` gets polled + let (tx, rx) = oneshot::channel(); + + // Submit the request to the "response builder" passed by the user at + // initialization. + if let Some(resp_builder) = resp_builder { + // If the response builder is too busy, silently drop `tx`. This + // will be reported by the corresponding request-response + // [`Behaviour`] through an `InboundFailure::Omission` event. + // Note that we use `async_channel::bounded` and not `mpsc::channel` + // because the latter allocates an extra slot for every cloned + // sender. + let _ = resp_builder.try_send(IncomingRequest { + peer, + payload: request, + pending_response: tx, + }); + } else { + debug_assert!(false, "Received message on outbound-only protocol."); + } + + let protocol = protocol.clone(); + + self.pending_responses.push(Box::pin(async move { + // The `tx` created above can be dropped if we are not capable of + // processing this request, which is reflected as a + // `InboundFailure::Omission` event. + if let Ok(response) = rx.await { + Some(RequestProcessingOutcome { + peer, + request_id, + protocol, + inner_channel: channel, + response, + }) + } else { + None + } + })); + + // This `continue` makes sure that `pending_responses` gets polled // after we have added the new element. continue 'poll_all }, From b265eb8cf85ad76bc4a4351000126f08063eca71 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 17:27:12 +0300 Subject: [PATCH 15/37] rustfmt --- .../src/protocol/notifications/behaviour.rs | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index ae8a0a28ac93f..0d3f326bc443a 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -850,7 +850,8 @@ impl Notifications { _ => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", incoming.peer_id, incoming.set_id); - self.protocol_controller_handles[usize::from(incoming.set_id)].dropped(incoming.peer_id); + self.protocol_controller_handles[usize::from(incoming.set_id)] + .dropped(incoming.peer_id); }, } return @@ -1188,7 +1189,8 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); + self.protocol_controller_handles[usize::from(set_id)] + .dropped(peer_id); *entry.get_mut() = PeerState::Backoff { timer, timer_deadline }; } else { *entry.get_mut() = PeerState::DisabledPendingEnable { @@ -1342,7 +1344,8 @@ impl NetworkBehaviour for Notifications { if connections.is_empty() { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); + self.protocol_controller_handles[usize::from(set_id)] + .dropped(peer_id); let ban_dur = Uniform::new(5, 10).sample(&mut rand::thread_rng()); let delay_id = self.next_delay_id; @@ -1364,7 +1367,8 @@ impl NetworkBehaviour for Notifications { matches!(s, ConnectionState::Opening | ConnectionState::Open(_)) }) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); + self.protocol_controller_handles[usize::from(set_id)] + .dropped(peer_id); *entry.get_mut() = PeerState::Disabled { connections, backoff_until: None }; @@ -1412,7 +1416,8 @@ impl NetworkBehaviour for Notifications { st @ PeerState::Requested | st @ PeerState::PendingRequest { .. } => { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); + self.protocol_controller_handles[usize::from(set_id)] + .dropped(peer_id); let now = Instant::now(); let ban_duration = match st { @@ -1741,7 +1746,8 @@ impl NetworkBehaviour for Notifications { .any(|(_, s)| matches!(s, ConnectionState::Opening)) { trace!(target: "sub-libp2p", "PSM <= Dropped({}, {:?})", peer_id, set_id); - self.protocol_controller_handles[usize::from(set_id)].dropped(peer_id); + self.protocol_controller_handles[usize::from(set_id)] + .dropped(peer_id); *entry.into_mut() = PeerState::Disabled { connections, backoff_until: None }; } else { @@ -2105,7 +2111,7 @@ mod tests { use crate::{ peer_store::PeerStoreProvider, protocol::notifications::handler::tests::*, - protocol_controller::{IncomingIndex, ProtoSetConfig, ProtocolHandle, ProtocolController}, + protocol_controller::{IncomingIndex, ProtoSetConfig, ProtocolController, ProtocolHandle}, ReputationChange, }; use libp2p::swarm::AddressRecord; @@ -2159,27 +2165,27 @@ mod tests { struct MockPeerStore {} impl PeerStoreProvider for MockPeerStore { - fn is_banned(&self, peer_id: &PeerId) -> bool { + fn is_banned(&self, _peer_id: &PeerId) -> bool { unimplemented!() } - fn register_protocol(&self, protocol_handle: ProtocolHandle) { + fn register_protocol(&self, _protocol_handle: ProtocolHandle) { unimplemented!() } - fn report_disconnect(&mut self, peer_id: PeerId) { + fn report_disconnect(&mut self, _peer_id: PeerId) { unimplemented!() } - fn report_peer(&mut self, peer_id: PeerId, change: ReputationChange) { + fn report_peer(&mut self, _peer_id: PeerId, _change: ReputationChange) { unimplemented!() } - fn peer_reputation(&self, peer_id: &PeerId) -> i32 { + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { unimplemented!() } - fn outgoing_candidates(&self, count: usize, ignored: HashSet<&PeerId>) -> Vec { + fn outgoing_candidates(&self, _count: usize, _ignored: HashSet<&PeerId>) -> Vec { unimplemented!() } } From 5339b2e64723324ebf4eacf535b3dee594213b91 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 17:28:22 +0300 Subject: [PATCH 16/37] Migrate request-response tests from `Peerset` to `PeerStore` --- client/network/src/behaviour.rs | 2 +- client/network/src/request_responses.rs | 89 +++++++++++++------------ 2 files changed, 46 insertions(+), 45 deletions(-) diff --git a/client/network/src/behaviour.rs b/client/network/src/behaviour.rs index 98c20a4132822..07c4e429acdea 100644 --- a/client/network/src/behaviour.rs +++ b/client/network/src/behaviour.rs @@ -178,7 +178,7 @@ impl Behaviour { discovery: disco_config.finish(), request_responses: request_responses::RequestResponsesBehaviour::new( request_response_protocols.into_iter(), - peer_store_handle, + Box::new(peer_store_handle), )?, }) } diff --git a/client/network/src/request_responses.rs b/client/network/src/request_responses.rs index 2cd8d15d09111..2ab14d295d5bb 100644 --- a/client/network/src/request_responses.rs +++ b/client/network/src/request_responses.rs @@ -35,7 +35,7 @@ //! is used to handle incoming requests. use crate::{ - peer_store::{PeerStoreHandle, PeerStoreProvider, BANNED_THRESHOLD}, + peer_store::{PeerStoreProvider, BANNED_THRESHOLD}, types::ProtocolName, ReputationChange, }; @@ -281,7 +281,7 @@ pub struct RequestResponsesBehaviour { send_feedback: HashMap>, /// Primarily used to get a reputation of a node. - peer_store: PeerStoreHandle, + peer_store: Box, } /// Generated by the response builder and waiting to be processed. @@ -298,7 +298,7 @@ impl RequestResponsesBehaviour { /// the same protocol is passed twice. pub fn new( list: impl Iterator, - peer_store: PeerStoreHandle, + peer_store: Box, ) -> Result { let mut protocols = HashMap::new(); for protocol in list { @@ -967,10 +967,7 @@ impl Codec for GenericCodec { mod tests { use super::*; - use crate::{ - peerset::{Peerset, PeersetConfig}, - protocol_controller::ProtoSetConfig, - }; + use crate::{peer_store::PeerStoreProvider, protocol_controller::ProtocolHandle}; use futures::{channel::oneshot, executor::LocalPool, task::Spawn}; use libp2p::{ core::{ @@ -982,7 +979,7 @@ mod tests { swarm::{Executor, Swarm, SwarmBuilder, SwarmEvent}, Multiaddr, }; - use std::{iter, time::Duration}; + use std::{collections::HashSet, iter, time::Duration}; struct TokioExecutor(tokio::runtime::Runtime); impl Executor for TokioExecutor { @@ -991,9 +988,39 @@ mod tests { } } + #[derive(Debug)] + struct MockPeerStore {} + + impl PeerStoreProvider for MockPeerStore { + fn is_banned(&self, _peer_id: &PeerId) -> bool { + unimplemented!() + } + + fn register_protocol(&self, _protocol_handle: ProtocolHandle) { + unimplemented!() + } + + fn report_disconnect(&mut self, _peer_id: PeerId) { + unimplemented!() + } + + fn report_peer(&mut self, _peer_id: PeerId, _change: ReputationChange) { + unimplemented!() + } + + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { + // We just need to make sure that the peer is not banned. + 0 + } + + fn outgoing_candidates(&self, _count: usize, _ignored: HashSet<&PeerId>) -> Vec { + unimplemented!() + } + } + fn build_swarm( list: impl Iterator, - ) -> (Swarm, Multiaddr, Peerset) { + ) -> (Swarm, Multiaddr) { let keypair = Keypair::generate_ed25519(); let transport = MemoryTransport::new() @@ -1002,19 +1029,7 @@ mod tests { .multiplex(libp2p::yamux::Config::default()) .boxed(); - let config = PeersetConfig { - bootnodes: vec![], - sets: vec![ProtoSetConfig { - in_peers: u32::max_value(), - out_peers: u32::max_value(), - reserved_nodes: Default::default(), - reserved_only: false, - }], - }; - - let (peerset, handle) = Peerset::from_config(config); - - let behaviour = RequestResponsesBehaviour::new(list, handle).unwrap(); + let behaviour = RequestResponsesBehaviour::new(list, Box::new(MockPeerStore {})).unwrap(); let runtime = tokio::runtime::Runtime::new().unwrap(); let mut swarm = SwarmBuilder::with_executor( @@ -1027,11 +1042,7 @@ mod tests { let listen_addr: Multiaddr = format!("/memory/{}", rand::random::()).parse().unwrap(); swarm.listen_on(listen_addr.clone()).unwrap(); - (swarm, listen_addr, peerset) - } - - async fn loop_peerset(peerset: Peerset) { - let _: Vec<_> = peerset.collect().await; + (swarm, listen_addr) } #[test] @@ -1083,9 +1094,7 @@ mod tests { Swarm::dial(&mut swarms[0].0, dial_addr).unwrap(); } - let (mut swarm, _, peerset) = swarms.remove(0); - // Process every peerset event in the background. - pool.spawner().spawn_obj(loop_peerset(peerset).boxed().into()).unwrap(); + let (mut swarm, _) = swarms.remove(0); // Running `swarm[0]` in the background. pool.spawner() .spawn_obj({ @@ -1105,9 +1114,7 @@ mod tests { .unwrap(); // Remove and run the remaining swarm. - let (mut swarm, _, peerset) = swarms.remove(0); - // Process every peerset event in the background. - pool.spawner().spawn_obj(loop_peerset(peerset).boxed().into()).unwrap(); + let (mut swarm, _) = swarms.remove(0); pool.run_until(async move { let mut response_receiver = None; @@ -1186,9 +1193,7 @@ mod tests { // Running `swarm[0]` in the background until a `InboundRequest` event happens, // which is a hint about the test having ended. - let (mut swarm, _, peerset) = swarms.remove(0); - // Process every peerset event in the background. - pool.spawner().spawn_obj(loop_peerset(peerset).boxed().into()).unwrap(); + let (mut swarm, _) = swarms.remove(0); pool.spawner() .spawn_obj({ async move { @@ -1208,9 +1213,7 @@ mod tests { .unwrap(); // Remove and run the remaining swarm. - let (mut swarm, _, peerset) = swarms.remove(0); - // Process every peerset event in the background. - pool.spawner().spawn_obj(loop_peerset(peerset).boxed().into()).unwrap(); + let (mut swarm, _) = swarms.remove(0); pool.run_until(async move { let mut response_receiver = None; @@ -1282,7 +1285,7 @@ mod tests { build_swarm(protocol_configs.into_iter()).0 }; - let (mut swarm_2, mut swarm_2_handler_1, mut swarm_2_handler_2, listen_add_2, peerset) = { + let (mut swarm_2, mut swarm_2_handler_1, mut swarm_2_handler_2, listen_add_2) = { let (tx_1, rx_1) = async_channel::bounded(64); let (tx_2, rx_2) = async_channel::bounded(64); @@ -1305,12 +1308,10 @@ mod tests { }, ]; - let (swarm, listen_addr, peerset) = build_swarm(protocol_configs.into_iter()); + let (swarm, listen_addr) = build_swarm(protocol_configs.into_iter()); - (swarm, rx_1, rx_2, listen_addr, peerset) + (swarm, rx_1, rx_2, listen_addr) }; - // Process every peerset event in the background. - pool.spawner().spawn_obj(loop_peerset(peerset).boxed().into()).unwrap(); // Ask swarm 1 to dial swarm 2. There isn't any discovery mechanism in place in this test, // so they wouldn't connect to each other. From 71ca0095799722c0d951d9a2e2b5e5dd02316571 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 17:29:22 +0300 Subject: [PATCH 17/37] Migrate `reconnect_after_disconnect` test to `PeerStore` & `ProtocolController` --- .../src/protocol/notifications/tests.rs | 53 ++++++++++++------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/client/network/src/protocol/notifications/tests.rs b/client/network/src/protocol/notifications/tests.rs index 9495ecdb4afaf..6f9e766ddc033 100644 --- a/client/network/src/protocol/notifications/tests.rs +++ b/client/network/src/protocol/notifications/tests.rs @@ -19,11 +19,12 @@ #![cfg(test)] use crate::{ + peer_store::PeerStore, protocol::notifications::{Notifications, NotificationsOut, ProtocolConfig}, - protocol_controller::{ProtoSetConfig, SetId}, + protocol_controller::{ProtoSetConfig, ProtocolController, SetId}, }; -use futures::prelude::*; +use futures::{future::BoxFuture, prelude::*}; use libp2p::{ core::{transport::MemoryTransport, upgrade, Endpoint}, identity, noise, @@ -34,6 +35,7 @@ use libp2p::{ }, yamux, Multiaddr, PeerId, Transport, }; +use sc_utils::mpsc::tracing_unbounded; use std::{ iter, pin::Pin, @@ -68,24 +70,31 @@ fn build_nodes() -> (Swarm, Swarm) { .timeout(Duration::from_secs(20)) .boxed(); - let (peerset, handle) = - crate::peerset::Peerset::from_config(crate::peerset::PeersetConfig { - bootnodes: if index == 0 { - keypairs.iter().skip(1).map(|keypair| keypair.public().to_peer_id()).collect() - } else { - vec![] - }, - sets: vec![ProtoSetConfig { - in_peers: 25, - out_peers: 25, - reserved_nodes: Default::default(), - reserved_only: false, - }], - }); + let peer_store = PeerStore::new(if index == 0 { + keypairs.iter().skip(1).map(|keypair| keypair.public().to_peer_id()).collect() + } else { + vec![] + }); + + let (to_notifications, from_controller) = + tracing_unbounded("test_protocol_controller_to_notifications", 10_000); + + let (controller_handle, controller) = ProtocolController::new( + SetId::from(0), + ProtoSetConfig { + in_peers: 25, + out_peers: 25, + reserved_nodes: Default::default(), + reserved_only: false, + }, + to_notifications, + Box::new(peer_store.handle()), + ); let behaviour = CustomProtoWithAddr { inner: Notifications::new( - peerset, + vec![controller_handle], + from_controller, iter::once(ProtocolConfig { name: "/foo".into(), fallback_names: Vec::new(), @@ -93,7 +102,8 @@ fn build_nodes() -> (Swarm, Swarm) { max_notification_size: 1024 * 1024, }), ), - _peerset_handle: handle, + peer_store_future: peer_store.run().boxed(), + protocol_controller_future: controller.run().boxed(), addrs: addrs .iter() .enumerate() @@ -129,8 +139,8 @@ fn build_nodes() -> (Swarm, Swarm) { /// Wraps around the `CustomBehaviour` network behaviour, and adds hardcoded node addresses to it. struct CustomProtoWithAddr { inner: Notifications, - // We need to keep `PeersetHandle` for `Peerset` not to shut down. - _peerset_handle: crate::peerset::PeersetHandle, + peer_store_future: BoxFuture<'static, ()>, + protocol_controller_future: BoxFuture<'static, ()>, addrs: Vec<(PeerId, Multiaddr)>, } @@ -229,6 +239,9 @@ impl NetworkBehaviour for CustomProtoWithAddr { cx: &mut Context, params: &mut impl PollParameters, ) -> Poll>> { + let _ = self.peer_store_future.poll_unpin(cx); + let _ = self.protocol_controller_future.poll_unpin(cx); + self.inner.poll(cx, params) } } From 8b64997aa4caa302781eeaaabc8dbd7186dd81a6 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 17:39:09 +0300 Subject: [PATCH 18/37] Fix `Notifications` tests --- client/network/src/protocol/notifications/behaviour.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 0d3f326bc443a..473783ac67f4d 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -2170,7 +2170,7 @@ mod tests { } fn register_protocol(&self, _protocol_handle: ProtocolHandle) { - unimplemented!() + // Not a real peer store, do nothing. } fn report_disconnect(&mut self, _peer_id: PeerId) { @@ -3014,8 +3014,6 @@ mod tests { // check peer information assert_eq!(notif.open_peers().collect::>(), vec![&peer],); - //assert_eq!(notif.num_discovered_peers(), 0usize); - todo!("^"); // close the other connection and verify that notification replacement event is emitted notif.on_swarm_event(FromSwarm::ConnectionClosed( From eb2693e93bc7629d9addd089caba52a7edfc7dc4 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Thu, 8 Jun 2023 17:45:30 +0300 Subject: [PATCH 19/37] Remove `Peerset` completely --- client/network/src/lib.rs | 1 - client/network/src/peerset.rs | 291 ---------------------------------- 2 files changed, 292 deletions(-) delete mode 100644 client/network/src/peerset.rs diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index ad5ffa2dffae5..f33eed98baf46 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -253,7 +253,6 @@ pub mod event; pub mod network_state; pub mod peer_info; pub mod peer_store; -pub mod peerset; pub mod protocol_controller; pub mod request_responses; pub mod transport; diff --git a/client/network/src/peerset.rs b/client/network/src/peerset.rs deleted file mode 100644 index 58ffb27db8a04..0000000000000 --- a/client/network/src/peerset.rs +++ /dev/null @@ -1,291 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 - -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with this program. If not, see . - -//! Peer Set Manager (PSM). Contains the strategy for choosing which nodes the network should be -//! connected to. -//! -//! The PSM handles *sets* of nodes. A set of nodes is defined as the nodes that are believed to -//! support a certain capability, such as handling blocks and transactions of a specific chain, -//! or collating a certain parachain. -//! -//! For each node in each set, the peerset holds a flag specifying whether the node is -//! connected to us or not. -//! -//! This connected/disconnected status is specific to the node and set combination, and it is for -//! example possible for a node to be connected through a specific set but not another. -//! -//! In addition, for each, set, the peerset also holds a list of reserved nodes towards which it -//! will at all time try to maintain a connection with. - -use crate::{ - peer_store::{PeerStore, PeerStoreHandle, PeerStoreProvider}, - protocol_controller::{ - IncomingIndex, Message, ProtoSetConfig, ProtocolController, ProtocolHandle, SetId, - }, -}; - -use futures::{ - channel::oneshot, - future::{join_all, BoxFuture, JoinAll}, - prelude::*, - stream::Stream, -}; -use log::debug; -use sc_network_common::types::ReputationChange; -use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnboundedSender}; -use std::{ - collections::HashSet, - pin::Pin, - task::{Context, Poll}, -}; - -use libp2p::PeerId; - -/// Log target for this file. -pub const LOG_TARGET: &str = "peerset"; - -#[derive(Debug)] -enum Action { - AddReservedPeer(SetId, PeerId), - RemoveReservedPeer(SetId, PeerId), - SetReservedPeers(SetId, HashSet), - SetReservedOnly(SetId, bool), - ReportPeer(PeerId, ReputationChange), - AddKnownPeer(PeerId), - PeerReputation(PeerId, oneshot::Sender), -} - -/// Shared handle to the peer set manager (PSM). Distributed around the code. -#[derive(Debug, Clone)] -pub struct PeersetHandle { - tx: TracingUnboundedSender, -} - -impl PeersetHandle { - /// Adds a new reserved peer. The peerset will make an effort to always remain connected to - /// this peer. - /// - /// Has no effect if the node was already a reserved peer. - /// - /// > **Note**: Keep in mind that the networking has to know an address for this node, - /// > otherwise it will not be able to connect to it. - pub fn add_reserved_peer(&self, set_id: SetId, peer_id: PeerId) { - let _ = self.tx.unbounded_send(Action::AddReservedPeer(set_id, peer_id)); - } - - /// Remove a previously-added reserved peer. - /// - /// Has no effect if the node was not a reserved peer. - pub fn remove_reserved_peer(&self, set_id: SetId, peer_id: PeerId) { - let _ = self.tx.unbounded_send(Action::RemoveReservedPeer(set_id, peer_id)); - } - - /// Sets whether or not the peerset only has connections with nodes marked as reserved for - /// the given set. - pub fn set_reserved_only(&self, set_id: SetId, reserved: bool) { - let _ = self.tx.unbounded_send(Action::SetReservedOnly(set_id, reserved)); - } - - /// Set reserved peers to the new set. - pub fn set_reserved_peers(&self, set_id: SetId, peer_ids: HashSet) { - let _ = self.tx.unbounded_send(Action::SetReservedPeers(set_id, peer_ids)); - } - - /// Reports an adjustment to the reputation of the given peer. - pub fn report_peer(&self, peer_id: PeerId, score_diff: ReputationChange) { - let _ = self.tx.unbounded_send(Action::ReportPeer(peer_id, score_diff)); - } - - /// Add a peer to the list of known peers. - pub fn add_known_peer(&self, peer_id: PeerId) { - let _ = self.tx.unbounded_send(Action::AddKnownPeer(peer_id)); - } - - /// Returns the reputation value of the peer. - pub async fn peer_reputation(self, peer_id: PeerId) -> Result { - let (tx, rx) = oneshot::channel(); - - let _ = self.tx.unbounded_send(Action::PeerReputation(peer_id, tx)); - - // The channel can only be closed if the peerset no longer exists. - rx.await.map_err(|_| ()) - } -} - -/// Configuration to pass when creating the peer set manager. -#[derive(Debug)] -pub struct PeersetConfig { - /// Bootnodes. - pub bootnodes: Vec, - /// List of sets of nodes the peerset manages. - pub sets: Vec, -} - -/// Side of the peer set manager owned by the network. In other words, the "receiving" side. -/// -/// Implements the `Stream` trait and can be polled for messages. The `Stream` never ends and never -/// errors. -pub struct Peerset { - /// Peer reputation store handle. - peer_store_handle: PeerStoreHandle, - /// Peer reputation store. - peer_store_future: BoxFuture<'static, ()>, - /// Protocol handles. - protocol_handles: Vec, - /// Protocol controllers responsible for connections, per `SetId`. - protocol_controller_futures: JoinAll>, - /// Commands sent from protocol controllers to `Notifications`. The size of this vector never - /// changes. - from_controllers: TracingUnboundedReceiver, - /// Receiver for messages from the `PeersetHandle` and from `to_self`. - from_handle: TracingUnboundedReceiver, -} - -impl Peerset { - /// Builds a new peerset from the given configuration. - pub fn from_config(config: PeersetConfig) -> (Peerset, PeersetHandle) { - let peer_store = PeerStore::new(config.bootnodes); - - let (to_notifications, from_controllers) = - tracing_unbounded("mpsc_protocol_controllers_to_notifications", 10_000); - - let controllers = config - .sets - .into_iter() - .enumerate() - .map(|(set, set_config)| { - ProtocolController::new( - SetId::from(set), - set_config, - to_notifications.clone(), - Box::new(peer_store.handle()), - ) - }) - .collect::>(); - - let (protocol_handles, protocol_controllers): (Vec, Vec<_>) = - controllers.into_iter().unzip(); - - let (tx, from_handle) = tracing_unbounded("mpsc_peerset_messages", 10_000); - - let handle = PeersetHandle { tx }; - - let protocol_controller_futures = - join_all(protocol_controllers.into_iter().map(|c| c.run().boxed())); - - let peerset = Peerset { - peer_store_handle: peer_store.handle(), - peer_store_future: peer_store.run().boxed(), - protocol_handles, - protocol_controller_futures, - from_controllers, - from_handle, - }; - - (peerset, handle) - } - - /// Returns the list of reserved peers. - pub fn reserved_peers(&self, set_id: SetId, pending_response: oneshot::Sender>) { - self.protocol_handles[usize::from(set_id)].reserved_peers(pending_response); - } - - /// Indicate that we received an incoming connection. Must be answered either with - /// a corresponding `Accept` or `Reject`, except if we were already connected to this peer. - /// - /// Note that this mechanism is orthogonal to `Connect`/`Drop`. Accepting an incoming - /// connection implicitly means `Connect`, but incoming connections aren't cancelled by - /// `dropped`. - // Implementation note: because of concurrency issues, it is possible that we push a `Connect` - // message to the output channel with a `PeerId`, and that `incoming` gets called with the same - // `PeerId` before that message has been read by the user. In this situation we must not answer. - pub fn incoming(&mut self, set_id: SetId, peer_id: PeerId, index: IncomingIndex) { - self.protocol_handles[usize::from(set_id)].incoming_connection(peer_id, index); - } - - /// Indicate that we dropped an active connection with a peer, or that we failed to connect. - /// - /// Must only be called after the PSM has either generated a `Connect` message with this - /// `PeerId`, or accepted an incoming connection with this `PeerId`. - pub fn dropped(&mut self, set_id: SetId, peer_id: PeerId) { - self.protocol_handles[usize::from(set_id)].dropped(peer_id); - } - - /// Returns the number of peers that we have discovered. - pub fn num_discovered_peers(&self) -> usize { - self.peer_store_handle.num_known_peers() - } -} - -impl Stream for Peerset { - type Item = Message; - - fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll> { - if let Poll::Ready(msg) = self.from_controllers.poll_next_unpin(cx) { - if let Some(msg) = msg { - return Poll::Ready(Some(msg)) - } else { - debug!( - target: LOG_TARGET, - "All `ProtocolController`s have terminated, terminating `Peerset`." - ); - return Poll::Ready(None) - } - } - - while let Poll::Ready(action) = self.from_handle.poll_next_unpin(cx) { - if let Some(action) = action { - match action { - Action::AddReservedPeer(set_id, peer_id) => - self.protocol_handles[usize::from(set_id)].add_reserved_peer(peer_id), - Action::RemoveReservedPeer(set_id, peer_id) => - self.protocol_handles[usize::from(set_id)].remove_reserved_peer(peer_id), - Action::SetReservedPeers(set_id, peer_ids) => - self.protocol_handles[usize::from(set_id)].set_reserved_peers(peer_ids), - Action::SetReservedOnly(set_id, reserved_only) => - self.protocol_handles[usize::from(set_id)].set_reserved_only(reserved_only), - Action::ReportPeer(peer_id, score_diff) => - self.peer_store_handle.report_peer(peer_id, score_diff), - Action::AddKnownPeer(peer_id) => self.peer_store_handle.add_known_peer(peer_id), - Action::PeerReputation(peer_id, pending_response) => { - let _ = - pending_response.send(self.peer_store_handle.peer_reputation(&peer_id)); - }, - } - } else { - debug!(target: LOG_TARGET, "`PeersetHandle` was dropped, terminating `Peerset`."); - return Poll::Ready(None) - } - } - - if let Poll::Ready(()) = self.peer_store_future.poll_unpin(cx) { - debug!(target: LOG_TARGET, "`PeerStore` has terminated, terminating `PeerSet`."); - return Poll::Ready(None) - } - - if let Poll::Ready(_) = self.protocol_controller_futures.poll_unpin(cx) { - debug!( - target: LOG_TARGET, - "All `ProtocolHandle`s have terminated, terminating `PeerSet`." - ); - return Poll::Ready(None) - } - - Poll::Pending - } -} From cb3b12cfa340794280328ac653f7f3058bf15e25 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 13:45:10 +0300 Subject: [PATCH 20/37] Fix bug with counting sync peers in `Protocol` --- client/network/src/protocol.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 5010739e752f2..e6617ba8e3676 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -93,7 +93,7 @@ pub struct Protocol { /// solve this, an entry is added to this map whenever an invalid handshake is received. /// Entries are removed when the corresponding "substream closed" is later received. bad_handshake_substreams: HashSet<(PeerId, SetId)>, - /// Connected peers. + /// Connected peers on sync protocol. peers: HashMap, sync_substream_validations: FuturesUnordered, tx: TracingUnboundedSender>, @@ -170,7 +170,6 @@ impl Protocol { if let Some(position) = self.notification_protocols.iter().position(|p| *p == protocol_name) { self.behaviour.disconnect_peer(peer_id, SetId::from(position)); - self.peers.remove(peer_id); } else { warn!(target: "sub-libp2p", "disconnect_peer() with invalid protocol name") } @@ -181,7 +180,7 @@ impl Protocol { self.behaviour.peerset_debug_info() } - /// Returns the number of peers we're connected to. + /// Returns the number of peers we're connected to on sync protocol. pub fn num_connected_peers(&self) -> usize { self.peers.len() } @@ -531,7 +530,6 @@ impl NetworkBehaviour for Protocol { self.bad_handshake_substreams.insert((peer_id, set_id)); self.behaviour.disconnect_peer(&peer_id, set_id); self.peer_store_handle.report_peer(peer_id, rep::BAD_MESSAGE); - self.peers.remove(&peer_id); CustomMessageOutcome::None }, } From 649b6c12704f348380cb04b09381a71fcb4fc5e8 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 13:56:41 +0300 Subject: [PATCH 21/37] Eliminate indirect calls to `PeerStore` via `Protocol` --- client/network/src/protocol.rs | 25 ------------------- .../src/protocol/notifications/behaviour.rs | 7 ------ client/network/src/service.rs | 18 ++++++------- 3 files changed, 9 insertions(+), 41 deletions(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index e6617ba8e3676..13cad14f9809c 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -21,7 +21,6 @@ use crate::{ peer_store::{PeerStoreHandle, PeerStoreProvider}, protocol_controller::{self, SetId}, types::ProtocolName, - ReputationChange, }; use bytes::Bytes; @@ -160,11 +159,6 @@ impl Protocol { self.behaviour.open_peers() } - /// Returns the number of discovered nodes that we keep in memory. - pub fn num_discovered_peers(&self) -> usize { - self.peer_store_handle.num_known_peers() - } - /// Disconnects the given peer if we are connected to it. pub fn disconnect_peer(&mut self, peer_id: &PeerId, protocol_name: ProtocolName) { if let Some(position) = self.notification_protocols.iter().position(|p| *p == protocol_name) @@ -175,21 +169,11 @@ impl Protocol { } } - /// Returns the state of the peerset manager, for debugging purposes. - pub fn peerset_debug_info(&mut self) -> serde_json::Value { - self.behaviour.peerset_debug_info() - } - /// Returns the number of peers we're connected to on sync protocol. pub fn num_connected_peers(&self) -> usize { self.peers.len() } - /// Adjusts the reputation of a node. - pub fn report_peer(&mut self, who: PeerId, reputation: ReputationChange) { - self.peer_store_handle.report_peer(who, reputation) - } - /// Set handshake for the notification protocol. pub fn set_notification_handshake(&mut self, protocol: ProtocolName, handshake: Vec) { if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { @@ -266,15 +250,6 @@ impl Protocol { ); } } - - /// Notify the protocol that we have learned about the existence of some peer. - /// - /// Can be called multiple times with the same `PeerId`. - pub fn add_known_peer(&mut self, peer_id: PeerId) { - // TODO: get rid of this function and call `Peerset`/`PeerStore` directly - // from `NetworkWorker`. - self.peer_store_handle.add_known_peer(peer_id); - } } /// Outcome of an incoming custom message. diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 473783ac67f4d..c459444475b41 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -538,13 +538,6 @@ impl Notifications { } } - /// Returns the state of the peerset manager, for debugging purposes. - pub fn peerset_debug_info(&mut self) -> serde_json::Value { - // TODO: Check what info we can include here. - // Issue reference: https://github.com/paritytech/substrate/issues/14160. - serde_json::json!("unimplemented") - } - /// Function that is called when the peerset wants us to connect to a peer. fn peerset_report_connect(&mut self, peer_id: PeerId, set_id: SetId) { // If `PeerId` is unknown to us, insert an entry, start dialing, and return early. diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 6ca4480cdf89c..7187991155e09 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -679,7 +679,11 @@ where external_addresses, connected_peers, not_connected_peers, - peerset: swarm.behaviour_mut().user_protocol_mut().peerset_debug_info(), + // TODO: Check what info we can include here. + // Issue reference: https://github.com/paritytech/substrate/issues/14160. + peerset: serde_json::json!( + "Unimplemented. See https://github.com/paritytech/substrate/issues/14160." + ), } } @@ -1295,8 +1299,7 @@ where } metrics .peerset_num_discovered - .set(self.network_service.behaviour_mut().user_protocol().num_discovered_peers() - as u64); + .set(self.peer_store_handle.num_known_peers() as u64); metrics.pending_connections.set( Swarm::network_info(&self.network_service).connection_counters().num_pending() as u64, @@ -1466,10 +1469,7 @@ where }, SwarmEvent::Behaviour(BehaviourOut::ReputationChanges { peer, changes }) => { for change in changes { - self.network_service - .behaviour_mut() - .user_protocol_mut() - .report_peer(peer, change); + self.peer_store_handle.report_peer(peer, change); } }, SwarmEvent::Behaviour(BehaviourOut::PeerIdentify { @@ -1492,10 +1492,10 @@ where .behaviour_mut() .add_self_reported_address_to_dht(&peer_id, &protocols, addr); } - self.network_service.behaviour_mut().user_protocol_mut().add_known_peer(peer_id); + self.peer_store_handle.add_known_peer(peer_id); }, SwarmEvent::Behaviour(BehaviourOut::Discovered(peer_id)) => { - self.network_service.behaviour_mut().user_protocol_mut().add_known_peer(peer_id); + self.peer_store_handle.add_known_peer(peer_id); }, SwarmEvent::Behaviour(BehaviourOut::RandomKademliaStarted) => { if let Some(metrics) = self.metrics.as_ref() { From 094ffe580dd411035df543b8f2aa274f97383b2c Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 15:08:14 +0300 Subject: [PATCH 22/37] Eliminate indirect calls to `ProtocolController` via `Protocol` --- client/network/src/protocol.rs | 73 -------------- client/network/src/service.rs | 144 ++++++++++++--------------- client/network/src/service/traits.rs | 22 +++- 3 files changed, 80 insertions(+), 159 deletions(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 13cad14f9809c..8e482bab751a0 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -77,10 +77,6 @@ type PendingSyncSubstreamValidation = pub struct Protocol { /// Used to report reputation changes. peer_store_handle: PeerStoreHandle, - /// Used to modify reserved peers. - protocol_controller_handles: Vec, - /// Shortcut for sync protocol controller handle. - sync_protocol_controller_handle: protocol_controller::ProtocolHandle, /// Handles opening the unique substream and sending and receiving raw messages. behaviour: Notifications, /// List of notifications protocols that have been registered. @@ -132,13 +128,8 @@ impl Protocol { ) }; - let sync_protocol_controller_handle = - protocol_controller_handles[usize::from(HARDCODED_PEERSETS_SYNC)].clone(); - let protocol = Self { peer_store_handle, - protocol_controller_handles, - sync_protocol_controller_handle, behaviour, notification_protocols: iter::once(block_announces_protocol.notifications_protocol) .chain(notification_protocols.iter().map(|s| s.notifications_protocol.clone())) @@ -186,70 +177,6 @@ impl Protocol { ); } } - - /// Set whether the syncing peers set is in reserved-only mode. - pub fn set_reserved_only(&self, reserved_only: bool) { - self.sync_protocol_controller_handle.set_reserved_only(reserved_only); - } - - /// Removes a `PeerId` from the list of reserved peers for syncing purposes. - pub fn remove_reserved_peer(&self, peer: PeerId) { - self.sync_protocol_controller_handle.remove_reserved_peer(peer); - } - - /// Returns the list of reserved peers. - pub fn reserved_peers(&self, pending_response: oneshot::Sender>) { - self.sync_protocol_controller_handle.reserved_peers(pending_response); - } - - /// Adds a `PeerId` to the list of reserved peers for syncing purposes. - pub fn add_reserved_peer(&self, peer: PeerId) { - self.sync_protocol_controller_handle.add_reserved_peer(peer); - } - - /// Sets the list of reserved peers for syncing purposes. - pub fn set_reserved_peers(&self, peers: HashSet) { - self.sync_protocol_controller_handle.set_reserved_peers(peers); - } - - /// Sets the list of reserved peers for the given protocol/peerset. - pub fn set_reserved_peerset_peers(&self, protocol: ProtocolName, peers: HashSet) { - if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.protocol_controller_handles[index].set_reserved_peers(peers); - } else { - error!( - target: "sub-libp2p", - "set_reserved_peerset_peers with unknown protocol: {}", - protocol - ); - } - } - - /// Removes a `PeerId` from the list of reserved peers. - pub fn remove_set_reserved_peer(&self, protocol: ProtocolName, peer: PeerId) { - if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.protocol_controller_handles[index].remove_reserved_peer(peer); - } else { - error!( - target: "sub-libp2p", - "remove_set_reserved_peer with unknown protocol: {}", - protocol - ); - } - } - - /// Adds a `PeerId` to the list of reserved peers. - pub fn add_set_reserved_peer(&self, protocol: ProtocolName, peer: PeerId) { - if let Some(index) = self.notification_protocols.iter().position(|p| *p == protocol) { - self.protocol_controller_handles[index].add_reserved_peer(peer); - } else { - error!( - target: "sub-libp2p", - "add_set_reserved_peer with unknown protocol: {}", - protocol - ); - } - } } /// Outcome of an incoming custom message. diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 7187991155e09..3a8b0b45ffbdd 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -38,7 +38,7 @@ use crate::{ }, peer_store::{PeerStore, PeerStoreHandle, PeerStoreProvider}, protocol::{self, NotifsHandlerError, Protocol, Ready}, - protocol_controller::{ProtoSetConfig, ProtocolController, SetId}, + protocol_controller::{self, ProtoSetConfig, ProtocolController, SetId}, request_responses::{IfDisconnected, RequestFailure}, service::{ signature::{Signature, SigningError}, @@ -124,6 +124,14 @@ pub struct NetworkService { /// Field extracted from the [`Metrics`] struct and necessary to report the /// notifications-related metrics. notifications_sizes_metric: Option, + /// Protocol name -> set id mapping for notification protocols. The map never changes after + /// initialization. + notification_protocol_ids: HashMap, + /// Handles to manage peer connections on notification protocols. The vector never changes + /// after initialization. + protocol_handles: Vec, + /// Shortcut to sync protocol handle (`protocol_handles[0]`). + sync_protocol_handle: protocol_controller::ProtocolHandle, /// Marker to pin the `H` generic. Serves no purpose except to not break backwards /// compatibility. _marker: PhantomData, @@ -298,17 +306,31 @@ where }) .unzip(); + // Shortcut to default peer set protocol handle. + let sync_protocol_handle = protocol_handles[0].clone(); + // Spawn `ProtocolController` runners. protocol_controllers .into_iter() .for_each(|controller| (params.executor)(controller.run().boxed())); + // Protocol name to protocol id mapping. The first protocol is always block announce (sync) + // protocol, aka default (hardcoded) peer set. + let notification_protocol_ids: HashMap = + iter::once(¶ms.block_announce_config) + .chain(notification_protocols.iter()) + .enumerate() + .map(|(index, protocol)| { + (protocol.notifications_protocol.clone(), SetId::from(index)) + }) + .collect(); + let protocol = Protocol::new( From::from(¶ms.role), notification_protocols.clone(), params.block_announce_config, peer_store_handle.clone(), - protocol_handles, + protocol_handles.clone(), from_protocol_controllers, params.tx, )?; @@ -507,6 +529,9 @@ where notifications_sizes_metric: metrics .as_ref() .map(|metrics| metrics.notifications_sizes.clone()), + notification_protocol_ids, + protocol_handles, + sync_protocol_handle, _marker: PhantomData, _block: Default::default(), }); @@ -696,14 +721,6 @@ where pub fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String> { self.service.add_reserved_peer(peer) } - - /// Returns the list of reserved peers. - fn reserved_peers(&self, pending_response: oneshot::Sender>) { - self.network_service - .behaviour() - .user_protocol() - .reserved_peers(pending_response); - } } impl NetworkService { @@ -733,11 +750,9 @@ impl NetworkService { pub async fn reserved_peers(&self) -> Result, ()> { let (tx, rx) = oneshot::channel(); - let _ = self - .to_worker - .unbounded_send(ServiceToWorkerMsg::ReservedPeers { pending_response: tx }); + self.sync_protocol_handle.reserved_peers(tx); - // The channel can only be closed if the network worker no longer exists. + // The channel can only be closed if `ProtocolController` no longer exists. rx.await.map_err(|_| ()) } @@ -850,13 +865,11 @@ where H: ExHashT, { fn set_authorized_peers(&self, peers: HashSet) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::SetReserved(peers)); + self.sync_protocol_handle.set_reserved_peers(peers); } fn set_authorized_only(&self, reserved_only: bool) { - let _ = self - .to_worker - .unbounded_send(ServiceToWorkerMsg::SetReservedOnly(reserved_only)); + self.sync_protocol_handle.set_reserved_only(reserved_only); } fn add_known_address(&self, peer_id: PeerId, addr: Multiaddr) { @@ -874,15 +887,15 @@ where } fn accept_unreserved_peers(&self) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::SetReservedOnly(false)); + self.sync_protocol_handle.set_reserved_only(false); } fn deny_unreserved_peers(&self) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::SetReservedOnly(true)); + self.sync_protocol_handle.set_reserved_only(true); } fn add_reserved_peer(&self, peer: MultiaddrWithPeerId) -> Result<(), String> { - // Make sure the local peer ID is never added to the PSM. + // Make sure the local peer ID is never added as a reserved peer. if peer.peer_id == self.local_peer_id { return Err("Local peer ID cannot be added as a reserved peer.".to_string()) } @@ -890,12 +903,12 @@ where let _ = self .to_worker .unbounded_send(ServiceToWorkerMsg::AddKnownAddress(peer.peer_id, peer.multiaddr)); - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::AddReserved(peer.peer_id)); + self.sync_protocol_handle.add_reserved_peer(peer.peer_id); Ok(()) } fn remove_reserved_peer(&self, peer_id: PeerId) { - let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::RemoveReserved(peer_id)); + self.sync_protocol_handle.remove_reserved_peer(peer_id); } fn set_reserved_peers( @@ -903,6 +916,10 @@ where protocol: ProtocolName, peers: HashSet, ) -> Result<(), String> { + let Some(set_id) = self.notification_protocol_ids.get(&protocol) else { + return Err(format!("Cannot set reserved peers for unknown protocol: {}", protocol)) + }; + let peers_addrs = self.split_multiaddr_and_peer_id(peers)?; let mut peers: HashSet = HashSet::with_capacity(peers_addrs.len()); @@ -922,9 +939,7 @@ where } } - let _ = self - .to_worker - .unbounded_send(ServiceToWorkerMsg::SetPeersetReserved(protocol, peers)); + self.protocol_handles[usize::from(*set_id)].set_reserved_peers(peers); Ok(()) } @@ -934,6 +949,12 @@ where protocol: ProtocolName, peers: HashSet, ) -> Result<(), String> { + let Some(set_id) = self.notification_protocol_ids.get(&protocol) else { + return Err( + format!("Cannot add peers to reserved set of unknown protocol: {}", protocol) + ) + }; + let peers = self.split_multiaddr_and_peer_id(peers)?; for (peer_id, addr) in peers.into_iter() { @@ -947,20 +968,29 @@ where .to_worker .unbounded_send(ServiceToWorkerMsg::AddKnownAddress(peer_id, addr)); } - let _ = self - .to_worker - .unbounded_send(ServiceToWorkerMsg::AddSetReserved(protocol.clone(), peer_id)); + + self.protocol_handles[usize::from(*set_id)].add_reserved_peer(peer_id); } Ok(()) } - fn remove_peers_from_reserved_set(&self, protocol: ProtocolName, peers: Vec) { + fn remove_peers_from_reserved_set( + &self, + protocol: ProtocolName, + peers: Vec, + ) -> Result<(), String> { + let Some(set_id) = self.notification_protocol_ids.get(&protocol) else { + return Err( + format!("Cannot remove peers from reserved set of unknown protocol: {}", protocol) + ) + }; + for peer_id in peers.into_iter() { - let _ = self - .to_worker - .unbounded_send(ServiceToWorkerMsg::RemoveSetReserved(protocol.clone(), peer_id)); + self.protocol_handles[usize::from(*set_id)].remove_reserved_peer(peer_id); } + + Ok(()) } fn sync_num_connected(&self) -> usize { @@ -1169,13 +1199,6 @@ enum ServiceToWorkerMsg { PutValue(KademliaKey, Vec), AddKnownAddress(PeerId, Multiaddr), ReportPeer(PeerId, ReputationChange), - SetReservedOnly(bool), - AddReserved(PeerId), - RemoveReserved(PeerId), - SetReserved(HashSet), - SetPeersetReserved(ProtocolName, HashSet), - AddSetReserved(ProtocolName, PeerId), - RemoveSetReserved(ProtocolName, PeerId), EventStream(out_events::Sender), Request { target: PeerId, @@ -1192,9 +1215,6 @@ enum ServiceToWorkerMsg { }, DisconnectPeer(PeerId, ProtocolName), SetNotificationHandshake(ProtocolName, Vec), - ReservedPeers { - pending_response: oneshot::Sender>, - }, } /// Main network worker. Must be polled in order for the network to advance. @@ -1316,41 +1336,6 @@ where 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::ReportPeer(peer_id, reputation_change) => @@ -1387,9 +1372,6 @@ where .behaviour_mut() .user_protocol_mut() .set_notification_handshake(protocol, handshake), - ServiceToWorkerMsg::ReservedPeers { pending_response } => { - self.reserved_peers(pending_response); - }, } } diff --git a/client/network/src/service/traits.rs b/client/network/src/service/traits.rs index 97680a9eb73b7..51aea9611588c 100644 --- a/client/network/src/service/traits.rs +++ b/client/network/src/service/traits.rs @@ -166,7 +166,7 @@ pub trait NetworkPeers { /// 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 + /// 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>; @@ -188,7 +188,8 @@ pub trait NetworkPeers { /// this step if the peer set is in reserved only mode. /// /// Returns an `Err` if one of the given addresses is invalid or contains an - /// invalid peer ID (which includes the local peer ID). + /// invalid peer ID (which includes the local peer ID), or if `protocol` does not + /// refer to a known protocol. fn set_reserved_peers( &self, protocol: ProtocolName, @@ -201,7 +202,8 @@ pub trait NetworkPeers { /// consist of only `/p2p/`. /// /// Returns an `Err` if one of the given addresses is invalid or contains an - /// invalid peer ID (which includes the local peer ID). + /// invalid peer ID (which includes the local peer ID), or if `protocol` does not + /// refer to a know protocol. fn add_peers_to_reserved_set( &self, protocol: ProtocolName, @@ -209,7 +211,13 @@ pub trait NetworkPeers { ) -> Result<(), String>; /// Remove peers from a peer set. - fn remove_peers_from_reserved_set(&self, protocol: ProtocolName, peers: Vec); + /// + /// Returns `Err` if `protocol` does not refer to a known protocol. + fn remove_peers_from_reserved_set( + &self, + protocol: ProtocolName, + peers: Vec, + ) -> Result<(), String>; /// Returns the number of peers in the sync peer set we're connected to. fn sync_num_connected(&self) -> usize; @@ -277,7 +285,11 @@ where T::add_peers_to_reserved_set(self, protocol, peers) } - fn remove_peers_from_reserved_set(&self, protocol: ProtocolName, peers: Vec) { + fn remove_peers_from_reserved_set( + &self, + protocol: ProtocolName, + peers: Vec, + ) -> Result<(), String> { T::remove_peers_from_reserved_set(self, protocol, peers) } From dd77fbab2fc5e2342cdea8857dbb904bdab389b0 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 16:20:44 +0300 Subject: [PATCH 23/37] Handle `Err` outcome from `remove_peers_from_reserved_set` --- client/consensus/grandpa/src/communication/tests.rs | 8 +++++++- client/network-gossip/src/bridge.rs | 13 ++++++++----- client/network-gossip/src/lib.rs | 6 ++++++ client/network-gossip/src/state_machine.rs | 8 +++++++- client/network/src/service.rs | 2 +- client/network/statement/src/lib.rs | 5 ++++- client/network/sync/src/service/mock.rs | 6 +++++- client/network/transactions/src/lib.rs | 5 ++++- client/offchain/src/api.rs | 6 +++++- client/offchain/src/lib.rs | 6 +++++- 10 files changed, 52 insertions(+), 13 deletions(-) diff --git a/client/consensus/grandpa/src/communication/tests.rs b/client/consensus/grandpa/src/communication/tests.rs index eb88382989175..dbe17676da98b 100644 --- a/client/consensus/grandpa/src/communication/tests.rs +++ b/client/consensus/grandpa/src/communication/tests.rs @@ -114,7 +114,13 @@ impl NetworkPeers for TestNetwork { unimplemented!(); } - fn remove_peers_from_reserved_set(&self, _protocol: ProtocolName, _peers: Vec) {} + fn remove_peers_from_reserved_set( + &self, + _protocol: ProtocolName, + _peers: Vec, + ) -> Result<(), String> { + unimplemented!(); + } fn sync_num_connected(&self) -> usize { unimplemented!(); diff --git a/client/network-gossip/src/bridge.rs b/client/network-gossip/src/bridge.rs index 2cd4e18171568..6a3790ee2b2b2 100644 --- a/client/network-gossip/src/bridge.rs +++ b/client/network-gossip/src/bridge.rs @@ -238,10 +238,7 @@ impl Future for GossipEngine { SyncEvent::PeerConnected(remote) => this.network.add_set_reserved(remote, this.protocol.clone()), SyncEvent::PeerDisconnected(remote) => - this.network.remove_peers_from_reserved_set( - this.protocol.clone(), - vec![remote], - ), + this.network.remove_set_reserved(remote, this.protocol.clone()), }, // The sync event stream closed. Do the same for [`GossipValidator`]. Poll::Ready(None) => { @@ -413,7 +410,13 @@ mod tests { unimplemented!(); } - fn remove_peers_from_reserved_set(&self, _protocol: ProtocolName, _peers: Vec) {} + fn remove_peers_from_reserved_set( + &self, + _protocol: ProtocolName, + _peers: Vec, + ) -> Result<(), String> { + unimplemented!(); + } fn sync_num_connected(&self) -> usize { unimplemented!(); diff --git a/client/network-gossip/src/lib.rs b/client/network-gossip/src/lib.rs index ef87dd599e010..5b02be5c23f69 100644 --- a/client/network-gossip/src/lib.rs +++ b/client/network-gossip/src/lib.rs @@ -89,6 +89,12 @@ pub trait Network: NetworkPeers + NetworkEventStream + NetworkNotific log::error!(target: "gossip", "add_set_reserved failed: {}", err); } } + fn remove_set_reserved(&self, who: PeerId, protocol: ProtocolName) { + let result = self.remove_peers_from_reserved_set(protocol, iter::once(who).collect()); + if let Err(err) = result { + log::error!(target: "gossip", "remove_set_reserved failed: {}", err); + } + } } impl Network for T where T: NetworkPeers + NetworkEventStream + NetworkNotification {} diff --git a/client/network-gossip/src/state_machine.rs b/client/network-gossip/src/state_machine.rs index 24373cd402513..3d438b6830f7c 100644 --- a/client/network-gossip/src/state_machine.rs +++ b/client/network-gossip/src/state_machine.rs @@ -643,7 +643,13 @@ mod tests { unimplemented!(); } - fn remove_peers_from_reserved_set(&self, _protocol: ProtocolName, _peers: Vec) {} + fn remove_peers_from_reserved_set( + &self, + _protocol: ProtocolName, + _peers: Vec, + ) -> Result<(), String> { + unimplemented!(); + } fn sync_num_connected(&self) -> usize { unimplemented!(); diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 3a8b0b45ffbdd..701179e6d0517 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -306,7 +306,7 @@ where }) .unzip(); - // Shortcut to default peer set protocol handle. + // Shortcut to default (sync) peer set protocol handle. let sync_protocol_handle = protocol_handles[0].clone(); // Spawn `ProtocolController` runners. diff --git a/client/network/statement/src/lib.rs b/client/network/statement/src/lib.rs index 2c966a346ad87..7e87107329486 100644 --- a/client/network/statement/src/lib.rs +++ b/client/network/statement/src/lib.rs @@ -297,10 +297,13 @@ where } }, SyncEvent::PeerDisconnected(remote) => { - self.network.remove_peers_from_reserved_set( + let result = self.network.remove_peers_from_reserved_set( self.protocol_name.clone(), iter::once(remote).collect(), ); + if let Err(err) = result { + log::error!(target: LOG_TARGET, "Remove reserved peer failed: {}", err); + } }, } } diff --git a/client/network/sync/src/service/mock.rs b/client/network/sync/src/service/mock.rs index b3ef0f328140b..885eb1f8da593 100644 --- a/client/network/sync/src/service/mock.rs +++ b/client/network/sync/src/service/mock.rs @@ -99,7 +99,11 @@ mockall::mock! { protocol: ProtocolName, peers: HashSet, ) -> Result<(), String>; - fn remove_peers_from_reserved_set(&self, protocol: ProtocolName, peers: Vec); + fn remove_peers_from_reserved_set( + &self, + protocol: ProtocolName, + peers: Vec + ) -> Result<(), String>; fn sync_num_connected(&self) -> usize; } diff --git a/client/network/transactions/src/lib.rs b/client/network/transactions/src/lib.rs index 78fd432ab0c4e..b46733d427230 100644 --- a/client/network/transactions/src/lib.rs +++ b/client/network/transactions/src/lib.rs @@ -338,10 +338,13 @@ where } }, SyncEvent::PeerDisconnected(remote) => { - self.network.remove_peers_from_reserved_set( + let result = self.network.remove_peers_from_reserved_set( self.protocol_name.clone(), iter::once(remote).collect(), ); + if let Err(err) = result { + log::error!(target: "sync", "Remove reserved peer failed: {}", err); + } }, } } diff --git a/client/offchain/src/api.rs b/client/offchain/src/api.rs index 3171fb133945f..c67dd8bb2c1a8 100644 --- a/client/offchain/src/api.rs +++ b/client/offchain/src/api.rs @@ -388,7 +388,11 @@ mod tests { unimplemented!(); } - fn remove_peers_from_reserved_set(&self, _protocol: ProtocolName, _peers: Vec) { + fn remove_peers_from_reserved_set( + &self, + _protocol: ProtocolName, + _peers: Vec, + ) -> Result<(), String> { unimplemented!(); } diff --git a/client/offchain/src/lib.rs b/client/offchain/src/lib.rs index 27f7df3837050..c74f0e3d7e3dd 100644 --- a/client/offchain/src/lib.rs +++ b/client/offchain/src/lib.rs @@ -328,7 +328,11 @@ mod tests { unimplemented!(); } - fn remove_peers_from_reserved_set(&self, _protocol: ProtocolName, _peers: Vec) { + fn remove_peers_from_reserved_set( + &self, + _protocol: ProtocolName, + _peers: Vec, + ) -> Result<(), String> { unimplemented!(); } From 5cddda0a7405978535ff40441f2e95708c5370a6 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 16:35:07 +0300 Subject: [PATCH 24/37] Add note about disconnecting sync peers in `Protocol` --- client/network/src/protocol.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 8e482bab751a0..17ab719b2f082 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -154,6 +154,9 @@ impl Protocol { pub fn disconnect_peer(&mut self, peer_id: &PeerId, protocol_name: ProtocolName) { if let Some(position) = self.notification_protocols.iter().position(|p| *p == protocol_name) { + // Note: no need to remove a peer from `self.peers` if we are dealing with sync + // protocol, because it will be done when handling + // `NotificationsOut::CustomProtocolClosed`. self.behaviour.disconnect_peer(peer_id, SetId::from(position)); } else { warn!(target: "sub-libp2p", "disconnect_peer() with invalid protocol name") From 84fb33d2a67d86511eb40dc936e310d08ffca32f Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 16:45:53 +0300 Subject: [PATCH 25/37] minor: remove unneeded `clone()` --- client/network/src/protocol.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 17ab719b2f082..a22590144c986 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -108,7 +108,7 @@ impl Protocol { ) -> error::Result { let behaviour = { Notifications::new( - protocol_controller_handles.clone(), + protocol_controller_handles, from_protocol_controllers, // NOTE: Block announcement protocol is still very much hardcoded into `Protocol`. // This protocol must be the first notification protocol given to From 7588e32811ffa046b99e0a89b88741926885530c Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 16:51:08 +0300 Subject: [PATCH 26/37] minor: extra comma removed --- client/network/src/service/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/service/traits.rs b/client/network/src/service/traits.rs index 51aea9611588c..c766055d5a8b5 100644 --- a/client/network/src/service/traits.rs +++ b/client/network/src/service/traits.rs @@ -166,7 +166,7 @@ pub trait NetworkPeers { /// 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, + /// 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>; From 7a84efce284ef57a7bbaed886aaad3330c2eb154 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 17:07:13 +0300 Subject: [PATCH 27/37] minor: use `Stream` API of `from_protocol_controllers` channel --- .../src/protocol/notifications/behaviour.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index c459444475b41..59652ef44ffc7 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -2004,30 +2004,29 @@ impl NetworkBehaviour for Notifications { return Poll::Ready(event) } - // Poll for instructions from the peerset. - // Note that the peerset is a *best effort* crate, and we have to use defensive programming. + // Poll for instructions from the protocol controllers. loop { - match self.from_protocol_controllers.next().now_or_never() { - Some(Some(Message::Accept(index))) => { + match futures::Stream::poll_next(Pin::new(&mut self.from_protocol_controllers), cx) { + Poll::Ready(Some(Message::Accept(index))) => { self.peerset_report_accept(index); }, - Some(Some(Message::Reject(index))) => { + Poll::Ready(Some(Message::Reject(index))) => { self.peerset_report_reject(index); }, - Some(Some(Message::Connect { peer_id, set_id, .. })) => { + Poll::Ready(Some(Message::Connect { peer_id, set_id, .. })) => { self.peerset_report_connect(peer_id, set_id); }, - Some(Some(Message::Drop { peer_id, set_id, .. })) => { + Poll::Ready(Some(Message::Drop { peer_id, set_id, .. })) => { self.peerset_report_disconnect(peer_id, set_id); }, - Some(None) => { + Poll::Ready(None) => { error!( target: "sub-libp2p", "Protocol controllers receiver stream has returned None", ); break }, - None => break, + Poll::Pending => break, } } From 7b39689e79c58345347326ab07e4070d07ec96a1 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 17:17:04 +0300 Subject: [PATCH 28/37] minor: remove TODO --- client/network/src/protocol_controller.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index 4881042d7cc8d..c209a6650e72a 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -87,8 +87,6 @@ impl From for usize { } /// Configuration for a set of nodes for a specific protocol. -// -// TODO: merge this with [`crate::config::SetConfig`]. #[derive(Debug)] pub struct ProtoSetConfig { /// Maximum number of ingoing links to peers. From a9b65dc93d26fd33dcc8e5c60e7ff30c290adf3f Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 17:21:33 +0300 Subject: [PATCH 29/37] minor: replace `.map().flatten()` with `.flat_map()` --- client/network/src/service.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 701179e6d0517..ac9c31de8d709 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -345,14 +345,13 @@ where .chain( notification_protocols .iter() - .map(|protocol| { + .flat_map(|protocol| { protocol .set_config .reserved_nodes .iter() .map(|reserved| (reserved.peer_id, reserved.multiaddr.clone())) }) - .flatten(), ) .chain( network_config From ac226736ddfa5a43e99e18cfebeaa61fded3bf70 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 17:27:12 +0300 Subject: [PATCH 30/37] minor: update `ProtocolController` docs --- client/network/src/protocol_controller.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index c209a6650e72a..79cae97c1094a 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -57,13 +57,11 @@ use crate::peer_store::PeerStoreProvider; /// Log target for this file. pub const LOG_TARGET: &str = "peerset"; -/// `Notifications` protocol identifier. For historical reasons it's called `SetId`, because it +/// `Notifications` protocol index. For historical reasons it's called `SetId`, because it /// used to refer to a set of peers in a peerset for this protocol. /// -/// Can be constructed using the `From` trait implementation based on the index of the set -/// within [`PeersetConfig::sets`]. For example, the first element of [`PeersetConfig::sets`] is -/// later referred to with `SetId::from(0)`. It is intended that the code responsible for building -/// the [`PeersetConfig`] is also responsible for constructing the [`SetId`]s. +/// Can be constructed using the `From` trait implementation based on the index of the +/// protocol in `Notifications`. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct SetId(usize); @@ -101,7 +99,7 @@ pub struct ProtoSetConfig { /// > otherwise it will not be able to connect to them. pub reserved_nodes: HashSet, - /// If true, we only accept nodes in [`SetConfig::reserved_nodes`]. + /// If true, we only accept nodes in [`ProtoSetConfig::reserved_nodes`]. pub reserved_only: bool, } @@ -261,8 +259,7 @@ impl Default for PeerState { } } -/// Side of [`ProtocolHandle`] responsible for all the logic. Currently all instances are -/// owned by [`crate::Peerset`], but they should eventually be moved to corresponding protocols. +/// Worker side of [`ProtocolHandle`] responsible for all the logic. #[derive(Debug)] pub struct ProtocolController { /// Set id to use when sending connect/drop requests to `Notifications`. From c63726076b7719c5ac616fe31f04019a933bb793 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Fri, 9 Jun 2023 17:34:22 +0300 Subject: [PATCH 31/37] rustfmt --- client/network/src/service.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/client/network/src/service.rs b/client/network/src/service.rs index ac9c31de8d709..0ecc3f2caa8a0 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -342,17 +342,13 @@ where .reserved_nodes .iter() .map(|reserved| (reserved.peer_id, reserved.multiaddr.clone())) - .chain( - notification_protocols + .chain(notification_protocols.iter().flat_map(|protocol| { + protocol + .set_config + .reserved_nodes .iter() - .flat_map(|protocol| { - protocol - .set_config - .reserved_nodes - .iter() - .map(|reserved| (reserved.peer_id, reserved.multiaddr.clone())) - }) - ) + .map(|reserved| (reserved.peer_id, reserved.multiaddr.clone())) + })) .chain( network_config .boot_nodes From 6f177142702dcd9477c4785c958a686a19875bcd Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 13 Jun 2023 12:07:27 +0300 Subject: [PATCH 32/37] Apply suggestions from code review Co-authored-by: Aaro Altonen <48052676+altonen@users.noreply.github.com> --- client/network/src/protocol_controller.rs | 4 ++-- client/network/src/request_responses.rs | 18 +++++++----------- client/network/src/service.rs | 2 +- client/network/statement/src/lib.rs | 2 +- 4 files changed, 11 insertions(+), 15 deletions(-) diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index 2f58f8dbcecf5..5b71293f61537 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -87,7 +87,7 @@ impl From for usize { /// Configuration for a set of nodes for a specific protocol. #[derive(Debug)] pub struct ProtoSetConfig { - /// Maximum number of ingoing links to peers. + /// Maximum number of incoming links to peers. pub in_peers: u32, /// Maximum number of outgoing links to peers. @@ -106,7 +106,7 @@ pub struct ProtoSetConfig { /// Message that is sent by [`ProtocolController`] to `Notifications`. #[derive(Debug, PartialEq)] pub enum Message { - /// Request to open a connection to the given peer. From the point of view of the PSM, we are + /// Request to open a connection to the given peer. From the point of view of the `ProtocolController`, we are /// immediately connected. Connect { /// Set id to connect on. diff --git a/client/network/src/request_responses.rs b/client/network/src/request_responses.rs index 2ab14d295d5bb..30c8092db67d0 100644 --- a/client/network/src/request_responses.rs +++ b/client/network/src/request_responses.rs @@ -668,17 +668,13 @@ impl NetworkBehaviour for RequestResponsesBehaviour { // The `tx` created above can be dropped if we are not capable of // processing this request, which is reflected as a // `InboundFailure::Omission` event. - if let Ok(response) = rx.await { - Some(RequestProcessingOutcome { - peer, - request_id, - protocol, - inner_channel: channel, - response, - }) - } else { - None - } + rx.await.map_or(None, |response| Some(RequestProcessingOutcome { + peer, + request_id, + protocol, + inner_channel: channel, + response, + })) })); // This `continue` makes sure that `pending_responses` gets polled diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 0ecc3f2caa8a0..717c64ee71885 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -124,7 +124,7 @@ pub struct NetworkService { /// Field extracted from the [`Metrics`] struct and necessary to report the /// notifications-related metrics. notifications_sizes_metric: Option, - /// Protocol name -> set id mapping for notification protocols. The map never changes after + /// Protocol name -> `SetId` mapping for notification protocols. The map never changes after /// initialization. notification_protocol_ids: HashMap, /// Handles to manage peer connections on notification protocols. The vector never changes diff --git a/client/network/statement/src/lib.rs b/client/network/statement/src/lib.rs index 7e87107329486..c5d83b59b260a 100644 --- a/client/network/statement/src/lib.rs +++ b/client/network/statement/src/lib.rs @@ -302,7 +302,7 @@ where iter::once(remote).collect(), ); if let Err(err) = result { - log::error!(target: LOG_TARGET, "Remove reserved peer failed: {}", err); + log::error!(target: LOG_TARGET, "Failed to remove reserved peer: {err}"); } }, } From b5ec10284993805a4561f4bf8cda3c2534df01d5 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Tue, 13 Jun 2023 12:47:17 +0300 Subject: [PATCH 33/37] Extract `MockPeerStore` to `mock.rs` --- client/network/src/lib.rs | 3 + client/network/src/mock.rs | 55 +++++++++++++++++++ .../src/protocol/notifications/behaviour.rs | 34 +----------- client/network/src/protocol_controller.rs | 4 +- client/network/src/request_responses.rs | 50 ++++------------- 5 files changed, 73 insertions(+), 73 deletions(-) create mode 100644 client/network/src/mock.rs diff --git a/client/network/src/lib.rs b/client/network/src/lib.rs index f33eed98baf46..ee30759687841 100644 --- a/client/network/src/lib.rs +++ b/client/network/src/lib.rs @@ -246,6 +246,9 @@ mod behaviour; mod protocol; mod service; +#[cfg(test)] +mod mock; + pub mod config; pub mod discovery; pub mod error; diff --git a/client/network/src/mock.rs b/client/network/src/mock.rs new file mode 100644 index 0000000000000..bc596b0fa579e --- /dev/null +++ b/client/network/src/mock.rs @@ -0,0 +1,55 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Mocked components for tests. + +use crate::{peer_store::PeerStoreProvider, protocol_controller::ProtocolHandle, ReputationChange}; +use libp2p::PeerId; +use std::collections::HashSet; + +/// No-op `PeerStore`. +#[derive(Debug)] +pub struct MockPeerStore {} + +impl PeerStoreProvider for MockPeerStore { + fn is_banned(&self, _peer_id: &PeerId) -> bool { + // Make sure that the peer is not banned. + false + } + + fn register_protocol(&self, _protocol_handle: ProtocolHandle) { + // Make sure not to fail. + } + + fn report_disconnect(&mut self, _peer_id: PeerId) { + // Make sure not to fail. + } + + fn report_peer(&mut self, _peer_id: PeerId, _change: ReputationChange) { + // Make sure not to fail. + } + + fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { + // Make sure that the peer is not banned. + 0 + } + + fn outgoing_candidates(&self, _count: usize, _ignored: HashSet<&PeerId>) -> Vec { + unimplemented!() + } +} diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 59652ef44ffc7..c242bfa7288e6 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -2101,10 +2101,9 @@ impl NetworkBehaviour for Notifications { mod tests { use super::*; use crate::{ - peer_store::PeerStoreProvider, + mock::MockPeerStore, protocol::notifications::handler::tests::*, - protocol_controller::{IncomingIndex, ProtoSetConfig, ProtocolController, ProtocolHandle}, - ReputationChange, + protocol_controller::{IncomingIndex, ProtoSetConfig, ProtocolController}, }; use libp2p::swarm::AddressRecord; use sc_utils::mpsc::tracing_unbounded; @@ -2153,35 +2152,6 @@ mod tests { } } - #[derive(Debug)] - struct MockPeerStore {} - - impl PeerStoreProvider for MockPeerStore { - fn is_banned(&self, _peer_id: &PeerId) -> bool { - unimplemented!() - } - - fn register_protocol(&self, _protocol_handle: ProtocolHandle) { - // Not a real peer store, do nothing. - } - - fn report_disconnect(&mut self, _peer_id: PeerId) { - unimplemented!() - } - - fn report_peer(&mut self, _peer_id: PeerId, _change: ReputationChange) { - unimplemented!() - } - - fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { - unimplemented!() - } - - fn outgoing_candidates(&self, _count: usize, _ignored: HashSet<&PeerId>) -> Vec { - unimplemented!() - } - } - fn development_notifs() -> (Notifications, ProtocolController) { let (to_notifications, from_controller) = tracing_unbounded("test_controller_to_notifications", 10_000); diff --git a/client/network/src/protocol_controller.rs b/client/network/src/protocol_controller.rs index 5b71293f61537..fc2d911dead0f 100644 --- a/client/network/src/protocol_controller.rs +++ b/client/network/src/protocol_controller.rs @@ -106,8 +106,8 @@ pub struct ProtoSetConfig { /// Message that is sent by [`ProtocolController`] to `Notifications`. #[derive(Debug, PartialEq)] pub enum Message { - /// Request to open a connection to the given peer. From the point of view of the `ProtocolController`, we are - /// immediately connected. + /// Request to open a connection to the given peer. From the point of view of the + /// `ProtocolController`, we are immediately connected. Connect { /// Set id to connect on. set_id: SetId, diff --git a/client/network/src/request_responses.rs b/client/network/src/request_responses.rs index 30c8092db67d0..928ffe1d7ae56 100644 --- a/client/network/src/request_responses.rs +++ b/client/network/src/request_responses.rs @@ -668,13 +668,15 @@ impl NetworkBehaviour for RequestResponsesBehaviour { // The `tx` created above can be dropped if we are not capable of // processing this request, which is reflected as a // `InboundFailure::Omission` event. - rx.await.map_or(None, |response| Some(RequestProcessingOutcome { - peer, - request_id, - protocol, - inner_channel: channel, - response, - })) + rx.await.map_or(None, |response| { + Some(RequestProcessingOutcome { + peer, + request_id, + protocol, + inner_channel: channel, + response, + }) + }) })); // This `continue` makes sure that `pending_responses` gets polled @@ -963,7 +965,7 @@ impl Codec for GenericCodec { mod tests { use super::*; - use crate::{peer_store::PeerStoreProvider, protocol_controller::ProtocolHandle}; + use crate::mock::MockPeerStore; use futures::{channel::oneshot, executor::LocalPool, task::Spawn}; use libp2p::{ core::{ @@ -975,7 +977,7 @@ mod tests { swarm::{Executor, Swarm, SwarmBuilder, SwarmEvent}, Multiaddr, }; - use std::{collections::HashSet, iter, time::Duration}; + use std::{iter, time::Duration}; struct TokioExecutor(tokio::runtime::Runtime); impl Executor for TokioExecutor { @@ -984,36 +986,6 @@ mod tests { } } - #[derive(Debug)] - struct MockPeerStore {} - - impl PeerStoreProvider for MockPeerStore { - fn is_banned(&self, _peer_id: &PeerId) -> bool { - unimplemented!() - } - - fn register_protocol(&self, _protocol_handle: ProtocolHandle) { - unimplemented!() - } - - fn report_disconnect(&mut self, _peer_id: PeerId) { - unimplemented!() - } - - fn report_peer(&mut self, _peer_id: PeerId, _change: ReputationChange) { - unimplemented!() - } - - fn peer_reputation(&self, _peer_id: &PeerId) -> i32 { - // We just need to make sure that the peer is not banned. - 0 - } - - fn outgoing_candidates(&self, _count: usize, _ignored: HashSet<&PeerId>) -> Vec { - unimplemented!() - } - } - fn build_swarm( list: impl Iterator, ) -> (Swarm, Multiaddr) { From 05202302a350ac5b149022863b25669a15dd9ca4 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 14 Jun 2023 18:04:33 +0300 Subject: [PATCH 34/37] Move `PeerStore` initialization to `build_network` --- client/network/src/config.rs | 4 ++++ client/network/src/service.rs | 16 ++++------------ client/network/test/src/lib.rs | 8 ++++++++ client/network/test/src/service.rs | 8 ++++++++ client/service/src/builder.rs | 14 ++++++++++++++ 5 files changed, 38 insertions(+), 12 deletions(-) diff --git a/client/network/src/config.rs b/client/network/src/config.rs index 6c2a6c51ee5dc..f379970ffd086 100644 --- a/client/network/src/config.rs +++ b/client/network/src/config.rs @@ -31,6 +31,7 @@ pub use crate::{ pub use libp2p::{identity::Keypair, multiaddr, Multiaddr, PeerId}; +use crate::peer_store::PeerStoreHandle; use codec::Encode; use prometheus_endpoint::Registry; use zeroize::Zeroize; @@ -708,6 +709,9 @@ pub struct Params { /// Network layer configuration. pub network_config: FullNetworkConfiguration, + /// Peer store with known nodes, peer reputations, etc. + pub peer_store: PeerStoreHandle, + /// Legacy name of the protocol to use on the wire. Should be different for each chain. pub protocol_id: ProtocolId, diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 717c64ee71885..da70d59ae3c9a 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -268,14 +268,6 @@ where ) }; - let peer_store = PeerStore::new( - network_config.boot_nodes.iter().map(|bootnode| bootnode.peer_id).collect(), - ); - let peer_store_handle = peer_store.handle(); - - // Spawn `PeerStore` runner. - (params.executor)(peer_store.run().boxed()); - let (to_notifications, from_protocol_controllers) = tracing_unbounded("mpsc_protocol_controllers_to_notifications", 10_000); @@ -301,7 +293,7 @@ where SetId::from(set_id), proto_set_config, to_notifications.clone(), - Box::new(peer_store_handle.clone()), + Box::new(params.peer_store.clone()), ) }) .unzip(); @@ -329,7 +321,7 @@ where From::from(¶ms.role), notification_protocols.clone(), params.block_announce_config, - peer_store_handle.clone(), + params.peer_store.clone(), protocol_handles.clone(), from_protocol_controllers, params.tx, @@ -437,7 +429,7 @@ where local_public, discovery_config, request_response_protocols, - peer_store_handle.clone(), + params.peer_store.clone(), ); match result { @@ -542,7 +534,7 @@ where peers_notifications_sinks, metrics, boot_node_ids, - peer_store_handle, + peer_store_handle: params.peer_store, _marker: Default::default(), _block: Default::default(), }) diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 9f0010f545527..ea73d9aed59de 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -55,6 +55,7 @@ use sc_network::{ FullNetworkConfiguration, MultiaddrWithPeerId, NetworkConfiguration, NonDefaultSetConfig, NonReservedPeerMode, ProtocolId, Role, SyncMode, TransportConfig, }, + peer_store::PeerStore, request_responses::ProtocolConfig as RequestResponseConfig, types::ProtocolName, Multiaddr, NetworkBlock, NetworkService, NetworkStateInfo, NetworkSyncForkRequest, @@ -917,6 +918,12 @@ where }); } + let peer_store = PeerStore::new( + network_config.boot_nodes.iter().map(|bootnode| bootnode.peer_id).collect(), + ); + let peer_store_handle = peer_store.handle(); + self.spawn_task(peer_store.run().boxed()); + let genesis_hash = client.hash(Zero::zero()).ok().flatten().expect("Genesis block exists; qed"); let network = NetworkWorker::new(sc_network::config::Params { @@ -925,6 +932,7 @@ where tokio::spawn(f); }), network_config: full_net_config, + peer_store: peer_store_handle, genesis_hash, protocol_id, fork_id, diff --git a/client/network/test/src/service.rs b/client/network/test/src/service.rs index 8c15d6b09ea45..e2a9cb5f3bafd 100644 --- a/client/network/test/src/service.rs +++ b/client/network/test/src/service.rs @@ -23,6 +23,7 @@ use sc_consensus::{ImportQueue, Link}; use sc_network::{ config::{self, FullNetworkConfiguration, MultiaddrWithPeerId, ProtocolId, TransportConfig}, event::Event, + peer_store::PeerStore, NetworkEventStream, NetworkNotification, NetworkPeers, NetworkService, NetworkStateInfo, NetworkWorker, }; @@ -220,6 +221,12 @@ impl TestNetworkBuilder { full_net_config.add_request_response_protocol(config); } + let peer_store = PeerStore::new( + network_config.boot_nodes.iter().map(|bootnode| bootnode.peer_id).collect(), + ); + let peer_store_handle = peer_store.handle(); + tokio::spawn(peer_store.run().boxed()); + let genesis_hash = client.hash(Zero::zero()).ok().flatten().expect("Genesis block exists; qed"); let worker = NetworkWorker::< @@ -233,6 +240,7 @@ impl TestNetworkBuilder { }), genesis_hash, network_config: full_net_config, + peer_store: peer_store_handle, protocol_id, fork_id, metrics_registry: None, diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index fe71e11945bb2..830c780471c05 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -43,6 +43,7 @@ use sc_executor::{ use sc_keystore::LocalKeystore; use sc_network::{ config::{FullNetworkConfiguration, SyncMode}, + peer_store::PeerStore, NetworkService, NetworkStateInfo, NetworkStatusProvider, }; use sc_network_bitswap::BitswapRequestHandler; @@ -896,6 +897,18 @@ where ); net_config.add_notification_protocol(transactions_handler_proto.set_config()); + // Create `PeerStore` and initialize it with bootnode peer ids. + let peer_store = PeerStore::new( + net_config + .network_config + .boot_nodes + .iter() + .map(|bootnode| bootnode.peer_id) + .collect(), + ); + let peer_store_handle = peer_store.handle(); + spawn_handle.spawn("peer-store", Some("networking"), peer_store.run()); + let (tx, rx) = sc_utils::mpsc::tracing_unbounded("mpsc_syncing_engine_protocol", 100_000); let (chain_sync_network_provider, chain_sync_network_handle) = NetworkServiceProvider::new(); let (engine, sync_service, block_announce_config) = SyncingEngine::new( @@ -927,6 +940,7 @@ where }) }, network_config: net_config, + peer_store: peer_store_handle, genesis_hash, protocol_id: protocol_id.clone(), fork_id: config.chain_spec.fork_id().map(ToOwned::to_owned), From e46fa49d32d5d46a5d1f0aaae486a23c1f36d5e3 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Wed, 14 Jun 2023 18:14:48 +0300 Subject: [PATCH 35/37] minor: remove unused import --- 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 da70d59ae3c9a..0b76e709489c2 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -36,7 +36,7 @@ use crate::{ network_state::{ NetworkState, NotConnectedPeer as NetworkStateNotConnectedPeer, Peer as NetworkStatePeer, }, - peer_store::{PeerStore, PeerStoreHandle, PeerStoreProvider}, + peer_store::{PeerStoreHandle, PeerStoreProvider}, protocol::{self, NotifsHandlerError, Protocol, Ready}, protocol_controller::{self, ProtoSetConfig, ProtocolController, SetId}, request_responses::{IfDisconnected, RequestFailure}, From bbea1d424210b60b4f5455a684877420529d8942 Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 24 Jul 2023 16:22:07 +0300 Subject: [PATCH 36/37] minor: clarify error message --- client/network/src/protocol/notifications/behaviour.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index 86b251eb48958..6023109055e23 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -2020,7 +2020,7 @@ impl NetworkBehaviour for Notifications { Poll::Ready(None) => { error!( target: "sub-libp2p", - "Protocol controllers receiver stream has returned None", + "Protocol controllers receiver stream has returned `None`. Ignore this error if the node is shutting down.", ); break }, From c516194d40d5ed89e1029909dd1d1fe475f70c5e Mon Sep 17 00:00:00 2001 From: Dmitry Markin Date: Mon, 24 Jul 2023 17:11:45 +0300 Subject: [PATCH 37/37] Convert `syncs_header_only_forks` test into single-threaded --- client/network/test/src/sync.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/client/network/test/src/sync.rs b/client/network/test/src/sync.rs index 7c6e341c30fa5..389177b4aaf1b 100644 --- a/client/network/test/src/sync.rs +++ b/client/network/test/src/sync.rs @@ -550,7 +550,10 @@ async fn can_sync_explicit_forks() { .await; } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +// TODO: for unknown reason, this test is flaky on a multithreaded runtime, so we run it +// in a single-threaded mode. +// See issue https://github.com/paritytech/substrate/issues/14622. +#[tokio::test] async fn syncs_header_only_forks() { sp_tracing::try_init_simple(); let mut net = TestNet::new(0);