Sync: Propagate block announcement data (#7903)

* Sync: Propagate block announcement data

This pr adds a feature to the sync protocol to propagate the data that
we received alongside a block announcement. This is done by adding a
cache that caches the last X block announcement data where X is set to
the number of `in_peers` (giving every peer the chance to send us a
different block). This will be required by parachains to ensure that
even peers who are not connected to a collator receive the data
alongside the block announcement to properly validate it and request the
block.

* Review comment

* Bring back the code and add new variant to ensure we don't insert block
announce data when something wasn't checked

* Also use out_peers

substrate/Cargo.lock

@@ -3366,9 +3366,9 @@ dependencies = [
 
 [[package]]
 name = "lru"
-version = "0.6.1"
+version = "0.6.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "be716eb6878ca2263eb5d00a781aa13264a794f519fe6af4fbb2668b2d5441c0"
+checksum = "3aae342b73d57ad0b8b364bd12584819f2c1fe9114285dfcf8b0722607671635"
 dependencies = [
  "hashbrown",
 ]
@@ -7153,6 +7153,7 @@ dependencies = [
  "linked-hash-map",
  "linked_hash_set",
  "log",
+ "lru",
  "nohash-hasher",
  "parity-scale-codec",
  "parking_lot 0.11.1",
@@ -9811,7 +9812,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "04f8ab788026715fa63b31960869617cba39117e520eb415b0139543e325ab59"
 dependencies = [
  "cfg-if 0.1.10",
- "rand 0.3.23",
+ "rand 0.7.3",
  "static_assertions",
 ]
 

substrate/client/finality-grandpa/src/communication/mod.rs

@@ -722,7 +722,7 @@ impl<Block: BlockT> Sink<Message<Block>> for OutgoingMessages<Block>
 			);
 
 			// announce the block we voted on to our peers.
-			self.network.lock().announce(target_hash, Vec::new());
+			self.network.lock().announce(target_hash, None);
 
 			// propagate the message to peers
 			let topic = round_topic::<Block>(self.round, self.set_id);

substrate/client/finality-grandpa/src/communication/tests.rs

@@ -68,7 +68,7 @@ impl sc_network_gossip::Network<Block> for TestNetwork {
 		let _ = self.sender.unbounded_send(Event::WriteNotification(who, message));
 	}
 
-	fn announce(&self, block: Hash, _associated_data: Vec<u8>) {
+	fn announce(&self, block: Hash, _associated_data: Option<Vec<u8>>) {
 		let _ = self.sender.unbounded_send(Event::Announce(block));
 	}
 }

substrate/client/network-gossip/src/bridge.rs

@@ -166,7 +166,7 @@ impl<B: BlockT> GossipEngine<B> {
 	///
 	/// Note: this method isn't strictly related to gossiping and should eventually be moved
 	/// somewhere else.
-	pub fn announce(&self, block: B::Hash, associated_data: Vec<u8>) {
+	pub fn announce(&self, block: B::Hash, associated_data: Option<Vec<u8>>) {
 		self.network.announce(block, associated_data);
 	}
 }
@@ -347,7 +347,7 @@ mod tests {
 			unimplemented!();
 		}
 
-		fn announce(&self, _: B::Hash, _: Vec<u8>) {
+		fn announce(&self, _: B::Hash, _: Option<Vec<u8>>) {
 			unimplemented!();
 		}
 	}

substrate/client/network-gossip/src/lib.rs

@@ -98,7 +98,7 @@ pub trait Network<B: BlockT> {
 	///
 	/// Note: this method isn't strictly related to gossiping and should eventually be moved
 	/// somewhere else.
-	fn announce(&self, block: B::Hash, associated_data: Vec<u8>);
+	fn announce(&self, block: B::Hash, associated_data: Option<Vec<u8>>);
 }
 
 impl<B: BlockT, H: ExHashT> Network<B> for Arc<NetworkService<B, H>> {
@@ -136,7 +136,7 @@ impl<B: BlockT, H: ExHashT> Network<B> for Arc<NetworkService<B, H>> {
 		NetworkService::write_notification(self, who, protocol, message)
 	}
 
-	fn announce(&self, block: B::Hash, associated_data: Vec<u8>) {
+	fn announce(&self, block: B::Hash, associated_data: Option<Vec<u8>>) {
 		NetworkService::announce_block(self, block, associated_data)
 	}
 }

substrate/client/network-gossip/src/state_machine.rs

@@ -570,7 +570,7 @@ mod tests {
 			unimplemented!();
 		}
 
-		fn announce(&self, _: B::Hash, _: Vec<u8>) {
+		fn announce(&self, _: B::Hash, _: Option<Vec<u8>>) {
 			unimplemented!();
 		}
 	}

substrate/client/network/Cargo.toml

@@ -35,6 +35,7 @@ hex = "0.4.0"
 ip_network = "0.3.4"
 linked-hash-map = "0.5.2"
 linked_hash_set = "0.1.3"
+lru = "0.6.3"
 log = "0.4.8"
 nohash-hasher = "0.2.0"
 parking_lot = "0.11.1"

substrate/client/network/src/protocol.rs

@@ -230,6 +230,8 @@ pub struct Protocol<B: BlockT, H: ExHashT> {
 	metrics: Option<Metrics>,
 	/// The `PeerId`'s of all boot nodes.
 	boot_node_ids: HashSet<PeerId>,
+	/// A cache for the data that was associated to a block announcement.
+	block_announce_data_cache: lru::LruCache<B::Hash, Vec<u8>>,
 }
 
 /// Peer information
@@ -491,6 +493,11 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 			)
 		};
 
+		let block_announce_data_cache = lru::LruCache::new(
+			network_config.default_peers_set.in_peers as usize
+				+ network_config.default_peers_set.out_peers as usize,
+		);
+
 		let protocol = Protocol {
 			tick_timeout: Box::pin(interval(TICK_TIMEOUT)),
 			propagate_timeout: Box::pin(interval(PROPAGATE_TIMEOUT)),
@@ -514,6 +521,7 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 				None
 			},
 			boot_node_ids,
+			block_announce_data_cache,
 		};
 
 		Ok((protocol, peerset_handle, known_addresses))
@@ -1069,7 +1077,7 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 	///
 	/// In chain-based consensus, we often need to make sure non-best forks are
 	/// at least temporarily synced.
-	pub fn announce_block(&mut self, hash: B::Hash, data: Vec<u8>) {
+	pub fn announce_block(&mut self, hash: B::Hash, data: Option<Vec<u8>>) {
 		let header = match self.chain.header(BlockId::Hash(hash)) {
 			Ok(Some(header)) => header,
 			Ok(None) => {
@@ -1090,6 +1098,8 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 		let is_best = self.chain.info().best_hash == hash;
 		debug!(target: "sync", "Reannouncing block {:?} is_best: {}", hash, is_best);
 
+		let data = data.or_else(|| self.block_announce_data_cache.get(&hash).cloned()).unwrap_or_default();
+
 		for (who, ref mut peer) in self.peers.iter_mut() {
 			let inserted = peer.known_blocks.insert(hash);
 			if inserted {
@@ -1160,9 +1170,17 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 		validation_result: sync::PollBlockAnnounceValidation<B::Header>,
 	) -> CustomMessageOutcome<B> {
 		let (header, is_best, who) = match validation_result {
-			sync::PollBlockAnnounceValidation::Nothing { is_best, who, header } => {
+			sync::PollBlockAnnounceValidation::Skip =>
+				return CustomMessageOutcome::None,
+			sync::PollBlockAnnounceValidation::Nothing { is_best, who, announce } => {
 				self.update_peer_info(&who);
 
+				if let Some(data) = announce.data {
+					if !data.is_empty() {
+						self.block_announce_data_cache.put(announce.header.hash(), data);
+					}
+				}
+
 				// `on_block_announce` returns `OnBlockAnnounce::ImportHeader`
 				// when we have all data required to import the block
 				// in the BlockAnnounce message. This is only when:
@@ -1170,14 +1188,21 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 				// AND
 				// 2) parent block is already imported and not pruned.
 				if is_best {
-					return CustomMessageOutcome::PeerNewBest(who, *header.number())
+					return CustomMessageOutcome::PeerNewBest(who, *announce.header.number())
 				} else {
 					return CustomMessageOutcome::None
 				}
 			}
-			sync::PollBlockAnnounceValidation::ImportHeader { header, is_best, who } => {
+			sync::PollBlockAnnounceValidation::ImportHeader { announce, is_best, who } => {
 				self.update_peer_info(&who);
-				(header, is_best, who)
+
+				if let Some(data) = announce.data {
+					if !data.is_empty() {
+						self.block_announce_data_cache.put(announce.header.hash(), data);
+					}
+				}
+
+				(announce.header, is_best, who)
 			}
 			sync::PollBlockAnnounceValidation::Failure { who, disconnect } => {
 				if disconnect {

substrate/client/network/src/protocol/sync.rs

@@ -362,8 +362,8 @@ pub enum PollBlockAnnounceValidation<H> {
 		who: PeerId,
 		/// Was this their new best block?
 		is_best: bool,
-		/// The header of the announcement.
-		header: H,
+		/// The announcement.
+		announce: BlockAnnounce<H>,
 	},
 	/// The announcement header should be imported.
 	ImportHeader {
@@ -371,9 +371,11 @@ pub enum PollBlockAnnounceValidation<H> {
 		who: PeerId,
 		/// Was this their new best block?
 		is_best: bool,
-		/// The header of the announcement.
-		header: H,
+		/// The announcement.
+		announce: BlockAnnounce<H>,
 	},
+	/// The block announcement should be skipped.
+	Skip,
 }
 
 /// Result of [`ChainSync::block_announce_validation`].
@@ -388,15 +390,6 @@ enum PreValidateBlockAnnounce<H> {
 		/// Should the peer be disconnected?
 		disconnect: bool,
 	},
-	/// The announcement does not require further handling.
-	Nothing {
-		/// Who sent the processed block announcement?
-		who: PeerId,
-		/// Was this their new best block?
-		is_best: bool,
-		/// The announcement.
-		announce: BlockAnnounce<H>,
-	},
 	/// The pre-validation was sucessful and the announcement should be
 	/// further processed.
 	Process {
@@ -407,6 +400,8 @@ enum PreValidateBlockAnnounce<H> {
 		/// The announcement.
 		announce: BlockAnnounce<H>,
 	},
+	/// The block announcement should be skipped.
+	Skip,
 }
 
 /// Result of [`ChainSync::on_block_justification`].
@@ -1278,7 +1273,7 @@ impl<B: BlockT> ChainSync<B> {
 					who,
 					hash,
 				);
-				PreValidateBlockAnnounce::Nothing { is_best, who, announce }
+				PreValidateBlockAnnounce::Skip
 			}.boxed());
 			return
 		}
@@ -1295,7 +1290,7 @@ impl<B: BlockT> ChainSync<B> {
 						hash,
 						who,
 					);
-					PreValidateBlockAnnounce::Nothing { is_best, who, announce }
+					PreValidateBlockAnnounce::Skip
 				}.boxed());
 				return
 			}
@@ -1308,7 +1303,7 @@ impl<B: BlockT> ChainSync<B> {
 						hash,
 						who,
 					);
-					PreValidateBlockAnnounce::Nothing { is_best, who, announce }
+					PreValidateBlockAnnounce::Skip
 				}.boxed());
 				return
 			}
@@ -1337,7 +1332,7 @@ impl<B: BlockT> ChainSync<B> {
 				}
 				Err(e) => {
 					error!(target: "sync", "💔 Block announcement validation errored: {}", e);
-					PreValidateBlockAnnounce::Nothing { is_best, who, announce }
+					PreValidateBlockAnnounce::Skip
 				}
 			}
 		}.boxed());
