Skip to content

fix(dash-spv): route header requests to the shortest queue - #1349

Merged
ZocoLini merged 2 commits into
devfrom
fix/header-requests-behind-bulk
Nov 6, 2026
Merged

ZocoLini merged 2 commits into
devfrom
fix/header-requests-behind-bulk

Conversation

@xdustinface

@xdustinface xdustinface commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1235.

Problem

Broker::route picked the peer with the lowest in_flight / cap for every message, whatever its class. A slow peer with an inflated cap looks least loaded, so a getheaders landed behind megabytes of filter and block backlog on it. In a fresh testnet sync, the gap between header replies on one segment was 5 to 10 s while only headers were in flight and 34 to 72 s (worst 251 s) once filters ran. Stored headers stood still for over a minute three times while downloads continued, and filter headers stalled behind them.

Fix

route asks a new choose_peer(open, class) which peer gets each message.

  • A header request goes to the peer with the shortest expected wait, in_flight / serving_rate. A peer with nothing in flight counts as 0 s. By Little's law this is the expected wait whether the peer is limited by bandwidth or by round trips, and peers answer in order.
  • If an open peer has work in flight but no measured serving_rate yet, header requests fall back to the cap ratio, since there is no common scale to compare that peer's wait against.
  • Every other class keeps the in_flight / cap comparator unchanged.

serving_rate is used rather than the limiter's one second rate, which swings 0.02 to 0.28 MB/s from one sample to the next on one slow peer, or the Latency average, which already includes the queue being avoided. PeerHandle::serving_rate() is new and ConnectedPeer::serving_rate delegates to it.

A separate test commit lets ConnectedPeer::for_test peers write, so a send to a test peer no longer fails with a broken pipe. #1343 carries the same helper fix, so whichever PR merges second drops its copy.

Tests

  • route_picks_the_peer_by_class, three cases with a slow peer holding 1 MB at 100 KB/s (a tenth of its cap) and a fast peer holding 256 KB (half its cap):
    • A header request with both rates measured goes to the fast peer. It fails without queue time routing.
    • A header request while the fast peer is busy and unmeasured goes to the slow peer by cap ratio. It fails if an unmeasured peer counts as a 0 s queue.
    • A bulk getcfilters with both rates measured still goes to the slow peer.
  • cargo test -p dash-spv --lib: 642 passed.
  • just dash-spv::test: all green, including dashd_sync 38/38 and dashd_masternode 11/11.

Not covered

  • serving_rate warms up for about 30 s, so a busy new peer turns on the fallback for header picks until then.
  • fix(dash-spv): stop swapping peers on idle rates #1333 samples serving_rate only while a peer is bandwidth bound, so a peer that never fills its cap may take longer to get a rate. The two should be measured together.
  • getcfheaders still uses the cap ratio.

PR Hygiene · 0ccd376

  • Bots — coderabbitai skipped after its own rate limit
  • Self-review — posted; again after any push
  • Build green
  • Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

The listener in `ConnectedPeer::for_test` was dropped before anything was written, so the kernel reset the connection and sends from a test peer could fail with a broken pipe. A failed send also cancels the peer token. The far end is now accepted and drained, so tests can route real sends to a test peer.
The router sent every message to the peer with the lowest `in_flight / cap`. A slow peer with an inflated cap looks least loaded by that ratio, so a `getheaders` landed behind megabytes of filter and block backlog on it and header sync stalled for minutes while bulk downloads continued.

A header request now goes to the peer that will answer it soonest. Peers answer in order, so that is the lowest `in_flight / serving_rate`, the bytes it already holds over the rate it serves at while it has work. While a busy peer has no measured serving rate yet the router keeps the cap ratio, as it does for every other class.
@coderabbitai

coderabbitai Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 57 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 4e49ea9 and 0ccd376.

Files selected for processing: 2

  • dash-spv/src/network/broker.rs
  • dash-spv/src/network/peer.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@codecov

codecov Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.17%. Comparing base (4e49ea9) to head (0ccd376).
⚠️ Report is 3 commits behind head on dev.

Files with missing lines Patch % Lines
dash-spv/src/network/broker.rs 97.72% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1349   +/-   ##
=======================================
  Coverage   85.17%   85.17%           
=======================================
  Files         268      268           
  Lines       68101    68145   +44     
=======================================
+ Hits        58005    58045   +40     
- Misses      10096    10100    +4     
Flag Coverage Δ
core 82.43% <ø> (ø)
rpc 52.10% <ø> (ø)
spv 94.02% <98.00%> (-0.01%) ⬇️
wallet 83.12% <ø> (ø)
Files with missing lines Coverage Δ
dash-spv/src/network/peer.rs 90.32% <100.00%> (+0.32%) ⬆️
dash-spv/src/network/broker.rs 96.06% <97.72%> (-0.30%) ⬇️

... and 3 files with indirect coverage changes

@xdustinface
xdustinface marked this pull request as ready for review November 6, 2026 08:44
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Nov 6, 2026
@xdustinface

Copy link
Copy Markdown
Collaborator Author

/self-reviewed

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Policy satisfied — this can merge.
Full checklist in the description.

@github-actions github-actions Bot removed the waiting-self-review Waiting for the author to post /self-reviewed label Nov 6, 2026
@xdustinface
xdustinface requested a review from ZocoLini November 6, 2026 09:17
@ZocoLini
ZocoLini merged commit f5d2b9b into dev Nov 6, 2026
38 checks passed
@ZocoLini
ZocoLini deleted the fix/header-requests-behind-bulk branch November 6, 2026 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-review-skipped A required review bot did not report; it was skipped by the window or by a person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dash-spv: header requests wait behind megabytes of bulk data on the same peer

2 participants