perf(dash-spv): prototype of the new peer selection algorithm#865
Open
ZocoLini wants to merge 1 commit into
Open
perf(dash-spv): prototype of the new peer selection algorithm#865ZocoLini wants to merge 1 commit into
ZocoLini wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughPeer reputation now uses behavior multipliers and RTT hints. Peer selection, connection establishment, maintenance, scouting, and request routing use latency, service flags, stalling state, and reputation outcomes. ChangesPeer reputation and quality
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant NetworkManager
participant PeerReputationManager
participant PeerPool
participant Peer
NetworkManager->>PeerReputationManager: load reputation hint
NetworkManager->>Peer: establish connection and probe RTT
Peer-->>NetworkManager: latency and behavior samples
NetworkManager->>PeerReputationManager: record multiplier and median RTT
NetworkManager->>PeerPool: send network request
PeerPool->>Peer: select and dispatch request
Peer-->>PeerPool: response or send result
PeerPool-->>NetworkManager: routing result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
ZocoLini
changed the title
ZocoLini
force-pushed
the
perf/peer-selection
branch
from
b83814d to
fd58fe1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #865 +/- ##
==========================================
+ Coverage 73.62% 73.80% +0.18%
==========================================
Files 324 324
Lines 73374 73572 +198
==========================================
+ Hits 54018 54303 +285
+ Misses 19356 19269 -87
|
![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: 8
🤖 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 798-804: Update the reader lifecycle around the peer handling flow
to capture an Instant when the reader starts, then use its elapsed duration for
the LongUptime threshold in the apply_reason block. Replace the
loop_iteration-based calculation while preserving the existing one-hour
comparison and reward behavior.
- Around line 1128-1143: Update the pong branch in the receive loop to count
each pending probe nonce only once: after confirming the nonce is pending, call
peer.handle_pong(*n) and remove the nonce from pending only when handling
succeeds, incrementing answered only for that successful removal. Preserve the
existing handling for pings and other messages.
- Around line 37-59: Centralize the hardcoded peer-selection and scoring
parameters in configuration: update dash-spv/src/network/manager.rs lines 37-59
to configure probe counts, wait duration, scout interval, batch size, and
latency-tier thresholds; update dash-spv/src/network/reputation.rs lines 37-48
to configure behavior-event weights; and update dash-spv/src/network/peer.rs
lines 56-70 to configure RTT buckets, sample count, and stall threshold, then
have the relevant manager, reputation, and peer logic consume those
configuration values.
- Around line 1054-1060: Update the low_quality peer handling around
replace_peer so stalled peers are disconnected in exclusive mode as well,
allowing the reconnect path to redial them and preventing selection from
retaining a stalled connection. Preserve the existing replacement behavior for
non-exclusive mode while ensuring exclusive mode performs the required
disconnect without attempting non-exclusive peer swapping.
In `@dash-spv/src/network/peer.rs`:
- Around line 861-871: The peer request-tracking flow must correlate responses
with individual outstanding requests instead of using one unconditional timer.
In dash-spv/src/network/peer.rs:861-871, update the peer state and
note_request_sent/note_response behavior to track a request identity and
deadline, or enforce a single in-flight request; in
dash-spv/src/network/manager.rs:659-663, clear the pending request and reward
only when Headers2 matches it; in dash-spv/src/network/manager.rs:718-724, stop
clearing or rewarding for arbitrary forwarded messages.
In `@dash-spv/src/network/pool.rs`:
- Around line 38-40: Update the NetworkMessage capability mapping so only
ordinary GetHeaders permits fallback to arbitrary peers; explicit GetHeaders2
must require ServiceFlags::NODE_HEADERS_COMPRESSED and disable fallback. Keep
the existing compressed-header capability requirement while changing the
fallback behavior for the GetHeaders2 variant.
- Around line 267-306: Update select_best eligibility so that when all probed
peers are stalling, live unprobed peers are eligible before any stalled probed
peer. Preserve the existing preference for non-stalling probed peers, and retain
the single-peer fallback so selection never becomes empty when no live peer
exists.
In `@dash-spv/src/network/reputation.rs`:
- Around line 1-8: Update the pull request title to use the allowed feat prefix,
specifically “feat(dash-spv): …”, instead of “perf(...)”.
🪄 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: 3b70d4ef-4511-4add-9837-27cbcdd95aa0
📒 Files selected for processing (5)
dash-spv/src/network/manager.rsdash-spv/src/network/peer.rsdash-spv/src/network/pool.rsdash-spv/src/network/reputation.rsdash-spv/src/network/reputation_tests.rs
Comment on lines
+37
to
+59
| /// Latency probes fired right after handshake to seed peer scoring. | ||
| const INITIAL_PROBE_PINGS: usize = 3; | ||
|
|
||
| /// How long to wait for the initial probe pongs before scoring the peer. | ||
| const INITIAL_PROBE_WAIT: Duration = Duration::from_millis(1200); | ||
|
|
||
| /// How often to probe known-but-unconnected peers looking for better ones. | ||
| const SCOUT_INTERVAL: Duration = Duration::from_secs(45); | ||
|
|
||
| /// Peers to probe per scout tick. | ||
| const SCOUT_BATCH: usize = 4; | ||
|
|
||
| /// Latency tier (lower is better): the band we rank and keep peers by. We only dial peers | ||
| /// in the best-known tier or one worse, and drop a connected peer more than one tier below | ||
| /// the best in the pool — so a genuinely slow peer never takes a slot. `<=50ms`=0, | ||
| /// `<=100ms`=1, `<=150ms`=2, else 3. | ||
| fn latency_tier(latency_ms: u32) -> u8 { | ||
| match latency_ms { | ||
| 0..=50 => 0, | ||
| 51..=100 => 1, | ||
| 101..=150 => 2, | ||
| _ => 3, | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Centralize the hardcoded peer-selection policy in configuration.
dash-spv/src/network/manager.rs#L37-L59: configure probe counts, waits, scout frequency, batch size, and latency tiers.dash-spv/src/network/reputation.rs#L37-L48: configure behavior-event weights.dash-spv/src/network/peer.rs#L56-L70: configure RTT buckets, sample count, and stall threshold.
As per coding guidelines, “Never hardcode network parameters, addresses, or keys.”
📍 Affects 3 files
dash-spv/src/network/manager.rs#L37-L59(this comment)dash-spv/src/network/reputation.rs#L37-L48dash-spv/src/network/peer.rs#L56-L70
🤖 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 37 - 59, Centralize the
hardcoded peer-selection and scoring parameters in configuration: update
dash-spv/src/network/manager.rs lines 37-59 to configure probe counts, wait
duration, scout interval, batch size, and latency-tier thresholds; update
dash-spv/src/network/reputation.rs lines 37-48 to configure behavior-event
weights; and update dash-spv/src/network/peer.rs lines 56-70 to configure RTT
buckets, sample count, and stall threshold, then have the relevant manager,
reputation, and peer logic consume those configuration values.
Source: Coding guidelines
Comment on lines
+798
to
+804
| // Persist the peer's final behaviour and latency before removing it. | ||
| if let Some(peer) = pool.get_peer(&addr).await { | ||
| let mut guard = peer.write().await; | ||
| // Reward a long, healthy connection. | ||
| if Duration::from_secs(60 * loop_iteration) > Duration::from_secs(3600) { | ||
| guard.apply_reason(ChangeReason::LongUptime); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Measure long uptime with elapsed time, not loop iterations.
loop_iteration increments on every read poll, not once per minute. Busy peers can receive the one-hour reward almost immediately; capture an Instant when the reader starts.
🤖 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 798 - 804, Update the reader
lifecycle around the peer handling flow to capture an Instant when the reader
starts, then use its elapsed duration for the LongUptime threshold in the
apply_reason block. Replace the loop_iteration-based calculation while
preserving the existing one-hour comparison and reward behavior.
Comment on lines
+1054
to
+1060
| // Replace peers that stalled, swapping in the best known unconnected peer and | ||
| // probing more to find a permanent one. | ||
| if !self.exclusive_mode { | ||
| for addr in low_quality { | ||
| self.replace_peer(addr).await; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Disconnect stalled peers in exclusive mode so they can reconnect.
low_quality peers are handled only outside exclusive mode. In exclusive mode they remain connected, preventing the reconnect branch from redialing them and allowing selection to keep falling back to the stalled connection.
🤖 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 1054 - 1060, Update the
low_quality peer handling around replace_peer so stalled peers are disconnected
in exclusive mode as well, allowing the reconnect path to redial them and
preventing selection from retaining a stalled connection. Preserve the existing
replacement behavior for non-exclusive mode while ensuring exclusive mode
performs the required disconnect without attempting non-exclusive peer swapping.
Comment on lines
+1128
to
+1143
| match tokio::time::timeout(remaining, peer.receive_message()).await { | ||
| Ok(Ok(Some(message))) => match message.inner() { | ||
| NetworkMessage::Pong(n) if pending.contains(n) => { | ||
| let _ = peer.handle_pong(*n); | ||
| answered += 1; | ||
| } | ||
| NetworkMessage::Ping(p) => { | ||
| let _ = peer.handle_ping(*p).await; | ||
| } | ||
| _ => {} | ||
| }, | ||
| _ => break, | ||
| } | ||
| } | ||
| for _ in answered..pending.len() { | ||
| peer.record_rtt(INITIAL_PROBE_WAIT); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Count each probe nonce at most once.
pending.contains(n) stays true after handle_pong removes the nonce, and its result is ignored. Replaying one valid pong can increment answered repeatedly and avoid the missing-probe penalties. Remove each nonce from pending only after successful handling.
🤖 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 1128 - 1143, Update the pong
branch in the receive loop to count each pending probe nonce only once: after
confirming the nonce is pending, call peer.handle_pong(*n) and remove the nonce
from pending only when handling succeeds, incrementing answered only for that
successful removal. Preserve the existing handling for pings and other messages.
Comment on lines
+861
to
+871
| /// Note that a request expecting a response was sent to this peer. | ||
| pub fn note_request_sent(&mut self) { | ||
| if self.awaiting_since.is_none() { | ||
| self.awaiting_since = Some(Instant::now()); | ||
| } | ||
| } | ||
|
|
||
| /// Note that the peer answered with data, clearing any pending-request timer. | ||
| pub fn note_response(&mut self) { | ||
| self.awaiting_since = None; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Correlate responses with individual outstanding requests.
The single timer plus unconditional response handling lets unrelated traffic clear every pending request and earn reputation.
dash-spv/src/network/peer.rs#L861-L871: track request identity and deadline, or enforce one in-flight request per peer.dash-spv/src/network/manager.rs#L659-L663: clear/reward only whenHeaders2matches the pending request.dash-spv/src/network/manager.rs#L718-L724: do not clear/reward for arbitrary forwarded messages.
📍 Affects 2 files
dash-spv/src/network/peer.rs#L861-L871(this comment)dash-spv/src/network/manager.rs#L659-L663dash-spv/src/network/manager.rs#L718-L724
🤖 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 861 - 871, The peer
request-tracking flow must correlate responses with individual outstanding
requests instead of using one unconditional timer. In
dash-spv/src/network/peer.rs:861-871, update the peer state and
note_request_sent/note_response behavior to track a request identity and
deadline, or enforce a single in-flight request; in
dash-spv/src/network/manager.rs:659-663, clear the pending request and reward
only when Headers2 matches it; in dash-spv/src/network/manager.rs:718-724, stop
clearing or rewarding for arbitrary forwarded messages.
Comment on lines
+38
to
+40
| NetworkMessage::GetHeaders(_) | NetworkMessage::GetHeaders2(_) => { | ||
| Some((ServiceFlags::NODE_HEADERS_COMPRESSED, false)) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require compressed-header support for explicit GetHeaders2.
When no capable peer exists, this currently falls back to arbitrary peers and sends them GetHeaders2. Only ordinary GetHeaders should permit fallback.
Proposed fix
- NetworkMessage::GetHeaders(_) | NetworkMessage::GetHeaders2(_) => {
+ NetworkMessage::GetHeaders(_) => {
Some((ServiceFlags::NODE_HEADERS_COMPRESSED, false))
}
+ NetworkMessage::GetHeaders2(_) => {
+ Some((ServiceFlags::NODE_HEADERS_COMPRESSED, true))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| NetworkMessage::GetHeaders(_) | NetworkMessage::GetHeaders2(_) => { | |
| Some((ServiceFlags::NODE_HEADERS_COMPRESSED, false)) | |
| } | |
| NetworkMessage::GetHeaders(_) => { | |
| Some((ServiceFlags::NODE_HEADERS_COMPRESSED, false)) | |
| } | |
| NetworkMessage::GetHeaders2(_) => { | |
| Some((ServiceFlags::NODE_HEADERS_COMPRESSED, true)) | |
| } |
🤖 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/pool.rs` around lines 38 - 40, Update the NetworkMessage
capability mapping so only ordinary GetHeaders permits fallback to arbitrary
peers; explicit GetHeaders2 must require ServiceFlags::NODE_HEADERS_COMPRESSED
and disable fallback. Keep the existing compressed-header capability requirement
while changing the fallback behavior for the GetHeaders2 variant.
Comment on lines
+267
to
+306
| /// Pick the best peer: once any peer has a latency sample, only probed peers | ||
| /// are eligible (a still-unprobed or silent peer is skipped); among those it | ||
| /// takes the highest quality (latency score × behaviour multiplier), breaking | ||
| /// ties by lowest median RTT. | ||
| async fn select_best( | ||
| &self, | ||
| candidates: &[(SocketAddr, Arc<RwLock<Peer>>)], | ||
| ) -> Option<(SocketAddr, Arc<RwLock<Peer>>)> { | ||
| if candidates.is_empty() { | ||
| return None; | ||
| } | ||
| let mut entries = Vec::with_capacity(candidates.len()); | ||
| for (addr, peer) in candidates { | ||
| let (probed, stalling, quality, median) = { | ||
| let guard = peer.read().await; | ||
| ( | ||
| guard.median_rtt().is_some(), | ||
| guard.is_stalling(), | ||
| guard.quality(), | ||
| guard.median_rtt(), | ||
| ) | ||
| }; | ||
| entries.push(( | ||
| *addr, | ||
| peer.clone(), | ||
| probed, | ||
| stalling, | ||
| quality, | ||
| median.unwrap_or(Duration::MAX), | ||
| )); | ||
| } | ||
|
|
||
| // Prefer non-stalling, probed peers; fall back only if none qualify so a | ||
| // single (even stalling) peer still gets used rather than stalling forever. | ||
| let any_probed = entries.iter().any(|e| e.2); | ||
| let any_live = entries.iter().any(|e| (!any_probed || e.2) && !e.3); | ||
| entries | ||
| .into_iter() | ||
| .filter(|e| (!any_probed || e.2) && (!any_live || !e.3)) | ||
| .max_by(|a, b| a.4.total_cmp(&b.4).then_with(|| b.5.cmp(&a.5))) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fall back to a live unprobed peer before a stalled probed peer.
If every probed peer is stalled, any_probed still excludes all unprobed peers and the filter retains the stalled set. This can repeatedly route through a hung peer despite a live alternative.
Proposed eligibility logic
- let any_probed = entries.iter().any(|e| e.2);
- let any_live = entries.iter().any(|e| (!any_probed || e.2) && !e.3);
+ let any_live_probed = entries.iter().any(|e| e.2 && !e.3);
+ let any_live = entries.iter().any(|e| !e.3);
entries
.into_iter()
- .filter(|e| (!any_probed || e.2) && (!any_live || !e.3))
+ .filter(|e| {
+ if any_live_probed {
+ e.2 && !e.3
+ } else if any_live {
+ !e.3
+ } else {
+ true
+ }
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Pick the best peer: once any peer has a latency sample, only probed peers | |
| /// are eligible (a still-unprobed or silent peer is skipped); among those it | |
| /// takes the highest quality (latency score × behaviour multiplier), breaking | |
| /// ties by lowest median RTT. | |
| async fn select_best( | |
| &self, | |
| candidates: &[(SocketAddr, Arc<RwLock<Peer>>)], | |
| ) -> Option<(SocketAddr, Arc<RwLock<Peer>>)> { | |
| if candidates.is_empty() { | |
| return None; | |
| } | |
| let mut entries = Vec::with_capacity(candidates.len()); | |
| for (addr, peer) in candidates { | |
| let (probed, stalling, quality, median) = { | |
| let guard = peer.read().await; | |
| ( | |
| guard.median_rtt().is_some(), | |
| guard.is_stalling(), | |
| guard.quality(), | |
| guard.median_rtt(), | |
| ) | |
| }; | |
| entries.push(( | |
| *addr, | |
| peer.clone(), | |
| probed, | |
| stalling, | |
| quality, | |
| median.unwrap_or(Duration::MAX), | |
| )); | |
| } | |
| // Prefer non-stalling, probed peers; fall back only if none qualify so a | |
| // single (even stalling) peer still gets used rather than stalling forever. | |
| let any_probed = entries.iter().any(|e| e.2); | |
| let any_live = entries.iter().any(|e| (!any_probed || e.2) && !e.3); | |
| entries | |
| .into_iter() | |
| .filter(|e| (!any_probed || e.2) && (!any_live || !e.3)) | |
| .max_by(|a, b| a.4.total_cmp(&b.4).then_with(|| b.5.cmp(&a.5))) | |
| /// Pick the best peer: once any peer has a latency sample, only probed peers | |
| /// are eligible (a still-unprobed or silent peer is skipped); among those it | |
| /// takes the highest quality (latency score × behaviour multiplier), breaking | |
| /// ties by lowest median RTT. | |
| async fn select_best( | |
| &self, | |
| candidates: &[(SocketAddr, Arc<RwLock<Peer>>)], | |
| ) -> Option<(SocketAddr, Arc<RwLock<Peer>>)> { | |
| if candidates.is_empty() { | |
| return None; | |
| } | |
| let mut entries = Vec::with_capacity(candidates.len()); | |
| for (addr, peer) in candidates { | |
| let (probed, stalling, quality, median) = { | |
| let guard = peer.read().await; | |
| ( | |
| guard.median_rtt().is_some(), | |
| guard.is_stalling(), | |
| guard.quality(), | |
| guard.median_rtt(), | |
| ) | |
| }; | |
| entries.push(( | |
| *addr, | |
| peer.clone(), | |
| probed, | |
| stalling, | |
| quality, | |
| median.unwrap_or(Duration::MAX), | |
| )); | |
| } | |
| // Prefer non-stalling, probed peers; fall back only if none qualify so a | |
| // single (even stalling) peer still gets used rather than stalling forever. | |
| let any_live_probed = entries.iter().any(|e| e.2 && !e.3); | |
| let any_live = entries.iter().any(|e| !e.3); | |
| entries | |
| .into_iter() | |
| .filter(|e| { | |
| if any_live_probed { | |
| e.2 && !e.3 | |
| } else if any_live { | |
| !e.3 | |
| } else { | |
| true | |
| } | |
| }) | |
| .max_by(|a, b| a.4.total_cmp(&b.4).then_with(|| b.5.cmp(&a.5))) |
🤖 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/pool.rs` around lines 267 - 306, Update select_best
eligibility so that when all probed peers are stalling, live unprobed peers are
eligible before any stalled probed peer. Preserve the existing preference for
non-stalling probed peers, and retain the single-peer fallback so selection
never becomes empty when no live peer exists.
Comment on lines
+1
to
+8
| //! Peer behaviour scoring for selection. | ||
| //! | ||
| //! This module implements a reputation system to track peer behavior and protect | ||
| //! against malicious peers. It tracks both positive and negative behaviors, | ||
| //! implements automatic banning for excessive misbehavior, and provides reputation | ||
| //! decay over time for recovery. | ||
| //! Each peer carries a behaviour multiplier in `[0, 1]`. Good actions nudge it up | ||
| //! toward 1, bad actions down toward 0, and a security violation (the peer fed us | ||
| //! invalid data — it lied) drops it straight to 0. Peer selection multiplies this | ||
| //! by the peer's response-time score, so a misbehaving peer is deprioritised and a | ||
| //! lying one is never chosen. This manager persists the multiplier (and last | ||
| //! measured latency) so the judgement survives across connections. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an allowed PR title prefix.
perf(...) is rejected by pr-title.yml. Since this PR adds peer-selection functionality, use feat(dash-spv): ....
As per path instructions, “the PR title matches one of: build, chore, ci, docs, feat, fix, refactor, test.”
🤖 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/reputation.rs` around lines 1 - 8, Update the pull
request title to use the allowed feat prefix, specifically “feat(dash-spv): …”,
instead of “perf(...)”.
Source: Path instructions
Member
ReviewWell-written, well-commented code with a sensible core idea, but it carries one availability bug I'd call blocking, plus several robustness regressions versus the system it replaces. Appropriate for a prototype; not mergeable as-is. OverviewThe PR replaces the score-based reputation/ban system with a simpler model (+902/−718 across 5 files in
I checked out the branch: it builds and all 35 network unit tests pass. The removed public APIs have no other callers in the repo, and old persisted reputation data degrades gracefully to defaults thanks to Critical issue: peers zero out permanently, and the client can brick itselfThe multiplier has no recovery path once it reaches 0:
Minimum fix: reintroduce time-based decay/expiry (or a floor > 0 for non-security penalties), and treat transport-level failures (can't connect) very differently from protocol violations. Other correctness issues
Design concerns
Test coverageUnit tests are decent for the new primitives (scoring buckets, tie-breaks, bounded samples, rank ordering, persistence round-trip) and pass locally. But none of the interesting behavior — inline probe, pre-pool rejection, stall→replace, tier-band dropping, scout loop — is covered, and these are exactly the paths the dashd integration suite exists for. The offline-bricking and exclusive-mode scenarios above would make excellent integration tests ( What's good
VerdictThe direction — measure latency, score behavior, keep the best peers — is right, and the code craftsmanship is above average. But the trust model regressed from "penalize with decay and time-limited bans" to "permanent unrecoverable blacklist persisted to disk," which under ordinary conditions (a couple of minutes offline) can permanently brick the client. That, plus the unproven perf claim and missing integration coverage, is why I land at 5/10 for merge-readiness — as a prototype for discussion, it does its job. 🤖 Generated with Claude Code |
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.
Summary by CodeRabbit
New Features
Bug Fixes