From 04c613aebeeccc3cb0726b8a545b0f0b57f53a34 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 17:27:30 +0300 Subject: [PATCH 01/24] strategy: Introduce LRU persistent disconnection state Signed-off-by: Alexandru Vasile --- substrate/client/network/sync/src/strategy.rs | 1 + .../src/strategy/persistent_peer_state.rs | 111 ++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 substrate/client/network/sync/src/strategy/persistent_peer_state.rs diff --git a/substrate/client/network/sync/src/strategy.rs b/substrate/client/network/sync/src/strategy.rs index b7afcbdb3a78..ba38b84fd75e 100644 --- a/substrate/client/network/sync/src/strategy.rs +++ b/substrate/client/network/sync/src/strategy.rs @@ -20,6 +20,7 @@ //! and specific syncing algorithms. pub mod chain_sync; +mod persistent_peer_state; mod state; pub mod state_sync; pub mod warp; diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs new file mode 100644 index 000000000000..f2d22f7be291 --- /dev/null +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -0,0 +1,111 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +use std::time::Duration; + +use sc_network_types::PeerId; +use schnellru::{ByLength, LruMap}; + +/// The maximum number of disconnected peers to keep track of. +/// +/// When a peer disconnects, we must keep track if it was in the middle of a request. +/// The peer may disconnect because it cannot keep up with the number of requests +/// (ie not having enough resources available to handle the requests); or because it is malicious. +const MAX_DISCONNECTED_PEERS_STATE: u32 = 512; + +/// The time we are going to backoff a peer that has disconnected with an inflight request. +/// +/// The backoff time is calculated as `num_disconnects * DISCONNECTED_PEER_BACKOFF_SECONDS`. +/// This is to prevent submitting a request to a peer that has disconnected because it could not +/// keep up with the number of requests. +/// +/// The peer will disconnect due to the keep-alive timeout, however disconnections without +/// an inflight request are not tracked. +const DISCONNECTED_PEER_BACKOFF_SECONDS: u64 = 30; + +/// Maximum number of disconnects with a request in flight before a peer is banned. +const MAX_NUM_DISCONNECTS: u64 = 3; + +pub struct DisconnectedPeerState { + /// The total number of disconnects. + num_disconnects: u64, + /// The time at the last disconnect. + last_disconnect: std::time::Instant, +} + +impl DisconnectedPeerState { + /// Create a new `DisconnectedPeerState`. + pub fn new() -> Self { + Self { num_disconnects: 1, last_disconnect: std::time::Instant::now() } + } + + /// Increment the number of disconnects. + pub fn increment(&mut self) { + self.num_disconnects = self.num_disconnects.saturating_add(1); + self.last_disconnect = std::time::Instant::now(); + } + + /// Get the number of disconnects. + pub fn num_disconnects(&self) -> u64 { + self.num_disconnects + } + + /// Get the time of the last disconnect. + pub fn last_disconnect(&self) -> std::time::Instant { + self.last_disconnect + } +} + +pub struct PersistentPeersState { + /// The state of disconnected peers. + disconnected_peers: LruMap, +} + +impl PersistentPeersState { + /// Create a new `PersistentPeersState`. + pub fn new() -> Self { + Self { disconnected_peers: LruMap::new(ByLength::new(MAX_DISCONNECTED_PEERS_STATE)) } + } + + /// Insert a new peer to the persistent state if not seen before, or update the state if seen. + /// + /// Returns true if the peer should be disconnected. + pub fn remove_peer(&mut self, peer: PeerId) -> bool { + if let Some(state) = self.disconnected_peers.get(&peer) { + state.increment(); + return state.num_disconnects() >= MAX_NUM_DISCONNECTS + } + + // First time we see this peer. + self.disconnected_peers.insert(peer, DisconnectedPeerState::new()); + false + } + + /// Check if a peer is available for queries. + pub fn is_peer_available(&mut self, peer_id: &PeerId) -> bool { + let Some(state) = self.disconnected_peers.get(peer_id) else { + return true; + }; + + let elapsed = state.last_disconnect().elapsed(); + let disconnected_backoff = + Duration::from_secs(DISCONNECTED_PEER_BACKOFF_SECONDS * state.num_disconnects); + + elapsed >= disconnected_backoff + } +} From edf3490e7697f20e8f1310474d625f90e91895a4 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 17:29:00 +0300 Subject: [PATCH 02/24] strategy/warp: Use persistent disconnection state Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/warp.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index 754f1f52bfd2..3fa850c01684 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -21,7 +21,7 @@ pub use sp_consensus_grandpa::{AuthorityList, SetId}; use crate::{ - strategy::chain_sync::validate_blocks, + strategy::{chain_sync::validate_blocks, persistent_peer_state::PersistentPeersState}, types::{BadPeer, SyncState, SyncStatus}, LOG_TARGET, }; @@ -240,6 +240,7 @@ pub struct WarpSync { total_proof_bytes: u64, total_state_bytes: u64, peers: HashMap>, + persistent_peers: PersistentPeersState, actions: Vec>, result: Option>, } @@ -264,6 +265,7 @@ where total_proof_bytes: 0, total_state_bytes: 0, peers: HashMap::new(), + persistent_peers: PersistentPeersState::new(), actions: vec![WarpSyncAction::Finished], result: None, } @@ -281,6 +283,7 @@ where total_proof_bytes: 0, total_state_bytes: 0, peers: HashMap::new(), + persistent_peers: PersistentPeersState::new(), actions: Vec::new(), result: None, } @@ -309,7 +312,11 @@ where /// Notify that a peer has disconnected. pub fn remove_peer(&mut self, peer_id: &PeerId) { - self.peers.remove(peer_id); + if let Some(state) = self.peers.remove(peer_id) { + if !state.state.is_available() { + self.persistent_peers.remove_peer(*peer_id); + } + } } /// Submit a validated block announcement. @@ -490,7 +497,10 @@ where // Find a random peer that is synced as much as peer majority and is above // `min_best_number`. for (peer_id, peer) in self.peers.iter_mut() { - if peer.state.is_available() && peer.best_number >= threshold { + if peer.state.is_available() && + peer.best_number >= threshold && + self.persistent_peers.is_peer_available(peer_id) + { peer.state = new_state; return Some(*peer_id) } From 79910252601ad5bf2b5eb0043e5049c107d88e56 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 17:29:30 +0300 Subject: [PATCH 03/24] strategy/state: Use persistent disconnection state Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/state.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index c21cb22e40bb..95e03d6c8b73 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -20,7 +20,10 @@ use crate::{ schema::v1::StateResponse, - strategy::state_sync::{ImportResult, StateSync, StateSyncProvider}, + strategy::{ + persistent_peer_state::PersistentPeersState, + state_sync::{ImportResult, StateSync, StateSyncProvider}, + }, types::{BadPeer, OpaqueStateRequest, OpaqueStateResponse, SyncState, SyncStatus}, LOG_TARGET, }; @@ -78,6 +81,7 @@ struct Peer { pub struct StateStrategy { state_sync: Box>, peers: HashMap>, + persistent_peers: PersistentPeersState, actions: Vec>, succeeded: bool, } @@ -109,6 +113,7 @@ impl StateStrategy { skip_proof, )), peers, + persistent_peers: PersistentPeersState::new(), actions: Vec::new(), succeeded: false, } @@ -140,7 +145,11 @@ impl StateStrategy { /// Notify that a peer has disconnected. pub fn remove_peer(&mut self, peer_id: &PeerId) { - self.peers.remove(peer_id); + if let Some(state) = self.peers.remove(peer_id) { + if !state.state.is_available() { + self.persistent_peers.remove_peer(*peer_id); + } + } } /// Submit a validated block announcement. @@ -305,7 +314,10 @@ impl StateStrategy { // Find a random peer that is synced as much as peer majority and is above // `min_best_number`. for (peer_id, peer) in self.peers.iter_mut() { - if peer.state.is_available() && peer.best_number >= threshold { + if peer.state.is_available() && + peer.best_number >= threshold && + self.persistent_peers.is_peer_available(peer_id) + { peer.state = new_state; return Some(*peer_id) } From 666373184ae6a150bb0a1f22fce95c85835a63a9 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 17:36:09 +0300 Subject: [PATCH 04/24] strategy/chain_sync: Use persistent disconnection state Signed-off-by: Alexandru Vasile --- .../network/sync/src/strategy/chain_sync.rs | 22 ++++++++++++++++--- 1 file changed, 19 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 fcda25907927..3baa0a9e1c56 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -33,6 +33,7 @@ use crate::{ justification_requests::ExtraRequests, schema::v1::StateResponse, strategy::{ + persistent_peer_state::PersistentPeersState, state_sync::{ImportResult, StateSync, StateSyncProvider}, warp::{WarpSyncPhase, WarpSyncProgress}, }, @@ -250,6 +251,7 @@ pub struct ChainSync { client: Arc, /// The active peers that we are using to sync and their PeerSync status peers: HashMap>, + persistent_peers: PersistentPeersState, /// A `BlockCollection` of blocks that are being downloaded from peers blocks: BlockCollection, /// The best block number in our queue of blocks to import @@ -378,6 +380,7 @@ where let mut sync = Self { client, peers: HashMap::new(), + persistent_peers: PersistentPeersState::new(), blocks: BlockCollection::new(), best_queued_hash: Default::default(), best_queued_number: Zero::zero(), @@ -1141,7 +1144,13 @@ where if let Some(gap_sync) = &mut self.gap_sync { gap_sync.blocks.clear_peer_download(peer_id) } - self.peers.remove(peer_id); + + if let Some(state) = self.peers.remove(peer_id) { + if !state.state.is_available() { + self.persistent_peers.remove_peer(*peer_id); + } + } + self.extra_justifications.peer_disconnected(peer_id); self.allowed_requests.set_all(); self.fork_targets.retain(|_, target| { @@ -1541,10 +1550,14 @@ where 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 persistent_peers = &mut self.persistent_peers; self.peers .iter_mut() .filter_map(move |(&id, peer)| { - if !peer.state.is_available() || !allowed_requests.contains(&id) { + if !peer.state.is_available() || + !allowed_requests.contains(&id) || + !persistent_peers.is_peer_available(&id) + { return None } @@ -1656,7 +1669,10 @@ where } for (id, peer) in self.peers.iter_mut() { - if peer.state.is_available() && peer.common_number >= sync.target_number() { + if peer.state.is_available() && + peer.common_number >= sync.target_number() && + self.persistent_peers.is_peer_available(&id) + { peer.state = PeerSyncState::DownloadingState; let request = sync.next_request(); trace!(target: LOG_TARGET, "New StateRequest for {}: {:?}", id, request); From a836a3e1375c1227e1bd232854aaa9d6013c351f Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 17:46:37 +0300 Subject: [PATCH 05/24] strategy/tests: Check behavior of persistent peers state Signed-off-by: Alexandru Vasile --- .../src/strategy/persistent_peer_state.rs | 35 +++++++++++++++++++ .../client/network/sync/src/strategy/state.rs | 1 + 2 files changed, 36 insertions(+) diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index f2d22f7be291..5a2e369cc494 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -109,3 +109,38 @@ impl PersistentPeersState { elapsed >= disconnected_backoff } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_persistent_peer_state() { + let mut state = PersistentPeersState::new(); + let peer = PeerId::random(); + + // Is not part of the disconnected peers yet. + assert_eq!(state.is_peer_available(&peer), true); + + assert_eq!(state.remove_peer(peer), false); + assert_eq!(state.remove_peer(peer), false); + assert_eq!(state.remove_peer(peer), true); + + // The peer is backed off. + assert_eq!(state.is_peer_available(&peer), false); + } + + #[test] + fn ensure_backoff_time() { + let mut state = PersistentPeersState::new(); + let peer = PeerId::random(); + + assert_eq!(state.remove_peer(peer), false); + assert_eq!(state.is_peer_available(&peer), false); + + // Wait until the backoff time has passed + std::thread::sleep(Duration::from_secs(DISCONNECTED_PEER_BACKOFF_SECONDS + 1)); + + assert_eq!(state.is_peer_available(&peer), true); + } +} diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index 95e03d6c8b73..2a55d80a69eb 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -133,6 +133,7 @@ impl StateStrategy { (peer_id, Peer { best_number, state: PeerState::Available }) }) .collect(), + persistent_peers: PersistentPeersState::new(), actions: Vec::new(), succeeded: false, } From 34667013a2a1827797cb5c6e770d2d40380a17ad Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 18:30:46 +0300 Subject: [PATCH 06/24] strategy: Forget peer state after backing off for more than 15 minutes Signed-off-by: Alexandru Vasile --- .../sync/src/strategy/persistent_peer_state.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index 5a2e369cc494..98acda22805d 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -16,8 +16,6 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -use std::time::Duration; - use sc_network_types::PeerId; use schnellru::{ByLength, LruMap}; @@ -41,6 +39,9 @@ const DISCONNECTED_PEER_BACKOFF_SECONDS: u64 = 30; /// Maximum number of disconnects with a request in flight before a peer is banned. const MAX_NUM_DISCONNECTS: u64 = 3; +/// Forget the persistent state after 15 minutes. +const FORGET_PERSISTENT_STATE_SECONDS: u64 = 900; + pub struct DisconnectedPeerState { /// The total number of disconnects. num_disconnects: u64, @@ -103,10 +104,12 @@ impl PersistentPeersState { }; let elapsed = state.last_disconnect().elapsed(); - let disconnected_backoff = - Duration::from_secs(DISCONNECTED_PEER_BACKOFF_SECONDS * state.num_disconnects); + if elapsed.as_secs() > FORGET_PERSISTENT_STATE_SECONDS { + self.disconnected_peers.remove(peer_id); + return true; + } - elapsed >= disconnected_backoff + elapsed.as_secs() >= DISCONNECTED_PEER_BACKOFF_SECONDS * state.num_disconnects } } From e6d2ee36a768b74d6420713bd3691482b53edd33 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 18:31:12 +0300 Subject: [PATCH 07/24] strategy/tests: Check backed off peers are not scheduled for requests Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/state.rs | 57 +++++++++++++++++++ .../client/network/sync/src/strategy/warp.rs | 57 +++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index 2a55d80a69eb..d4d3668911b2 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -373,6 +373,7 @@ mod test { use sc_block_builder::BlockBuilderBuilder; use sc_client_api::KeyValueStates; use sc_consensus::{ImportedAux, ImportedState}; + use sp_core::H256; use sp_runtime::traits::Zero; use substrate_test_runtime_client::{ runtime::{Block, Hash}, @@ -478,6 +479,62 @@ mod test { } } + #[test] + fn backedoff_number_peer_is_not_scheduled() { + let client = Arc::new(TestClientBuilder::new().set_no_genesis().build()); + let target_block = BlockBuilderBuilder::new(&*client) + .on_parent_block(client.chain_info().best_hash) + .with_parent_block_number(client.chain_info().best_number) + .build() + .unwrap() + .build() + .unwrap() + .block; + + let peers = (1..=10) + .map(|best_number| (PeerId::random(), best_number)) + .collect::>(); + let ninth_peer = peers[8].0.clone(); + let tenth_peer = peers[9].0.clone(); + let initial_peers = peers.iter().map(|(p, n)| (*p, *n)); + + let mut state_strategy = StateStrategy::new( + client.clone(), + target_block.header().clone(), + None, + None, + false, + initial_peers, + ); + + // Disconnecting a peer without an inflight request has no effect on persistent states. + state_strategy.remove_peer(&tenth_peer); + state_strategy.remove_peer(&tenth_peer); + state_strategy.remove_peer(&tenth_peer); + assert!(state_strategy.persistent_peers.is_peer_available(&tenth_peer)); + + // Disconnect the peer with an inflight request. + state_strategy.add_peer(tenth_peer.clone(), H256::random(), 10); + let peer_id: Option = + state_strategy.schedule_next_peer(PeerState::DownloadingState, 10); + assert_eq!(tenth_peer, peer_id.unwrap()); + state_strategy.remove_peer(&tenth_peer); + + // Peer is backed off. + assert!(!state_strategy.persistent_peers.is_peer_available(&tenth_peer)); + + // No peer available for 10'th best block because of the backoff. + state_strategy.add_peer(tenth_peer.clone(), H256::random(), 10); + let peer_id: Option = + state_strategy.schedule_next_peer(PeerState::DownloadingState, 10); + assert!(peer_id.is_none()); + + // Other requests can still happen. + let peer_id: Option = + state_strategy.schedule_next_peer(PeerState::DownloadingState, 9); + assert_eq!(ninth_peer, peer_id.unwrap()); + } + #[test] fn state_request_contains_correct_hash() { let client = Arc::new(TestClientBuilder::new().set_no_genesis().build()); diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index 3fa850c01684..bdf29bc565a3 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -660,6 +660,7 @@ mod test { use sc_block_builder::BlockBuilderBuilder; use sp_blockchain::{BlockStatus, Error as BlockchainError, HeaderBackend, Info}; use sp_consensus_grandpa::{AuthorityList, SetId}; + use sp_core::H256; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use std::{io::ErrorKind, sync::Arc}; use substrate_test_runtime_client::{ @@ -870,6 +871,62 @@ mod test { } } + #[test] + fn backedoff_number_peer_is_not_scheduled() { + let client = mock_client_without_state(); + let mut provider = MockWarpSyncProvider::::new(); + provider + .expect_current_authorities() + .once() + .return_const(AuthorityList::default()); + let config = WarpSyncConfig::WithProvider(Arc::new(provider)); + let mut warp_sync = WarpSync::new(Arc::new(client), config); + + for best_number in 1..11 { + warp_sync.add_peer(PeerId::random(), Hash::random(), best_number); + } + + let ninth_peer = warp_sync + .peers + .iter() + .find(|(_peer, state)| state.best_number == 9) + .unwrap() + .0 + .clone(); + let tenth_peer = warp_sync + .peers + .iter() + .find(|(_peer, state)| state.best_number == 10) + .unwrap() + .0 + .clone(); + + // Disconnecting a peer without an inflight request has no effect on persistent states. + warp_sync.remove_peer(&tenth_peer); + warp_sync.remove_peer(&tenth_peer); + warp_sync.remove_peer(&tenth_peer); + assert!(warp_sync.persistent_peers.is_peer_available(&tenth_peer)); + + warp_sync.add_peer(tenth_peer.clone(), H256::random(), 10); + let peer_id = warp_sync.schedule_next_peer(PeerState::DownloadingProofs, Some(10)); + assert_eq!(tenth_peer, peer_id.unwrap()); + warp_sync.remove_peer(&tenth_peer); + + // Peer is backed off. + assert!(!warp_sync.persistent_peers.is_peer_available(&tenth_peer)); + + // No peer available for 10'th best block because of the backoff. + warp_sync.add_peer(tenth_peer.clone(), H256::random(), 10); + let peer_id: Option = + warp_sync.schedule_next_peer(PeerState::DownloadingProofs, Some(10)); + assert!(peer_id.is_none()); + + // Other requests can still happen. + let peer_id: Option = + warp_sync.schedule_next_peer(PeerState::DownloadingProofs, Some(9)); + assert_eq!(ninth_peer, peer_id.unwrap()); + } + #[test] fn no_warp_proof_request_in_another_phase() { let client = mock_client_without_state(); From 79d18504d2e726328d4080b1ae97964ac750cd62 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Mon, 15 Jul 2024 18:32:26 +0300 Subject: [PATCH 08/24] strategy: Align backoff time with protocol timeout time Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/persistent_peer_state.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index 98acda22805d..42ec28c7dd9b 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -32,9 +32,9 @@ const MAX_DISCONNECTED_PEERS_STATE: u32 = 512; /// This is to prevent submitting a request to a peer that has disconnected because it could not /// keep up with the number of requests. /// -/// The peer will disconnect due to the keep-alive timeout, however disconnections without +/// The peer may disconnect due to the keep-alive timeout, however disconnections without /// an inflight request are not tracked. -const DISCONNECTED_PEER_BACKOFF_SECONDS: u64 = 30; +const DISCONNECTED_PEER_BACKOFF_SECONDS: u64 = 20; /// Maximum number of disconnects with a request in flight before a peer is banned. const MAX_NUM_DISCONNECTS: u64 = 3; From 3e5152596dd6ac51b13124a2d029f9ee21a2b5ab Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 16 Jul 2024 13:29:50 +0300 Subject: [PATCH 09/24] strategy/persistent: Adjust constants, report bans and add logs Signed-off-by: Alexandru Vasile --- .../src/strategy/persistent_peer_state.rs | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index 42ec28c7dd9b..09deb9af1adc 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -16,6 +16,8 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . +use crate::types::BadPeer; +use sc_network::ReputationChange as Rep; use sc_network_types::PeerId; use schnellru::{ByLength, LruMap}; @@ -34,14 +36,15 @@ const MAX_DISCONNECTED_PEERS_STATE: u32 = 512; /// /// The peer may disconnect due to the keep-alive timeout, however disconnections without /// an inflight request are not tracked. -const DISCONNECTED_PEER_BACKOFF_SECONDS: u64 = 20; +const DISCONNECTED_PEER_BACKOFF_SECONDS: u64 = 60; /// Maximum number of disconnects with a request in flight before a peer is banned. const MAX_NUM_DISCONNECTS: u64 = 3; -/// Forget the persistent state after 15 minutes. -const FORGET_PERSISTENT_STATE_SECONDS: u64 = 900; +/// Peer is slow to respond. +pub const SLOW_PEER: Rep = Rep::new_fatal("Slow peer after backoffs"); +#[derive(Debug)] pub struct DisconnectedPeerState { /// The total number of disconnects. num_disconnects: u64, @@ -86,20 +89,44 @@ impl PersistentPeersState { /// Insert a new peer to the persistent state if not seen before, or update the state if seen. /// /// Returns true if the peer should be disconnected. - pub fn remove_peer(&mut self, peer: PeerId) -> bool { + pub fn remove_peer(&mut self, peer: PeerId) -> Option { if let Some(state) = self.disconnected_peers.get(&peer) { state.increment(); - return state.num_disconnects() >= MAX_NUM_DISCONNECTS + + let should_ban = state.num_disconnects() >= MAX_NUM_DISCONNECTS; + log::debug!( + target: LOG_TARGET, + "Remove known peer {peer} state: {state:?}, should ban: {should_ban}", + ); + + return should_ban.then(|| { + // The peer is banned from the peerstore. We can lose track of the peer state + // and let the banning mechanism handle the peer backoff. + // + // After the peer banning expires, if the peer continues to misbehave, it will be + // backed off again. + self.disconnected_peers.remove(&peer); + BadPeer(peer, SLOW_PEER) + }) } + log::debug!( + target: LOG_TARGET, + "Added peer {peer} for the first time" + ); // First time we see this peer. self.disconnected_peers.insert(peer, DisconnectedPeerState::new()); - false + None } /// Check if a peer is available for queries. pub fn is_peer_available(&mut self, peer_id: &PeerId) -> bool { let Some(state) = self.disconnected_peers.get(peer_id) else { + log::debug!( + target: LOG_TARGET, + "Peer {peer_id} is not in the disconnected peers state", + ); + return true; }; From 62f405424914108c84eed9f192ec75c3ed5784bd Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 16 Jul 2024 13:27:37 +0300 Subject: [PATCH 10/24] strategy: Ban peers reported by the persistent store Signed-off-by: Alexandru Vasile --- .../network/sync/src/strategy/chain_sync.rs | 4 +++- .../sync/src/strategy/persistent_peer_state.rs | 18 +++++++++--------- .../client/network/sync/src/strategy/state.rs | 4 +++- .../client/network/sync/src/strategy/warp.rs | 4 +++- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/chain_sync.rs b/substrate/client/network/sync/src/strategy/chain_sync.rs index 3baa0a9e1c56..41c11774457e 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -1147,7 +1147,9 @@ where if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - self.persistent_peers.remove_peer(*peer_id); + if let Some(bad_peer) = self.persistent_peers.remove_peer(*peer_id) { + self.actions.push(ChainSyncAction::DropPeer(bad_peer)); + } } } diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index 09deb9af1adc..e0aaf4404326 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -21,6 +21,8 @@ use sc_network::ReputationChange as Rep; use sc_network_types::PeerId; use schnellru::{ByLength, LruMap}; +const LOG_TARGET: &str = "sync::persistent_peer_state"; + /// The maximum number of disconnected peers to keep track of. /// /// When a peer disconnects, we must keep track if it was in the middle of a request. @@ -122,21 +124,19 @@ impl PersistentPeersState { /// Check if a peer is available for queries. pub fn is_peer_available(&mut self, peer_id: &PeerId) -> bool { let Some(state) = self.disconnected_peers.get(peer_id) else { - log::debug!( - target: LOG_TARGET, - "Peer {peer_id} is not in the disconnected peers state", - ); - + log::debug!(target: LOG_TARGET,"Peer {peer_id} is not in the disconnected peers state"); return true; }; let elapsed = state.last_disconnect().elapsed(); - if elapsed.as_secs() > FORGET_PERSISTENT_STATE_SECONDS { + if elapsed.as_secs() >= DISCONNECTED_PEER_BACKOFF_SECONDS * state.num_disconnects { + log::debug!(target: LOG_TARGET, "Peer {peer_id} is available for queries"); self.disconnected_peers.remove(peer_id); - return true; + true + } else { + log::debug!(target: LOG_TARGET,"Peer {peer_id} is backedoff"); + false } - - elapsed.as_secs() >= DISCONNECTED_PEER_BACKOFF_SECONDS * state.num_disconnects } } diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index d4d3668911b2..a4fbae5714da 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -148,7 +148,9 @@ impl StateStrategy { pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - self.persistent_peers.remove_peer(*peer_id); + if let Some(bad_peer) = self.persistent_peers.remove_peer(*peer_id) { + self.actions.push(StateStrategyAction::DropPeer(bad_peer)); + } } } } diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index bdf29bc565a3..f120ec16e027 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -314,7 +314,9 @@ where pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - self.persistent_peers.remove_peer(*peer_id); + if let Some(bad_peer) = self.persistent_peers.remove_peer(*peer_id) { + self.actions.push(WarpSyncAction::DropPeer(bad_peer)); + } } } } From 02c26e0aa4b15d7cd054a2aee82f2795fed7ee61 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 16 Jul 2024 14:46:18 +0300 Subject: [PATCH 11/24] sync/tests: Fix clippy Signed-off-by: Alexandru Vasile --- .../network/sync/src/strategy/persistent_peer_state.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index e0aaf4404326..e7f6b3032572 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -143,6 +143,7 @@ impl PersistentPeersState { #[cfg(test)] mod tests { use super::*; + use std::time::Duration; #[test] fn test_persistent_peer_state() { @@ -152,9 +153,9 @@ mod tests { // Is not part of the disconnected peers yet. assert_eq!(state.is_peer_available(&peer), true); - assert_eq!(state.remove_peer(peer), false); - assert_eq!(state.remove_peer(peer), false); - assert_eq!(state.remove_peer(peer), true); + assert!(state.remove_peer(peer).is_none()); + assert!(state.remove_peer(peer).is_none()); + assert!(state.remove_peer(peer).is_some()); // The peer is backed off. assert_eq!(state.is_peer_available(&peer), false); @@ -165,7 +166,7 @@ mod tests { let mut state = PersistentPeersState::new(); let peer = PeerId::random(); - assert_eq!(state.remove_peer(peer), false); + assert!(state.remove_peer(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); // Wait until the backoff time has passed From a6e76aabdd5d0591e88a7641aa7186132eb76c3d Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 16 Jul 2024 19:44:15 +0300 Subject: [PATCH 12/24] sync/tests: Remove clone Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/state.rs | 8 ++++---- .../client/network/sync/src/strategy/warp.rs | 18 ++++++------------ 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index a4fbae5714da..2af849c2ea36 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -496,8 +496,8 @@ mod test { let peers = (1..=10) .map(|best_number| (PeerId::random(), best_number)) .collect::>(); - let ninth_peer = peers[8].0.clone(); - let tenth_peer = peers[9].0.clone(); + let ninth_peer = peers[8].0; + let tenth_peer = peers[9].0; let initial_peers = peers.iter().map(|(p, n)| (*p, *n)); let mut state_strategy = StateStrategy::new( @@ -516,7 +516,7 @@ mod test { assert!(state_strategy.persistent_peers.is_peer_available(&tenth_peer)); // Disconnect the peer with an inflight request. - state_strategy.add_peer(tenth_peer.clone(), H256::random(), 10); + state_strategy.add_peer(tenth_peer, H256::random(), 10); let peer_id: Option = state_strategy.schedule_next_peer(PeerState::DownloadingState, 10); assert_eq!(tenth_peer, peer_id.unwrap()); @@ -526,7 +526,7 @@ mod test { assert!(!state_strategy.persistent_peers.is_peer_available(&tenth_peer)); // No peer available for 10'th best block because of the backoff. - state_strategy.add_peer(tenth_peer.clone(), H256::random(), 10); + state_strategy.add_peer(tenth_peer, H256::random(), 10); let peer_id: Option = state_strategy.schedule_next_peer(PeerState::DownloadingState, 10); assert!(peer_id.is_none()); diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index f120ec16e027..afac5a31f68e 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -888,20 +888,14 @@ mod test { warp_sync.add_peer(PeerId::random(), Hash::random(), best_number); } - let ninth_peer = warp_sync - .peers - .iter() - .find(|(_peer, state)| state.best_number == 9) - .unwrap() - .0 - .clone(); + let ninth_peer = + warp_sync.peers.iter().find(|(_, state)| state.best_number == 9).unwrap().0; let tenth_peer = warp_sync .peers .iter() - .find(|(_peer, state)| state.best_number == 10) + .find(|(_ state)| state.best_number == 10) .unwrap() - .0 - .clone(); + .0; // Disconnecting a peer without an inflight request has no effect on persistent states. warp_sync.remove_peer(&tenth_peer); @@ -909,7 +903,7 @@ mod test { warp_sync.remove_peer(&tenth_peer); assert!(warp_sync.persistent_peers.is_peer_available(&tenth_peer)); - warp_sync.add_peer(tenth_peer.clone(), H256::random(), 10); + warp_sync.add_peer(tenth_peer, H256::random(), 10); let peer_id = warp_sync.schedule_next_peer(PeerState::DownloadingProofs, Some(10)); assert_eq!(tenth_peer, peer_id.unwrap()); warp_sync.remove_peer(&tenth_peer); @@ -918,7 +912,7 @@ mod test { assert!(!warp_sync.persistent_peers.is_peer_available(&tenth_peer)); // No peer available for 10'th best block because of the backoff. - warp_sync.add_peer(tenth_peer.clone(), H256::random(), 10); + warp_sync.add_peer(tenth_peer, H256::random(), 10); let peer_id: Option = warp_sync.schedule_next_peer(PeerState::DownloadingProofs, Some(10)); assert!(peer_id.is_none()); From 2ac8fcd17760e3a8d00fb886815b7cdbbcbca6be Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 16 Jul 2024 19:44:43 +0300 Subject: [PATCH 13/24] sync/tests: Cargo fmt Signed-off-by: Alexandru Vasile --- substrate/client/network/sync/src/strategy/warp.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index afac5a31f68e..9a92e35c48b4 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -890,12 +890,8 @@ mod test { let ninth_peer = warp_sync.peers.iter().find(|(_, state)| state.best_number == 9).unwrap().0; - let tenth_peer = warp_sync - .peers - .iter() - .find(|(_ state)| state.best_number == 10) - .unwrap() - .0; + let tenth_peer = + warp_sync.peers.iter().find(|(_, state)| state.best_number == 10).unwrap().0; // Disconnecting a peer without an inflight request has no effect on persistent states. warp_sync.remove_peer(&tenth_peer); From 2b7fabab7feba1950d26bc3f026d4fe6c8f89a42 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 17 Jul 2024 10:46:27 +0300 Subject: [PATCH 14/24] sync/tests: Fix clippy Signed-off-by: Alexandru Vasile --- substrate/client/network/sync/src/strategy/warp.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index 9a92e35c48b4..9398750eb2ba 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -889,9 +889,9 @@ mod test { } let ninth_peer = - warp_sync.peers.iter().find(|(_, state)| state.best_number == 9).unwrap().0; + *warp_sync.peers.iter().find(|(_, state)| state.best_number == 9).unwrap().0; let tenth_peer = - warp_sync.peers.iter().find(|(_, state)| state.best_number == 10).unwrap().0; + *warp_sync.peers.iter().find(|(_, state)| state.best_number == 10).unwrap().0; // Disconnecting a peer without an inflight request has no effect on persistent states. warp_sync.remove_peer(&tenth_peer); From 177dec9f54051afe311819d09a5efcec0479e783 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 17 Jul 2024 12:25:53 +0300 Subject: [PATCH 15/24] sync/tests: Adjust state management expectations Signed-off-by: Alexandru Vasile --- .../network/sync/src/strategy/persistent_peer_state.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index e7f6b3032572..973a73b740a1 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -154,11 +154,15 @@ mod tests { assert_eq!(state.is_peer_available(&peer), true); assert!(state.remove_peer(peer).is_none()); - assert!(state.remove_peer(peer).is_none()); - assert!(state.remove_peer(peer).is_some()); + assert_eq!(state.is_peer_available(&peer), false); - // The peer is backed off. + assert!(state.remove_peer(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); + + assert!(state.remove_peer(peer).is_some()); + // Peer is supposed to get banned and disconnected. + // The state ownership moves to the PeerStore. + assert!(state.disconnected_peers.get(&peer).is_none()); } #[test] From 6efa4d1f24c91be24445e11bde22a9c706bd7f50 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 30 Jul 2024 16:06:12 +0300 Subject: [PATCH 16/24] peer-state: Rename disconnected peer state and make it private Signed-off-by: Alexandru Vasile --- .../network/sync/src/strategy/persistent_peer_state.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs index 973a73b740a1..a865e0e119af 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/persistent_peer_state.rs @@ -47,15 +47,15 @@ const MAX_NUM_DISCONNECTS: u64 = 3; pub const SLOW_PEER: Rep = Rep::new_fatal("Slow peer after backoffs"); #[derive(Debug)] -pub struct DisconnectedPeerState { +struct DisconnectedState { /// The total number of disconnects. num_disconnects: u64, /// The time at the last disconnect. last_disconnect: std::time::Instant, } -impl DisconnectedPeerState { - /// Create a new `DisconnectedPeerState`. +impl DisconnectedState { + /// Create a new `DisconnectedState`. pub fn new() -> Self { Self { num_disconnects: 1, last_disconnect: std::time::Instant::now() } } @@ -79,7 +79,7 @@ impl DisconnectedPeerState { pub struct PersistentPeersState { /// The state of disconnected peers. - disconnected_peers: LruMap, + disconnected_peers: LruMap, } impl PersistentPeersState { @@ -117,7 +117,7 @@ impl PersistentPeersState { "Added peer {peer} for the first time" ); // First time we see this peer. - self.disconnected_peers.insert(peer, DisconnectedPeerState::new()); + self.disconnected_peers.insert(peer, DisconnectedState::new()); None } From 48534c2feb3e5b235e0007c2793c247b06f93f6c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 30 Jul 2024 16:21:38 +0300 Subject: [PATCH 17/24] peer-state: Rename to disconnected peers for clarity Signed-off-by: Alexandru Vasile --- substrate/client/network/sync/src/strategy.rs | 2 +- .../network/sync/src/strategy/chain_sync.rs | 14 ++++----- ...nt_peer_state.rs => disconnected_peers.rs} | 29 ++++++++++++------- .../client/network/sync/src/strategy/state.rs | 16 +++++----- .../client/network/sync/src/strategy/warp.rs | 16 +++++----- 5 files changed, 42 insertions(+), 35 deletions(-) rename substrate/client/network/sync/src/strategy/{persistent_peer_state.rs => disconnected_peers.rs} (85%) diff --git a/substrate/client/network/sync/src/strategy.rs b/substrate/client/network/sync/src/strategy.rs index ba38b84fd75e..72f84d1c286e 100644 --- a/substrate/client/network/sync/src/strategy.rs +++ b/substrate/client/network/sync/src/strategy.rs @@ -20,7 +20,7 @@ //! and specific syncing algorithms. pub mod chain_sync; -mod persistent_peer_state; +mod disconnected_peers; mod state; pub mod state_sync; pub mod warp; diff --git a/substrate/client/network/sync/src/strategy/chain_sync.rs b/substrate/client/network/sync/src/strategy/chain_sync.rs index 41c11774457e..ad468d8d479f 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -33,7 +33,7 @@ use crate::{ justification_requests::ExtraRequests, schema::v1::StateResponse, strategy::{ - persistent_peer_state::PersistentPeersState, + disconnected_peers::DisconnectedPeers, state_sync::{ImportResult, StateSync, StateSyncProvider}, warp::{WarpSyncPhase, WarpSyncProgress}, }, @@ -251,7 +251,7 @@ pub struct ChainSync { client: Arc, /// The active peers that we are using to sync and their PeerSync status peers: HashMap>, - persistent_peers: PersistentPeersState, + disconnected_peers: DisconnectedPeers, /// A `BlockCollection` of blocks that are being downloaded from peers blocks: BlockCollection, /// The best block number in our queue of blocks to import @@ -380,7 +380,7 @@ where let mut sync = Self { client, peers: HashMap::new(), - persistent_peers: PersistentPeersState::new(), + disconnected_peers: DisconnectedPeers::new(), blocks: BlockCollection::new(), best_queued_hash: Default::default(), best_queued_number: Zero::zero(), @@ -1147,7 +1147,7 @@ where if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.persistent_peers.remove_peer(*peer_id) { + if let Some(bad_peer) = self.disconnected_peers.remove_peer(*peer_id) { self.actions.push(ChainSyncAction::DropPeer(bad_peer)); } } @@ -1552,13 +1552,13 @@ where 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 persistent_peers = &mut self.persistent_peers; + let disconnected_peers = &mut self.disconnected_peers; self.peers .iter_mut() .filter_map(move |(&id, peer)| { if !peer.state.is_available() || !allowed_requests.contains(&id) || - !persistent_peers.is_peer_available(&id) + !disconnected_peers.is_peer_available(&id) { return None } @@ -1673,7 +1673,7 @@ where for (id, peer) in self.peers.iter_mut() { if peer.state.is_available() && peer.common_number >= sync.target_number() && - self.persistent_peers.is_peer_available(&id) + self.disconnected_peers.is_peer_available(&id) { peer.state = PeerSyncState::DownloadingState; let request = sync.next_request(); diff --git a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs b/substrate/client/network/sync/src/strategy/disconnected_peers.rs similarity index 85% rename from substrate/client/network/sync/src/strategy/persistent_peer_state.rs rename to substrate/client/network/sync/src/strategy/disconnected_peers.rs index a865e0e119af..2a9d0836a4d6 100644 --- a/substrate/client/network/sync/src/strategy/persistent_peer_state.rs +++ b/substrate/client/network/sync/src/strategy/disconnected_peers.rs @@ -43,9 +43,13 @@ const DISCONNECTED_PEER_BACKOFF_SECONDS: u64 = 60; /// Maximum number of disconnects with a request in flight before a peer is banned. const MAX_NUM_DISCONNECTS: u64 = 3; -/// Peer is slow to respond. -pub const SLOW_PEER: Rep = Rep::new_fatal("Slow peer after backoffs"); +/// Peer disconnected with a request in flight after backoffs. +/// +/// The peer may be slow to respond to the request after backoffs, or it refuses to respond. +/// Report the peer and let the reputation system handle disconnecting the peer. +pub const REPUTATION_REPORT: Rep = Rep::new_fatal("Peer disconnected with inflight after backoffs"); +/// The state of a disconnected peer with a request in flight. #[derive(Debug)] struct DisconnectedState { /// The total number of disconnects. @@ -77,13 +81,17 @@ impl DisconnectedState { } } -pub struct PersistentPeersState { +/// Tracks the state of disconnected peers with a request in flight. +/// +/// This helps to prevent submitting requests to peers that have disconnected +/// before responding to the request to offload the peer. +pub struct DisconnectedPeers { /// The state of disconnected peers. disconnected_peers: LruMap, } -impl PersistentPeersState { - /// Create a new `PersistentPeersState`. +impl DisconnectedPeers { + /// Create a new `DisconnectedPeers`. pub fn new() -> Self { Self { disconnected_peers: LruMap::new(ByLength::new(MAX_DISCONNECTED_PEERS_STATE)) } } @@ -102,13 +110,13 @@ impl PersistentPeersState { ); return should_ban.then(|| { - // The peer is banned from the peerstore. We can lose track of the peer state - // and let the banning mechanism handle the peer backoff. + // We can lose track of the peer state and let the banning mechanism handle + // the peer backoff. // // After the peer banning expires, if the peer continues to misbehave, it will be // backed off again. self.disconnected_peers.remove(&peer); - BadPeer(peer, SLOW_PEER) + BadPeer(peer, REPUTATION_REPORT) }) } @@ -124,7 +132,6 @@ impl PersistentPeersState { /// Check if a peer is available for queries. pub fn is_peer_available(&mut self, peer_id: &PeerId) -> bool { let Some(state) = self.disconnected_peers.get(peer_id) else { - log::debug!(target: LOG_TARGET,"Peer {peer_id} is not in the disconnected peers state"); return true; }; @@ -147,7 +154,7 @@ mod tests { #[test] fn test_persistent_peer_state() { - let mut state = PersistentPeersState::new(); + let mut state = DisconnectedPeers::new(); let peer = PeerId::random(); // Is not part of the disconnected peers yet. @@ -167,7 +174,7 @@ mod tests { #[test] fn ensure_backoff_time() { - let mut state = PersistentPeersState::new(); + let mut state = DisconnectedPeers::new(); let peer = PeerId::random(); assert!(state.remove_peer(peer).is_none()); diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index 2af849c2ea36..88d54d631b26 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -21,7 +21,7 @@ use crate::{ schema::v1::StateResponse, strategy::{ - persistent_peer_state::PersistentPeersState, + disconnected_peers::DisconnectedPeers, state_sync::{ImportResult, StateSync, StateSyncProvider}, }, types::{BadPeer, OpaqueStateRequest, OpaqueStateResponse, SyncState, SyncStatus}, @@ -81,7 +81,7 @@ struct Peer { pub struct StateStrategy { state_sync: Box>, peers: HashMap>, - persistent_peers: PersistentPeersState, + disconnected_peers: DisconnectedPeers, actions: Vec>, succeeded: bool, } @@ -113,7 +113,7 @@ impl StateStrategy { skip_proof, )), peers, - persistent_peers: PersistentPeersState::new(), + disconnected_peers: DisconnectedPeers::new(), actions: Vec::new(), succeeded: false, } @@ -133,7 +133,7 @@ impl StateStrategy { (peer_id, Peer { best_number, state: PeerState::Available }) }) .collect(), - persistent_peers: PersistentPeersState::new(), + disconnected_peers: DisconnectedPeers::new(), actions: Vec::new(), succeeded: false, } @@ -148,7 +148,7 @@ impl StateStrategy { pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.persistent_peers.remove_peer(*peer_id) { + if let Some(bad_peer) = self.disconnected_peers.remove_peer(*peer_id) { self.actions.push(StateStrategyAction::DropPeer(bad_peer)); } } @@ -319,7 +319,7 @@ impl StateStrategy { for (peer_id, peer) in self.peers.iter_mut() { if peer.state.is_available() && peer.best_number >= threshold && - self.persistent_peers.is_peer_available(peer_id) + self.disconnected_peers.is_peer_available(peer_id) { peer.state = new_state; return Some(*peer_id) @@ -513,7 +513,7 @@ mod test { state_strategy.remove_peer(&tenth_peer); state_strategy.remove_peer(&tenth_peer); state_strategy.remove_peer(&tenth_peer); - assert!(state_strategy.persistent_peers.is_peer_available(&tenth_peer)); + assert!(state_strategy.disconnected_peers.is_peer_available(&tenth_peer)); // Disconnect the peer with an inflight request. state_strategy.add_peer(tenth_peer, H256::random(), 10); @@ -523,7 +523,7 @@ mod test { state_strategy.remove_peer(&tenth_peer); // Peer is backed off. - assert!(!state_strategy.persistent_peers.is_peer_available(&tenth_peer)); + assert!(!state_strategy.disconnected_peers.is_peer_available(&tenth_peer)); // No peer available for 10'th best block because of the backoff. state_strategy.add_peer(tenth_peer, H256::random(), 10); diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index 9398750eb2ba..f898215bb31e 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -21,7 +21,7 @@ pub use sp_consensus_grandpa::{AuthorityList, SetId}; use crate::{ - strategy::{chain_sync::validate_blocks, persistent_peer_state::PersistentPeersState}, + strategy::{chain_sync::validate_blocks, disconnected_peers::DisconnectedPeers}, types::{BadPeer, SyncState, SyncStatus}, LOG_TARGET, }; @@ -240,7 +240,7 @@ pub struct WarpSync { total_proof_bytes: u64, total_state_bytes: u64, peers: HashMap>, - persistent_peers: PersistentPeersState, + disconnected_peers: DisconnectedPeers, actions: Vec>, result: Option>, } @@ -265,7 +265,7 @@ where total_proof_bytes: 0, total_state_bytes: 0, peers: HashMap::new(), - persistent_peers: PersistentPeersState::new(), + disconnected_peers: DisconnectedPeers::new(), actions: vec![WarpSyncAction::Finished], result: None, } @@ -283,7 +283,7 @@ where total_proof_bytes: 0, total_state_bytes: 0, peers: HashMap::new(), - persistent_peers: PersistentPeersState::new(), + disconnected_peers: DisconnectedPeers::new(), actions: Vec::new(), result: None, } @@ -314,7 +314,7 @@ where pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.persistent_peers.remove_peer(*peer_id) { + if let Some(bad_peer) = self.disconnected_peers.remove_peer(*peer_id) { self.actions.push(WarpSyncAction::DropPeer(bad_peer)); } } @@ -501,7 +501,7 @@ where for (peer_id, peer) in self.peers.iter_mut() { if peer.state.is_available() && peer.best_number >= threshold && - self.persistent_peers.is_peer_available(peer_id) + self.disconnected_peers.is_peer_available(peer_id) { peer.state = new_state; return Some(*peer_id) @@ -897,7 +897,7 @@ mod test { warp_sync.remove_peer(&tenth_peer); warp_sync.remove_peer(&tenth_peer); warp_sync.remove_peer(&tenth_peer); - assert!(warp_sync.persistent_peers.is_peer_available(&tenth_peer)); + assert!(warp_sync.disconnected_peers.is_peer_available(&tenth_peer)); warp_sync.add_peer(tenth_peer, H256::random(), 10); let peer_id = warp_sync.schedule_next_peer(PeerState::DownloadingProofs, Some(10)); @@ -905,7 +905,7 @@ mod test { warp_sync.remove_peer(&tenth_peer); // Peer is backed off. - assert!(!warp_sync.persistent_peers.is_peer_available(&tenth_peer)); + assert!(!warp_sync.disconnected_peers.is_peer_available(&tenth_peer)); // No peer available for 10'th best block because of the backoff. warp_sync.add_peer(tenth_peer, H256::random(), 10); From 0a47d68adf83d91a879e7f81a4bea4a787c07305 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 30 Jul 2024 16:23:31 +0300 Subject: [PATCH 18/24] peer-state: Rename remove to on_disconnect Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/chain_sync.rs | 2 +- .../network/sync/src/strategy/disconnected_peers.rs | 10 +++++----- substrate/client/network/sync/src/strategy/state.rs | 2 +- substrate/client/network/sync/src/strategy/warp.rs | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/chain_sync.rs b/substrate/client/network/sync/src/strategy/chain_sync.rs index ad468d8d479f..7bf9ce0460d4 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -1147,7 +1147,7 @@ where if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.disconnected_peers.remove_peer(*peer_id) { + if let Some(bad_peer) = self.disconnected_peers.on_disconnect(*peer_id) { self.actions.push(ChainSyncAction::DropPeer(bad_peer)); } } diff --git a/substrate/client/network/sync/src/strategy/disconnected_peers.rs b/substrate/client/network/sync/src/strategy/disconnected_peers.rs index 2a9d0836a4d6..df5dc9919a83 100644 --- a/substrate/client/network/sync/src/strategy/disconnected_peers.rs +++ b/substrate/client/network/sync/src/strategy/disconnected_peers.rs @@ -99,7 +99,7 @@ impl DisconnectedPeers { /// Insert a new peer to the persistent state if not seen before, or update the state if seen. /// /// Returns true if the peer should be disconnected. - pub fn remove_peer(&mut self, peer: PeerId) -> Option { + pub fn on_disconnect(&mut self, peer: PeerId) -> Option { if let Some(state) = self.disconnected_peers.get(&peer) { state.increment(); @@ -160,13 +160,13 @@ mod tests { // Is not part of the disconnected peers yet. assert_eq!(state.is_peer_available(&peer), true); - assert!(state.remove_peer(peer).is_none()); + assert!(state.on_disconnect(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); - assert!(state.remove_peer(peer).is_none()); + assert!(state.on_disconnect(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); - assert!(state.remove_peer(peer).is_some()); + assert!(state.on_disconnect(peer).is_some()); // Peer is supposed to get banned and disconnected. // The state ownership moves to the PeerStore. assert!(state.disconnected_peers.get(&peer).is_none()); @@ -177,7 +177,7 @@ mod tests { let mut state = DisconnectedPeers::new(); let peer = PeerId::random(); - assert!(state.remove_peer(peer).is_none()); + assert!(state.on_disconnect(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); // Wait until the backoff time has passed diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index 88d54d631b26..eccea8f04aea 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -148,7 +148,7 @@ impl StateStrategy { pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.disconnected_peers.remove_peer(*peer_id) { + if let Some(bad_peer) = self.disconnected_peers.on_disconnect(*peer_id) { self.actions.push(StateStrategyAction::DropPeer(bad_peer)); } } diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index f898215bb31e..498bd6445175 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -314,7 +314,7 @@ where pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.disconnected_peers.remove_peer(*peer_id) { + if let Some(bad_peer) = self.disconnected_peers.on_disconnect(*peer_id) { self.actions.push(WarpSyncAction::DropPeer(bad_peer)); } } From d993f59ab2ae400b125959008725e88dca7d3322 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 30 Jul 2024 16:32:45 +0300 Subject: [PATCH 19/24] engine: Disconnect logs Signed-off-by: Alexandru Vasile --- substrate/client/network/sync/src/engine.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/substrate/client/network/sync/src/engine.rs b/substrate/client/network/sync/src/engine.rs index bb6e7a98a810..ee7576c22f16 100644 --- a/substrate/client/network/sync/src/engine.rs +++ b/substrate/client/network/sync/src/engine.rs @@ -536,6 +536,10 @@ where }, BlockAnnounceValidationResult::Failure { peer_id, disconnect } => { if disconnect { + log::debug!( + target: LOG_TARGET, + "Disconnecting peer {peer_id} due to block announce validation failure", + ); self.network_service .disconnect_peer(peer_id, self.block_announce_protocol_name.clone()); } From bfc32b9970c24e2b561865499c1f742cff09c17c Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 30 Jul 2024 18:43:43 +0300 Subject: [PATCH 20/24] disconnected-peers: Address feedback Signed-off-by: Alexandru Vasile --- .../sync/src/strategy/disconnected_peers.rs | 46 +++++++++++-------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/disconnected_peers.rs b/substrate/client/network/sync/src/strategy/disconnected_peers.rs index df5dc9919a83..1e729ba40ea0 100644 --- a/substrate/client/network/sync/src/strategy/disconnected_peers.rs +++ b/substrate/client/network/sync/src/strategy/disconnected_peers.rs @@ -88,12 +88,17 @@ impl DisconnectedState { pub struct DisconnectedPeers { /// The state of disconnected peers. disconnected_peers: LruMap, + /// Backoff duration in seconds. + backoff_seconds: u64, } impl DisconnectedPeers { /// Create a new `DisconnectedPeers`. pub fn new() -> Self { - Self { disconnected_peers: LruMap::new(ByLength::new(MAX_DISCONNECTED_PEERS_STATE)) } + Self { + disconnected_peers: LruMap::new(ByLength::new(MAX_DISCONNECTED_PEERS_STATE)), + backoff_seconds: DISCONNECTED_PEER_BACKOFF_SECONDS, + } } /// Insert a new peer to the persistent state if not seen before, or update the state if seen. @@ -106,10 +111,10 @@ impl DisconnectedPeers { let should_ban = state.num_disconnects() >= MAX_NUM_DISCONNECTS; log::debug!( target: LOG_TARGET, - "Remove known peer {peer} state: {state:?}, should ban: {should_ban}", + "Disconnected known peer {peer} state: {state:?}, should ban: {should_ban}", ); - return should_ban.then(|| { + should_ban.then(|| { // We can lose track of the peer state and let the banning mechanism handle // the peer backoff. // @@ -118,15 +123,15 @@ impl DisconnectedPeers { self.disconnected_peers.remove(&peer); BadPeer(peer, REPUTATION_REPORT) }) + } else { + log::debug!( + target: LOG_TARGET, + "Added peer {peer} for the first time" + ); + // First time we see this peer. + self.disconnected_peers.insert(peer, DisconnectedState::new()); + None } - - log::debug!( - target: LOG_TARGET, - "Added peer {peer} for the first time" - ); - // First time we see this peer. - self.disconnected_peers.insert(peer, DisconnectedState::new()); - None } /// Check if a peer is available for queries. @@ -136,7 +141,7 @@ impl DisconnectedPeers { }; let elapsed = state.last_disconnect().elapsed(); - if elapsed.as_secs() >= DISCONNECTED_PEER_BACKOFF_SECONDS * state.num_disconnects { + if elapsed.as_secs() >= self.backoff_seconds * state.num_disconnects { log::debug!(target: LOG_TARGET, "Peer {peer_id} is available for queries"); self.disconnected_peers.remove(peer_id); true @@ -160,11 +165,10 @@ mod tests { // Is not part of the disconnected peers yet. assert_eq!(state.is_peer_available(&peer), true); - assert!(state.on_disconnect(peer).is_none()); - assert_eq!(state.is_peer_available(&peer), false); - - assert!(state.on_disconnect(peer).is_none()); - assert_eq!(state.is_peer_available(&peer), false); + for _ in 0..MAX_NUM_DISCONNECTS { + assert!(state.on_disconnect(peer).is_none()); + assert_eq!(state.is_peer_available(&peer), false); + } assert!(state.on_disconnect(peer).is_some()); // Peer is supposed to get banned and disconnected. @@ -174,14 +178,18 @@ mod tests { #[test] fn ensure_backoff_time() { - let mut state = DisconnectedPeers::new(); + const TEST_BACKOFF_SECONDS: u64 = 2; + let mut state = DisconnectedPeers { + disconnected_peers: LruMap::new(ByLength::new(1)), + backoff_seconds: TEST_BACKOFF_SECONDS, + }; let peer = PeerId::random(); assert!(state.on_disconnect(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); // Wait until the backoff time has passed - std::thread::sleep(Duration::from_secs(DISCONNECTED_PEER_BACKOFF_SECONDS + 1)); + std::thread::sleep(Duration::from_secs(TEST_BACKOFF_SECONDS + 1)); assert_eq!(state.is_peer_available(&peer), true); } From 7f90cee5fce09191cfaf388b24000041436ac9b8 Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 30 Jul 2024 18:48:26 +0300 Subject: [PATCH 21/24] Add PR doc for better documentation Signed-off-by: Alexandru Vasile --- prdoc/pr_5029.prdoc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 prdoc/pr_5029.prdoc diff --git a/prdoc/pr_5029.prdoc b/prdoc/pr_5029.prdoc new file mode 100644 index 000000000000..d446ddf274b8 --- /dev/null +++ b/prdoc/pr_5029.prdoc @@ -0,0 +1,16 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Backoff slow peers to avoid duplicate requests + +doc: + - audience: Node Dev + description: | + This PR introduces a backoff strategy mechanism. Whenever a peer disconnects with an inflight + block (or state) request, the peer is backed off for a period of time before receiving requests. + After several attempts, the peer is disconnected and banned. The strategy aims to offload + the pressure from peers that are slow to respond or overloaded. + +crates: +- name: sc-network-sync + bump: minor From c0d0f3c65ee9b47c22f9538e3d614d741268b16e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Tue, 30 Jul 2024 18:57:41 +0300 Subject: [PATCH 22/24] disconnected-peers: Fix test Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/disconnected_peers.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/disconnected_peers.rs b/substrate/client/network/sync/src/strategy/disconnected_peers.rs index 1e729ba40ea0..a74ff08e9ca8 100644 --- a/substrate/client/network/sync/src/strategy/disconnected_peers.rs +++ b/substrate/client/network/sync/src/strategy/disconnected_peers.rs @@ -158,14 +158,14 @@ mod tests { use std::time::Duration; #[test] - fn test_persistent_peer_state() { + fn test_disconnected_peer_state() { let mut state = DisconnectedPeers::new(); let peer = PeerId::random(); // Is not part of the disconnected peers yet. assert_eq!(state.is_peer_available(&peer), true); - for _ in 0..MAX_NUM_DISCONNECTS { + for _ in 0..MAX_NUM_DISCONNECTS - 1 { assert!(state.on_disconnect(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); } From 16e55cfbcd87254a66aefe0b6ee1350346445c6e Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 31 Jul 2024 11:25:37 +0300 Subject: [PATCH 23/24] disconnected-peers: Raname to on_disconnect_during_request Signed-off-by: Alexandru Vasile --- .../client/network/sync/src/strategy/chain_sync.rs | 4 +++- .../network/sync/src/strategy/disconnected_peers.rs | 10 +++++----- substrate/client/network/sync/src/strategy/state.rs | 4 +++- substrate/client/network/sync/src/strategy/warp.rs | 4 +++- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/chain_sync.rs b/substrate/client/network/sync/src/strategy/chain_sync.rs index 7bf9ce0460d4..52870d5ba151 100644 --- a/substrate/client/network/sync/src/strategy/chain_sync.rs +++ b/substrate/client/network/sync/src/strategy/chain_sync.rs @@ -1147,7 +1147,9 @@ where if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.disconnected_peers.on_disconnect(*peer_id) { + if let Some(bad_peer) = + self.disconnected_peers.on_disconnect_during_request(*peer_id) + { self.actions.push(ChainSyncAction::DropPeer(bad_peer)); } } diff --git a/substrate/client/network/sync/src/strategy/disconnected_peers.rs b/substrate/client/network/sync/src/strategy/disconnected_peers.rs index a74ff08e9ca8..ea94a5a1df54 100644 --- a/substrate/client/network/sync/src/strategy/disconnected_peers.rs +++ b/substrate/client/network/sync/src/strategy/disconnected_peers.rs @@ -21,7 +21,7 @@ use sc_network::ReputationChange as Rep; use sc_network_types::PeerId; use schnellru::{ByLength, LruMap}; -const LOG_TARGET: &str = "sync::persistent_peer_state"; +const LOG_TARGET: &str = "sync::disconnected_peers"; /// The maximum number of disconnected peers to keep track of. /// @@ -104,7 +104,7 @@ impl DisconnectedPeers { /// Insert a new peer to the persistent state if not seen before, or update the state if seen. /// /// Returns true if the peer should be disconnected. - pub fn on_disconnect(&mut self, peer: PeerId) -> Option { + pub fn on_disconnect_during_request(&mut self, peer: PeerId) -> Option { if let Some(state) = self.disconnected_peers.get(&peer) { state.increment(); @@ -166,11 +166,11 @@ mod tests { assert_eq!(state.is_peer_available(&peer), true); for _ in 0..MAX_NUM_DISCONNECTS - 1 { - assert!(state.on_disconnect(peer).is_none()); + assert!(state.on_disconnect_during_request(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); } - assert!(state.on_disconnect(peer).is_some()); + assert!(state.on_disconnect_during_request(peer).is_some()); // Peer is supposed to get banned and disconnected. // The state ownership moves to the PeerStore. assert!(state.disconnected_peers.get(&peer).is_none()); @@ -185,7 +185,7 @@ mod tests { }; let peer = PeerId::random(); - assert!(state.on_disconnect(peer).is_none()); + assert!(state.on_disconnect_during_request(peer).is_none()); assert_eq!(state.is_peer_available(&peer), false); // Wait until the backoff time has passed diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index eccea8f04aea..1cb33d8616ab 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -148,7 +148,9 @@ impl StateStrategy { pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.disconnected_peers.on_disconnect(*peer_id) { + if let Some(bad_peer) = + self.disconnected_peers.on_disconnect_during_request(*peer_id) + { self.actions.push(StateStrategyAction::DropPeer(bad_peer)); } } diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index 498bd6445175..bad40721700a 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -314,7 +314,9 @@ where pub fn remove_peer(&mut self, peer_id: &PeerId) { if let Some(state) = self.peers.remove(peer_id) { if !state.state.is_available() { - if let Some(bad_peer) = self.disconnected_peers.on_disconnect(*peer_id) { + if let Some(bad_peer) = + self.disconnected_peers.on_disconnect_during_request(*peer_id) + { self.actions.push(WarpSyncAction::DropPeer(bad_peer)); } } From 00a5cb3a0b43492391141f0a98f5e1666ebfd3cf Mon Sep 17 00:00:00 2001 From: Alexandru Vasile Date: Wed, 31 Jul 2024 11:31:01 +0300 Subject: [PATCH 24/24] strategy: Adjust testing Signed-off-by: Alexandru Vasile --- substrate/client/network/sync/src/strategy/state.rs | 2 -- substrate/client/network/sync/src/strategy/warp.rs | 2 -- 2 files changed, 4 deletions(-) diff --git a/substrate/client/network/sync/src/strategy/state.rs b/substrate/client/network/sync/src/strategy/state.rs index 1cb33d8616ab..ff229863a68b 100644 --- a/substrate/client/network/sync/src/strategy/state.rs +++ b/substrate/client/network/sync/src/strategy/state.rs @@ -513,8 +513,6 @@ mod test { // Disconnecting a peer without an inflight request has no effect on persistent states. state_strategy.remove_peer(&tenth_peer); - state_strategy.remove_peer(&tenth_peer); - state_strategy.remove_peer(&tenth_peer); assert!(state_strategy.disconnected_peers.is_peer_available(&tenth_peer)); // Disconnect the peer with an inflight request. diff --git a/substrate/client/network/sync/src/strategy/warp.rs b/substrate/client/network/sync/src/strategy/warp.rs index bad40721700a..00855578695d 100644 --- a/substrate/client/network/sync/src/strategy/warp.rs +++ b/substrate/client/network/sync/src/strategy/warp.rs @@ -897,8 +897,6 @@ mod test { // Disconnecting a peer without an inflight request has no effect on persistent states. warp_sync.remove_peer(&tenth_peer); - warp_sync.remove_peer(&tenth_peer); - warp_sync.remove_peer(&tenth_peer); assert!(warp_sync.disconnected_peers.is_peer_available(&tenth_peer)); warp_sync.add_peer(tenth_peer, H256::random(), 10);