Add test to demonstrate the failure scenario

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

@@ -2618,12 +2618,25 @@ mod test {
 
 	/// Build and import a new best block.
 	fn build_block(client: &mut Arc<TestClient>, at: Option<Hash>, fork: bool) -> Block {
+		if fork {
+			build_block_with_fork_data(client, at, Some((vec![1, 2, 3], vec![4, 5, 6])))
+		} else {
+			build_block_with_fork_data(client, at, None)
+		}
+	}
+
+	/// Build and import a new best block and the fork data.
+	fn build_block_with_fork_data(
+		client: &mut Arc<TestClient>,
+		at: Option<Hash>,
+		fork_data: Option<(Vec<u8>, Vec<u8>)>,
+	) -> Block {
 		let at = at.unwrap_or_else(|| client.info().best_hash);
 
 		let mut block_builder = client.new_block_at(at, Default::default(), false).unwrap();
 
-		if fork {
-			block_builder.push_storage_change(vec![1, 2, 3], Some(vec![4, 5, 6])).unwrap();
+		if let Some((key, value)) = fork_data {
+			block_builder.push_storage_change(key, Some(value)).unwrap();
 		}
 
 		let block = block_builder.build().unwrap().block;
@@ -3364,4 +3377,127 @@ mod test {
 		pending_responses.remove(&peers[1]);
 		assert_eq!(pending_responses.len(), 0);
 	}
+
+	#[test]
+	#[should_panic]
+	fn request_across_forks() {
+		sp_tracing::try_init_simple();
+
+		let (_chain_sync_network_provider, chain_sync_network_handle) =
+			NetworkServiceProvider::new();
+		let mut client = Arc::new(TestClientBuilder::new().build());
+		let blocks = (0..100).map(|_| build_block(&mut client, None, false)).collect::<Vec<_>>();
+
+		let fork_a_blocks = {
+			let mut client = Arc::new(TestClientBuilder::new().build());
+			let mut fork_blocks = blocks[..]
+				.into_iter()
+				.inspect(|b| {
+					assert!(matches!(client.block(*b.header.parent_hash()), Ok(Some(_))));
+					block_on(client.import(BlockOrigin::Own, (*b).clone())).unwrap()
+				})
+				.cloned()
+				.collect::<Vec<_>>();
+			for i in 1..=10 {
+				fork_blocks.push(build_block_with_fork_data(
+					&mut client,
+					None,
+					Some((vec![2 * i], vec![2 * i + 1])),
+				));
+			}
+			fork_blocks
+		};
+
+		let fork_b_blocks = {
+			let mut client = Arc::new(TestClientBuilder::new().build());
+			let mut fork_blocks = blocks[..]
+				.into_iter()
+				.inspect(|b| {
+					assert!(matches!(client.block(*b.header.parent_hash()), Ok(Some(_))));
+					block_on(client.import(BlockOrigin::Own, (*b).clone())).unwrap()
+				})
+				.cloned()
+				.collect::<Vec<_>>();
+			for i in 1..=10 {
+				fork_blocks.push(build_block_with_fork_data(
+					&mut client,
+					None,
+					Some((vec![100 + 2 * i], vec![100 + 2 * i + 1])),
+				));
+			}
+			fork_blocks
+		};
+
+		let mut sync = ChainSync::new(
+			SyncMode::Full,
+			client.clone(),
+			ProtocolName::from("test-block-announce-protocol"),
+			5,
+			64,
+			None,
+			chain_sync_network_handle,
+		)
+		.unwrap();
+
+		// Add the peers, all at the common ancestor 100.
+		let common_block = blocks.last().unwrap();
+		let peer_id1 = PeerId::random();
+		sync.new_peer(peer_id1, common_block.hash(), *common_block.header().number())
+			.unwrap();
+		let peer_id2 = PeerId::random();
+		sync.new_peer(peer_id2, common_block.hash(), *common_block.header().number())
+			.unwrap();
+
+		// Peer 1 announces 107 from fork 1, 100-107 get downloaded.
+		{
+			let block = (&fork_a_blocks[106]).clone();
+			let peer = peer_id1;
+			log::trace!(target: LOG_TARGET, "<1> {peer} announces from fork 1");
+			send_block_announce(block.header().clone(), peer, &mut sync);
+			let request = get_block_request(&mut sync, FromBlock::Hash(block.hash()), 7, &peer);
+			let mut resp_blocks = fork_a_blocks[100_usize..107_usize].to_vec();
+			resp_blocks.reverse();
+			let response = create_block_response(resp_blocks.clone());
+			let res = sync.on_block_data(&peer, Some(request), response).unwrap();
+			assert!(matches!(
+				res,
+				OnBlockData::Import(ImportBlocksAction{ origin: _, blocks }) if blocks.len() == 7_usize
+			),);
+			assert_eq!(sync.best_queued_number, 107);
+			assert_eq!(sync.best_queued_hash, block.hash());
+			assert!(sync.is_known(&block.header.parent_hash()));
+		}
+
+		// Peer 2 also announces 107 from fork 1.
+		{
+			let prev_best_number = sync.best_queued_number;
+			let prev_best_hash = sync.best_queued_hash;
+			let peer = peer_id2;
+			log::trace!(target: LOG_TARGET, "<2> {peer} announces from fork 1");
+			for i in 100..107 {
+				let block = (&fork_a_blocks[i]).clone();
+				send_block_announce(block.header().clone(), peer, &mut sync);
+				assert!(sync.block_requests().is_empty());
+			}
+			assert_eq!(sync.best_queued_number, prev_best_number);
+			assert_eq!(sync.best_queued_hash, prev_best_hash);
+		}
+
+		// Peer 2 undergoes reorg, announces 108 from fork 2, gets downloaded even though we
+		// don't have the parent from fork 2.
+		{
+			let block = (&fork_b_blocks[107]).clone();
+			let peer = peer_id2;
+			log::trace!(target: LOG_TARGET, "<3> {peer} announces from fork 2");
+			send_block_announce(block.header().clone(), peer, &mut sync);
+			let request = get_block_request(&mut sync, FromBlock::Hash(block.hash()), 1, &peer);
+			let response = create_block_response(vec![block.clone()]);
+			let res = sync.on_block_data(&peer, Some(request), response).unwrap();
+			assert!(matches!(
+				res,
+				OnBlockData::Import(ImportBlocksAction{ origin: _, blocks }) if blocks.len() == 1_usize
+			),);
+			assert!(sync.is_known(&block.header.parent_hash()));
+		}
+	}
 }