From 7c9e34b576ad91aba2575ddaaddca0ad1aecae83 Mon Sep 17 00:00:00 2001
From: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date: Thu, 21 Nov 2024 11:58:44 +0200
Subject: [PATCH] network-gossip: Ensure sync event is processed on unknown
 peer roles  (#6553)

The `GossipEngine::poll_next` implementation polls both the
`notification_service` and the `sync_event_stream`.

If both polls produce valid data to be processed
(`Poll::Ready(Some(..))`), then the sync event is ignored when we
receive `NotificationEvent::NotificationStreamOpened` and the role
cannot be deduced.

This PR ensures both events are processed gracefully. While at it, I
have added a warning to the sync engine related to
`notification_service` producing `Poll::Ready(None)`.

This effectively ensures that `SyncEvents` propagate to the network
potentially fixing any state mismatch.


For more context: https://github.com/paritytech/polkadot-sdk/issues/6507

cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
---
 prdoc/pr_6553.prdoc                           | 13 ++++++++++++
 substrate/client/network-gossip/src/bridge.rs | 20 +++++++++----------
 substrate/client/network/sync/src/engine.rs   |  9 ++++++++-
 3 files changed, 30 insertions(+), 12 deletions(-)
 create mode 100644 prdoc/pr_6553.prdoc

diff --git a/prdoc/pr_6553.prdoc b/prdoc/pr_6553.prdoc
new file mode 100644
index 000000000000..8692eba3a9f5
--- /dev/null
+++ b/prdoc/pr_6553.prdoc
@@ -0,0 +1,13 @@
+title: Ensure sync event is processed on unknown peer roles
+
+doc:
+  - audience: Node Dev
+    description: |
+      The GossipEngine::poll_next implementation polls both the notification_service and the sync_event_stream.
+      This PR ensures both events are processed gracefully.
+
+crates:
+  - name: sc-network-gossip
+    bump: patch
+  - name: sc-network-sync
+    bump: patch
diff --git a/substrate/client/network-gossip/src/bridge.rs b/substrate/client/network-gossip/src/bridge.rs
index a4bd922a76d5..2daf1e49ee4b 100644
--- a/substrate/client/network-gossip/src/bridge.rs
+++ b/substrate/client/network-gossip/src/bridge.rs
@@ -220,18 +220,16 @@ impl<B: BlockT> Future for GossipEngine<B> {
 							},
 							NotificationEvent::NotificationStreamOpened {
 								peer, handshake, ..
-							} => {
-								let Some(role) = this.network.peer_role(peer, handshake) else {
+							} =>
+								if let Some(role) = this.network.peer_role(peer, handshake) {
+									this.state_machine.new_peer(
+										&mut this.notification_service,
+										peer,
+										role,
+									);
+								} else {
 									log::debug!(target: "gossip", "role for {peer} couldn't be determined");
-									continue
-								};
-
-								this.state_machine.new_peer(
-									&mut this.notification_service,
-									peer,
-									role,
-								);
-							},
+								},
 							NotificationEvent::NotificationStreamClosed { peer } => {
 								this.state_machine
 									.peer_disconnected(&mut this.notification_service, peer);
diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs
index cc2089d1974c..349c41ee1f4a 100644
--- a/substrate/client/network/sync/src/engine.rs
+++ b/substrate/client/network/sync/src/engine.rs
@@ -545,7 +545,14 @@ where
 					self.process_service_command(command),
 				notification_event = self.notification_service.next_event() => match notification_event {
 					Some(event) => self.process_notification_event(event),
-					None => return,
+					None => {
+						error!(
+							target: LOG_TARGET,
+							"Terminating `SyncingEngine` because `NotificationService` has terminated.",
+						);
+
+						return;
+					}
 				},
 				response_event = self.pending_responses.select_next_some() =>
 					self.process_response_event(response_event),