@@ -1393,10 +1388,6 @@ impl<B: BlockT> ChainSync<B> {
 		);
 
 		let (announce, is_best, who) = match pre_validation_result {
-			PreValidateBlockAnnounce::Nothing { is_best, who, announce } => {
-				self.peer_block_announce_validation_finished(&who);
-				return PollBlockAnnounceValidation::Nothing { is_best, who, header: announce.header }
-			},
 			PreValidateBlockAnnounce::Failure { who, disconnect } => {
 				self.peer_block_announce_validation_finished(&who);
 				return PollBlockAnnounceValidation::Failure { who, disconnect }
@@ -1405,12 +1396,12 @@ impl<B: BlockT> ChainSync<B> {
 				self.peer_block_announce_validation_finished(&who);
 				(announce, is_new_best, who)
 			},
+			PreValidateBlockAnnounce::Skip => return PollBlockAnnounceValidation::Skip,
 		};
 
-		let header = announce.header;
-		let number = *header.number();
-		let hash = header.hash();
-		let parent_status = self.block_status(header.parent_hash()).unwrap_or(BlockStatus::Unknown);
+		let number = *announce.header.number();
+		let hash = announce.header.hash();
+		let parent_status = self.block_status(announce.header.parent_hash()).unwrap_or(BlockStatus::Unknown);
 		let known_parent = parent_status != BlockStatus::Unknown;
 		let ancient_parent = parent_status == BlockStatus::InChainPruned;
 
@@ -1419,7 +1410,7 @@ impl<B: BlockT> ChainSync<B> {
 			peer
 		} else {
 			error!(target: "sync", "💔 Called on_block_announce with a bad peer ID");
-			return PollBlockAnnounceValidation::Nothing { is_best, who, header }
+			return PollBlockAnnounceValidation::Nothing { is_best, who, announce }
 		};
 
 		if is_best {
@@ -1430,7 +1421,7 @@ impl<B: BlockT> ChainSync<B> {
 
 		if let PeerSyncState::AncestorSearch {..} = peer.state {
 			trace!(target: "sync", "Peer state is ancestor search.");
-			return PollBlockAnnounceValidation::Nothing { is_best, who, header }
+			return PollBlockAnnounceValidation::Nothing { is_best, who, announce }
 		}
 
 		// If the announced block is the best they have and is not ahead of us, our common number
@@ -1438,7 +1429,7 @@ impl<B: BlockT> ChainSync<B> {
 		if is_best {
 			if known && self.best_queued_number >= number {
 				peer.update_common_number(number);
-			} else if header.parent_hash() == &self.best_queued_hash
+			} else if announce.header.parent_hash() == &self.best_queued_hash
 				|| known_parent && self.best_queued_number >= number
 			{
 				peer.update_common_number(number - One::one());
@@ -1452,37 +1443,52 @@ impl<B: BlockT> ChainSync<B> {
 			if let Some(target) = self.fork_targets.get_mut(&hash) {
 				target.peers.insert(who.clone());
 			}
-			return PollBlockAnnounceValidation::Nothing { is_best, who, header }
+			return PollBlockAnnounceValidation::Nothing { is_best, who, announce }
 		}
 
 		if ancient_parent {
-			trace!(target: "sync", "Ignored ancient block announced from {}: {} {:?}", who, hash, header);
-			return PollBlockAnnounceValidation::Nothing { is_best, who, header }
+			trace!(
+				target: "sync",
+				"Ignored ancient block announced from {}: {} {:?}",
+				who,
+				hash,
+				announce.header,
+			);
+			return PollBlockAnnounceValidation::Nothing { is_best, who, announce }
 		}
 
 		let requires_additional_data = !self.role.is_light() || !known_parent;
 		if !requires_additional_data {
-			trace!(target: "sync", "Importing new header announced from {}: {} {:?}", who, hash, header);
-			return PollBlockAnnounceValidation::ImportHeader { is_best, header, who }
+			trace!(
+				target: "sync",
+				"Importing new header announced from {}: {} {:?}",
+				who,
+				hash,
+				announce.header,
+			);
+			return PollBlockAnnounceValidation::ImportHeader { is_best, announce, who }
 		}
 
 		if number <= self.best_queued_number {
 			trace!(
 				target: "sync",
-				"Added sync target for block announced from {}: {} {:?}", who, hash, header
+				"Added sync target for block announced from {}: {} {:?}",
+				who,
+				hash,
+				announce.header,
 			);
 			self.fork_targets
 				.entry(hash.clone())
 				.or_insert_with(|| ForkTarget {
 					number,
-					parent_hash: Some(*header.parent_hash()),
+					parent_hash: Some(*announce.header.parent_hash()),
 					peers: Default::default(),
 				})
 				.peers.insert(who.clone());
 		}
 
 		trace!(target: "sync", "Announce validation result is nothing");
-		PollBlockAnnounceValidation::Nothing { is_best, who, header }
+		PollBlockAnnounceValidation::Nothing { is_best, who, announce }
 	}
 
 	/// Call when a peer has disconnected.

substrate/client/network/src/service.rs

@@ -858,7 +858,7 @@ impl<B: BlockT + 'static, H: ExHashT> NetworkService<B, H> {
 	///
 	/// In chain-based consensus, we often need to make sure non-best forks are
 	/// at least temporarily synced. This function forces such an announcement.
-	pub fn announce_block(&self, hash: B::Hash, data: Vec<u8>) {
+	pub fn announce_block(&self, hash: B::Hash, data: Option<Vec<u8>>) {
 		let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::AnnounceBlock(hash, data));
 	}
 
@@ -1236,7 +1236,7 @@ enum ServiceToWorkerMsg<B: BlockT, H: ExHashT> {
 	PropagateTransaction(H),
 	PropagateTransactions,
 	RequestJustification(B::Hash, NumberFor<B>),
-	AnnounceBlock(B::Hash, Vec<u8>),
+	AnnounceBlock(B::Hash, Option<Vec<u8>>),
 	GetValue(record::Key),
 	PutValue(record::Key, Vec<u8>),
 	AddKnownAddress(PeerId, Multiaddr),

substrate/client/network/test/src/lib.rs

@@ -51,7 +51,10 @@ use sp_consensus::Error as ConsensusError;
 use sp_consensus::{BlockOrigin, ForkChoiceStrategy, BlockImportParams, BlockCheckParams, JustificationImport};
 use futures::prelude::*;
 use futures::future::BoxFuture;
-use sc_network::{NetworkWorker, NetworkService, config::ProtocolId};
+use sc_network::{
+	NetworkWorker, NetworkService, config::{ProtocolId, MultiaddrWithPeerId, NonReservedPeerMode},
+	Multiaddr,
+};
 use sc_network::config::{NetworkConfiguration, NonDefaultSetConfig, TransportConfig};
 use libp2p::PeerId;
 use parking_lot::Mutex;
@@ -228,6 +231,7 @@ pub struct Peer<D> {
 	network: NetworkWorker<Block, <Block as BlockT>::Hash>,
 	imported_blocks_stream: Pin<Box<dyn Stream<Item = BlockImportNotification<Block>> + Send>>,
 	finality_notification_stream: Pin<Box<dyn Stream<Item = FinalityNotification<Block>> + Send>>,
+	listen_addr: Multiaddr,
 }
 
 impl<D> Peer<D> {
@@ -267,7 +271,7 @@ impl<D> Peer<D> {
 	}
 
 	/// Announces an important block on the network.
-	pub fn announce_block(&self, hash: <Block as BlockT>::Hash, data: Vec<u8>) {
+	pub fn announce_block(&self, hash: <Block as BlockT>::Hash, data: Option<Vec<u8>>) {
 		self.network.service().announce_block(hash, data);
 	}
 
@@ -281,7 +285,7 @@ impl<D> Peer<D> {
 		where F: FnMut(BlockBuilder<Block, PeersFullClient, substrate_test_runtime_client::Backend>) -> Block
 	{
 		let best_hash = self.client.info().best_hash;
-		self.generate_blocks_at(BlockId::Hash(best_hash), count, origin, edit_block, false, true)
+		self.generate_blocks_at(BlockId::Hash(best_hash), count, origin, edit_block, false, true, true)
 	}
 
 	/// Add blocks to the peer -- edit the block before adding. The chain will
@@ -294,6 +298,7 @@ impl<D> Peer<D> {
 		mut edit_block: F,
 		headers_only: bool,
 		inform_sync_about_new_best_block: bool,
+		announce_block: bool,
 	) -> H256 where F: FnMut(BlockBuilder<Block, PeersFullClient, substrate_test_runtime_client::Backend>) -> Block {
 		let full_client = self.client.as_full()
 			.expect("blocks could only be generated by full clients");
@@ -327,7 +332,9 @@ impl<D> Peer<D> {
 			};
 
 			self.block_import.import_block(import_block, cache).expect("block_import failed");
-			self.network.service().announce_block(hash, Vec::new());
+			if announce_block {
+				self.network.service().announce_block(hash, None);
+			}
 			at = hash;
 		}
 
@@ -337,7 +344,6 @@ impl<D> Peer<D> {
 				full_client.header(&BlockId::Hash(at)).ok().flatten().unwrap().number().clone(),
 			);
 		}
-		self.network.service().announce_block(at.clone(), Vec::new());
 		at
 	}
 
@@ -350,13 +356,13 @@ impl<D> Peer<D> {
 	/// Push blocks to the peer (simplified: with or without a TX)
 	pub fn push_headers(&mut self, count: usize) -> H256 {
 		let best_hash = self.client.info().best_hash;
-		self.generate_tx_blocks_at(BlockId::Hash(best_hash), count, false, true, true)
+		self.generate_tx_blocks_at(BlockId::Hash(best_hash), count, false, true, true, true)
 	}
 
 	/// Push blocks to the peer (simplified: with or without a TX) starting from
 	/// given hash.
 	pub fn push_blocks_at(&mut self, at: BlockId<Block>, count: usize, with_tx: bool) -> H256 {
-		self.generate_tx_blocks_at(at, count, with_tx, false, true)
+		self.generate_tx_blocks_at(at, count, with_tx, false, true, true)
 	}
 
 	/// Push blocks to the peer (simplified: with or without a TX) starting from
@@ -367,7 +373,18 @@ impl<D> Peer<D> {
 		count: usize,
 		with_tx: bool,
 	) -> H256 {
-		self.generate_tx_blocks_at(at, count, with_tx, false, false)
+		self.generate_tx_blocks_at(at, count, with_tx, false, false, true)
+	}
+
+	/// Push blocks to the peer (simplified: with or without a TX) starting from
+	/// given hash without announcing the block.
+	pub fn push_blocks_at_without_announcing(
+		&mut self,
+		at: BlockId<Block>,
+		count: usize,
+		with_tx: bool,
+	) -> H256 {
+		self.generate_tx_blocks_at(at, count, with_tx, false, true, false)
 	}
 
 	/// Push blocks/headers to the peer (simplified: with or without a TX) starting from
@@ -379,6 +396,7 @@ impl<D> Peer<D> {
 		with_tx: bool,
 		headers_only: bool,
 		inform_sync_about_new_best_block: bool,
+		announce_block: bool,
 	) -> H256 {
 		let mut nonce = 0;
 		if with_tx {
@@ -398,6 +416,7 @@ impl<D> Peer<D> {
 				},
 				headers_only,
 				inform_sync_about_new_best_block,
+				announce_block,
 			)
 		} else {
 			self.generate_blocks_at(
@@ -407,6 +426,7 @@ impl<D> Peer<D> {
 				|builder| builder.build().unwrap().block,
 				headers_only,
 				inform_sync_about_new_best_block,
+				announce_block,
 			)
 		}
 	}
@@ -585,6 +605,10 @@ pub struct FullPeerConfig {
 	pub block_announce_validator: Option<Box<dyn BlockAnnounceValidator<Block> + Send + Sync>>,
 	/// List of notification protocols that the network must support.
 	pub notifications_protocols: Vec<Cow<'static, str>>,
+	/// The indices of the peers the peer should be connected to.
+	///
+	/// If `None`, it will be connected to all other peers.
+	pub connect_to_peers: Option<Vec<usize>>,
 }
 
 pub trait TestNetFactory: Sized {
@@ -689,6 +713,15 @@ pub trait TestNetFactory: Sized {
 				set_config: Default::default()
 			}
 		}).collect();
+		if let Some(connect_to) = config.connect_to_peers {
+			let addrs = connect_to.iter().map(|v| {
+				let peer_id = self.peer(*v).network_service().local_peer_id().clone();
+				let multiaddr = self.peer(*v).listen_addr.clone();
+				MultiaddrWithPeerId { peer_id, multiaddr }
+			}).collect();
+			network_config.default_peers_set.reserved_nodes = addrs;
+			network_config.default_peers_set.non_reserved_mode = NonReservedPeerMode::Deny;
+		}
 
 		let protocol_id = ProtocolId::from("test-protocol-name");
 
@@ -715,9 +748,12 @@ pub trait TestNetFactory: Sized {
 
 		trace!(target: "test_network", "Peer identifier: {}", network.service().local_peer_id());
 
-		self.mut_peers(|peers| {
+		self.mut_peers(move |peers| {
 			for peer in peers.iter_mut() {
-				peer.network.add_known_address(network.service().local_peer_id().clone(), listen_addr.clone());
+				peer.network.add_known_address(
+					network.service().local_peer_id().clone(),
+					listen_addr.clone(),
+				);
 			}
 
 			let imported_blocks_stream = Box::pin(client.import_notification_stream().fuse());
@@ -733,6 +769,7 @@ pub trait TestNetFactory: Sized {
 				block_import,
 				verifier,
 				network,
+				listen_addr,
 			});
 		});
 	}
@@ -813,6 +850,7 @@ pub trait TestNetFactory: Sized {
 				imported_blocks_stream,
 				finality_notification_stream,
 				network,
+				listen_addr,
 			});
 		});
 	}
@@ -912,7 +950,7 @@ pub trait TestNetFactory: Sized {
 
 				// We poll `imported_blocks_stream`.
 				while let Poll::Ready(Some(notification)) = peer.imported_blocks_stream.as_mut().poll_next(cx) {
-					peer.network.service().announce_block(notification.hash, Vec::new());
+					peer.network.service().announce_block(notification.hash, None);
 				}
 
 				// We poll `finality_notification_stream`, but we only take the last event.

substrate/client/network/test/src/sync.rs

@@ -436,7 +436,7 @@ fn can_sync_small_non_best_forks() {
 	assert!(net.peer(0).client().header(&BlockId::Hash(small_hash)).unwrap().is_some());
 	assert!(!net.peer(1).client().header(&BlockId::Hash(small_hash)).unwrap().is_some());
 
-	net.peer(0).announce_block(small_hash, Vec::new());
+	net.peer(0).announce_block(small_hash, None);
 
 	// after announcing, peer 1 downloads the block.
 
@@ -452,7 +452,7 @@ fn can_sync_small_non_best_forks() {
 	net.block_until_sync();
 
 	let another_fork = net.peer(0).push_blocks_at(BlockId::Number(35), 2, true);
-	net.peer(0).announce_block(another_fork, Vec::new());
+	net.peer(0).announce_block(another_fork, None);
 	block_on(futures::future::poll_fn::<(), _>(|cx| {
 		net.poll(cx);
 		if net.peer(1).client().header(&BlockId::Hash(another_fork)).unwrap().is_none() {
@@ -500,7 +500,7 @@ fn light_peer_imports_header_from_announce() {
 	sp_tracing::try_init_simple();
 
 	fn import_with_announce(net: &mut TestNet, hash: H256) {
-		net.peer(0).announce_block(hash, Vec::new());
+		net.peer(0).announce_block(hash, None);
 
 		block_on(futures::future::poll_fn::<(), _>(|cx| {
 			net.poll(cx);
@@ -610,7 +610,7 @@ fn does_not_sync_announced_old_best_block() {
 	net.peer(0).push_blocks(18, true);
 	net.peer(1).push_blocks(20, true);
 
-	net.peer(0).announce_block(old_hash, Vec::new());
+	net.peer(0).announce_block(old_hash, None);
 	block_on(futures::future::poll_fn::<(), _>(|cx| {
 		// poll once to import announcement
 		net.poll(cx);
@@ -618,7 +618,7 @@ fn does_not_sync_announced_old_best_block() {
 	}));
 	assert!(!net.peer(1).is_major_syncing());
 
-	net.peer(0).announce_block(old_hash_with_parent, Vec::new());
+	net.peer(0).announce_block(old_hash_with_parent, None);
 	block_on(futures::future::poll_fn::<(), _>(|cx| {
 		// poll once to import announcement
 		net.poll(cx);
@@ -653,8 +653,8 @@ fn imports_stale_once() {
 
 	fn import_with_announce(net: &mut TestNet, hash: H256) {
 		// Announce twice
-		net.peer(0).announce_block(hash, Vec::new());
-		net.peer(0).announce_block(hash, Vec::new());
+		net.peer(0).announce_block(hash, None);
+		net.peer(0).announce_block(hash, None);
 
 		block_on(futures::future::poll_fn::<(), _>(|cx| {
 			net.poll(cx);
@@ -842,3 +842,58 @@ fn sync_to_tip_when_we_sync_together_with_multiple_peers() {
 		net.block_until_idle();
 	}
 }
+
+/// Ensures that when we receive a block announcement with some data attached, that we propagate
+/// this data when reannouncing the block.
+#[test]
+fn block_announce_data_is_propagated() {
+	struct TestBlockAnnounceValidator;
+
+	impl BlockAnnounceValidator<Block> for TestBlockAnnounceValidator {
+		fn validate(
+			&mut self,
+			_: &Header,
+			data: &[u8],
+		) -> Pin<Box<dyn Future<Output = Result<Validation, Box<dyn std::error::Error + Send>>> + Send>> {
+			let correct = data.get(0) == Some(&137);
+			async move {
+				if correct {
+					Ok(Validation::Success { is_new_best: true })
+				} else {
+					Ok(Validation::Failure { disconnect: false })
+				}
+			}.boxed()
+		}
+	}
+
+	sp_tracing::try_init_simple();
+	let mut net = TestNet::new(1);
+
+	net.add_full_peer_with_config(FullPeerConfig {
+		block_announce_validator: Some(Box::new(TestBlockAnnounceValidator)),
+		..Default::default()
+	});
+
+	net.add_full_peer_with_config(FullPeerConfig {
+		block_announce_validator: Some(Box::new(TestBlockAnnounceValidator)),
+		connect_to_peers: Some(vec![1]),
+		..Default::default()
+	});
+
+	// Wait until peer 1 is connected to both nodes.
+	block_on(futures::future::poll_fn::<(), _>(|cx| {
+		net.poll(cx);
+		if net.peer(1).num_peers() == 2 {
+			Poll::Ready(())
+		} else {
+			Poll::Pending
+		}
+	}));
+
+	let block_hash = net.peer(0).push_blocks_at_without_announcing(BlockId::Number(0), 1, true);
+	net.peer(0).announce_block(block_hash, Some(vec![137]));
+
+	while !net.peer(1).has_block(&block_hash) || !net.peer(2).has_block(&block_hash) {
+		net.block_until_idle();
+	}
+}

substrate/client/service/src/lib.rs

@@ -248,7 +248,7 @@ async fn build_network_future<
 				};
 
 				if announce_imported_blocks {
-					network.service().announce_block(notification.hash, Vec::new());
+					network.service().announce_block(notification.hash, None);
 				}
 
 				if notification.is_new_best {