Modular block request handler (#1524)

Submit the outstanding PRs from the old repos(these were already
reviewed and approved before the repo rorg, but not yet submitted):
Main PR: https://github.com/paritytech/substrate/pull/14014
Companion PRs: https://github.com/paritytech/polkadot/pull/7134,
https://github.com/paritytech/cumulus/pull/2489

The changes in the PR:
1. ChainSync currently calls into the block request handler directly.
Instead, move the block request handler behind a trait. This allows new
protocols to be plugged into ChainSync.
2. BuildNetworkParams is changed so that custom relay protocol
implementations can be (optionally) passed in during network creation
time. If custom protocol is not specified, it defaults to the existing
block handler
3. BlockServer and BlockDownloader traits are introduced for the
protocol implementation. The existing block handler has been changed to
implement these traits
4. Other changes:
[X] Make TxHash serializable. This is needed for exchanging the
serialized hash in the relay protocol messages
[X] Clean up types no longer used(OpaqueBlockRequest,
OpaqueBlockResponse)

---------

Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Co-authored-by: command-bot <>

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

@@ -29,13 +29,14 @@
 //! order to update it.
 
 use crate::{
+	block_relay_protocol::{BlockDownloader, BlockResponseError},
 	blocks::BlockCollection,
 	schema::v1::{StateRequest, StateResponse},
 	state::StateSync,
 	warp::{WarpProofImportResult, WarpSync, WarpSyncConfig},
 };
 
-use codec::{Decode, DecodeAll, Encode};
+use codec::Encode;
 use extra_requests::ExtraRequests;
 use futures::{channel::oneshot, task::Poll, Future, FutureExt};
 use libp2p::{request_response::OutboundFailure, PeerId};
@@ -63,8 +64,8 @@ use sc_network_common::{
 		},
 		warp::{EncodedProof, WarpProofRequest, WarpSyncPhase, WarpSyncProgress},
 		BadPeer, ChainSync as ChainSyncT, ImportResult, Metrics, OnBlockData, OnBlockJustification,
-		OnStateData, OpaqueBlockRequest, OpaqueBlockResponse, OpaqueStateRequest,
-		OpaqueStateResponse, PeerInfo, PeerRequest, SyncMode, SyncState, SyncStatus,
+		OnStateData, OpaqueStateRequest, OpaqueStateResponse, PeerInfo, PeerRequest, SyncMode,
+		SyncState, SyncStatus,
 	},
 };
 use sp_arithmetic::traits::Saturating;
@@ -93,6 +94,7 @@ mod extra_requests;
 mod futures_stream;
 mod schema;
 
+pub mod block_relay_protocol;
 pub mod block_request_handler;
 pub mod blocks;
 pub mod engine;
@@ -320,8 +322,8 @@ pub struct ChainSync<B: BlockT, Client> {
 	network_service: service::network::NetworkServiceHandle,
 	/// Protocol name used for block announcements
 	block_announce_protocol_name: ProtocolName,
-	/// Protocol name used to send out block requests
-	block_request_protocol_name: ProtocolName,
+	/// Block downloader stub
+	block_downloader: Arc<dyn BlockDownloader<B>>,
 	/// Protocol name used to send out state requests
 	state_request_protocol_name: ProtocolName,
 	/// Protocol name used to send out warp sync requests
@@ -1167,72 +1169,6 @@ where
 		}
 	}
 
