From f5d3fc1c20286fe16f88d344e2661f5bf08e5cef Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <bkchr@users.noreply.github.com>
Date: Wed, 16 Sep 2020 16:43:43 +0200
Subject: [PATCH] Send import notification always for re-orgs (#7118)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Send import notification always for re-orgs

This pr changes the behavior of sending import notifications. Before we
only send notifications when importing blocks on the tip of the chain or
on similar conditions. However we did not send a notification when we
for example being in a state where we import multiple blocks to catch
up. If we re-org in this process, systems like the transaction pool
would not be notified about this re-org. This means, that we would also
not resubmit transactions of these retracted blocks. This pr fixes this,
by always sending a notification on a re-org.

See
https://github.com/substrate-developer-hub/substrate-node-template/issues/82
for some context about the bug.

* Update client/service/src/client/client.rs

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
---
 client/service/src/client/client.rs   |  3 +-
 client/service/test/src/client/mod.rs | 54 +++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs
index d0859f4ee0392..fd76084988dbc 100644
--- a/client/service/src/client/client.rs
+++ b/client/service/src/client/client.rs
@@ -802,7 +802,8 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 
 		operation.op.insert_aux(aux)?;
 
-		if make_notifications {
+		// we only notify when we are already synced to the tip of the chain or if this import triggers a re-org
+		if make_notifications || tree_route.is_some() {
 			if finalized {
 				operation.notify_finalized.push(hash);
 			}
diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs
index 8d073df272fd9..ea3eaa7ffbabf 100644
--- a/client/service/test/src/client/mod.rs
+++ b/client/service/test/src/client/mod.rs
@@ -1802,3 +1802,57 @@ fn cleans_up_closed_notification_sinks_on_block_import() {
 	assert_eq!(client.finality_notification_sinks().lock().len(), 0);
 }
 
+/// Test that ensures that we always send an import notification for re-orgs.
+#[test]
+fn reorg_triggers_a_notification_even_for_sources_that_should_not_trigger_notifications() {
+	let mut client = TestClientBuilder::new().build();
+
+	let mut notification_stream = futures::executor::block_on_stream(
+		client.import_notification_stream()
+	);
+
+	let a1 = client.new_block_at(
+		&BlockId::Number(0),
+		Default::default(),
+		false,
+	).unwrap().build().unwrap().block;
+	client.import(BlockOrigin::NetworkInitialSync, a1.clone()).unwrap();
+
+	let a2 = client.new_block_at(
+		&BlockId::Hash(a1.hash()),
+		Default::default(),
+		false,
+	).unwrap().build().unwrap().block;
+	client.import(BlockOrigin::NetworkInitialSync, a2.clone()).unwrap();
+
+	let mut b1 = client.new_block_at(
+		&BlockId::Number(0),
+		Default::default(),
+		false,
+	).unwrap();
+	// needed to make sure B1 gets a different hash from A1
+	b1.push_transfer(Transfer {
+		from: AccountKeyring::Alice.into(),
+		to: AccountKeyring::Ferdie.into(),
+		amount: 1,
+		nonce: 0,
+	}).unwrap();
+	let b1 = b1.build().unwrap().block;
+	client.import(BlockOrigin::NetworkInitialSync, b1.clone()).unwrap();
+
+	let b2 = client.new_block_at(
+		&BlockId::Hash(b1.hash()),
+		Default::default(),
+		false,
+	).unwrap().build().unwrap().block;
+
+	// Should trigger a notification because we reorg
+	client.import_as_best(BlockOrigin::NetworkInitialSync, b2.clone()).unwrap();
+
+	// There should be one notification
+	let notification = notification_stream.next().unwrap();
+
+	// We should have a tree route of the re-org
+	let tree_route = notification.tree_route.unwrap();
+	assert_eq!(tree_route.enacted()[0].hash, b1.hash());
+}