Enable download of future forks

client/network/src/protocol/sync.rs

@@ -2418,7 +2418,11 @@ fn fork_sync_request<B: BlockT>(
 		if !r.peers.contains(id) {
 			continue
 		}
-		if r.number <= best_num {
+		// Download the fork only if it is behind or not too far ahead our tip of the chain
+		// Otherwise it should be downloaded in full sync mode.
+		if r.number <= best_num ||
+			(r.number - best_num).saturated_into::<u32>() < MAX_BLOCKS_TO_REQUEST as u32
+		{
 			let parent_status = r.parent_hash.as_ref().map_or(BlockStatus::Unknown, check_block);
 			let count = if parent_status == BlockStatus::Unknown {
 				(r.number - finalized).saturated_into::<u32>() // up to the last finalized block
@@ -2438,6 +2442,8 @@ fn fork_sync_request<B: BlockT>(
 					max: Some(count),
 				},
 			))
+		} else {
+			trace!(target: "sync", "Fork too far in the future: {:?} (#{})", hash, r.number);
 		}
 	}
 	None

client/network/test/src/lib.rs

@@ -86,23 +86,14 @@ type AuthorityId = sp_consensus_babe::AuthorityId;
 #[derive(Clone)]
 pub struct PassThroughVerifier {
 	finalized: bool,
-	fork_choice: ForkChoiceStrategy,
 }
 
 impl PassThroughVerifier {
 	/// Create a new instance.
 	///
 	/// Every verified block will use `finalized` for the `BlockImportParams`.
 	pub fn new(finalized: bool) -> Self {
-		Self { finalized, fork_choice: ForkChoiceStrategy::LongestChain }
-	}
-
-	/// Create a new instance.
-	///
-	/// Every verified block will use `finalized` for the `BlockImportParams` and
-	/// the given [`ForkChoiceStrategy`].
-	pub fn new_with_fork_choice(finalized: bool, fork_choice: ForkChoiceStrategy) -> Self {
-		Self { finalized, fork_choice }
+		Self { finalized }
 	}
 }
 
@@ -121,8 +112,10 @@ impl<B: BlockT> Verifier<B> for PassThroughVerifier {
 					.or_else(|| l.try_as_raw(OpaqueDigestItemId::Consensus(b"babe")))
 			})
 			.map(|blob| vec![(well_known_cache_keys::AUTHORITIES, blob.to_vec())]);
+		if block.fork_choice.is_none() {
+			block.fork_choice = Some(ForkChoiceStrategy::LongestChain);
+		};
 		block.finalized = self.finalized;
-		block.fork_choice = Some(self.fork_choice.clone());
 		Ok((block, maybe_keys))
 	}
 }
@@ -294,7 +287,13 @@ where
 	}
 
 	/// Add blocks to the peer -- edit the block before adding
-	pub fn generate_blocks<F>(&mut self, count: usize, origin: BlockOrigin, edit_block: F) -> H256
+	pub fn generate_blocks<F>(
+		&mut self,
+		count: usize,
+		origin: BlockOrigin,
+		edit_block: F,
+		fork_choice: ForkChoiceStrategy,
+	) -> H256
 	where
 		F: FnMut(
 			BlockBuilder<Block, PeersFullClient, substrate_test_runtime_client::Backend>,
@@ -309,6 +308,7 @@ where
 			false,
 			true,
 			true,
+			fork_choice,
 		)
 	}
 
@@ -323,6 +323,7 @@ where
 		headers_only: bool,
 		inform_sync_about_new_best_block: bool,
 		announce_block: bool,