-	fn block_response_into_blocks(
-		&self,
-		request: &BlockRequest<B>,
-		response: OpaqueBlockResponse,
-	) -> Result<Vec<BlockData<B>>, String> {
-		let response: Box<schema::v1::BlockResponse> = response.0.downcast().map_err(|_error| {
-			"Failed to downcast opaque block response during encoding, this is an \
-				implementation bug."
-				.to_string()
-		})?;
-
-		response
-			.blocks
-			.into_iter()
-			.map(|block_data| {
-				Ok(BlockData::<B> {
-					hash: Decode::decode(&mut block_data.hash.as_ref())?,
-					header: if !block_data.header.is_empty() {
-						Some(Decode::decode(&mut block_data.header.as_ref())?)
-					} else {
-						None
-					},
-					body: if request.fields.contains(BlockAttributes::BODY) {
-						Some(
-							block_data
-								.body
-								.iter()
-								.map(|body| Decode::decode(&mut body.as_ref()))
-								.collect::<Result<Vec<_>, _>>()?,
-						)
-					} else {
-						None
-					},
-					indexed_body: if request.fields.contains(BlockAttributes::INDEXED_BODY) {
-						Some(block_data.indexed_body)
-					} else {
-						None
-					},
-					receipt: if !block_data.receipt.is_empty() {
-						Some(block_data.receipt)
-					} else {
-						None
-					},
-					message_queue: if !block_data.message_queue.is_empty() {
-						Some(block_data.message_queue)
-					} else {
-						None
-					},
-					justification: if !block_data.justification.is_empty() {
-						Some(block_data.justification)
-					} else if block_data.is_empty_justification {
-						Some(Vec::new())
-					} else {
-						None
-					},
-					justifications: if !block_data.justifications.is_empty() {
-						Some(DecodeAll::decode_all(&mut block_data.justifications.as_ref())?)
-					} else {
-						None
-					},
-				})
-			})
-			.collect::<Result<_, _>>()
-			.map_err(|error: codec::Error| error.to_string())
-	}
-
 	fn poll(&mut self, cx: &mut std::task::Context) -> Poll<()> {
 		self.process_outbound_requests();
 
@@ -1248,30 +1184,18 @@ where
 	}
 
 	fn send_block_request(&mut self, who: PeerId, request: BlockRequest<B>) {
-		let (tx, rx) = oneshot::channel();
-		let opaque_req = self.create_opaque_block_request(&request);
-
 		if self.peers.contains_key(&who) {
-			self.pending_responses
-				.insert(who, Box::pin(async move { (who, PeerRequest::Block(request), rx.await) }));
-		}
-
-		match self.encode_block_request(&opaque_req) {
-			Ok(data) => {
-				self.network_service.start_request(
-					who,
-					self.block_request_protocol_name.clone(),
-					data,
-					tx,
-					IfDisconnected::ImmediateError,
-				);
-			},
-			Err(err) => {
-				log::warn!(
-					target: LOG_TARGET,
-					"Failed to encode block request {opaque_req:?}: {err:?}",
-				);
-			},
+			let downloader = self.block_downloader.clone();
+			self.pending_responses.insert(
+				who,
+				Box::pin(async move {
+					(
+						who,
+						PeerRequest::Block(request.clone()),
+						downloader.download_blocks(who, request).await,
+					)
+				}),
+			);
 		}
 	}
 }
@@ -1301,7 +1225,7 @@ where
 		metrics_registry: Option<&Registry>,
 		network_service: service::network::NetworkServiceHandle,
 		import_queue: Box<dyn ImportQueueService<B>>,
-		block_request_protocol_name: ProtocolName,
+		block_downloader: Arc<dyn BlockDownloader<B>>,
 		state_request_protocol_name: ProtocolName,
 		warp_sync_protocol_name: Option<ProtocolName>,
 	) -> Result<(Self, NonDefaultSetConfig), ClientError> {
@@ -1337,7 +1261,7 @@ where
 			import_existing: false,
 			gap_sync: None,
 			network_service,
-			block_request_protocol_name,
+			block_downloader,
 			state_request_protocol_name,
 			warp_sync_config,
 			warp_sync_target_block_header: None,
@@ -1710,13 +1634,6 @@ where
 		}
 	}
 
