From 5948416d0ebd5d3a154088b7be0adf05c16a0812 Mon Sep 17 00:00:00 2001 From: Arkadiy Paronyan Date: Wed, 2 Feb 2022 12:29:36 +0100 Subject: [PATCH] Enable download of future forks (#10739) * Enable download of future forks * Fixed external tests --- client/network/src/protocol/sync.rs | 8 +++- client/network/test/src/lib.rs | 58 ++++++++++++++++---------- client/network/test/src/sync.rs | 63 ++++++++++++++++++++++++----- 3 files changed, 97 insertions(+), 32 deletions(-) diff --git a/client/network/src/protocol/sync.rs b/client/network/src/protocol/sync.rs index d6513ca2e5b9d..fbb4e376b1b4f 100644 --- a/client/network/src/protocol/sync.rs +++ b/client/network/src/protocol/sync.rs @@ -2418,7 +2418,11 @@ fn fork_sync_request( 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::() < 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::() // up to the last finalized block @@ -2438,6 +2442,8 @@ fn fork_sync_request( max: Some(count), }, )) + } else { + trace!(target: "sync", "Fork too far in the future: {:?} (#{})", hash, r.number); } } None diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index b2345f3701b0a..3986ac47f3616 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -86,7 +86,6 @@ type AuthorityId = sp_consensus_babe::AuthorityId; #[derive(Clone)] pub struct PassThroughVerifier { finalized: bool, - fork_choice: ForkChoiceStrategy, } impl PassThroughVerifier { @@ -94,15 +93,7 @@ impl PassThroughVerifier { /// /// 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 Verifier 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)) } } @@ -309,6 +302,33 @@ where false, true, true, + ForkChoiceStrategy::LongestChain, + ) + } + + /// Add blocks to the peer -- edit the block before adding and use custom fork choice rule. + pub fn generate_blocks_with_fork_choice( + &mut self, + count: usize, + origin: BlockOrigin, + edit_block: F, + fork_choice: ForkChoiceStrategy, + ) -> H256 + where + F: FnMut( + BlockBuilder, + ) -> Block, + { + let best_hash = self.client.info().best_hash; + self.generate_blocks_at( + BlockId::Hash(best_hash), + count, + origin, + edit_block, + false, + true, + true, + fork_choice, ) } @@ -323,6 +343,7 @@ where headers_only: bool, inform_sync_about_new_best_block: bool, announce_block: bool, + fork_choice: ForkChoiceStrategy, ) -> H256 where F: FnMut( @@ -346,6 +367,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 +464,7 @@ where headers_only, inform_sync_about_new_best_block, announce_block, + ForkChoiceStrategy::LongestChain, ) } else { self.generate_blocks_at( @@ -452,6 +475,7 @@ where headers_only, inform_sync_about_new_best_block, announce_block, + ForkChoiceStrategy::LongestChain, ) } } @@ -989,14 +1013,6 @@ where pub struct TestNet { peers: Vec>, - 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 { @@ -1006,7 +1022,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( @@ -1015,7 +1031,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( diff --git a/client/network/test/src/sync.rs b/client/network/test/src/sync.rs index 5b607a7762b34..84a5c2ca13fa5 100644 --- a/client/network/test/src/sync.rs +++ b/client/network/test/src/sync.rs @@ -453,6 +453,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_with_fork_choice( + 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 +710,7 @@ impl BlockAnnounceValidator 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 +720,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_with_fork_choice( + 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 +754,7 @@ impl BlockAnnounceValidator 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 +763,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_with_fork_choice( + 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 +886,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 +1151,7 @@ fn syncs_indexed_blocks() { false, true, true, + ForkChoiceStrategy::LongestChain, ); let indexed_key = sp_runtime::traits::BlakeTwo256::hash(&42u64.to_le_bytes()); assert!(net