refactor: move get_peer_best_height into PeerPool

[xdustinface]

Move the logic into PeerPool, just makes more sense imo.

Summary by CodeRabbit

• Refactor

  • Simplified peer best-height handling and moved aggregation responsibility to the pool.

• New Features

  • Added a method to compute the maximum height across connected peers.
  • Exposed a public accessor for peer version information.

• Tests

  • Updated mock network manager to align with the new peer-height retrieval behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Returned peer-height handling was refactored: network trait/manager now return Option<CoreBlockHeight> and delegate aggregation to PeerPool::get_best_height(). Peer::version() getter added; sync coordinator call sites simplified to use the option directly. Tests updated to match the new signature.

Changes

Cohort / File(s)	Summary
Network API & Manager dash-spv/src/network/mod.rs, dash-spv/src/network/manager.rs	Changed get_peer_best_height signature from NetworkResult<Option<u32>> to Option<CoreBlockHeight>; manager now delegates to pool; added CoreBlockHeight import.
Peer Pool & Peer dash-spv/src/network/pool.rs, dash-spv/src/network/peer.rs	Added PeerPool::get_best_height() to aggregate peer heights (logs per-peer and returns max or None). Added pub fn version(&self) -> Option<u32> on Peer.
Client Usage dash-spv/src/client/sync_coordinator.rs	Replaced nested ok().flatten() chains with direct await returning Option<CoreBlockHeight> and unwrap_or(current_height) fallback in two progress-emission paths.
Test Utilities dash-spv/src/test_utils/network.rs	Updated MockNetworkManager to include peer_best_height: Option<u32> and to implement new get_peer_best_height() -> Option<CoreBlockHeight> returning that field; added import for CoreBlockHeight.

Sequence Diagram(s)

sequenceDiagram
    participant Sync as SyncCoordinator
    participant Net as NetworkManager
    participant Pool as PeerPool
    participant Peer as Peer (many)

    Sync->>Net: get_peer_best_height().await
    Net->>Pool: get_best_height().await
    Pool->>Peer: get_all_peers() / iterate
    loop per peer
        Pool->>Peer: read best_height / version
        Peer-->>Pool: best_height (Option<CoreBlockHeight>)
    end
    Pool-->>Net: Some(max_height) / None
    Net-->>Sync: Some(max_height) / None
    Sync->>Sync: use unwrap_or(current_height) for progress

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through pools and peers tonight,
Gathered heights by moonlit byte,
One clean path from many springs,
No more nested result-springs,
A tidy hop — the sync feels right! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main refactoring change: moving get_peer_best_height logic from NetworkManager into PeerPool, which is the primary objective of this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@dash-spv/src/test_utils/network.rs`:
- Around line 158-160: The mock get_peer_best_height currently falls back to
self.headers_chain.len() and thus returns a height even when no peer is
connected; change get_peer_best_height to return None when self.peer_best_height
is None (treat None as "no peers/disconnected") and only return Some(height)
when peer_best_height is Some(value) (do not use headers_chain as a fallback).
Update the logic in get_peer_best_height to check self.peer_best_height and
return Some(h) or None accordingly, referencing the get_peer_best_height
function, the peer_best_height field, and headers_chain.

🧹 Nitpick comments (1)

dash-spv/src/network/pool.rs (1)

103-149: Consider skipping disconnected peers to avoid stale heights.
If a peer lingers in the pool after disconnect, its cached height can skew the aggregate.

♻️ Proposed tweak

         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 !peer_guard.is_connected() {
+                continue;
+            }

             if let Some(peer_height) = peer_guard.best_height() {

[ZocoLini]

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

[xdustinface]

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.

[ZocoLini]

Appoving, the suggestions in the function body are not blocking tbh