Rework the event system of sc-network (#1370)

This commit introduces a new concept called `NotificationService` which allows Polkadot protocols to communicate with the underlying notification protocol implementation directly, without routing events through `NetworkWorker`. This implies that each protocol has its own service which it uses to communicate with remote peers and that each `NotificationService` is unique with respect to the underlying notification protocol, meaning `NotificationService` for the transaction protocol can only be used to send and receive transaction-related notifications. The `NotificationService` concept introduces two additional benefits: * allow protocols to start using custom handshakes * allow protocols to accept/reject inbound peers Previously the validation of inbound connections was solely the responsibility of `ProtocolController`. This caused issues with light peers and `SyncingEngine` as `ProtocolController` would accept more peers than `SyncingEngine` could accept which caused peers to have differing views of their own states. `SyncingEngine` would reject excess peers but these rejections were not properly communicated to those peers causing them to assume that they were accepted. With `NotificationService`, the local handshake is not sent to remote peer if peer is rejected which allows it to detect that it was rejected. This commit also deprecates the use of `NetworkEventStream` for all notification-related events and going forward only DHT events are provided through `NetworkEventStream`. If protocols wish to follow each other's events, they must introduce additional abtractions, as is done for GRANDPA and transactions protocols by following the syncing protocol through `SyncEventStream`. Fixes https://github.com/paritytech/polkadot-sdk/issues/512 Fixes https://github.com/paritytech/polkadot-sdk/issues/514 Fixes https://github.com/paritytech/polkadot-sdk/issues/515 Fixes https://github.com/paritytech/polkadot-sdk/issues/554 Fixes https://github.com/paritytech/polkadot-sdk/issues/556 --- These changes are transferred from https://github.com/paritytech/substrate/pull/14197 but there are no functional changes compared to that PR --------- Co-authored-by: Dmitry Markin <dmitry@markin.tech> Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
2026-04-29 21:57:55 +00:00 · 2023-11-28 20:18:52 +02:00
parent ec3a61ed86
commit e71c484d5b
102 changed files with 5694 additions and 2603 deletions
@@ -32,7 +32,7 @@ use polkadot_node_subsystem::{
 /// To be passed to [`FullNetworkConfiguration::add_notification_protocol`]().
 pub use polkadot_node_network_protocol::peer_set::{peer_sets_info, IsAuthority};
 use polkadot_node_network_protocol::request_response::Requests;
-use sc_network::ReputationChange;
+use sc_network::{MessageSink, ReputationChange};

 use crate::validator_discovery;

@@ -60,6 +60,7 @@ pub struct NetworkBridgeTx<N, AD> {
 	metrics: Metrics,
 	req_protocol_names: ReqProtocolNames,
 	peerset_protocol_names: PeerSetProtocolNames,
+	notification_sinks: Arc<Mutex<HashMap<(PeerSet, PeerId), Box<dyn MessageSink>>>>,
 }

 impl<N, AD> NetworkBridgeTx<N, AD> {
@@ -74,6 +75,7 @@ impl<N, AD> NetworkBridgeTx<N, AD> {
 		metrics: Metrics,
 		req_protocol_names: ReqProtocolNames,
 		peerset_protocol_names: PeerSetProtocolNames,
+		notification_sinks: Arc<Mutex<HashMap<(PeerSet, PeerId), Box<dyn MessageSink>>>>,
 	) -> Self {
 		Self {
 			network_service,
@@ -81,6 +83,7 @@ impl<N, AD> NetworkBridgeTx<N, AD> {
 			metrics,
 			req_protocol_names,
 			peerset_protocol_names,
+			notification_sinks,
 		}
 	}
 }
@@ -107,6 +110,7 @@ async fn handle_subsystem_messages<Context, N, AD>(
 	metrics: Metrics,
 	req_protocol_names: ReqProtocolNames,
 	peerset_protocol_names: PeerSetProtocolNames,
+	notification_sinks: Arc<Mutex<HashMap<(PeerSet, PeerId), Box<dyn MessageSink>>>>,
 ) -> Result<(), Error>
 where
 	N: Network,
@@ -130,6 +134,7 @@ where
 						&metrics,
 						&req_protocol_names,
 						&peerset_protocol_names,
+						&notification_sinks,
 					)
 					.await;
 			},
@@ -140,13 +145,14 @@ where
 #[overseer::contextbounds(NetworkBridgeTx, prefix = self::overseer)]
 async fn handle_incoming_subsystem_communication<Context, N, AD>(
 	_ctx: &mut Context,
-	mut network_service: N,
+	network_service: N,
 	validator_discovery: &mut validator_discovery::Service<N, AD>,
 	mut authority_discovery_service: AD,
 	msg: NetworkBridgeTxMessage,
 	metrics: &Metrics,
 	req_protocol_names: &ReqProtocolNames,
 	peerset_protocol_names: &PeerSetProtocolNames,
+	notification_sinks: &Arc<Mutex<HashMap<(PeerSet, PeerId), Box<dyn MessageSink>>>>,
 ) -> (N, AD)
 where
 	N: Network,
@@ -194,25 +200,22 @@ where

 			match msg {
 				Versioned::V1(msg) => send_validation_message_v1(
-					&mut network_service,
 					peers,
-					peerset_protocol_names,
 					WireMessage::ProtocolMessage(msg),
 					&metrics,
+					notification_sinks,
 				),
 				Versioned::VStaging(msg) => send_validation_message_vstaging(
-					&mut network_service,
 					peers,
-					peerset_protocol_names,
 					WireMessage::ProtocolMessage(msg),
 					&metrics,
+					notification_sinks,
 				),
 				Versioned::V2(msg) => send_validation_message_v2(
-					&mut network_service,
 					peers,
-					peerset_protocol_names,
 					WireMessage::ProtocolMessage(msg),
 					&metrics,
+					notification_sinks,
 				),
 			}
 		},
