Fix block propagation between non-collator nodes (#363)

* Create builder for test nodes

* Fix syncing issue

* Adds missing file

cumulus/client/collator/src/lib.rs

@@ -79,7 +79,7 @@ where
 	fn new(
 		block_status: Arc<BS>,
 		spawner: Arc<dyn SpawnNamed + Send + Sync>,
-		announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+		announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 		backend: Arc<Backend>,
 		parachain_consensus: Box<dyn ParachainConsensus<Block>>,
 	) -> Self {
@@ -337,7 +337,7 @@ pub struct StartCollatorParams<Block: BlockT, Backend, BS, Spawner> {
 	pub para_id: ParaId,
 	pub backend: Arc<Backend>,
 	pub block_status: Arc<BS>,
-	pub announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	pub announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 	pub overseer_handler: OverseerHandler,
 	pub spawner: Spawner,
 	pub key: CollatorPair,

cumulus/client/consensus/common/Cargo.toml

@@ -26,7 +26,7 @@ polkadot-runtime = { git = "https://github.com/paritytech/polkadot", branch = "m
 futures = { version = "0.3.8", features = ["compat"] }
 tokio = "0.1.22"
 codec = { package = "parity-scale-codec", version = "2.0.0", features = [ "derive" ] }
-tracing = "0.1.22"
+tracing = "0.1.25"
 async-trait = "0.1.42"
 dyn-clone = "1.0.4"
 

cumulus/client/consensus/common/src/lib.rs

@@ -143,7 +143,7 @@ pub async fn run_parachain_consensus<P, R, Block, B>(
 	para_id: ParaId,
 	parachain: Arc<P>,
 	relay_chain: R,
-	announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 ) -> ClientResult<()>
 where
 	Block: BlockT,
@@ -175,7 +175,7 @@ async fn follow_new_best<P, R, Block, B>(
 	para_id: ParaId,
 	parachain: Arc<P>,
 	relay_chain: R,
-	announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 ) -> ClientResult<()>
 where
 	Block: BlockT,
@@ -203,7 +203,6 @@ where
 					Some(h) => handle_new_best_parachain_head(
 						h,
 						&*parachain,
-						&*announce_block,
 						&mut unset_best_header,
 					),
 					None => {
@@ -241,12 +240,19 @@ fn handle_new_block_imported<Block, P>(
 	notification: BlockImportNotification<Block>,
 	unset_best_header_opt: &mut Option<Block::Header>,
 	parachain: &P,
-	announce_block: &dyn Fn(Block::Hash, Vec<u8>),
+	announce_block: &dyn Fn(Block::Hash, Option<Vec<u8>>),
 ) where
 	Block: BlockT,
 	P: UsageProvider<Block> + Send + Sync + BlockBackend<Block>,
 	for<'a> &'a P: BlockImport<Block>,
 {
+	// HACK
+	//
+	// Remove after https://github.com/paritytech/substrate/pull/8052 or similar is merged
+	if notification.origin != BlockOrigin::Own {
+		announce_block(notification.hash, None);
+	}
+
 	let unset_best_header = match (notification.is_new_best, &unset_best_header_opt) {
 		// If this is the new best block or we don't have any unset block, we can end it here.
 		(true, _) | (_, None) => return,
@@ -274,12 +280,12 @@ fn handle_new_block_imported<Block, P>(
 				.take()
 				.expect("We checked above that the value is set; qed");
 
-			import_block_as_new_best(unset_hash, unset_best_header, parachain, announce_block);
+			import_block_as_new_best(unset_hash, unset_best_header, parachain);
 		}
 		state => tracing::debug!(
 			target: "cumulus-consensus",
-			unset_best_header = ?unset_best_header,
-			imported_header = ?notification.header,
+			?unset_best_header,
+			?notification.header,
 			?state,
 			"Unexpected state for unset best header.",
 		),
@@ -290,7 +296,6 @@ fn handle_new_block_imported<Block, P>(
 fn handle_new_best_parachain_head<Block, P>(
 	head: Vec<u8>,
 	parachain: &P,
-	announce_block: &dyn Fn(Block::Hash, Vec<u8>),
 	unset_best_header: &mut Option<Block::Header>,
 ) where
 	Block: BlockT,
@@ -323,7 +328,7 @@ fn handle_new_best_parachain_head<Block, P>(
 			Ok(BlockStatus::InChainWithState) => {
 				unset_best_header.take();
 
-				import_block_as_new_best(hash, parachain_head, parachain, announce_block);
+				import_block_as_new_best(hash, parachain_head, parachain);
 			}
 			Ok(BlockStatus::InChainPruned) => {
 				tracing::error!(
@@ -358,7 +363,6 @@ fn import_block_as_new_best<Block, P>(
 	hash: Block::Hash,
 	header: Block::Header,
 	parachain: &P,
-	announce_block: &dyn Fn(Block::Hash, Vec<u8>),
 ) where
 	Block: BlockT,
 	P: UsageProvider<Block> + Send + Sync + BlockBackend<Block>,
@@ -376,8 +380,6 @@ fn import_block_as_new_best<Block, P>(
 			error = ?err,
 			"Failed to set new best block.",
 		);
-	} else {
-		(*announce_block)(hash, Vec::new());
 	}
 }
 

cumulus/client/network/Cargo.toml

@@ -30,11 +30,15 @@ parking_lot = "0.10.2"
 derive_more = "0.99.2"
 
 [dev-dependencies]
+tokio = { version = "0.2.21", features = ["macros"] }
+
 # Cumulus deps
 cumulus-test-service = { path = "../../test/service" }
+cumulus-primitives-core = { path = "../../primitives/core" }
 
 # Polkadot deps
 polkadot-test-client = { git = "https://github.com/paritytech/polkadot", branch = "master" }
+polkadot-test-service = { git = "https://github.com/paritytech/polkadot", branch = "master" }
 
 # substrate deps
 sc-client-api = { git = "https://github.com/paritytech/substrate", branch = "master" }
@@ -42,3 +46,6 @@ sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
 sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" }
 sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
 sp-consensus = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sc-cli = { git = "https://github.com/paritytech/substrate", branch = "master" }
+substrate-test-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sc-service = { git = "https://github.com/paritytech/substrate", branch = "master" }

cumulus/client/network/src/lib.rs

@@ -473,14 +473,14 @@ where
 /// the previous task running.
 pub struct WaitToAnnounce<Block: BlockT> {
 	spawner: Arc<dyn SpawnNamed + Send + Sync>,
-	announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 }
 
 impl<Block: BlockT> WaitToAnnounce<Block> {
 	/// Create the `WaitToAnnounce` object
 	pub fn new(
 		spawner: Arc<dyn SpawnNamed + Send + Sync>,
-		announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+		announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 	) -> WaitToAnnounce<Block> {
 		WaitToAnnounce {
 			spawner,
@@ -522,7 +522,7 @@ impl<Block: BlockT> WaitToAnnounce<Block> {
 async fn wait_to_announce<Block: BlockT>(
 	block_hash: <Block as BlockT>::Hash,
 	pov_hash: PHash,
-	announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 	signed_stmt_recv: oneshot::Receiver<SignedFullStatement>,
 ) {
 	let statement = match signed_stmt_recv.await {
@@ -541,7 +541,7 @@ async fn wait_to_announce<Block: BlockT>(
 	match statement.payload() {
 		Statement::Seconded(c) if &c.descriptor.pov_hash == &pov_hash => {
 			if let Ok(data) = BlockAnnounceData::try_from(statement) {
-				announce_block(block_hash, data.encode());
+				announce_block(block_hash, Some(data.encode()));
 			}
 		}
 		_ => tracing::debug!(

cumulus/client/network/tests/sync.rs

@@ -0,0 +1,86 @@
+// Copyright 2021 Parity Technologies (UK) Ltd.
+// This file is part of Substrate.
+
+// Substrate is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Substrate is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+use cumulus_primitives_core::ParaId;
+use cumulus_test_service::{initial_head_data, Keyring::*};
+use futures::join;
+use sc_service::TaskExecutor;
+
+#[substrate_test_utils::test]
+async fn sync_blocks_from_tip_without_being_connected_to_a_collator(task_executor: TaskExecutor) {
+	let mut builder = sc_cli::LoggerBuilder::new("");
+	builder.with_colors(false);
+	let _ = builder.init();
+
+	let para_id = ParaId::from(100);
+
+	// start alice
+	let alice =
+		polkadot_test_service::run_validator_node(task_executor.clone(), Alice, || {}, vec![]);
+
+	// start bob
+	let bob = polkadot_test_service::run_validator_node(
+		task_executor.clone(),
+		Bob,
+		|| {},
+		vec![alice.addr.clone()],
+	);
+
+	// register parachain
+	alice
+		.register_parachain(
+			para_id,
+			cumulus_test_service::runtime::WASM_BINARY
+				.expect("You need to build the WASM binary to run this test!")
+				.to_vec(),
+			initial_head_data(para_id),
+		)
+		.await
+		.unwrap();
+
+	// run charlie as parachain collator
+	let charlie =
+		cumulus_test_service::TestNodeBuilder::new(para_id, task_executor.clone(), Charlie)
+			.enable_collator()
+			.connect_to_relay_chain_nodes(vec![&alice, &bob])
+			.build()
+			.await;
+
+	// run dave as parachain full node
+	let dave = cumulus_test_service::TestNodeBuilder::new(para_id, task_executor.clone(), Dave)
+		.connect_to_parachain_node(&charlie)
+		.connect_to_relay_chain_nodes(vec![&alice, &bob])
+		.build()
+		.await;
+
+	// run eve as parachain full node that is only connected to dave
+	let eve = cumulus_test_service::TestNodeBuilder::new(para_id, task_executor.clone(), Eve)
+		.connect_to_parachain_node(&dave)
+		.exclusively_connect_to_registered_parachain_nodes()
+		.connect_to_relay_chain_nodes(vec![&alice, &bob])
+		.build()
+		.await;
+
+	eve.wait_for_blocks(7).await;
+
+	join!(
+		alice.task_manager.clean_shutdown(),
+		bob.task_manager.clean_shutdown(),
+		charlie.task_manager.clean_shutdown(),
+		dave.task_manager.clean_shutdown(),
+		eve.task_manager.clean_shutdown(),
+	);
+}

cumulus/client/service/src/lib.rs

@@ -44,7 +44,7 @@ pub struct StartCollatorParams<'a, Block: BlockT, BS, Client, Backend, Spawner,
 	pub backend: Arc<Backend>,
 	pub block_status: Arc<BS>,
 	pub client: Arc<Client>,
-	pub announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	pub announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 	pub spawner: Spawner,
 	pub para_id: ParaId,
 	pub collator_key: CollatorPair,
@@ -121,7 +121,7 @@ pub struct StartFullNodeParams<'a, Block: BlockT, Client, PClient> {
 	pub client: Arc<Client>,
 	pub polkadot_full_node: RFullNode<PClient>,
 	pub task_manager: &'a mut TaskManager,
-	pub announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	pub announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 }
 
 /// Start a full node for a parachain.
@@ -165,7 +165,7 @@ where
 
 struct StartConsensus<'a, Block: BlockT, Client, Backend> {
 	para_id: ParaId,
-	announce_block: Arc<dyn Fn(Block::Hash, Vec<u8>) + Send + Sync>,
+	announce_block: Arc<dyn Fn(Block::Hash, Option<Vec<u8>>) + Send + Sync>,
 	client: Arc<Client>,
 	task_manager: &'a mut TaskManager,
 	_phantom: PhantomData<Backend>,