From 0185864ba9ecec1c8b9cd9953d90ba52819444e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 16 Sep 2020 15:17:46 +0200 Subject: [PATCH 1/2] 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. --- client/service/src/client/client.rs | 2 +- client/service/test/src/client/mod.rs | 54 +++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index d0859f4ee0392..741b0995feef8 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -802,7 +802,7 @@ impl Client where operation.op.insert_aux(aux)?; - if make_notifications { + 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()); +} From 684dedf4f156217a11f5d3444503007dc49faead Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 16 Sep 2020 16:07:10 +0200 Subject: [PATCH 2/2] Update client/service/src/client/client.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- client/service/src/client/client.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 741b0995feef8..fd76084988dbc 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -802,6 +802,7 @@ impl Client where operation.op.insert_aux(aux)?; + // 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);