Skip to content

Commit

Permalink
Do not request blocks below the common number when syncing (#2045)
Browse files Browse the repository at this point in the history 
This changes `BlockCollection` logic so we don't download block ranges
from peers with which we have these ranges already in sync.

Improves situation with
#1915.
  • Loading branch information
@dmitry-markin
dmitry-markin authored Nov 3, 2023
1 parent f6f4c5a commit 8dc41ba
Show file tree
Hide file tree
Showing 3 changed files with 255 additions and 122 deletions.
232 changes: 223 additions & 9 deletions substrate/client/network/sync/src/blocks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
);
}
Expand Down Expand Up @@ -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
);
}
}
38 changes: 32 additions & 6 deletions substrate/client/network/sync/src/chain_sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,12 +520,15 @@ where
Err(BadPeer(who, rep::BLOCKCHAIN_READ_ERROR))
},
Ok(BlockStatus::KnownBad) => {
info!("💔 New peer with known bad best block {} ({}).", best_hash, best_number);
info!("💔 New peer {who} with known bad best block {best_hash} ({best_number}).");
Err(BadPeer(who, rep::BAD_BLOCK))
},
Ok(BlockStatus::Unknown) => {
if best_number.is_zero() {
info!("💔 New peer with unknown genesis hash {} ({}).", best_hash, best_number);
info!(
"💔 New peer {} with unknown genesis hash {} ({}).",
who, best_hash, best_number,
);
return Err(BadPeer(who, rep::GENESIS_MISMATCH))
}

Expand All @@ -535,7 +538,8 @@ where
if self.queue_blocks.len() > MAJOR_SYNC_BLOCKS.into() {
debug!(
target:LOG_TARGET,
"New peer with unknown best hash {} ({}), assuming common block.",
"New peer {} with unknown best hash {} ({}), assuming common block.",
who,
self.best_queued_hash,
self.best_queued_number
);
Expand All @@ -556,7 +560,7 @@ where
let (state, req) = if self.best_queued_number.is_zero() {
debug!(
target:LOG_TARGET,
"New peer with best hash {best_hash} ({best_number}).",
"New peer {who} with best hash {best_hash} ({best_number}).",
);

(PeerSyncState::Available, None)
Expand All @@ -565,7 +569,8 @@ where

debug!(
target:LOG_TARGET,
"New peer with unknown best hash {} ({}), searching for common ancestor.",
"New peer {} with unknown best hash {} ({}), searching for common ancestor.",
who,
best_hash,
best_number
);
Expand Down Expand Up @@ -613,7 +618,7 @@ where
Ok(BlockStatus::InChainPruned) => {
debug!(
target: LOG_TARGET,
"New peer with known best hash {best_hash} ({best_number}).",
"New peer {who} with known best hash {best_hash} ({best_number}).",
);
self.peers.insert(
who,
Expand Down Expand Up @@ -848,8 +853,22 @@ where
// We've made progress on this chain since the search was started.
// Opportunistically set common number to updated number
// instead of the one that started the search.
trace!(
target: LOG_TARGET,
"Ancestry search: opportunistically updating peer {} common number from={} => to={}.",
*who,
peer.common_number,
self.best_queued_number,
);
peer.common_number = self.best_queued_number;
} else if peer.common_number < *current {
trace!(
target: LOG_TARGET,
"Ancestry search: updating peer {} common number from={} => to={}.",
*who,
peer.common_number,
*current,
);
peer.common_number = *current;
}
}
Expand Down Expand Up @@ -1338,6 +1357,13 @@ where
// should be kept in that state.
if let PeerSyncState::DownloadingJustification(_) = p.state {
// We make sure our commmon number is at least something we have.
trace!(
target: LOG_TARGET,
"Keeping peer {} after restart, updating common number from={} => to={} (our best).",
peer_id,
p.common_number,
self.best_queued_number,
);
p.common_number = self.best_queued_number;
self.peers.insert(peer_id, p);
return None
Expand Down
Loading

0 comments on commit 8dc41ba

Please sign in to comment.