Skip to content

fix: disconnect peers after pong timeout #424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xdustinface wants to merge 1 commit into
base: v0.42-dev
Choose a base branch
from
+15 −3
Open

fix: disconnect peers after pong timeout #424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions dash-spv/src/network/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,7 +867,8 @@ impl PeerNetworkManager {
}
}

// Send ping to all peers if needed
// Send ping to all peers if needed and disconnect unresponsive ones
let mut peers_to_disconnect = Vec::new();
for (addr, peer) in pool.get_all_peers().await {
let mut peer_guard = peer.write().await;
if peer_guard.should_ping() {
Expand All @@ -881,7 +882,14 @@ impl PeerNetworkManager {
).await;
}
}
peer_guard.cleanup_old_pings();
let expired = peer_guard.cleanup_old_pings();
if expired > 0 {
peers_to_disconnect.push(addr);
}
}
for addr in peers_to_disconnect {
log::warn!("Disconnecting peer {} - ping timeout", addr);
pool.remove_peer(&addr).await;
}
Comment on lines +890 to 893
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 8, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

connected_peer_count is never decremented for ping-timeout disconnections, causing counter drift.

When pool.remove_peer is called here, the peer reader task will find the peer gone from the pool (Line 354) and break, but since remove_peer already returned None at that point (Line 637), the fetch_sub on connected_peer_count (Line 640) is skipped. Over time the cached counter will overcount connected peers, breaking is_connected() and peer_count().

You should also emit NetworkEvent::PeerDisconnected / PeersUpdated here to keep event consumers in sync, matching the cleanup in start_peer_reader (Lines 638–654).

Proposed fix 
                 for addr in peers_to_disconnect {
                     log::warn!("Disconnecting peer {} - ping timeout", addr);
-                    pool.remove_peer(&addr).await;
+                    if pool.remove_peer(&addr).await.is_some() {
+                        connected_peer_count.fetch_sub(1, Ordering::Relaxed);
+                        let count = connected_peer_count.load(Ordering::Relaxed);
+                        let addresses = pool.get_connected_addresses().await;
+                        let best_height = pool.get_best_height().await;
+                        let _ = network_event_sender.send(NetworkEvent::PeerDisconnected {
+                            address: addr,
+                        });
+                        let _ = network_event_sender.send(NetworkEvent::PeersUpdated {
+                            connected_count: count,
+                            addresses,
+                            best_height,
+                        });
+                    }
                 }

This requires capturing network_event_sender into the maintenance loop closure (it isn't currently captured).

🤖 Prompt for AI Agents 
In `@dash-spv/src/network/manager.rs` around lines 890 - 893, The maintenance loop
calls pool.remove_peer(&addr) on ping-timeout peers but doesn't decrement the
cached connected_peer_count or emit network events, causing counter drift and
stale event state; modify the maintenance loop closure to capture
network_event_sender, and after successful removal invoke the same cleanup path
used in start_peer_reader: call connected_peer_count.fetch_sub(1,
Ordering::SeqCst) (or the existing decrement helper), send
NetworkEvent::PeerDisconnected with the peer id and then send
NetworkEvent::PeersUpdated so consumers stay in sync; ensure you reference
pool.remove_peer, connected_peer_count, network_event_sender, and mirror the
logic from start_peer_reader to avoid double-removal.


// Only save known peers if not in exclusive mode
Expand Down
6 changes: 5 additions & 1 deletion dash-spv/src/network/peer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,8 @@ impl Peer {
}

/// Clean up old pending pings that haven't received responses.
pub fn cleanup_old_pings(&mut self) {
/// Returns the number of expired pings that were removed.
pub fn cleanup_old_pings(&mut self) -> usize {
const PING_TIMEOUT: Duration = Duration::from_secs(60); // 1 minute timeout for pings

let now = SystemTime::now();
Expand All @@ -777,10 +778,13 @@ impl Peer {
}
}

let expired_count = expired_nonces.len();
for nonce in expired_nonces {
self.pending_pings.remove(&nonce);
tracing::warn!("Ping timeout for {} with nonce {}", self.address, nonce);
}

expired_count
}

/// Get ping/pong statistics.
Expand Down
Loading