feat(dash-spv): expose per-peer connection stats (ping RTT, bytes received)#860
feat(dash-spv): expose per-peer connection stats (ping RTT, bytes received)#860QuantumExplorer wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesPeer statistics
Archive extraction compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DashSpvClient
participant PeerNetworkManager
participant PeerPool
participant Peer
DashSpvClient->>PeerNetworkManager: peer_stats()
PeerNetworkManager->>PeerPool: iterate connected peers
PeerPool->>Peer: stats_snapshot()
Peer-->>PeerNetworkManager: PeerStatsSnapshot
PeerNetworkManager-->>DashSpvClient: vector of snapshots
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/network/peer.rs (1)
415-428: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake byte accounting cancellation-safe.
receive_message()stores successful reads in a local counter and commits it only after the async body completes. However,PeerNetworkManager::start_peer_reader()can cancel this future viatokio::select!; bytes already buffered then remain inframing_bufferbut are never counted on a later call. Persist or commit the count immediately after each successful socket read, and add a regression test for partial-frame cancellation.As per coding guidelines, new Rust functionality must have unit coverage.
Also applies to: 484-484, 503-503, 551-551, 642-645
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash-spv/src/network/peer.rs` around lines 415 - 428, Make receive_message() cancellation-safe by committing byte counts to the peer’s persistent accounting immediately after every successful read, including reads in all referenced paths, instead of relying on the local bytes_read total after the async body completes. Update the relevant read/accounting logic in receive_message() and add unit coverage that cancels a partial-frame read, then verifies the buffered bytes are counted on the subsequent call.Source: Coding guidelines
🧹 Nitpick comments (1)
contrib/setup-dashd.py (1)
89-94: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueFallback to unfiltered extraction is acceptable for trusted sources, but consider tightening the exception handling.
The
try/except TypeErrorapproach correctly handles Python versions that don't support thefilterkwarg. The archives are downloaded from trusted GitHub release URLs over HTTPS, so the path-traversal risk flagged by static analysis on the fallbacktf.extractall(dest_dir)is low in practice.One minor note:
except TypeErroris broad. Ifextractallwere to raiseTypeErrorfor an unrelated reason, the script would silently fall back to unfiltered extraction instead of surfacing the error. A more defensive pattern would check the Python/tarfile version explicitly, though the current approach is pragmatic and readable for a test-setup script.As per path instructions, the PR title should use a
fixprefix since this is a compatibility fix — ensure the actual PR title follows this.Optional: version-gated approach instead of broad except
try: tf.extractall(dest_dir, filter="data") except TypeError: - # The `filter` kwarg needs Python 3.9.17+/3.10.12+/3.11.4+; - # fall back for older interpreters (e.g. macOS system 3.9.6). - tf.extractall(dest_dir) + # The `filter` kwarg needs Python 3.9.17+/3.10.12+/3.11.4+; + # fall back for older interpreters (e.g. macOS system 3.9.6). + if "filter" in str(sys.exc_info()[1]): + tf.extractall(dest_dir) + else: + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/setup-dashd.py` around lines 89 - 94, Use an explicit Python/tarfile capability or version check before calling TarFile.extractall with the filter argument, instead of broadly catching TypeError; retain the unfiltered fallback only for interpreters that lack filter support, while allowing unrelated extraction TypeErrors to surface. No code change is required for the trusted-source path concern, but ensure the PR title uses the fix prefix.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 313-319: When the initial `Peer::send_ping()` in the connection
setup path fails, stop setup immediately instead of continuing as if connected.
Remove the connecting marker, clean up the failed peer/connection state, and
return before recording the connection, adding the disconnected peer,
incrementing counts, or emitting `PeerConnected`; update the `if let Err(e)`
branch in the connection manager accordingly.
In `@dash-spv/src/network/peer.rs`:
- Around line 55-64: Reset all session-scoped ping state in the public
Peer::connect_instance() path when replacing the socket: clear pending_pings,
set last_ping_rtt and last_pong_received to None, and reset ping_rtt_sum and
ping_rtt_samples. Ensure reconnect behavior matches the initialization performed
by Peer::connect().
- Around line 905-908: Replace the hardcoded `127.0.0.1:9999` in
`pong_records_rtt_and_snapshot_averages` with the shared test helper that
creates a localhost address using an ephemeral port; reuse the helper’s returned
`SocketAddr` when constructing `Peer::dummy`.
---
Outside diff comments:
In `@dash-spv/src/network/peer.rs`:
- Around line 415-428: Make receive_message() cancellation-safe by committing
byte counts to the peer’s persistent accounting immediately after every
successful read, including reads in all referenced paths, instead of relying on
the local bytes_read total after the async body completes. Update the relevant
read/accounting logic in receive_message() and add unit coverage that cancels a
partial-frame read, then verifies the buffered bytes are counted on the
subsequent call.
---
Nitpick comments:
In `@contrib/setup-dashd.py`:
- Around line 89-94: Use an explicit Python/tarfile capability or version check
before calling TarFile.extractall with the filter argument, instead of broadly
catching TypeError; retain the unfiltered fallback only for interpreters that
lack filter support, while allowing unrelated extraction TypeErrors to surface.
No code change is required for the trusted-source path concern, but ensure the
PR title uses the fix prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 406c0ce5-6d1f-46f1-b6b0-773e1115e2ad
📒 Files selected for processing (5)
contrib/setup-dashd.pydash-spv/src/client/queries.rsdash-spv/src/network/manager.rsdash-spv/src/network/mod.rsdash-spv/src/network/peer.rs
| // Ping immediately so latency stats are available | ||
| // right away instead of after the first | ||
| // maintenance tick. | ||
| if let Err(e) = peer.send_ping().await { | ||
| tracing::warn!("Failed to send initial ping to {}: {}", addr, e); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Abort connection setup when the initial ping fails.
Peer::send_message() clears state and connected_at on a write failure, but this branch only logs the error and continues to record a successful connection, add the disconnected peer, increment the count, and emit PeerConnected. This can expose a false connection event and transient disconnected snapshot.
Remove the connecting marker and return on failure instead of adding the peer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dash-spv/src/network/manager.rs` around lines 313 - 319, When the initial
`Peer::send_ping()` in the connection setup path fails, stop setup immediately
instead of continuing as if connected. Remove the connecting marker, clean up
the failed peer/connection state, and return before recording the connection,
adding the disconnected peer, incrementing counts, or emitting `PeerConnected`;
update the `if let Err(e)` branch in the connection manager accordingly.
| bytes_received: u64, | ||
| network: Network, | ||
| // Ping/pong state | ||
| last_ping_sent: Option<SystemTime>, | ||
| last_pong_received: Option<SystemTime>, | ||
| pending_pings: HashMap<u64, SystemTime>, // nonce -> sent_time | ||
| // Measured ping round-trips (running sum/count for the average) | ||
| last_ping_rtt: Option<Duration>, | ||
| ping_rtt_sum: Duration, | ||
| ping_rtt_samples: u32, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset session-scoped ping state on connect_instance().
Peer::connect() initializes the RTT fields, but the public connect_instance() path only replaces the socket and timestamp. Reconnecting a Peer can therefore retain pending_pings, last_ping_rtt, and the RTT accumulator from the previous session, producing incorrect snapshots.
Also applies to: 138-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dash-spv/src/network/peer.rs` around lines 55 - 64, Reset all session-scoped
ping state in the public Peer::connect_instance() path when replacing the
socket: clear pending_pings, set last_ping_rtt and last_pong_received to None,
and reset ping_rtt_sum and ping_rtt_samples. Ensure reconnect behavior matches
the initialization performed by Peer::connect().
| #[test] | ||
| fn pong_records_rtt_and_snapshot_averages() { | ||
| let addr: SocketAddr = "127.0.0.1:9999".parse().unwrap(); | ||
| let mut peer = Peer::dummy(addr); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid the fixed test address and port.
Use a shared test helper with an ephemeral port instead of 127.0.0.1:9999.
As per coding guidelines, Rust code must never hardcode network parameters, addresses, or keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dash-spv/src/network/peer.rs` around lines 905 - 908, Replace the hardcoded
`127.0.0.1:9999` in `pong_records_rtt_and_snapshot_averages` with the shared
test helper that creates a localhost address using an ephemeral port; reuse the
helper’s returned `SocketAddr` when constructing `Peer::dummy`.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #860 +/- ##
==========================================
- Coverage 73.85% 73.61% -0.25%
==========================================
Files 250 324 +74
Lines 53168 73278 +20110
==========================================
+ Hits 39268 53943 +14675
- Misses 13900 19335 +5435
|
…eived) Adds a PeerStatsSnapshot surface so client consumers can measure how well individual peers serve data: - Peer now counts bytes_received at the socket read (closing the long-standing TODO in Peer::stats()) and records ping round-trips (last + running average) in handle_pong. - The manager sends an initial ping right after the handshake completes, so latency is measured within the first round-trip instead of waiting for the first 10s maintenance tick. - New NetworkManager::peer_stats() trait method (default empty impl keeps mock managers compiling) implemented by PeerNetworkManager and exposed as DashSpvClient::peer_stats(). Also fixes contrib/setup-dashd.py on Python < 3.9.17, where tarfile.extractall() does not accept the `filter` kwarg. Verified: 470 lib unit tests, all integration binaries, and the dashd suites (dashd_masternode 10/10, dashd_sync 29/29) pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/modest-sinoussi-db08c7
branch
from
3a9a020 to
088b252
Compare
1 participant
Summary
Adds a
PeerStatsSnapshotsurface so client consumers can measure how well individual peers serve data (built for an external SPV network-health dashboard, but generally useful):Peernow countsbytes_receivedat the socket read, closing the long-standingTODO: Track bytes receivedinPeer::stats(). Bytes are counted even when a frame later fails to parse — the peer still served them.handle_pong; previously the RTT was computed and only logged.should_ping()naturally suppresses re-pinging withinPING_INTERVAL.NetworkManager::peer_stats() -> Vec<PeerStatsSnapshot>(default empty impl keeps mock managers source-compatible), implemented byPeerNetworkManagerover the peer pool, and exposed asDashSpvClient::peer_stats().Also fixes
contrib/setup-dashd.pyon Python < 3.9.17, wheretarfile.extractall()does not accept thefilterkwarg (macOS system Python is 3.9.6).Testing
pong_records_rtt_and_snapshot_averagescovers RTT recording, averaging, and unknown-nonce rejection.dashd_masternode10/10,dashd_sync29/29) viacontrib/setup-dashd.py.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes