refactor(dash-spv): reputation module cleanup

[ZocoLini]

• Reputation changes made using enum variants intead of plain strings and i32
• unit tests moved inside the reputation module
• dropped unused fields and methods
• privatized struct fields
• PeerReputationManager no longer used Arc<RwLock<>> to store reputatuions

Summary by CodeRabbit

• Refactor
  • Restructured peer reputation management system with improved async locking and concurrency handling.
  • Simplified network penalty API by removing dynamic reason parameters and introducing a structured reputation change system.
  • Enhanced peer reputation tracking with automatic decay and ban duration logic.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request refactors the peer reputation system from static misbehavior scores to a ChangeReason enum-based approach. The reputation manager transitions from an Arc<RwLock<>> pattern to Arc<Mutex<>> wrapping, and internal operations shift from async locking to mutable reference-based mutations. Related API signatures and penalty mechanisms are updated accordingly across the codebase.

Changes

Cohort / File(s)	Summary
Reputation System Core dash-spv/src/network/reputation.rs	Complete restructure: replaced static misbehavior/positive score constants with ChangeReason enum. Transitioned from Arc<RwLock> to plain HashMap within PeerReputationManager. Introduced ChangeReason::score() method and Display trait. Refactored API from async locking patterns (update_reputation, is_banned, select_best_peers, should_connect_to_peer, save_to_storage, get_all_reputations) to mutable reference-based operations. Added BAN_DURATION, DECAY_INTERVAL, and DECAY_AMOUNT constants. Removed legacy fields and event tracking.
Network Manager Integration dash-spv/src/network/manager.rs	Updated PeerNetworkManager.reputation_manager field from Arc<PeerReputationManager> to Arc<Mutex<PeerReputationManager>>. Modified public trait methods: penalize_peer now takes (address, reason: ChangeReason) instead of (address, score_change, reason). Removed reason parameters from penalize_peer_invalid_chainlock and penalize_peer_invalid_instantlock. Updated all reputation manager calls to acquire locks via .lock().await before operations.
Network Module Public API dash-spv/src/network/mod.rs	Reduced reputation module visibility from pub mod to private. Added ChangeReason to public imports. Changed PeerReputation visibility to pub(crate). Updated NetworkManager trait signatures to match manager refactoring: simplified penalty method signatures; default trait implementations now construct ChangeReason variants instead of using score constants.
Penalty Call Sites dash-spv/src/client/chainlock.rs	Removed dynamic reason string parameters from penalize_peer_invalid_chainlock(peer_address, &reason) and penalize_peer_invalid_instantlock(peer_address, &reason) calls; now pass only peer address.
Storage and Cleanup dash-spv/src/storage/peers.rs	Updated import path for PeerReputation from dashcore::network::reputation::PeerReputation to dashcore::network::PeerReputation. Moved peers vector declaration scope inside spawn_blocking closure.
Test Removal dash-spv/src/network/reputation_tests.rs	Deleted in-module unit test file containing five tokio-based tests (test_basic_reputation_operations, test_banning_mechanism, test_reputation_persistence, test_peer_selection, test_connection_tracking).
Minor Formatting dash-spv/src/client/lifecycle.rs	Added blank line before storage.store_headers_at_height call. No functional changes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

• xdustinface

Poem

🐰 The reputation hops have changed!

Where scores once lived in locks so tight,
Now reason reigns through mutex light,
From async webs to refs so free,
The peers hop forth in harmony! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'refactor(dash-spv): reputation module cleanup' accurately summarizes the main change: refactoring the reputation module by replacing string/i32 representations with enum variants, removing unused fields, and improving the overall module structure.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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 and usage tips.}

[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.