Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions dash-spv/src/client/sync_coordinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,8 @@ impl<W: WalletInterface, N: NetworkManager, S: StorageManager> DashSpvClient<W,
storage.get_tip_height().await.unwrap_or(0)
};
let current_height = current_tip_height;
let peer_best = self
.network
.get_peer_best_height()
.await
.ok()
.flatten()
.unwrap_or(current_height);
let peer_best =
self.network.get_peer_best_height().await.unwrap_or(current_height);

// Calculate headers downloaded this second
if current_tip_height > last_height {
Expand Down Expand Up @@ -298,13 +293,8 @@ impl<W: WalletInterface, N: NetworkManager, S: StorageManager> DashSpvClient<W,

{
// Build and emit a fresh DetailedSyncProgress snapshot reflecting current filter progress
let peer_best = self
.network
.get_peer_best_height()
.await
.ok()
.flatten()
.unwrap_or(abs_header_height);
let peer_best =
self.network.get_peer_best_height().await.unwrap_or(abs_header_height);

let phase_snapshot = self.sync_manager.current_phase().clone();
let status_display = self.create_status_display().await;
Expand Down
42 changes: 3 additions & 39 deletions dash-spv/src/network/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use async_trait::async_trait;
use dashcore::network::constants::ServiceFlags;
use dashcore::network::message::NetworkMessage;
use dashcore::network::message_headers2::CompressionState;
use dashcore::prelude::CoreBlockHeight;
use dashcore::Network;
use tokio::sync::mpsc::UnboundedReceiver;
use tokio_util::sync::CancellationToken;
Expand Down Expand Up @@ -1205,45 +1206,8 @@ impl NetworkManager for PeerNetworkManager {
self.connected_peer_count.load(Ordering::Relaxed)
}

async fn get_peer_best_height(&self) -> NetworkResult<Option<u32>> {
let peers = self.pool.get_all_peers().await;

if peers.is_empty() {
log::debug!("get_peer_best_height: No peers available");
return Ok(None);
}

let mut best_height = 0u32;
let mut peer_count = 0;

for (addr, peer) in peers.iter() {
let peer_guard = peer.read().await;

peer_count += 1;

if let Some(peer_height) = peer_guard.best_height() {
if peer_height > 0 {
best_height = best_height.max(peer_height);
log::debug!(
"get_peer_best_height: Updated best_height to {} from peer {}",
best_height,
addr
);
}
}
}

log::debug!(
"get_peer_best_height: Checked {} peers, best_height: {}",
peer_count,
best_height
);

if best_height > 0 {
Ok(Some(best_height))
} else {
Ok(None)
}
async fn get_peer_best_height(&self) -> Option<CoreBlockHeight> {
self.pool.get_best_height().await
}

async fn has_peer_with_service(&self, service_flags: ServiceFlags) -> bool {
Expand Down
3 changes: 2 additions & 1 deletion dash-spv/src/network/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ mod tests;
use crate::error::NetworkResult;
use async_trait::async_trait;
use dashcore::network::message::NetworkMessage;
use dashcore::prelude::CoreBlockHeight;
use dashcore::BlockHash;
pub use handshake::{HandshakeManager, HandshakeState};
pub use manager::PeerNetworkManager;
Expand Down Expand Up @@ -51,7 +52,7 @@ pub trait NetworkManager: Send + Sync + 'static {
fn peer_count(&self) -> usize;

/// Get the best block height reported by connected peers.
async fn get_peer_best_height(&self) -> NetworkResult<Option<u32>>;
async fn get_peer_best_height(&self) -> Option<CoreBlockHeight>;

/// Check if any connected peer supports a specific service.
async fn has_peer_with_service(
Expand Down
4 changes: 4 additions & 0 deletions dash-spv/src/network/peer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ impl Peer {
})
}

pub fn version(&self) -> Option<u32> {
self.version
}

pub fn best_height(&self) -> Option<u32> {
self.best_height
}
Expand Down
56 changes: 52 additions & 4 deletions dash-spv/src/network/pool.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
//! Peer pool for managing multiple peer connections

use crate::error::{NetworkError, SpvError as Error};
use crate::network::constants::{MAX_PEERS, MIN_PEERS};
use crate::network::peer::Peer;
use dashcore::prelude::CoreBlockHeight;
use std::collections::{HashMap, HashSet};
use std::net::SocketAddr;
use std::sync::Arc;
use tokio::sync::RwLock;

use crate::error::{NetworkError, SpvError as Error};
use crate::network::constants::{MAX_PEERS, MIN_PEERS};
use crate::network::peer::Peer;

/// Pool for managing multiple peer instances
pub struct PeerPool {
/// Active peers mapped by address
Expand Down Expand Up @@ -100,6 +100,54 @@ impl PeerPool {
self.peers.read().await.keys().copied().collect()
}

pub async fn get_best_height(&self) -> Option<CoreBlockHeight> {
let peers = self.get_all_peers().await;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to clean this function code in this PR I suggest a few things:

  • peer_count is redundant, we have the collection and can query its len (i hope)
  • I dont see the fi as somethign we need when updating best_height, suggested code would be:
            if let Some(peer_height) = peer_guard.best_height() {
                best_height = best_height.max(peer_height);
            }
  • Why return option, in case there is nor best_height we can return the 0, its a valid value since there is always a genesius block, in fact, if a network is new and only has genesis this method would report None when Some(0) is actually correct, and, in the worst scenario, genesis is hardcoded in the lib, By returning always CoreBlockHeight we reduce nesting and branches when testing edge cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely there logic can be improved but im just moving the method around here to avoid some extra housekeeping in for the peer best height in the sync rewrite PR. The optional return might make sense i was also thinking about it but something made me not do it, dont quite remember.

if peers.is_empty() {
log::debug!("get_peer_best_height: No peers available");
return None;
}

let mut best_height = 0u32;
let mut peer_count = 0;

for (addr, peer) in peers.iter() {
let peer_guard = peer.read().await;
peer_count += 1;

log::debug!(
"get_peer_best_height: Peer {} - best_height: {:?}, version: {:?}, connected: {}",
addr,
peer_guard.best_height(),
peer_guard.version(),
peer_guard.is_connected(),
);

if let Some(peer_height) = peer_guard.best_height() {
if peer_height > 0 {
best_height = best_height.max(peer_height);
log::debug!(
"get_peer_best_height: Updated best_height to {} from peer {}",
best_height,
addr
);
}
}
}

log::debug!(
"get_peer_best_height: Checked {} peers, best_height: {}",
peer_count,
best_height
);

if best_height > 0 {
Some(best_height)
} else {
None
}
}

/// Check if we need more peers
pub async fn needs_more_peers(&self) -> bool {
self.peer_count().await < MIN_PEERS
Expand Down
7 changes: 5 additions & 2 deletions dash-spv/src/test_utils/network.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::error::{NetworkError, NetworkResult};
use crate::network::{Message, MessageDispatcher, MessageType, NetworkManager};
use async_trait::async_trait;
use dashcore::prelude::CoreBlockHeight;
use dashcore::{
block::Header as BlockHeader, network::constants::ServiceFlags,
network::message::NetworkMessage, network::message_blockdata::GetHeadersMessage, BlockHash,
Expand All @@ -19,6 +20,7 @@ pub struct MockNetworkManager {
connected: bool,
connected_peer: SocketAddr,
headers_chain: Vec<BlockHeader>,
peer_best_height: Option<u32>,
message_dispatcher: MessageDispatcher,
sent_messages: Vec<NetworkMessage>,
}
Expand All @@ -30,6 +32,7 @@ impl MockNetworkManager {
connected: true,
connected_peer: SocketAddr::new(std::net::Ipv4Addr::LOCALHOST.into(), 9999),
headers_chain: Vec::new(),
peer_best_height: None,
message_dispatcher: MessageDispatcher::default(),
sent_messages: Vec::new(),
}
Expand Down Expand Up @@ -152,8 +155,8 @@ impl NetworkManager for MockNetworkManager {
}
}

async fn get_peer_best_height(&self) -> NetworkResult<Option<u32>> {
Ok(Some(self.headers_chain.len() as u32))
async fn get_peer_best_height(&self) -> Option<CoreBlockHeight> {
self.peer_best_height
}

async fn has_peer_with_service(&self, _service_flags: ServiceFlags) -> bool {
Expand Down