@@ -227,25 +230,22 @@ where
 			for (peers, msg) in msgs {
 				match msg {
 					Versioned::V1(msg) => send_validation_message_v1(
-						&mut network_service,
 						peers,
-						peerset_protocol_names,
 						WireMessage::ProtocolMessage(msg),
 						&metrics,
+						notification_sinks,
 					),
 					Versioned::VStaging(msg) => send_validation_message_vstaging(
-						&mut network_service,
 						peers,
-						peerset_protocol_names,
 						WireMessage::ProtocolMessage(msg),
 						&metrics,
+						notification_sinks,
 					),
 					Versioned::V2(msg) => send_validation_message_v2(
-						&mut network_service,
 						peers,
-						peerset_protocol_names,
 						WireMessage::ProtocolMessage(msg),
 						&metrics,
+						notification_sinks,
 					),
 				}
 			}
@@ -259,18 +259,16 @@ where

 			match msg {
 				Versioned::V1(msg) => send_collation_message_v1(
-					&mut network_service,
 					peers,
-					peerset_protocol_names,
 					WireMessage::ProtocolMessage(msg),
 					&metrics,
+					notification_sinks,
 				),
 				Versioned::V2(msg) | Versioned::VStaging(msg) => send_collation_message_v2(
-					&mut network_service,
 					peers,
-					peerset_protocol_names,
 					WireMessage::ProtocolMessage(msg),
 					&metrics,
+					notification_sinks,
 				),
 			}
 		},
@@ -284,18 +282,16 @@ where
 			for (peers, msg) in msgs {
 				match msg {
 					Versioned::V1(msg) => send_collation_message_v1(
-						&mut network_service,
 						peers,
-						peerset_protocol_names,
 						WireMessage::ProtocolMessage(msg),
 						&metrics,
+						notification_sinks,
 					),
 					Versioned::V2(msg) | Versioned::VStaging(msg) => send_collation_message_v2(
-						&mut network_service,
 						peers,
-						peerset_protocol_names,
 						WireMessage::ProtocolMessage(msg),
 						&metrics,
+						notification_sinks,
 					),
 				}
 			}
@@ -389,6 +385,7 @@ where
 		metrics,
 		req_protocol_names,
 		peerset_protocol_names,
+		notification_sinks,
 	} = bridge;

 	handle_subsystem_messages(
@@ -398,6 +395,7 @@ where
 		metrics,
 		req_protocol_names,
 		peerset_protocol_names,
+		notification_sinks,
 	)
 	.await?;

@@ -15,15 +15,18 @@
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.

 use super::*;
-use futures::{executor, stream::BoxStream};
+use futures::executor;
 use polkadot_node_subsystem_util::TimeoutExt;

 use async_trait::async_trait;
 use parking_lot::Mutex;
 use std::collections::HashSet;

-use sc_network::{Event as NetworkEvent, IfDisconnected, ProtocolName, ReputationChange};
+use sc_network::{
+	IfDisconnected, ObservedRole as SubstrateObservedRole, ProtocolName, ReputationChange, Roles,
+};