+		fork_choice: ForkChoiceStrategy,
 	) -> H256
 	where
 		F: FnMut(
@@ -346,6 +347,7 @@ where
 			let header = block.header.clone();
 			let mut import_block = BlockImportParams::new(origin, header.clone());
 			import_block.body = if headers_only { None } else { Some(block.extrinsics) };
+			import_block.fork_choice = Some(fork_choice);
 			let (import_block, cache) =
 				futures::executor::block_on(self.verifier.verify(import_block)).unwrap();
 			let cache = if let Some(cache) = cache {
@@ -442,6 +444,7 @@ where
 				headers_only,
 				inform_sync_about_new_best_block,
 				announce_block,
+				ForkChoiceStrategy::LongestChain,
 			)
 		} else {
 			self.generate_blocks_at(
@@ -452,15 +455,21 @@ where
 				headers_only,
 				inform_sync_about_new_best_block,
 				announce_block,
+				ForkChoiceStrategy::LongestChain,
 			)
 		}
 	}
 
 	pub fn push_authorities_change_block(&mut self, new_authorities: Vec<AuthorityId>) -> H256 {
-		self.generate_blocks(1, BlockOrigin::File, |mut builder| {
-			builder.push(Extrinsic::AuthoritiesChange(new_authorities.clone())).unwrap();
-			builder.build().unwrap().block
-		})
+		self.generate_blocks(
+			1,
+			BlockOrigin::File,
+			|mut builder| {
+				builder.push(Extrinsic::AuthoritiesChange(new_authorities.clone())).unwrap();
+				builder.build().unwrap().block
+			},
+			ForkChoiceStrategy::LongestChain,
+		)
 	}
 
 	/// Get a reference to the client.
