Do not request blocks below the common number when syncing

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

@@ -123,20 +123,36 @@ impl<B: BlockT> BlockCollection<B> {
 		let first_different = common + <NumberFor<B>>::one();
 		let count = (count as u32).into();
 		let (mut range, downloading) = {
+			// Iterate through the ranges in `self.blocks` looking for a range to download
 			let mut downloading_iter = self.blocks.iter().peekable();
 			let mut prev: Option<(&NumberFor<B>, &BlockRangeState<B>)> = None;
 			loop {
 				let next = downloading_iter.next();
 				break match (prev, next) {
+					// If we are already downloading this range, request it from `max_parallel`
+					// peers (`max_parallel = 5` by default).
+					// Do not request already downloading range from peers with common number above
+					// the range start.
 					(Some((start, &BlockRangeState::Downloading { ref len, downloading })), _)
-						if downloading < max_parallel =>
+						if downloading < max_parallel && *start >= first_different =>
 						(*start..*start + *len, downloading),
-					(Some((start, r)), Some((next_start, _))) if *start + r.len() < *next_start =>
-						(*start + r.len()..cmp::min(*next_start, *start + r.len() + count), 0), // gap
-					(Some((start, r)), None) => (*start + r.len()..*start + r.len() + count, 0), /* last range */
-					(None, None) => (first_different..first_different + count, 0),               /* empty */
+					// If there is a gap between ranges requested, download this gap unless the peer
+					// has common number above the gap start
+					(Some((start, r)), Some((next_start, _)))
+						if *start + r.len() < *next_start &&
+							*start + r.len() >= first_different =>
+						(*start + r.len()..cmp::min(*next_start, *start + r.len() + count), 0),
+					// Download `count` blocks after the last range requested unless the peer
+					// has common number above this new range
+					(Some((start, r)), None) if *start + r.len() >= first_different =>
+						(*start + r.len()..*start + r.len() + count, 0),
+					// If there are no ranges currently requested, download `count` blocks after
+					// `common` number
+					(None, None) => (first_different..first_different + count, 0),
+					// If the first range starts above `common + 1`, download the gap at the start
 					(None, Some((start, _))) if *start > first_different =>
-						(first_different..cmp::min(first_different + count, *start), 0), /* gap at the start */
+						(first_different..cmp::min(first_different + count, *start), 0),
+					// Move on to the next range pair
 					_ => {
 						prev = next;
 						continue
@@ -365,10 +381,10 @@ mod test {
 		bc.blocks.insert(114305, BlockRangeState::Complete(blocks));
 
 		let peer0 = PeerId::random();
-		assert_eq!(bc.needed_blocks(peer0, 128, 10000, 000, 1, 200), Some(1..100));
-		assert_eq!(bc.needed_blocks(peer0, 128, 10000, 600, 1, 200), None); // too far ahead
+		assert_eq!(bc.needed_blocks(peer0, 128, 10000, 0, 1, 200), Some(1..100));
+		assert_eq!(bc.needed_blocks(peer0, 128, 10000, 0, 1, 200), None); // too far ahead
 		assert_eq!(
-			bc.needed_blocks(peer0, 128, 10000, 600, 1, 200000),
+			bc.needed_blocks(peer0, 128, 10000, 0, 1, 200000),
 			Some(100 + 128..100 + 128 + 128)
 		);
 	}
@@ -431,4 +447,202 @@ mod test {
 		assert!(bc.blocks.is_empty());
 		assert!(bc.queued_blocks.is_empty());
 	}
+
+	#[test]
+	fn downloaded_range_is_requested_from_max_parallel_peers() {
+		let mut bc = BlockCollection::new();
+		assert!(is_empty(&bc));
+
+		let count = 5;
+		// identical ranges requested from 2 peers
+		let max_parallel = 2;
+		let max_ahead = 200;
+
+		let peer1 = PeerId::random();
+		let peer2 = PeerId::random();
+		let peer3 = PeerId::random();
+
+		// common for all peers
+		let best = 100;
+		let common = 10;
+
+		assert_eq!(
+			bc.needed_blocks(peer1, count, best, common, max_parallel, max_ahead),
+			Some(11..16)
+		);
+		assert_eq!(
+			bc.needed_blocks(peer2, count, best, common, max_parallel, max_ahead),
+			Some(11..16)
+		);
+		assert_eq!(
+			bc.needed_blocks(peer3, count, best, common, max_parallel, max_ahead),
+			Some(16..21)
+		);
+	}
+	#[test]
+	fn downloaded_range_not_requested_from_peers_with_higher_common_number() {
+		// A peer connects with a common number falling behind our best number
+		// (either a fork or lagging behind).
+		// We request a range from this peer starting at its common number + 1.
+		// Even though we have less than `max_parallel` downloads, we do not request
+		// this range from peers with a common number above the start of this range.
+
+		let mut bc = BlockCollection::new();
+		assert!(is_empty(&bc));
+
+		let count = 5;
+		let max_parallel = 2;
+		let max_ahead = 200;
+
+		let peer1 = PeerId::random();
+		let peer1_best = 20;
+		let peer1_common = 10;
+
+		// `peer2` has first different above the start of the range downloaded from `peer1`
+		let peer2 = PeerId::random();
+		let peer2_best = 20;
+		let peer2_common = 11; // first_different = 12
+
+		assert_eq!(
+			bc.needed_blocks(peer1, count, peer1_best, peer1_common, max_parallel, max_ahead),
+			Some(11..16),
+		);
+		assert_eq!(
+			bc.needed_blocks(peer2, count, peer2_best, peer2_common, max_parallel, max_ahead),
+			Some(16..21),
+		);
+	}
+
+	#[test]
+	fn gap_above_common_number_requested() {
+		let mut bc = BlockCollection::new();
+		assert!(is_empty(&bc));
+
+		let count = 5;
+		let best = 30;
+		// We need at least 3 ranges requested to have a gap, so to minimize the number of peers
+		// set `max_parallel = 1`
+		let max_parallel = 1;
+		let max_ahead = 200;
+
+		let peer1 = PeerId::random();
+		let peer2 = PeerId::random();
+		let peer3 = PeerId::random();
+
+		let common = 10;
+		assert_eq!(
+			bc.needed_blocks(peer1, count, best, common, max_parallel, max_ahead),
+			Some(11..16),
+		);
+		assert_eq!(
+			bc.needed_blocks(peer2, count, best, common, max_parallel, max_ahead),
+			Some(16..21),
+		);
+		assert_eq!(
+			bc.needed_blocks(peer3, count, best, common, max_parallel, max_ahead),
+			Some(21..26),
+		);
+
+		// For some reason there is now a gap at 16..21. We just disconnect `peer2`, but it might
+		// also happen that 16..21 received first and got imported if our best is actually >= 15.
+		bc.clear_peer_download(&peer2);
+
+		// Some peer connects with common number below the gap. The gap is requested from it.
+		assert_eq!(
+			bc.needed_blocks(peer2, count, best, common, max_parallel, max_ahead),
+			Some(16..21),
+		);
+	}
+
+	#[test]
+	fn gap_below_common_number_not_requested() {
+		let mut bc = BlockCollection::new();
+		assert!(is_empty(&bc));
+
+		let count = 5;
+		let best = 30;
+		// We need at least 3 ranges requested to have a gap, so to minimize the number of peers
+		// set `max_parallel = 1`
+		let max_parallel = 1;
+		let max_ahead = 200;
+
+		let peer1 = PeerId::random();
+		let peer2 = PeerId::random();
+		let peer3 = PeerId::random();
+
+		let common = 10;
+		assert_eq!(
+			bc.needed_blocks(peer1, count, best, common, max_parallel, max_ahead),
+			Some(11..16),
+		);
+		assert_eq!(
+			bc.needed_blocks(peer2, count, best, common, max_parallel, max_ahead),
+			Some(16..21),
+		);
+		assert_eq!(
+			bc.needed_blocks(peer3, count, best, common, max_parallel, max_ahead),
+			Some(21..26),
+		);
+
+		// For some reason there is now a gap at 16..21. We just disconnect `peer2`, but it might
+		// also happen that 16..21 received first and got imported if our best is actually >= 15.
+		bc.clear_peer_download(&peer2);
+
+		// Some peer connects with common number above the gap. The gap is not requested from it.
+		let common = 23;
+		assert_eq!(
+			bc.needed_blocks(peer2, count, best, common, max_parallel, max_ahead),
+			Some(26..31), // not 16..21
+		);
+	}
+
+	#[test]
+	fn range_at_the_end_above_common_number_requested() {
+		let mut bc = BlockCollection::new();
+		assert!(is_empty(&bc));
+
+		let count = 5;
+		let best = 30;
+		let max_parallel = 1;
+		let max_ahead = 200;
+
+		let peer1 = PeerId::random();
+		let peer2 = PeerId::random();
+
+		let common = 10;
+		assert_eq!(
+			bc.needed_blocks(peer1, count, best, common, max_parallel, max_ahead),
+			Some(11..16),
+		);
+		assert_eq!(
+			bc.needed_blocks(peer2, count, best, common, max_parallel, max_ahead),
+			Some(16..21),
+		);
+	}
+
+	#[test]
+	fn range_at_the_end_below_common_number_not_requested() {
+		let mut bc = BlockCollection::new();
+		assert!(is_empty(&bc));
+
+		let count = 5;
+		let best = 30;
+		let max_parallel = 1;
+		let max_ahead = 200;
+
+		let peer1 = PeerId::random();
+		let peer2 = PeerId::random();
+
+		let common = 10;
+		assert_eq!(
+			bc.needed_blocks(peer1, count, best, common, max_parallel, max_ahead),
+			Some(11..16),
+		);
+
+		let common = 20;
+		assert_eq!(
+			bc.needed_blocks(peer2, count, best, common, max_parallel, max_ahead),
+			Some(21..26), // not 16..21
+		);
+	}
 }