fix(dash-spv): reliable peer selection and stall handling during sync - #943
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe network layer adds latency-aware peer selection, request-stall detection, guarded peer eviction, and revised reputation persistence. Peer discovery and equal-score selection now randomize candidates. Tests cover timing, routing, reputation, persistence, eviction, and filter lookahead behavior. ChangesPeer reputation model
Latency-aware peer management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PeerNetworkManager
participant PeerReader
participant PeerLatency
participant PeerReputationManager
PeerNetworkManager->>PeerReader: send classified request
PeerReader->>PeerNetworkManager: return response
PeerNetworkManager->>PeerLatency: record response latency
PeerNetworkManager->>PeerReputationManager: record request timeout
PeerNetworkManager->>PeerNetworkManager: route or evict peer
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/manager.rs (1)
1172-1191: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRun the stall sweep in exclusive mode as well.
sweep_stalled_peersis the only place that prunesoutstanding_requests(line 1024). In exclusive mode the sweep never runs, so two effects follow:
- Entries survive a disconnect. Arming uses
or_insert_with(line 1494), so a reconnected configured peer keeps its pre-disconnect timestamp. The next matching response recordssent.elapsed()measured from before the disconnect, which inflates that peer's latency mean and removes it from routing untilSAMPLE_TTLexpires.- Configured peers get no stall penalty and no stall latency sample, so latency-aware routing across several configured peers loses its input.
Move the sweep out of the
elsebranch. Keep eviction gated to non-exclusive mode.🐛 Proposed fix: sweep in both modes, evict only in non-exclusive mode
+ // The sweep is the only place `outstanding_requests` is pruned, so it must + // run in exclusive mode too or stale timers survive a reconnect. + let grace_tick = self.sweep_stalled_peers().await; if self.exclusive_mode { // In exclusive mode, only reconnect to originally specified peers for addr in self.initial_peers.iter() { if !self.pool.is_connected(addr).await && !self.pool.is_connecting(addr).await { tracing::info!("Reconnecting to exclusive peer: {}", addr); self.connect_to_peer(*addr).await; } } } else { - // Penalize peers stalling on sync requests so they drop out of routing - // and become eviction candidates before the top-up below. - let grace_tick = self.sweep_stalled_peers().await; // Evict peers that lack required services before top-up so replacements // can be pulled in during the same tick. self.evict_mismatched_peers().await;🤖 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 1172 - 1191, Move the sweep_stalled_peers call before the exclusive_mode conditional so it runs for both exclusive and non-exclusive modes. Preserve the returned grace_tick for the existing non-exclusive eviction logic, while keeping evict_mismatched_peers and evict_worst_stuck_peer gated to the non-exclusive branch.
🧹 Nitpick comments (3)
dash-spv/src/network/manager.rs (1)
1443-1453: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemove the panic path from
next_peer.Line 1451 computes
% eligible.len(), and line 1452 indexes the vector. Ifpeersis empty,eligibleis empty and both operations panic. The current callers check for an empty peer list first, so this is an invariant, not a live defect. The invariant is not enforced by the signature.Return
Option<(SocketAddr, Arc<RwLock<Peer>>)>and let each caller mapNoneto the existingNetworkError::ConnectionFailed("No connected peers"). This also removes the panic risk from thetest_next_peerhelper at line 1889, which passes the pool contents without an emptiness check.Based on learnings from the coding guidelines: "Avoid
unwrap()andexpect()in library code; use proper error types (e.g., viathiserror)" — the same reasoning applies to index and modulo operations that can panic on an empty slice.🤖 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 1443 - 1453, Change next_peer to return Option<(SocketAddr, Arc<RwLock<Peer>>)> and return None before modulo/indexing when eligible is empty; otherwise preserve the round-robin selection. Update every caller, including test_next_peer, to handle None by returning the existing NetworkError::ConnectionFailed("No connected peers").Source: Coding guidelines
dash-spv/src/network/tests.rs (1)
292-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two untested eviction guards.
The new tests cover the pool-floor, replacement, and all-peers-bad guards. Two guards in
evict_worst_stuck_peerhave no test:
EVICTION_COOLDOWN: a second call inside the cooldown window must not evict a second peer.is_sole_service_provider: the worst peer must survive when it is the only connected peer that advertisesrequired_services.Both are reachable with the existing helpers plus
tokio::time::advance.As per coding guidelines: "Implement comprehensive unit tests in-module for individual components using
#[cfg(test)]and integration tests in thetests/directory".🤖 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/tests.rs` around lines 292 - 346, Add tests for the remaining guards in evict_worst_stuck_peer: verify a second eviction attempt within EVICTION_COOLDOWN does not remove another peer, using tokio::time::advance to cover the cooldown boundary, and verify the worst peer is retained when is_sole_service_provider identifies it as the only connected peer advertising required_services. Reuse the existing test helpers and keep both tests in the module’s #[cfg(test)] suite.Source: Coding guidelines
dash-spv/src/network/latency.rs (1)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
tokio::time::Instantfor observation timestamps.Production calls pass durations from
tokio::time::Instant::elapsed(), butPeerLatencystores and checks timestamps withstd::time::Instant.tokio::time::advancedoes not advance the standard clock, so paused-time tests cannot exerciseSAMPLE_TTLexpiry. Usetokio::time::InstantforObservation,record,eligible, and expiry tests.🤖 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/latency.rs` around lines 9 - 11, Replace std::time::Instant with tokio::time::Instant throughout PeerLatency, including the Observation timestamp type and the record, eligible, and expiry-test logic. Keep Duration imports and existing timestamp comparisons unchanged so durations from tokio::time::Instant::elapsed() and tokio::time::advance-based expiry tests use the same clock.
🤖 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/reputation_tests.rs`:
- Around line 74-76: Replace the hard-coded addresses and ports used to
construct reputation-test peers with the shared test-address fixture. Update the
`AddrV2Message` setup for `clean_peer`, `other_clean_peer`, and `bad_peer`, plus
the `peer`, `slight`, `bad`, `unknown`, `near_ban`, and `offline` constructions
in dash-spv/src/network/reputation_tests.rs at lines 74-76, 116, 130-132, and
149-151; preserve each test’s intended peer distinctions while reusing the
fixture.
In `@dash-spv/src/network/reputation.rs`:
- Around line 381-385: Update the reputation refresh flow around the write guard
and save_peers_reputation: set each reputation.last_seen, clone the updated
reputations into a snapshot, explicitly release the reputations write lock, then
await save_peers_reputation using that snapshot. Ensure no storage I/O occurs
while the guard is held, preserving the updated values being persisted.
---
Outside diff comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 1172-1191: Move the sweep_stalled_peers call before the
exclusive_mode conditional so it runs for both exclusive and non-exclusive
modes. Preserve the returned grace_tick for the existing non-exclusive eviction
logic, while keeping evict_mismatched_peers and evict_worst_stuck_peer gated to
the non-exclusive branch.
---
Nitpick comments:
In `@dash-spv/src/network/latency.rs`:
- Around line 9-11: Replace std::time::Instant with tokio::time::Instant
throughout PeerLatency, including the Observation timestamp type and the record,
eligible, and expiry-test logic. Keep Duration imports and existing timestamp
comparisons unchanged so durations from tokio::time::Instant::elapsed() and
tokio::time::advance-based expiry tests use the same clock.
In `@dash-spv/src/network/manager.rs`:
- Around line 1443-1453: Change next_peer to return Option<(SocketAddr,
Arc<RwLock<Peer>>)> and return None before modulo/indexing when eligible is
empty; otherwise preserve the round-robin selection. Update every caller,
including test_next_peer, to handle None by returning the existing
NetworkError::ConnectionFailed("No connected peers").
In `@dash-spv/src/network/tests.rs`:
- Around line 292-346: Add tests for the remaining guards in
evict_worst_stuck_peer: verify a second eviction attempt within
EVICTION_COOLDOWN does not remove another peer, using tokio::time::advance to
cover the cooldown boundary, and verify the worst peer is retained when
is_sole_service_provider identifies it as the only connected peer advertising
required_services. Reuse the existing test helpers and keep both tests in the
module’s #[cfg(test)] suite.
🪄 Autofix
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 Plus
Run ID: bd318542-0425-4f2e-a893-7abb0a016a4a
📒 Files selected for processing (7)
dash-spv/src/network/discovery.rsdash-spv/src/network/latency.rsdash-spv/src/network/manager.rsdash-spv/src/network/mod.rsdash-spv/src/network/reputation.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/tests.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/manager.rs (1)
2010-2016: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the test helper aligned with the reader path.
test_deliver_responserecords block-transfer latency. The peer reader records latency only whenresponse_time_is_fair()is true, and that method excludesRequestKind::Blocks. This can conceal a routing regression in tests.Proposed fix
if let Some(kind) = timed_response_kind(msg) { let sent = self.outstanding_requests.lock().await.remove(&(addr, kind)); if let Some(sent) = sent { - self.latency.lock().await.record(addr, sent.elapsed()); + if kind.response_time_is_fair() { + self.latency.lock().await.record(addr, sent.elapsed()); + } } }🤖 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 2010 - 2016, Update test_deliver_response to match the peer reader’s response_time_is_fair behavior by excluding RequestKind::Blocks from latency recording. Keep outstanding-request removal intact, and record latency only for response kinds accepted by response_time_is_fair.
🤖 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 105-120: Replace the hardcoded REQUEST_STALL_TIMEOUT and
REQUEST_OWED_TIMEOUT constants in dash-spv/src/network/manager.rs:105-120 with
typed configuration fields and sensible defaults, then inject those values into
request routing. Also move the hardcoded block download timeout in
dash-spv/src/sync/blocks/pipeline.rs:20-25 into the download or client
configuration and use the configured value, ensuring all affected network timing
policy is configurable.
---
Outside diff comments:
In `@dash-spv/src/network/manager.rs`:
- Around line 2010-2016: Update test_deliver_response to match the peer reader’s
response_time_is_fair behavior by excluding RequestKind::Blocks from latency
recording. Keep outstanding-request removal intact, and record latency only for
response kinds accepted by response_time_is_fair.
🪄 Autofix
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 Plus
Run ID: 7b52e6af-b79f-47ed-899d-a600405d08f9
📒 Files selected for processing (4)
dash-spv/src/network/manager.rsdash-spv/src/network/tests.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/filters/manager.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #943 +/- ##
===========================================
+ Coverage 20.00% 75.55% +55.54%
===========================================
Files 4 329 +325
Lines 2069 79136 +77067
===========================================
+ Hits 414 59793 +59379
- Misses 1655 19343 +17688
|
ZocoLini
approved these changes
Aug 11, 2026
… low-IP nodes `discover_peers` sorts addresses to dedup them, so every client picked the same lowest-IP peers via the `take`/`truncate` callers, piling testers onto a handful of nodes and slowing sync for everyone. Shuffle the merged masternode/DNS list after dedup so clients fan out across the whole peer set. Also shuffle score-tied candidates in `select_best_peers` before the stable sort, so fresh installs (every peer at score 0) no longer converge on a fixed address order.
…decay The reputation score was dominated by noise. `ReadTimeout` fired on every idle socket read on healthy connections, and the only reward, `LongUptime`, was a fake loop-iteration heuristic rather than real elapsed time. Neither reflected whether a peer actually serves our requests. Remove both. Reward `ResponseDelivered` (-2) whenever a peer returns a substantive response to a sync request (headers, filter headers, filters, blocks, masternode diffs, quorum info), so productive peers rank above idle or stalling ones. Also harden persistence so stale suspicion cannot lock the client out of known-good peers. Persist a wall-clock `last_seen`, credit decay for the time the client was offline on load, clamp every loaded score to a restart ceiling so no peer returns one strike short of a ban, and drop the previous `.max(50)` bump that ratcheted suspicion the wrong way.
…round-robin `next_peer` rotated across all connected peers equally, so a slow or flaky peer kept an equal share of every request and could stall a sync phase. Bias selection toward the best reputation scores: peers within `GOOD_BAND_DELTA` of the best eligible score form a good band that still round-robins, so load spreads across the healthy peers while a clearly-worse peer drops out of rotation until decay recovers it. Add `scores_for` to read decayed scores under the reputation lock alone. The caller already holds owned `Arc`s rather than a pool guard, so this keeps a single lock order and cannot deadlock against the pool lock.
A peer that accepted a sync request and then went silent was invisible to scoring. It kept its slot, kept getting an equal share of requests, and was never blamed, so the client could stay stuck on it. Track the instant each connected peer's oldest unanswered request was sent, keyed in the network manager. Any substantive response clears it. The maintenance sweep penalizes a peer whose request stayed unanswered past `REQUEST_STALL_TIMEOUT`, which drops it out of the routing good band and makes it an eviction candidate, so retries land on a better-scoring peer. Guard against false blame. A response of any kind resets the peer's timer, so a peer streaming a large block is not punished for the download taking a while, and a maintenance gap larger than `SUSPEND_GAP` (the shape of a backgrounded iOS app) refreshes all timers and skips penalties for that round instead of blaming every peer at once. The sweep runs only outside exclusive mode, so explicitly configured peers are never scored down and refused.
Bad peers were only removed on hard failures (ping timeout, decode error, capability mismatch). A peer that pongs fine but stalls on sync requests kept its slot forever, so the client stayed stuck on it. Once such a peer accrues stall penalties, drop the single worst-scoring connected peer during maintenance so the existing address-book top-up pulls in a fresh replacement the same tick. Heavily guarded so a small pool (default `max_peers` 3) can never churn toward zero. Keep a connection floor, only act on a full pool with a replacement candidate ready, skip when every peer is equally bad (a network or device problem rather than this peer), never drop the sole peer providing a required service, evict at most one peer per cooldown, and never on a suspend/resume grace tick.
Add unit coverage for the new peer-selection behavior: the `RequestTimeout` stall penalty and its ban threshold, `scores_for` including the default-zero for an unknown peer, load-time score clamping to the restart ceiling plus offline decay crediting, reputation-biased `next_peer` excluding a low-scoring peer while spreading load across the good band, and every eviction guard (removes the stalling peer, but skips without a replacement, when all peers are equally bad, and when the pool is not full).
Integration testing surfaced a regression. `next_peer` took the reputation write lock via `scores_for` on every send, and the reader rewarded on every inbound message. Under the send flood that follows a mid-sync peer disconnect, those writes saturated the reputation lock and starved the reader loop, so it took seconds instead of milliseconds to notice the closed socket and emit `PeerDisconnected`, which broke reconnection-under-disconnect sync. Make `scores_for` a read-only lookup (decay still runs in `update_reputation` and the periodic maintenance pass), and reward a peer only when a response actually clears a tracked request. The reputation write lock now stays off both hot paths.
The stall timeout was dictated by the worst case (a multi-megabyte block or masternode diff), so a peer could sit unresponsive for 45s before being blamed. Headers, filter headers and filters are tens of KB and a healthy peer answers in well under a second, so time only those and drop the threshold to 10s. Large payloads are deliberately no longer stall-tracked: a peer sends nothing until such a transfer completes, so timing them would punish honest peers on slow links, and the sync layer's own download timeouts already retry them elsewhere. Re-arm a stalling peer's timer instead of dropping it. One stall already freezes a peer out of routing via `GOOD_BAND_DELTA`, so it receives no new request to re-arm the timer and could never earn a second strike. That left `STUCK_PEER_EVICTION_SCORE` unreachable and the peer squatting a connection slot while the full pool blocked top-up from replacing it. With re-arming, two consecutive unanswered windows evict and replace it in ~20s. Stop accruing strikes once a peer already sits at the eviction threshold, so a peer the guards decline to evict (e.g. at the connection floor) cannot ratchet itself to an outright ban.
…pool A mainnet run routed 98% of its requests to a single peer. The other two were healthy, answering every request they got, but stopped receiving any four seconds in and never recovered for the rest of the run. `next_peer` ranked peers by the misbehavior score, and `ChangeReason::ResponseDelivered` credited every answer against it. That made the score a ratchet: answering earned a peer more traffic, more traffic earned it more credit, and once it led by more than `GOOD_BAND_DELTA` the rest fell out of the band. A starved peer receives nothing, so it can never answer, so it can never earn its way back. Three answers of head start were enough to make that permanent, which is what the log shows. The two roles are now separate. `latency` measures how fast each peer actually answers, and routing rotates over every peer within reach of the fastest. The measurement is bounded by what a peer did rather than by how often it was picked, and it expires, so a peer that is out of rotation becomes unmeasured and is retried instead of starved. The `Instant` needed for it was already stored in `outstanding_requests` and previously discarded. A stalling peer is recorded with the time its request has gone unanswered, which keeps its mean both bad and fresh, so it stays out of rotation while it is still failing and is evicted rather than probed. The score reverts to pure misbehavior, so its floor is now zero. Letting it run negative banked credit a peer could later spend: decay alone carried a long-lived peer toward -50, and it would then need seven stalls rather than two to reach eviction, making the peers most likely to be quietly failing the hardest ones to remove. Scores persisted by earlier versions clamp up quietly, since a peer having behaved well is no reason to warn. `is_timed_response` drops block, masternode-diff and quorum-info responses to match `is_tracked_request`, which no longer arms a timer for them.
… hide `outstanding_requests` held one timer per peer and any timed response cleared it, so a peer fast at one request kind concealed being slow at another: its quick responses cleared the timer a slow request had armed, the slow response then found nothing to clear and was never recorded, and the sweep never saw an aged entry to penalize. A mainnet peer averaging 11.9s on filter headers, with a 32s p90 and a 65s worst case, was measured at 52ms. It took 40% of the traffic and never earned a single stall penalty, holding the filter-header phase at 595s against 274s for the same work on a healthy pool. Only 255 of its 1050 filter-header timers were cleared by an actual `cfheaders`, the other 780 by an unrelated `cfilter` that happened to arrive first. Timers are now keyed by `(peer, RequestKind)`, so a response only clears a request it answers. Replaying the same log through this finds 6 stalls on that peer where the old code found none, which is past the eviction threshold. A sweep still costs a peer at most one strike: stalling on several kinds at once is one failing peer, not several. Latency is still averaged per peer rather than per kind, so routing on its own would not have demoted this peer, only eviction removes it. Routing each kind by that kind's measured latency is the remaining gap.
`add_peer` fails in exactly two ways, a full pool and an address already connected, and both are ordinary outcomes of dialling several candidates for one slot. Topping up after an eviction does precisely that, so every eviction logged an `ERROR` for the dials that arrived second. Nothing was wrong in those runs, but an `ERROR` line is what a tester reports, so the noise costs real triage time on a beta.
…t response A peer that silently discards a `getdata` is invisible to latency-based routing. Latency is only ever recorded from a response, so a peer that answers nothing is never measured, keeps its turn in the rotation, and then wins the retry of the request it just dropped. Seen on a mainnet sync: one of three peers dropped every block request it received. Filter batches commit in order and cannot commit while a matched block is outstanding, so the client spent 66% of a 16 minute run frozen, in stalls of exactly one, two and four `BLOCK_TIMEOUT` periods. Track block `getdata` as a request kind and skip peers that still owe a response of the kind being sent. The bar is what the peer owes rather than what it did wrong, so this needs no judgement about how slow is too slow, and it clears itself the moment the peer answers. Skipping stays a preference: if every candidate owes us something, the full set is used, because refusing to send is worse than sending to a busy peer. Block timings are tracked but never scored. A block body is megabytes and the peer sends nothing until the transfer completes, so its elapsed time measures the payload rather than the peer. Scoring it would punish an honest peer on a slow link and, because response times feed one shared routing metric, push that peer out of serving the filters and headers it was answering perfectly well. A `getdata` only arms the block timer when it actually asks for blocks. The mempool sends `getdata` for transactions, which a `block` response would never clear, so timing those would leave a peer owing a response it was never asked for.
A filter batch cannot commit while any block it matched is still outstanding, so this timeout is the price of a single peer dropping a request, and it is paid with filter sync stopped rather than merely slowed. 30s stays well past the point where a peer is plausibly still transferring: it treats a peer that will never answer as merely slow. 15s is still comfortably above the transfer time of a full block on a slow link, and halves the cost of each bad draw.
…stant `test_max_lookahead_constant` asserted `MAX_LOOKAHEAD_BATCHES == 3`, which only fails when someone edits the constant and tells you nothing about what lookahead does. Replace it with a test that lookahead actually fills to the cap, stops there while the batches are uncommitted, and tiles the range contiguously from the processing head. Document why the cap cannot simply be raised. Every scanned batch queues its matched blocks into one FIFO download window, so a deeper lookahead fills that window with blocks belonging to batches that cannot commit for a long time, ahead of the blocks the committing batch is waiting on. Raising it to 6 made `test_runtime_add_during_initial_sync` time out with `committed_height` still at 0.
ZocoLini
force-pushed
the
fix/spv-sync-improvements
branch
from
August 11, 2026 09:54
5d99ca9 to
9b8936a
Compare
ZocoLini
added a commit
that referenced
this pull request
Aug 11, 2026
The old network module gave each sync manager an `on_peer_disconnect` hook that requeued its own in-flight work, and three separate fixes landed against it (#941, #943, #953) — one for the block pipeline, one for progress being discarded along with the requeue, one for requeued work never being reissued. The broker owns a request from send to response, so it replaced all three hooks with a single central requeue, and their regression tests went with the hooks: the replacement path had no coverage at all, in the area with the worst track record. Both callers — the timeout monitor kicking a stalled peer and the pump seeing a socket close — did this inline and identically, buried in spawned tasks where nothing could reach them. Lift it into `requeue_requests_from` and pin the three properties the old tests guarded: - a departed peer's requests come back, a healthy peer's do not - the key stays registered as `Queued`, so a pipeline re-declaring the request cannot queue a duplicate on top of the retry - only the response retires the key, so a requeued request stays owned by someone Checked against injected regressions: dropping the key instead of requeuing it fails two of the three, and requeuing nothing fails all three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ToH2xGXqVcxiwMkNYaWkh7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Peer selection was blind round-robin over whatever peers connected first. Slow or stalling peers received as much sync work as good ones, could hide behind fast request kinds, and were never evicted, so a single bad peer degraded or stalled the whole sync. Deterministic initial selection also herded fresh clients onto the same low-IP nodes.
Fix
Covered by new tests for scoring, biased routing, and guarded eviction. All dashd integration tests pass.
Summary by CodeRabbit