Send import notification always for re-orgs (paritytech#7118)

* 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 <[email protected]>

Co-authored-by: André Silva <[email protected]>

Author: 2 people

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);
 			}

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());
+}