@@ -993,14 +1002,6 @@ where
 
 pub struct TestNet {
 	peers: Vec<Peer<(), PeersClient>>,
-	fork_choice: ForkChoiceStrategy,
-}
-
-impl TestNet {
-	/// Create a `TestNet` that used the given fork choice rule.
-	pub fn with_fork_choice(fork_choice: ForkChoiceStrategy) -> Self {
-		Self { peers: Vec::new(), fork_choice }
-	}
 }
 
 impl TestNetFactory for TestNet {
@@ -1010,7 +1011,7 @@ impl TestNetFactory for TestNet {
 
 	/// Create new test network with peers and given config.
 	fn from_config(_config: &ProtocolConfig) -> Self {
-		TestNet { peers: Vec::new(), fork_choice: ForkChoiceStrategy::LongestChain }
+		TestNet { peers: Vec::new() }
 	}
 
 	fn make_verifier(
@@ -1019,7 +1020,7 @@ impl TestNetFactory for TestNet {
 		_config: &ProtocolConfig,
 		_peer_data: &(),
 	) -> Self::Verifier {
-		PassThroughVerifier::new_with_fork_choice(false, self.fork_choice.clone())
+		PassThroughVerifier::new(false)
 	}
 
 	fn make_block_import(

client/network/test/src/sync.rs

@@ -378,8 +378,12 @@ fn own_blocks_are_announced() {
 	sp_tracing::try_init_simple();
 	let mut net = TestNet::new(3);
 	net.block_until_sync(); // connect'em
-	net.peer(0)
-		.generate_blocks(1, BlockOrigin::Own, |builder| builder.build().unwrap().block);
+	net.peer(0).generate_blocks(
+		1,
+		BlockOrigin::Own,
+		|builder| builder.build().unwrap().block,
+		ForkChoiceStrategy::LongestChain,
+	);
 
 	net.block_until_sync();
 
@@ -453,6 +457,38 @@ fn can_sync_small_non_best_forks() {
 	}));
 }
 
+#[test]
+fn can_sync_forks_ahead_of_the_best_chain() {
+	sp_tracing::try_init_simple();
+	let mut net = TestNet::new(2);
+	net.peer(0).push_blocks(1, false);
+	net.peer(1).push_blocks(1, false);
+
+	net.block_until_connected();
+	// Peer 0 is on 2-block fork which is announced with is_best=false
+	let fork_hash = net.peer(0).generate_blocks(
+		2,
+		BlockOrigin::Own,
+		|builder| builder.build().unwrap().block,
+		ForkChoiceStrategy::Custom(false),
+	);
+	// Peer 1 is on 1-block fork
+	net.peer(1).push_blocks(1, false);
+	assert!(net.peer(0).client().header(&BlockId::Hash(fork_hash)).unwrap().is_some());
+	assert_eq!(net.peer(0).client().info().best_number, 1);
+	assert_eq!(net.peer(1).client().info().best_number, 2);
+
+	// after announcing, peer 1 downloads the block.
+	block_on(futures::future::poll_fn::<(), _>(|cx| {
+		net.poll(cx);
+
+		if net.peer(1).client().header(&BlockId::Hash(fork_hash)).unwrap().is_none() {
+			return Poll::Pending
+		}
+		Poll::Ready(())
+	}));
+}
+
 #[test]
 fn can_sync_explicit_forks() {
 	sp_tracing::try_init_simple();
@@ -678,7 +714,7 @@ impl BlockAnnounceValidator<Block> for FailingBlockAnnounceValidator {
 #[test]
 fn sync_blocks_when_block_announce_validator_says_it_is_new_best() {
 	sp_tracing::try_init_simple();
-	let mut net = TestNet::with_fork_choice(ForkChoiceStrategy::Custom(false));
+	let mut net = TestNet::new(0);
 	net.add_full_peer_with_config(Default::default());
 	net.add_full_peer_with_config(Default::default());
 	net.add_full_peer_with_config(FullPeerConfig {
@@ -688,16 +724,17 @@ fn sync_blocks_when_block_announce_validator_says_it_is_new_best() {
 
 	net.block_until_connected();
 
-	let block_hash = net.peer(0).push_blocks(1, false);
+	// Add blocks but don't set them as best
+	let block_hash = net.peer(0).generate_blocks(
+		1,
+		BlockOrigin::Own,
+		|builder| builder.build().unwrap().block,
+		ForkChoiceStrategy::Custom(false),
+	);
 
 	while !net.peer(2).has_block(&block_hash) {
 		net.block_until_idle();
 	}
-
-	// Peer1 should not have the block, because peer 0 did not reported the block
-	// as new best. However, peer2 has a special block announcement validator
-	// that flags all blocks as `is_new_best` and thus, it should have synced the blocks.
-	assert!(!net.peer(1).has_block(&block_hash));
 }
 
 /// Waits for some time until the validation is successfull.
@@ -721,7 +758,7 @@ impl BlockAnnounceValidator<Block> for DeferredBlockAnnounceValidator {
 #[test]
 fn wait_until_deferred_block_announce_validation_is_ready() {
 	sp_tracing::try_init_simple();
-	let mut net = TestNet::with_fork_choice(ForkChoiceStrategy::Custom(false));
+	let mut net = TestNet::new(0);
 	net.add_full_peer_with_config(Default::default());
 	net.add_full_peer_with_config(FullPeerConfig {
 		block_announce_validator: Some(Box::new(NewBestBlockAnnounceValidator)),
@@ -730,7 +767,13 @@ fn wait_until_deferred_block_announce_validation_is_ready() {
 
 	net.block_until_connected();
 
-	let block_hash = net.peer(0).push_blocks(1, true);
+	// Add blocks but don't set them as best
+	let block_hash = net.peer(0).generate_blocks(
+		1,
+		BlockOrigin::Own,
+		|builder| builder.build().unwrap().block,
+		ForkChoiceStrategy::Custom(false),
+	);
 
 	while !net.peer(1).has_block(&block_hash) {
 		net.block_until_idle();
@@ -847,7 +890,10 @@ fn block_announce_data_is_propagated() {
 	// 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 {
+		if net.peer(1).num_peers() == 2 &&
+			net.peer(0).num_peers() == 1 &&
+			net.peer(2).num_peers() == 1
+		{
 			Poll::Ready(())
 		} else {
 			Poll::Pending
@@ -1109,6 +1155,7 @@ fn syncs_indexed_blocks() {
 		false,
 		true,
 		true,
+		ForkChoiceStrategy::LongestChain,
 	);
 	let indexed_key = sp_runtime::traits::BlakeTwo256::hash(&42u64.to_le_bytes());
 	assert!(net
@@ -1178,19 +1225,29 @@ fn syncs_huge_blocks() {
 	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(
+		1,
+		BlockOrigin::Own,
+		|mut builder| {
+			builder.push_storage_change(HEAP_PAGES.to_vec(), Some(256u64.encode())).unwrap();
+			builder.build().unwrap().block
+		},
+		ForkChoiceStrategy::LongestChain,
+	);
 
-	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.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
+		},
+		ForkChoiceStrategy::LongestChain,
+	);
 
 	net.block_until_sync();
 	assert_eq!(net.peer(0).client.info().best_number, 33);