refactor(dash-spv): reputation module cleanup

[ZocoLini]

Refactors the reputation system from manual (score: i32, reason: &str) calls to a typed ChangeReason enum, and removes dead code surfaced along the way. No behavior change

• Replace ad-hoc score/string arguments with a ChangeReason enum that owns each reason's score delta and label; all update_reputation call sites now pass a typed variant.
• Drop the misbehavior_scores / positive_scores constant modules (folded into ChangeReason::score()).
• Remove the write-only event subsystem (ReputationEvent, recent_events, record_event, get_recent_events) — nothing ever read it.
• Remove unused methods: reset_reputation, get_peers_by_reputation, get_score, temporary_ban_peer.
• Make the reputation module private again.

Summary by CodeRabbit

• New Features
  • Reputation changes now use clearer, standardized reasons across peer connection, messaging, and ban actions.
• Bug Fixes
  • Improved consistency in how peer reputation updates are applied and logged.
  • Fixed reputation-related behavior in peer persistence and selection flows to better match expected scores after save/load.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e0ff9e47-41e4-4c36-bfbb-de507bb1f5fa

📥 Commits

Reviewing files that changed from the base of the PR and between e4c1f51 and b169873.

📒 Files selected for processing (5)

• dash-spv/src/network/manager.rs
• dash-spv/src/network/mod.rs
• dash-spv/src/network/reputation.rs
• dash-spv/src/network/reputation_tests.rs
• dash-spv/src/storage/peers.rs

✅ Files skipped from review due to trivial changes (1)

• dash-spv/src/storage/peers.rs

---

📝 Walkthrough

Walkthrough

Replaces numeric misbehavior/positive score constants and string-based reasons with a typed ChangeReason enum throughout peer reputation handling. PeerReputationManager::update_reputation now takes ChangeReason, event tracking and several query methods are removed, module visibility is tightened, tests are updated, and a peer storage import/closure fix is included.

Changes

Typed reputation change flow

Layer / File(s)	Summary
ChangeReason enum and score/Display mapping dash-spv/src/network/reputation.rs	Adds ChangeReason enum with a score() method for signed reputation deltas and a Display impl for human-readable labels, replacing the prior constant-based scoring modules.
PeerReputationManager refactor dash-spv/src/network/reputation.rs	update_reputation now accepts ChangeReason and computes the delta internally; removes event tracking (ReputationEvent, recent_events) and public methods get_score, temporary_ban_peer, get_recent_events, reset_reputation, get_peers_by_reputation.
Network manager call sites dash-spv/src/network/manager.rs	All update_reputation calls (handshake/connection failures, headers2 decompression failure, read timeout, invalid transaction, long uptime, ping failure, manual ban) switch to typed ChangeReason variants.
Module visibility dash-spv/src/network/mod.rs	reputation module becomes private; PeerReputation re-export visibility changes to pub(crate).
Reputation tests dash-spv/src/network/reputation_tests.rs	Adds a score() helper and rewrites tests to use ChangeReason updates, adjusting the expected persisted score for peer2.

Peer storage import and closure fix

Layer / File(s)	Summary
Import path and closure-local vector dash-spv/src/storage/peers.rs	Updates PeerReputation import path and moves the peers vector declaration inside the spawn_blocking closure in load_peers.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant NetworkManager
  participant PeerReputationManager
  participant ChangeReason
  NetworkManager->>PeerReputationManager: update_reputation(addr, ChangeReason::HandshakeFailed)
  PeerReputationManager->>ChangeReason: reason.score()
  ChangeReason-->>PeerReputationManager: signed delta
  PeerReputationManager->>PeerReputationManager: apply delta, evaluate ban
  PeerReputationManager-->>NetworkManager: banned: bool

Loading

Suggested reviewers: xdustinface

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately reflects the main reputation-module refactor and cleanup.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore/reputation-cleanup

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[coderabbitai]

Actionable comments posted: 0

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/reputation.rs (1)

117-187: Decay moves scores away from neutral; should trend toward 0.

apply_decay subtracts DECAY_AMOUNT regardless of sign, which makes negative (good) scores grow more negative over time without new positive behavior. If decay is meant to normalize toward neutral, adjust based on the sign.

🐛 Suggested fix

-            let decay = intervals_i32.saturating_mul(DECAY_AMOUNT);
-            self.score = (self.score - decay).max(MIN_MISBEHAVIOR_SCORE);
+            let decay = intervals_i32.saturating_mul(DECAY_AMOUNT);
+            if self.score > 0 {
+                self.score = (self.score - decay).max(0);
+            } else if self.score < 0 {
+                self.score = (self.score + decay).min(0);
+            }

🧹 Nitpick comments (2)

dash-spv/src/network/reputation.rs (1)

200-215: Avoid silently discarding reputation load failures.

Swallowing the storage error hides corruption and makes diagnostics harder; consider logging it before falling back to an empty map.

♻️ Suggested tweak

-        let mut reputations =
-            storage.load_peers_reputation().await.unwrap_or_else(|_| HashMap::new());
+        let mut reputations = match storage.load_peers_reputation().await {
+            Ok(reputations) => reputations,
+            Err(e) => {
+                log::warn!("Failed to load peer reputations: {e}");
+                HashMap::new()
+            }
+        };

dash-spv/src/network/manager.rs (1)

170-173: Avoid awaiting while holding the reputation mutex (possible contention / clippy::await_holding_lock).

Patterns like lock().await.*(...).await hold the mutex across .await, and save_to_storage also awaits IO. Consider making PeerReputationManager methods synchronous or snapshotting reputations before async IO so the lock can be released early. As per coding guidelines, please ensure clippy passes with warnings-as-errors.

Also applies to: 758-759, 1023-1025

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.

[codecov]

Codecov Report

❌ Patch coverage is 47.05882% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.42%. Comparing base (3170ad3) to head (b169873).

Files with missing lines	Patch %	Lines
dash-spv/src/network/reputation.rs	52.00%	12 Missing ⚠️
dash-spv/src/network/manager.rs	25.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #328      +/-   ##
==========================================
- Coverage   73.57%   73.42%   -0.15%     
==========================================
  Files         324      324              
  Lines       72929    72872      -57     
==========================================
- Hits        53654    53505     -149     
- Misses      19275    19367      +92     

Flag	Coverage Δ
core	76.94% <ø> (ø)
ffi	45.64% <ø> (-1.93%)	⬇️
rpc	20.00% <ø> (ø)
spv	90.88% <47.05%> (+0.49%)	⬆️
wallet	72.97% <ø> (ø)

Files with missing lines	Coverage Δ
dash-spv/src/network/mod.rs	98.09% <ø> (ø)
dash-spv/src/storage/peers.rs	82.08% <100.00%> (+0.27%)	⬆️
dash-spv/src/network/manager.rs	73.86% <25.00%> (+3.86%)	⬆️
dash-spv/src/network/reputation.rs	79.12% <52.00%> (+4.54%)	⬆️

... and 20 files with indirect coverage changes