From ab11cede3dfe74be958a23d3ec0fa5d7cbdf217a Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Tue, 10 Sep 2024 21:21:39 +0800 Subject: [PATCH 1/4] Avoid updating the state of `allowed_requests` if no block requests are sent Previously, the state of `allowed_requests` will always be reset to the default value even if there are no new block requests in the end. This could cause an edge cause that `peer_block_request()` will early return next time when no ongoing block requests in fact. --- .../client/network/sync/src/strategy/chain_sync.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/chain_sync.rs b/substrate/client/network/sync/src/strategy/chain_sync.rs index fd0e3ea1a76c..ac5fd35c01d2 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -148,6 +148,7 @@ impl Metrics { } } +#[derive(Debug, Clone)] enum AllowedRequests { Some(HashSet), All, @@ -1714,13 +1715,14 @@ where let best_queued = self.best_queued_number; let client = &self.client; let queue_blocks = &self.queue_blocks; - let allowed_requests = self.allowed_requests.take(); + let allowed_requests = self.allowed_requests.clone(); let max_parallel = if is_major_syncing { 1 } else { self.max_parallel_downloads }; let max_blocks_per_request = self.max_blocks_per_request; let gap_sync = &mut self.gap_sync; let disconnected_peers = &mut self.disconnected_peers; let metrics = self.metrics.as_ref(); - self.peers + let requests = self + .peers .iter_mut() .filter_map(move |(&id, peer)| { if !peer.state.is_available() || @@ -1819,7 +1821,13 @@ where None } }) - .collect() + .collect::>(); + + if !requests.is_empty() { + self.allowed_requests.take(); + } + + requests } /// Get a state request scheduled by sync to be sent out (if any). From b6d1217e4678d1c67d38325372017e93daadc36e Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Thu, 19 Sep 2024 22:00:42 +0800 Subject: [PATCH 2/4] Add comment --- substrate/client/network/sync/src/strategy/chain_sync.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/substrate/client/network/sync/src/strategy/chain_sync.rs b/substrate/client/network/sync/src/strategy/chain_sync.rs index ac5fd35c01d2..b0e28d00f64a 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -1823,6 +1823,8 @@ where }) .collect::>(); + // Clear the allowed_requests state when sending new block requests + // to prevent multiple inflight block requests from being issued. if !requests.is_empty() { self.allowed_requests.take(); } From 65d9c4ff139e6670e244c45b34c24a930d29db3a Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Thu, 19 Sep 2024 22:11:34 +0800 Subject: [PATCH 3/4] Add prdoc --- prdoc/pr_5774.prdoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 prdoc/pr_5774.prdoc diff --git a/prdoc/pr_5774.prdoc b/prdoc/pr_5774.prdoc new file mode 100644 index 000000000000..f81eb768df81 --- /dev/null +++ b/prdoc/pr_5774.prdoc @@ -0,0 +1,13 @@ +title: Avoid unnecessary state reset of allowed_requests when no block requests are sent + +doc: + - audience: Node Dev + description: | + Previously, the state of `allowed_requests` will always be reset to the default value + even if there are no new block requests in the end. This could cause an edge cause that + `peer_block_request()` will early return next time when no ongoing block requests in fact. + This patch fixes it by checking whether block requests are empty before updating the state. + +crates: + - name: sc-network-sync + bump: patch From 7088a4ddf1241b6fee40fac5b36b8064b7bffb78 Mon Sep 17 00:00:00 2001 From: Liu-Cheng Xu Date: Thu, 19 Sep 2024 23:05:09 +0800 Subject: [PATCH 4/4] Update prdoc/pr_5774.prdoc Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> --- prdoc/pr_5774.prdoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/prdoc/pr_5774.prdoc b/prdoc/pr_5774.prdoc index f81eb768df81..15aa64f54104 100644 --- a/prdoc/pr_5774.prdoc +++ b/prdoc/pr_5774.prdoc @@ -3,9 +3,9 @@ title: Avoid unnecessary state reset of allowed_requests when no block requests doc: - audience: Node Dev description: | - Previously, the state of `allowed_requests` will always be reset to the default value - even if there are no new block requests in the end. This could cause an edge cause that - `peer_block_request()` will early return next time when no ongoing block requests in fact. + Previously, the state of `allowed_requests` was always reset to the default + even if there were no new block requests. This could cause an edge case + because `peer_block_request()` will return early next time when there are no ongoing block requests. This patch fixes it by checking whether block requests are empty before updating the state. crates: