refactor: remove current_sync_peer from network manager

[xdustinface]

Simplify peer selection in send_to_single_peer by removing the sticky sync peer tracking. Peers are now selected directly based on message type requirements and the round-robin counter.

Based on:

• refactor: extract capability lookup into PeerPool helpers #509

Summary by CodeRabbit

Release Notes

• Refactor
  • Enhanced network peer selection mechanism with capability-based matching and improved fallback handling.
  • Streamlined peer management by removing sticky peer tracking system.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors PeerNetworkManager to remove sticky peer tracking via the current_sync_peer field and replaces the selection logic with a service-capability-based approach using a new next_peer round-robin helper method for improved peer selection.

Changes

Cohort / File(s)	Summary
Peer Selection Refactoring dash-spv/src/network/manager.rs	Removed current_sync_peer field and sticky-sync peer tracking. Introduced preferred_service logic to map messages to required capabilities. Added next_peer helper for round-robin peer selection. Updated send_to_single_peer and distributed-sending paths to use the new selection mechanism instead of legacy headers2-sticky logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 No more sticky peers causing delay,
Round-robin hops lead us the way,
Service capabilities light the path,
Smarter selection—no more aftermath,
Peers dance fairly, in balance they sway! 🌟

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: removing the current_sync_peer field from the network manager.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/drop-current-sync-peer

📝 Coding Plan

• Generate coding plan for human review comments

---

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.}

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.18%. Comparing base (d5bec1e) to head (5da971f).
⚠️ Report is 2 commits behind head on v0.42-dev.

Files with missing lines	Patch %	Lines
dash-spv/src/network/manager.rs	50.00%	10 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## v0.42-dev #511 +/- ##
=============================================
+ Coverage 66.13% 66.18% +0.04% 
=============================================
Files 311 311 Lines 64757 64717 -40 =============================================
+ Hits 42829 42832 +3 + Misses 21928 21885 -43 

Flag	Coverage Δ
core	75.13% <ø> (ø)
ffi	37.14% <ø> (ø)
rpc	28.28% <ø> (ø)
spv	81.09% <50.00%> (+0.28%)	⬆️
wallet	65.49% <ø> (ø)

Files with missing lines	Coverage Δ
dash-spv/src/network/manager.rs	60.10% <50.00%> (+3.02%)	⬆️

... and 1 file with indirect coverage changes

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[QuantumExplorer]

Review from Claude

Issue 1: send_to_single_peer ignores headers2_disabled set for preferred peers

Severity: Medium

The new send_to_single_peer uses self.pool.peer_with_service(ServiceFlags::NODE_HEADERS_COMPRESSED) for GetHeaders/GetHeaders2 but does NOT check self.headers2_disabled. By contrast, send_distributed correctly filters:

let disabled = self.headers2_disabled.lock().await;
headers2_peers.retain(|(addr, _)| !disabled.contains(addr));

If the "first" peer returned by peer_with_service is in headers2_disabled (due to a prior decompression failure), it will be selected repeatedly, downgraded to plain GetHeaders, while other healthy headers2 peers sit idle.

Issue 2: peer_with_service always returns the same peer — accidental stickiness

Severity: Medium

peer_with_service iterates the HashMap and returns the first match. Since HashMap iteration order is stable until the map is modified, this creates accidental stickiness — but it's fragile and breaks whenever peers join/leave (HashMap rehash).

This is neither properly sticky (for sync consistency, like the old current_sync_peer) nor properly round-robin (for load distribution). For headers sync specifically, if the "first" peer changes mid-sync due to a peer add/remove, you get wasted round-trips as the new peer re-serves headers you already have.

Suggested fix for both: Either (a) apply round-robin to service-filtered peers (collect all matching, then use next_peer), and filter headers2_disabled in the single-peer path too, or (b) if stickiness is desired for headers, make it explicit.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

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

986-1021: Add regression tests for the new single-peer selection behavior.

Please add unit tests for: (1) excluding headers2_disabled peers in single-peer headers flow, and (2) round-robin across capability-filtered peer subsets.

As per coding guidelines, **/*.rs: Write unit tests for new functionality in Rust code.

Also applies to: 1070-1084

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.
In `@dash-spv/src/network/manager.rs` around lines 986 - 1021, Add unit tests
exercising send_to_single_peer to cover (1) that peers with headers2_disabled
are excluded from the single-peer headers flow by simulating peers returned by
Pool::peer_with_service and ensuring NetworkMessage::GetHeaders2 is not sent to
those peers, and (2) that next_peer performs round-robin across
capability-filtered subsets by creating multiple peers with the same service
flags and asserting subsequent send_to_single_peer calls cycle through their
addresses; use the existing Pool::get_all_peers, Pool::peer_with_service,
send_to_single_peer, next_peer and send_message_to_peer hooks to inject fake
peers/messages and verify selection behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 998-1000: The code marks NetworkMessage::GetHeaders2 as optional
(returns Some((ServiceFlags::NODE_HEADERS_COMPRESSED, false))) which allows
selecting peers without headers2 support and then attempting to send GetHeaders2
later; either make GetHeaders2 required by flipping the tuple to
Some((ServiceFlags::NODE_HEADERS_COMPRESSED, true)) so peer selection only
chooses peers advertising NODE_HEADERS_COMPRESSED, or add downgrade logic at the
send site that checks the chosen peer's service flags and sends a plain
GetHeaders when NODE_HEADERS_COMPRESSED is not present (ensure the send path
that currently emits GetHeaders2 consults the peer's flags first).
- Around line 1004-1015: The current preferred_service branch directly uses
pool.peer_with_service(...) which bypasses the round-robin helper and won't
exclude headers2_disabled peers; replace that direct selection with a filtered
round-robin selection similar to send_distributed: query the available peers
supporting the given flags (e.g. pool.peers_with_service(flags) or equivalent),
filter out headers2_disabled peers, if the filtered list is empty handle the
required=true case by logging and returning NetworkError::ProtocolError,
otherwise call next_peer(&filtered_peers) to pick the address/peer and log the
selection; keep existing behavior for the None => self.next_peer(&peers) branch.
---
Nitpick comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 986-1021: Add unit tests exercising send_to_single_peer to cover
(1) that peers with headers2_disabled are excluded from the single-peer headers
flow by simulating peers returned by Pool::peer_with_service and ensuring
NetworkMessage::GetHeaders2 is not sent to those peers, and (2) that next_peer
performs round-robin across capability-filtered subsets by creating multiple
peers with the same service flags and asserting subsequent send_to_single_peer
calls cycle through their addresses; use the existing Pool::get_all_peers,
Pool::peer_with_service, send_to_single_peer, next_peer and send_message_to_peer
hooks to inject fake peers/messages and verify selection behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1e08ee24-f489-46db-811a-719fc2a7ff83

📥 Commits

Reviewing files that changed from the base of the PR and between d5bec1e and 5da971f.

📒 Files selected for processing (1)

• dash-spv/src/network/manager.rs