-	fn decode_block_response(response: &[u8]) -> Result<OpaqueBlockResponse, String> {
-		let response = schema::v1::BlockResponse::decode(response)
-			.map_err(|error| format!("Failed to decode block response: {error}"))?;
-
-		Ok(OpaqueBlockResponse(Box::new(response)))
-	}
-
 	fn decode_state_response(response: &[u8]) -> Result<OpaqueStateResponse, String> {
 		let response = StateResponse::decode(response)
 			.map_err(|error| format!("Failed to decode state response: {error}"))?;
@@ -1780,22 +1697,8 @@ where
 		&mut self,
 		peer_id: PeerId,
 		request: BlockRequest<B>,
-		response: OpaqueBlockResponse,
+		blocks: Vec<BlockData<B>>,
 	) -> Option<ImportResult<B>> {
-		let blocks = match self.block_response_into_blocks(&request, response) {
-			Ok(blocks) => blocks,
-			Err(err) => {
-				debug!(
-					target: LOG_TARGET,
-					"Failed to decode block response from {}: {}",
-					peer_id,
-					err,
-				);
-				self.network_service.report_peer(peer_id, rep::BAD_MESSAGE);
-				return None
-			},
-		};
-
 		let block_response = BlockResponse::<B> { id: request.id, blocks };
 
 		let blocks_range = || match (
@@ -1915,22 +1818,34 @@ where
 			match response {
 				Ok(Ok(resp)) => match request {
 					PeerRequest::Block(req) => {
-						let response = match Self::decode_block_response(&resp[..]) {
-							Ok(proto) => proto,
-							Err(e) => {
+						match self.block_downloader.block_response_into_blocks(&req, resp) {
+							Ok(blocks) => {
+								if let Some(import) = self.on_block_response(id, req, blocks) {
+									return Poll::Ready(import)
+								}
+							},
+							Err(BlockResponseError::DecodeFailed(e)) => {
 								debug!(
 									target: LOG_TARGET,
-									"Failed to decode block response from peer {id:?}: {e:?}.",
+									"Failed to decode block response from peer {:?}: {:?}.",
+									id,
+									e
 								);
 								self.network_service.report_peer(id, rep::BAD_MESSAGE);
 								self.network_service
 									.disconnect_peer(id, self.block_announce_protocol_name.clone());
 								continue
 							},
-						};
-
-						if let Some(import) = self.on_block_response(id, req, response) {
-							return Poll::Ready(import)
+							Err(BlockResponseError::ExtractionFailed(e)) => {
+								debug!(
+									target: LOG_TARGET,
+									"Failed to extract blocks from peer response {:?}: {:?}.",
+									id,
+									e
+								);
+								self.network_service.report_peer(id, rep::BAD_MESSAGE);
+								continue
+							},
 						}
 					},
 					PeerRequest::State => {
@@ -2010,31 +1925,6 @@ where
 		Poll::Pending
 	}
 
-	/// Create implementation-specific block request.
-	fn create_opaque_block_request(&self, request: &BlockRequest<B>) -> OpaqueBlockRequest {
-		OpaqueBlockRequest(Box::new(schema::v1::BlockRequest {
-			fields: request.fields.to_be_u32(),
-			from_block: match request.from {
-				FromBlock::Hash(h) => Some(schema::v1::block_request::FromBlock::Hash(h.encode())),
-				FromBlock::Number(n) =>
-					Some(schema::v1::block_request::FromBlock::Number(n.encode())),
-			},
-			direction: request.direction as i32,
-			max_blocks: request.max.unwrap_or(0),
-			support_multiple_justifications: true,
-		}))
-	}
-
-	fn encode_block_request(&self, request: &OpaqueBlockRequest) -> Result<Vec<u8>, String> {
-		let request: &schema::v1::BlockRequest = request.0.downcast_ref().ok_or_else(|| {
-			"Failed to downcast opaque block response during encoding, this is an \
-				implementation bug."
-				.to_string()
-		})?;
-
-		Ok(request.encode_to_vec())
-	}
-
 	fn encode_state_request(&self, request: &OpaqueStateRequest) -> Result<Vec<u8>, String> {
 		let request: &StateRequest = request.0.downcast_ref().ok_or_else(|| {
 			"Failed to downcast opaque state response during encoding, this is an \
@@ -2909,7 +2799,7 @@ fn validate_blocks<Block: BlockT>(
 #[cfg(test)]
 mod test {
 	use super::*;
-	use crate::service::network::NetworkServiceProvider;
+	use crate::{mock::MockBlockDownloader, service::network::NetworkServiceProvider};
 	use futures::executor::block_on;
 	use sc_block_builder::BlockBuilderProvider;
 	use sc_network_common::{
@@ -2947,7 +2837,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3013,7 +2903,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3187,7 +3077,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3313,7 +3203,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3470,7 +3360,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3612,7 +3502,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3756,7 +3646,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3801,7 +3691,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)
@@ -3853,7 +3743,7 @@ mod test {
 			None,
 			chain_sync_network_handle,
 			import_queue,
-			ProtocolName::from("block-request"),
+			Arc::new(MockBlockDownloader::new()),
 			ProtocolName::from("state-request"),
 			None,
 		)