Fixed block response limit check (#9692)

* Fixed block response limit check

* Fixed start block detection and added a test

* Missing test

substrate/client/network/src/block_request_handler.rs

@@ -62,7 +62,7 @@ pub fn generate_protocol_config(protocol_id: &ProtocolId) -> ProtocolConfig {
 		name: generate_protocol_name(protocol_id).into(),
 		max_request_size: 1024 * 1024,
 		max_response_size: 16 * 1024 * 1024,
-		request_timeout: Duration::from_secs(40),
+		request_timeout: Duration::from_secs(20),
 		inbound_queue: None,
 	}
 }
@@ -355,7 +355,8 @@ impl<B: BlockT> BlockRequestHandler<B> {
 				indexed_body,
 			};
 
-			total_size += block_data.body.len();
+			total_size += block_data.body.iter().map(|ex| ex.len()).sum::<usize>();
+			total_size += block_data.indexed_body.iter().map(|ex| ex.len()).sum::<usize>();
 			blocks.push(block_data);
 
 			if blocks.len() >= max_blocks as usize || total_size > MAX_BODY_BYTES {

substrate/client/network/src/protocol.rs

@@ -629,7 +629,7 @@ impl<B: BlockT> Protocol<B> {
 					} else {
 						None
 					},
-					receipt: if !block_data.message_queue.is_empty() {
+					receipt: if !block_data.receipt.is_empty() {
 						Some(block_data.receipt)
 					} else {
 						None

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

@@ -70,7 +70,7 @@ mod state;
 mod warp;
 
 /// Maximum blocks to request in a single packet.
-const MAX_BLOCKS_TO_REQUEST: usize = 128;
+const MAX_BLOCKS_TO_REQUEST: usize = 64;
 
 /// Maximum blocks to store in the import queue.
 const MAX_IMPORTING_BLOCKS: usize = 2048;
@@ -1054,12 +1054,14 @@ impl<B: BlockT> ChainSync<B> {
 			self.pending_requests.add(who);
 			if let Some(request) = request {
 				match &mut peer.state {
-					PeerSyncState::DownloadingNew(start_block) => {
+					PeerSyncState::DownloadingNew(_) => {
 						self.blocks.clear_peer_download(who);
-						let start_block = *start_block;
 						peer.state = PeerSyncState::Available;
-						validate_blocks::<B>(&blocks, who, Some(request))?;
+						if let Some(start_block) =
-						self.blocks.insert(start_block, blocks, who.clone());
+							validate_blocks::<B>(&blocks, who, Some(request))?
+						{
+							self.blocks.insert(start_block, blocks, who.clone());
+						}
 						self.drain_blocks()
 					},
 					PeerSyncState::DownloadingStale(_) => {
@@ -2315,13 +2317,14 @@ where
 }
 
 /// Validate that the given `blocks` are correct.
+/// Returns the number of the first block in the sequence.
 ///
-/// It is expected that `blocks` are in asending order.
+/// It is expected that `blocks` are in ascending order.
 fn validate_blocks<Block: BlockT>(
 	blocks: &Vec<message::BlockData<Block>>,
 	who: &PeerId,
 	request: Option<BlockRequest<Block>>,
-) -> Result<(), BadPeer> {
+) -> Result<Option<NumberFor<Block>>, BadPeer> {
 	if let Some(request) = request {
 		if Some(blocks.len() as _) > request.max {
 			debug!(
@@ -2415,7 +2418,7 @@ fn validate_blocks<Block: BlockT>(
 		}
 	}
 
-	Ok(())
+	Ok(blocks.first().and_then(|b| b.header.as_ref()).map(|h| *h.number()))
 }
 
 #[cfg(test)]

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

@@ -194,7 +194,7 @@ impl<B: BlockT> BlockCollection<B> {
 		for r in ranges {
 			self.blocks.remove(&r);
 		}
-		trace!(target: "sync", "Drained {} blocks", drained.len());
+		trace!(target: "sync", "Drained {} blocks from {:?}", drained.len(), from);
 		drained
 	}
 

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

@@ -1193,3 +1193,32 @@ fn syncs_indexed_blocks() {
 		.unwrap()
 		.is_some());
 }
+
+#[test]
+fn syncs_huge_blocks() {
+	use sp_core::storage::well_known_keys::HEAP_PAGES;
+	use sp_runtime::codec::Encode;
+	use substrate_test_runtime_client::BlockBuilderExt;
+
+	sp_tracing::try_init_simple();
+	let mut net = TestNet::new(2);
+
+	// Increase heap space for bigger blocks.
+	net.peer(0).generate_blocks(1, BlockOrigin::Own, |mut builder| {
+		builder.push_storage_change(HEAP_PAGES.to_vec(), Some(256u64.encode())).unwrap();
+		builder.build().unwrap().block
+	});
+
+	net.peer(0).generate_blocks(32, BlockOrigin::Own, |mut builder| {
+		// Add 32 extrinsics 32k each = 1MiB total
+		for _ in 0..32 {
+			let ex = Extrinsic::IncludeData([42u8; 32 * 1024].to_vec());
+			builder.push(ex).unwrap();
+		}
+		builder.build().unwrap().block
+	});
+
+	net.block_until_sync();
+	assert_eq!(net.peer(0).client.info().best_number, 33);
+	assert_eq!(net.peer(1).client.info().best_number, 33);
+}