From 7cd144569d63466fe22fcc665d7320e447725fa4 Mon Sep 17 00:00:00 2001 From: Rahul Subramaniyam Date: Fri, 13 Oct 2023 11:03:57 -0700 Subject: [PATCH 1/3] Fix tests --- substrate/client/network/sync/src/lib.rs | 46 ++++++++++++++++-------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/substrate/client/network/sync/src/lib.rs b/substrate/client/network/sync/src/lib.rs index 6d999980ea62..4cc584cfce91 100644 --- a/substrate/client/network/sync/src/lib.rs +++ b/substrate/client/network/sync/src/lib.rs @@ -3881,6 +3881,7 @@ mod test { let non_canonical_chain_length = common_ancestor + 3; let canonical_chain_length = common_ancestor + max_blocks_per_request + 10; + let import_queue = Box::new(sc_consensus::import_queue::mock::MockImportQueueHandle::new()); let (_chain_sync_network_provider, chain_sync_network_handle) = NetworkServiceProvider::new(); let mut client = Arc::new(TestClientBuilder::new().build()); @@ -3908,14 +3909,22 @@ mod test { .collect::>() }; - let mut sync = ChainSync::new( - SyncMode::Full, + let (mut sync, _) = ChainSync::new( + Arc::new(Atomic::new(SyncMode::Full)), client.clone(), - ProtocolName::from("test-block-announce-protocol"), + ProtocolId::from("test-protocol-name"), + &Some(String::from("test-fork-id")), + Roles::from(&Role::Full), 1, max_blocks_per_request, None, + None, chain_sync_network_handle, + import_queue, + Arc::new(MockBlockDownloader::new()), + ProtocolName::from("state-request"), + None, + true, ) .unwrap(); @@ -3923,7 +3932,7 @@ mod test { let peer_id = PeerId::random(); let canonical_tip = canonical_blocks.last().unwrap().clone(); let mut request = sync - .new_peer(peer_id, canonical_tip.hash(), *canonical_tip.header().number()) + .new_peer(peer_id, canonical_tip.hash(), *canonical_tip.header().number(), true) .unwrap() .unwrap(); assert_eq!(FromBlock::Number(client.info().best_number), request.from); @@ -3968,7 +3977,7 @@ mod test { let res = sync.on_block_data(&peer_id, Some(request), response).unwrap(); assert!(matches!( res, - OnBlockData::Import(ImportBlocksAction{ origin: _, blocks }) if blocks.is_empty() + OnBlockData::Import(_, blocks) if blocks.is_empty() ),); // Gap filled, expect max_blocks_per_request being imported now. @@ -3978,7 +3987,7 @@ mod test { let response = create_block_response(resp_blocks.clone()); let res = sync.on_block_data(&peer_id, Some(request), response).unwrap(); let to_import: Vec<_> = match &res { - OnBlockData::Import(ImportBlocksAction { origin: _, blocks }) => { + OnBlockData::Import(_, blocks) => { assert_eq!(blocks.len(), sync.max_blocks_per_request as usize); blocks .iter() @@ -4029,7 +4038,7 @@ mod test { let res = sync.on_block_data(&peer_id, Some(request), response).unwrap(); assert!(matches!( res, - OnBlockData::Import(ImportBlocksAction{ origin: _, blocks }) if blocks.len() == 10 as usize + OnBlockData::Import(_, blocks) if blocks.len() == 10 as usize ),); let _ = sync.on_blocks_processed( max_blocks_per_request as usize, @@ -4069,6 +4078,7 @@ mod test { let non_canonical_chain_length = common_ancestor + 3; let canonical_chain_length = common_ancestor + max_blocks_per_request + 10; + let import_queue = Box::new(sc_consensus::import_queue::mock::MockImportQueueHandle::new()); let (_chain_sync_network_provider, chain_sync_network_handle) = NetworkServiceProvider::new(); let mut client = Arc::new(TestClientBuilder::new().build()); @@ -4096,14 +4106,22 @@ mod test { .collect::>() }; - let mut sync = ChainSync::new( - SyncMode::Full, + let (mut sync, _) = ChainSync::new( + Arc::new(Atomic::new(SyncMode::Full)), client.clone(), - ProtocolName::from("test-block-announce-protocol"), + ProtocolId::from("test-protocol-name"), + &Some(String::from("test-fork-id")), + Roles::from(&Role::Full), 1, max_blocks_per_request, None, + None, chain_sync_network_handle, + import_queue, + Arc::new(MockBlockDownloader::new()), + ProtocolName::from("state-request"), + None, + true, ) .unwrap(); @@ -4111,7 +4129,7 @@ mod test { let peer_id = PeerId::random(); let canonical_tip = canonical_blocks.last().unwrap().clone(); let mut request = sync - .new_peer(peer_id, canonical_tip.hash(), *canonical_tip.header().number()) + .new_peer(peer_id, canonical_tip.hash(), *canonical_tip.header().number(), true) .unwrap() .unwrap(); assert_eq!(FromBlock::Number(client.info().best_number), request.from); @@ -4156,7 +4174,7 @@ mod test { let res = sync.on_block_data(&peer_id, Some(request), response).unwrap(); assert!(matches!( res, - OnBlockData::Import(ImportBlocksAction{ origin: _, blocks }) if blocks.is_empty() + OnBlockData::Import(_, blocks) if blocks.is_empty() ),); // Gap filled, expect max_blocks_per_request being imported now. @@ -4166,7 +4184,7 @@ mod test { let response = create_block_response(resp_blocks.clone()); let res = sync.on_block_data(&peer_id, Some(request), response).unwrap(); let to_import: Vec<_> = match &res { - OnBlockData::Import(ImportBlocksAction { origin: _, blocks }) => { + OnBlockData::Import(_, blocks) => { assert_eq!(blocks.len(), sync.max_blocks_per_request as usize); blocks .iter() @@ -4217,7 +4235,7 @@ mod test { let res = sync.on_block_data(&peer_id, Some(request), response).unwrap(); assert!(matches!( res, - OnBlockData::Import(ImportBlocksAction{ origin: _, blocks }) if blocks.len() == 10 as usize + OnBlockData::Import(_, blocks) if blocks.len() == 10 as usize ),); let _ = sync.on_blocks_processed( max_blocks_per_request as usize, From c03d8ea246dccdd2c852145e93dbef0443e354c8 Mon Sep 17 00:00:00 2001 From: Rahul Subramaniyam Date: Fri, 13 Oct 2023 11:20:10 -0700 Subject: [PATCH 2/3] Debug logs --- substrate/client/network/sync/src/blocks.rs | 6 ++++-- substrate/client/network/sync/src/lib.rs | 21 ++++++++++++++++++--- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/substrate/client/network/sync/src/blocks.rs b/substrate/client/network/sync/src/blocks.rs index cad50fef3e32..ee724fd27e4f 100644 --- a/substrate/client/network/sync/src/blocks.rs +++ b/substrate/client/network/sync/src/blocks.rs @@ -146,7 +146,8 @@ impl BlockCollection { }; // crop to peers best if range.start > peer_best { - trace!(target: "sync", "Out of range for peer {} ({} vs {})", who, range.start, peer_best); + trace!(target: "sync", "Out of range for peer {who}, peer_common = {common}, \ + peer_best = {peer_best}, range = {range:?}"); return None } range.end = cmp::min(peer_best + One::one(), range.end); @@ -157,7 +158,8 @@ impl BlockCollection { .next() .map_or(false, |(n, _)| range.start > *n + max_ahead.into()) { - trace!(target: "sync", "Too far ahead for peer {} ({})", who, range.start); + trace!(target: "sync", "Too far ahead for peer {who}, peer_common = {common}, \ + peer_best = {peer_best}, range = {range:?}, max_ahead = {max_ahead}"); return None } diff --git a/substrate/client/network/sync/src/lib.rs b/substrate/client/network/sync/src/lib.rs index 4cc584cfce91..ab54df27fda7 100644 --- a/substrate/client/network/sync/src/lib.rs +++ b/substrate/client/network/sync/src/lib.rs @@ -1561,12 +1561,27 @@ where // the start block has a parent on chain. let parent_on_chain = self.blocks.first_ready_block_header(start_block).map_or(false, |hdr| { - std::matches!( - self.block_status(hdr.parent_hash()).unwrap_or(BlockStatus::Unknown), + let block_status = self.block_status(hdr.parent_hash()); + let parent_on_chain = std::matches!( + *block_status.as_ref().unwrap_or(&BlockStatus::Unknown), BlockStatus::InChainWithState | BlockStatus::InChainPruned | BlockStatus::Queued - ) + ); + if !parent_on_chain { + trace!( + target: LOG_TARGET, + "Ready block parent not on chain: block_status={:?}, best_queued={}/{}, \ + first_ready={}/{}, first_ready_parent={}", + block_status, + self.best_queued_number, + self.best_queued_hash, + *hdr.number(), + hdr.hash(), + hdr.parent_hash() + ); + } + parent_on_chain }); if !parent_on_chain { From ef243fee1949d5e28b536ad91671bfc333764048 Mon Sep 17 00:00:00 2001 From: Rahul Subramaniyam Date: Thu, 19 Oct 2023 01:06:38 -0700 Subject: [PATCH 3/3] Set peer common number to parent on reorg --- substrate/client/network/sync/src/lib.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/substrate/client/network/sync/src/lib.rs b/substrate/client/network/sync/src/lib.rs index ab54df27fda7..11eaf1e85a6c 100644 --- a/substrate/client/network/sync/src/lib.rs +++ b/substrate/client/network/sync/src/lib.rs @@ -1094,7 +1094,22 @@ where return } + let mut is_reorg = false; if is_best { + if peer.best_number == number && peer.best_hash != hash { + trace!( + target: "sync", + "Reorg detected on block announce from {}: {}, hash {} -> {}, \ + known_parent = {}, {:?}", + who, + number, + peer.best_hash, + hash, + known_parent, + announce.summary(), + ); + is_reorg = true; + } // update their best block peer.best_number = number; peer.best_hash = hash; @@ -1107,7 +1122,11 @@ where // If the announced block is the best they have and is not ahead of us, our common number // is either one further ahead or it's the one they just announced, if we know about it. if is_best { - if known && self.best_queued_number >= number { + if is_reorg && known_parent { + // If the peer announced different hash for same block number, + // fall back to the parent as the common ancestor. + peer.common_number = number.saturating_sub(One::one()); + } else if known && self.best_queued_number >= number { self.update_peer_common_number(&who, number); } else if announce.header.parent_hash() == &self.best_queued_hash || known_parent && self.best_queued_number >= number