+use parity_scale_codec::DecodeAll;
 use polkadot_node_network_protocol::{
 	peer_set::{PeerSetProtocolNames, ValidationVersion},
 	request_response::{outgoing::Requests, ReqProtocolNames},
@@ -51,10 +54,9 @@ pub enum NetworkAction {
 	WriteNotification(PeerId, PeerSet, Vec<u8>),
 }

-// The subsystem's view of the network - only supports a single call to `event_stream`.
+// The subsystem's view of the network.
 #[derive(Clone)]
 struct TestNetwork {
-	net_events: Arc<Mutex<Option<metered::MeteredReceiver<NetworkEvent>>>>,
 	action_tx: Arc<Mutex<metered::UnboundedMeteredSender<NetworkAction>>>,
 	peerset_protocol_names: Arc<PeerSetProtocolNames>,
 }
@@ -66,37 +68,78 @@ struct TestAuthorityDiscovery;
 // of `NetworkAction`s.
 struct TestNetworkHandle {
 	action_rx: metered::UnboundedMeteredReceiver<NetworkAction>,
-	net_tx: metered::MeteredSender<NetworkEvent>,
-	peerset_protocol_names: PeerSetProtocolNames,
+	_peerset_protocol_names: PeerSetProtocolNames,
+	notification_sinks: Arc<Mutex<HashMap<(PeerSet, PeerId), Box<dyn MessageSink>>>>,
+	action_tx: Arc<Mutex<metered::UnboundedMeteredSender<NetworkAction>>>,
+}
+
+struct TestMessageSink {
+	peer: PeerId,
+	peer_set: PeerSet,
+	action_tx: Arc<Mutex<metered::UnboundedMeteredSender<NetworkAction>>>,
+}
+
+impl TestMessageSink {
+	fn new(
+		peer: PeerId,
+		peer_set: PeerSet,
+		action_tx: Arc<Mutex<metered::UnboundedMeteredSender<NetworkAction>>>,
+	) -> TestMessageSink {
+		Self { peer, peer_set, action_tx }
+	}
+}
+
+#[async_trait::async_trait]
+impl MessageSink for TestMessageSink {
+	fn send_sync_notification(&self, notification: Vec<u8>) {
+		self.action_tx
+			.lock()
+			.unbounded_send(NetworkAction::WriteNotification(
+				self.peer,
+				self.peer_set,
+				notification,
+			))
+			.unwrap();
+	}
+
+	async fn send_async_notification(
+		&self,
+		_notification: Vec<u8>,
+	) -> Result<(), sc_network::error::Error> {
+		unimplemented!();
+	}
 }

 fn new_test_network(
 	peerset_protocol_names: PeerSetProtocolNames,
-) -> (TestNetwork, TestNetworkHandle, TestAuthorityDiscovery) {
-	let (net_tx, net_rx) = metered::channel(10);
+) -> (
+	TestNetwork,
+	TestNetworkHandle,
+	TestAuthorityDiscovery,
+	Arc<Mutex<HashMap<(PeerSet, PeerId), Box<dyn MessageSink>>>>,
+) {
 	let (action_tx, action_rx) = metered::unbounded();
+	let notification_sinks = Arc::new(Mutex::new(HashMap::new()));
+	let action_tx = Arc::new(Mutex::new(action_tx));

 	(
 		TestNetwork {
-			net_events: Arc::new(Mutex::new(Some(net_rx))),
-			action_tx: Arc::new(Mutex::new(action_tx)),
+			action_tx: action_tx.clone(),
 			peerset_protocol_names: Arc::new(peerset_protocol_names.clone()),
 		},
-		TestNetworkHandle { action_rx, net_tx, peerset_protocol_names },
+		TestNetworkHandle {
+			action_rx,
+			_peerset_protocol_names: peerset_protocol_names,
+			action_tx,
+			notification_sinks: notification_sinks.clone(),
+		},
 		TestAuthorityDiscovery,
+		notification_sinks,
 	)
 }

 #[async_trait]
 impl Network for TestNetwork {
-	fn event_stream(&mut self) -> BoxStream<'static, NetworkEvent> {
-		self.net_events
-			.lock()
-			.take()
-			.expect("Subsystem made more than one call to `event_stream`")
-			.boxed()
-	}
-
 	async fn set_reserved_peers(
 		&mut self,
 		_protocol: ProtocolName,
@@ -130,7 +173,8 @@ impl Network for TestNetwork {
 	}

 	fn disconnect_peer(&self, who: PeerId, protocol: ProtocolName) {
-		let (peer_set, _) = self.peerset_protocol_names.try_get_protocol(&protocol).unwrap();
+		let (peer_set, version) = self.peerset_protocol_names.try_get_protocol(&protocol).unwrap();
+		assert_eq!(version, peer_set.get_main_version());

 		self.action_tx
 			.lock()
@@ -138,13 +182,10 @@ impl Network for TestNetwork {
 			.unwrap();
 	}

-	fn write_notification(&self, who: PeerId, protocol: ProtocolName, message: Vec<u8>) {
-		let (peer_set, _) = self.peerset_protocol_names.try_get_protocol(&protocol).unwrap();
-
-		self.action_tx
-			.lock()
-			.unbounded_send(NetworkAction::WriteNotification(who, peer_set, message))
-			.unwrap();
+	fn peer_role(&self, _peer_id: PeerId, handshake: Vec<u8>) -> Option<SubstrateObservedRole> {
+		Roles::decode_all(&mut &handshake[..])
+			.ok()
+			.and_then(|role| Some(SubstrateObservedRole::from(role)))
 	}
 }

@@ -174,23 +215,14 @@ impl TestNetworkHandle {
 	async fn connect_peer(
 		&mut self,
 		peer: PeerId,
-		protocol_version: ValidationVersion,
+		_protocol_version: ValidationVersion,
 		peer_set: PeerSet,
-		role: ObservedRole,
+		_role: ObservedRole,
 	) {
-		let protocol_version = ProtocolVersion::from(protocol_version);
-		self.send_network_event(NetworkEvent::NotificationStreamOpened {
-			remote: peer,
-			protocol: self.peerset_protocol_names.get_name(peer_set, protocol_version),
-			negotiated_fallback: None,
-			role: role.into(),
-			received_handshake: vec![],
-		})
-		.await;
-	}
-
-	async fn send_network_event(&mut self, event: NetworkEvent) {
-		self.net_tx.send(event).await.expect("subsystem concluded early");
+		self.notification_sinks.lock().insert(
+			(peer_set, peer),
+			Box::new(TestMessageSink::new(peer, peer_set, self.action_tx.clone())),
+		);
 	}
 }

@@ -208,7 +240,8 @@ fn test_harness<T: Future<Output = VirtualOverseer>>(test: impl FnOnce(TestHarne
 	let peerset_protocol_names = PeerSetProtocolNames::new(genesis_hash, fork_id);

 	let pool = sp_core::testing::TaskExecutor::new();
-	let (network, network_handle, discovery) = new_test_network(peerset_protocol_names.clone());
+	let (network, network_handle, discovery, network_notification_sinks) =
+		new_test_network(peerset_protocol_names.clone());

 	let (context, virtual_overseer) =
 		polkadot_node_subsystem_test_helpers::make_subsystem_context(pool);
@@ -219,6 +252,7 @@ fn test_harness<T: Future<Output = VirtualOverseer>>(test: impl FnOnce(TestHarne
 		Metrics(None),
 		req_protocol_names,
 		peerset_protocol_names,
+		network_notification_sinks,
 	);

 	let network_bridge_out_fut = run_network_out(bridge_out, context)
@@ -364,9 +398,9 @@ fn network_protocol_versioning_send() {
 				approval_distribution_message.clone(),
 			);

-			// Note that bridge doesn't ensure neither peer's protocol version
-			// or peer set match the message.
-			let receivers = vec![peer_ids[0], peer_ids[3]];
+			// only `peer_ids[0]` opened the validation protocol v2
+			// so only they will be sent a notification
+			let receivers = vec![peer_ids[0]];
 			virtual_overseer
 				.send(FromOrchestra::Communication {
 					msg: NetworkBridgeTxMessage::SendValidationMessage(
@@ -406,7 +440,9 @@ fn network_protocol_versioning_send() {
 			let msg =
 				protocol_v2::CollationProtocol::CollatorProtocol(collator_protocol_message.clone());

-			let receivers = vec![peer_ids[1], peer_ids[2]];
+			// only `peer_ids[0]` opened the collation protocol v2
+			// so only they will be sent a notification
+			let receivers = vec![peer_ids[1]];

 			virtual_overseer
 				.send(FromOrchestra::Communication {