Refactor/peer persistence #327

ZocoLini · 2025-12-30T16:58:58Z

The peers and peers reputation storage logic moved into the storage module following the trait pattern introduced by PR #311. Also moved PeerReputation values validation logic into serde's pipeline

This PR is built on top of #311

Summary by CodeRabbit

New Features
- Persistent on-disk peer storage added, preserving known peers and reputation across restarts.
- Reputation data now serialized/deserialized with stricter bounds for more consistent scoring.
Refactor
- Replaced legacy file-based persistence with a unified storage abstraction for peers and reputations.
- Public storage surface expanded to expose peer storage capabilities.
Tests
- Tests updated to use isolated temporary storage paths for reliability.

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

github-actions · 2025-12-30T16:59:10Z

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

coderabbitai · 2025-12-30T16:59:12Z

📝 Walkthrough

Walkthrough

This PR replaces the file-based PeerStore persistence with a storage-backed PersistentPeerStorage, moves peer and reputation persistence into the storage layer, updates reputation (de)serialization and clamping, removes the old persist module, extends DiskStorageManager to manage peers, and updates tests to use temporary storage paths.

Changes

Cohort / File(s)	Summary
Network manager & wiring `dash-spv/src/network/manager.rs`	Replaced `Arc<PeerStore>` with `Arc<PersistentPeerStorage>`; initialize via `PersistentPeerStorage::open(...)`; route reputation and peer load/save through storage handle.
Network module cleanup `dash-spv/src/network/mod.rs`	Removed `pub mod persist` export.
Removed legacy peer persistence `dash-spv/src/network/persist.rs`	Deleted `PeerStore`, its save/load/clear methods, serialization types, and associated tests.
Reputation model & persistence `dash-spv/src/network/reputation.rs`	Added serde (de)serializers with clamping for fields, new constants (e.g., MAX_MISBEHAVIOR_SCORE, MAX_BAN_COUNT), made `PeerReputation` serializable, and switched save/load signatures to use `&PersistentPeerStorage`.
Reputation tests `dash-spv/src/network/reputation_tests.rs`	Refactored tests to use `PersistentPeerStorage` and temp dirs instead of direct file paths.
Storage layer: peers `dash-spv/src/storage/peers.rs`	New `PeerStorage` trait and `PersistentPeerStorage` implementing peer and reputation persistence (peers.dat, reputations.json), atomic writes, error handling, and unit tests.
Storage manager integration `dash-spv/src/storage/mod.rs`	Added `peers: Arc<RwLock<PersistentPeerStorage>>` to `DiskStorageManager`, open peers storage during init, persist peers in background worker, and publicly re-export `PeerStorage`/`PersistentPeerStorage`.
Tests updated to use storage path `dash-spv/tests/*_test.rs`	`header_sync_test.rs`, `wallet_integration_test.rs`, `handshake_test.rs` now create temp dirs and set `ClientConfig::with_storage_path(...)`.
Gitignore cleanup `.gitignore`	Removed entries ignoring peer_reputation.json files (now managed by storage).

Sequence Diagram(s)

sequenceDiagram
    participant Startup as PeerNetworkManager
    participant Storage as PersistentPeerStorage
    participant DiskMgr as DiskStorageManager
    participant RepMgr as PeerReputationManager

    Startup->>Storage: open(storage_path)
    Storage-->>Startup: storage handle

    Startup->>RepMgr: load_from_storage(&peer_store)
    RepMgr->>Storage: load_peers_reputation()
    Storage-->>RepMgr: reputations map

    Startup->>Storage: load_peers()
    Storage-->>Startup: Vec<SocketAddr>

    note over Startup,RepMgr: Runtime maintenance loop updates reputations

    RepMgr->>Storage: save_peers_reputation()
    Startup->>Storage: save_peers()
    Storage-->>DiskMgr: persisted

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Clean/remove memory storage #275: Related storage-layer refactor and DiskStorageManager exports that align with the new PersistentPeerStorage integration.

Suggested reviewers

xdustinface

Poem

🐰 Hopping bytes from file to store,

Peers settle in a tidy drawer,
Reputation clamped and neat,
Saved safe on disk beneath my feet,
A carrot for tidy code galore! 🥕

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Refactor/peer persistence' is vague and generic, using non-descriptive terms that don't clearly convey what was changed. It lacks specificity about whether peers are being moved, refactored, or completely restructured.	Consider a more specific title such as 'Move peer persistence to storage module' or 'Refactor peer storage into storage layer abstraction' to better communicate the core change to team members reviewing history.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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

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

github-actions · 2026-01-05T16:30:49Z

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: 3

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/storage/mod.rs (1)
209-250: Missing peers re-initialization in clear() method.

After clearing storage and re-opening all storage components (lines 233-244), the peers storage is not re-initialized. This will leave self.peers pointing to the old storage instance with stale data, potentially causing issues after a clear() operation.
🐛 Add peers re-initialization in clear()
         self.masternodestate =
             Arc::new(RwLock::new(PersistentMasternodeStateStorage::open(storage_path).await?));
+        self.peers = Arc::new(RwLock::new(PersistentPeerStorage::open(storage_path).await?));
 
         // Restart the background worker for future operations
         self.start_worker().await;

🤖 Fix all issues with AI agents

In @dash-spv/src/storage/peers.rs:
- Around line 79-94: Remove the blocking std::fs::create_dir_all call and its
unwrap; instead compute the parent once using peers_file.parent().ok_or(...) (as
in peers_file_parent) and call the async
tokio::fs::create_dir_all(peers_file_parent).await to create directories;
eliminate the duplicate directory creation block and ensure any errors are
mapped to StorageError::WriteFailed where appropriate (keep the existing buffer
encoding loop and error handling for item.consensus_encode).
- Around line 101-127: In load_peers, replace blocking std::fs operations with
async equivalents: use tokio::fs::try_exists(peers_file) to check existence,
open the file with tokio::fs::File::open and wrap it in tokio::io::BufReader, or
simpler use tokio::fs::read(peers_file) to load the whole file into a Vec<u8>
and decode from a std::io::Cursor; then feed that reader/bytes to
AddrV2Message::consensus_decode (or its slice-based variant) instead of using
the blocking BufReader/File and std::fs::exists; ensure imports include
tokio::fs::File, tokio::fs::try_exists, tokio::fs::read and tokio::io::BufReader
and update error handling around consensus_decode accordingly.
- Around line 148-159: In load_peers_reputation, replace the blocking
reputation_file.exists() call with the async
tokio::fs::try_exists(reputation_file).await and handle its Result (e.g., let
exists = tokio::fs::try_exists(reputation_file).await?; if !exists { return
Ok(HashMap::new()); }) so the function stays fully async and errors propagate
consistently alongside the existing tokio::fs::read_to_string(...).await?.

🧹 Nitpick comments (3)

dash-spv/tests/wallet_integration_test.rs (1)
18-31: Inconsistent storage paths between network and disk storage managers.

The test creates two separate temporary directories:

Line 18: temp_dir used for ClientConfig.with_storage_path() → used by PeerNetworkManager for peer storage

Line 29: A new TempDir for DiskStorageManager

This means peer data and other storage (headers, filters, etc.) are written to different directories. While this works for basic testing, it creates an inconsistency that could mask integration issues where peers and storage should share the same root path.

Consider using a single temporary directory for both:
♻️ Suggested fix to use consistent storage paths
 async fn create_test_client(
 ) -> DashSpvClient<WalletManager<ManagedWalletInfo>, PeerNetworkManager, DiskStorageManager> {
     let temp_dir = tempfile::TempDir::new().expect("Failed to create temporary directory");
+    let storage_path = temp_dir.path().to_path_buf();
+    
     let config = ClientConfig::testnet()
         .without_filters()
         .without_masternodes()
-        .with_storage_path(temp_dir.path().to_path_buf());
+        .with_storage_path(storage_path.clone());
 
     // Create network manager
     let network_manager = PeerNetworkManager::new(&config).await.unwrap();
 
     // Create storage manager
-    let storage_manager =
-        DiskStorageManager::new(TempDir::new().expect("Failed to create tmp dir").path())
-            .await
-            .expect("Failed to create tmp storage");
+    let storage_manager = DiskStorageManager::new(&storage_path)
+        .await
+        .expect("Failed to create tmp storage");
+
+    // Keep temp_dir alive by leaking it (or store it in the client if possible)
+    std::mem::forget(temp_dir);
Note: If you keep separate directories intentionally for test isolation, consider adding a comment explaining this design choice.
dash-spv/tests/header_sync_test.rs (1)

264-280: Same storage path inconsistency as wallet_integration_test.rs.

Similar to the wallet integration test, this creates two separate temporary directories - one for ClientConfig (used by PeerNetworkManager) and another for DiskStorageManager. This means peer storage and block header storage will be in different locations.

For consistency with production behavior where all storage shares a common root, consider using the same path for both managers.
dash-spv/src/storage/peers.rs (1)
162-206: Consider adding tests for reputation persistence methods.

The tests cover peer save/load well, but save_peers_reputation and load_peers_reputation are not tested. Adding tests for these methods would improve coverage and confidence in the serialization/deserialization logic.
💡 Example test structure
#[tokio::test]
async fn test_persistent_peer_storage_reputation_save_load() {
    let temp_dir = TempDir::new().expect("Failed to create temporary directory");
    let store = PersistentPeerStorage::open(temp_dir.path())
        .await
        .expect("Failed to open persistent peer storage");

    let mut reputations = HashMap::new();
    let addr: SocketAddr = "192.168.1.1:9999".parse().unwrap();
    let reputation = PeerReputation::default(); // or construct with specific values
    reputations.insert(addr, reputation);

    store.save_peers_reputation(&reputations)
        .await
        .expect("Failed to save reputations");

    let loaded = store.load_peers_reputation()
        .await
        .expect("Failed to load reputations");
    
    assert_eq!(loaded.len(), 1);
    assert!(loaded.contains_key(&addr));
}

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 189a66d and 4a7925a.

📒 Files selected for processing (11)

.gitignore
dash-spv/src/network/manager.rs
dash-spv/src/network/mod.rs
dash-spv/src/network/persist.rs
dash-spv/src/network/reputation.rs
dash-spv/src/network/reputation_tests.rs
dash-spv/src/storage/mod.rs
dash-spv/src/storage/peers.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/tests/wallet_integration_test.rs

💤 Files with no reviewable changes (3)

dash-spv/src/network/persist.rs
dash-spv/src/network/mod.rs
.gitignore

🧰 Additional context used

📓 Path-based instructions (8)

dash-spv/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features

Files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/reputation_tests.rs
dash-spv/src/network/manager.rs
dash-spv/src/network/reputation.rs
dash-spv/src/storage/peers.rs

dash-spv/tests/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/tests/**/*.rs: Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite

Files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with #[cfg(test)] annotation; integration tests use the tests/ directory
Use snake_case for function and variable names
Use UpperCamelCase for types and traits
Use SCREAMING_SNAKE_CASE for constants
Format code with rustfmt before commits; ensure cargo fmt --all is run
Run cargo clippy --workspace --all-targets -- -D warnings for linting; avoid warnings in CI
Prefer async/await via tokio for asynchronous operations

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys

Files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/reputation_tests.rs
dash-spv/src/network/manager.rs
dash-spv/src/network/reputation.rs
dash-spv/src/storage/peers.rs

**/tests/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/tests/**/*.rs: Use descriptive test names (e.g., test_parse_address_mainnet)
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored

Files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs

**/*integration*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

Write integration tests for network operations

Files:

dash-spv/tests/wallet_integration_test.rs

**/*test*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate

Files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/src/network/reputation_tests.rs

dash-spv/src/storage/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory

Files:

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

dash-spv/src/network/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/src/network/**/*.rs: Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Implement configurable timeouts with recovery mechanisms for network operations

Files:

dash-spv/src/network/reputation_tests.rs
dash-spv/src/network/manager.rs
dash-spv/src/network/reputation.rs

🧠 Learnings (27)

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/src/network/reputation_tests.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/src/storage/mod.rs

📚 Learning: 2025-12-19T00:07:22.904Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations

Applied to files:

dash-spv/tests/wallet_integration_test.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-12-30T22:00:57.000Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurations

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-06-26T16:01:37.609Z

Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/src/storage/mod.rs

📚 Learning: 2025-12-01T07:59:58.608Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/src/network/reputation_tests.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/manager.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-19T00:07:22.904Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic testing with known test vectors and fixed seeds for reproducible results

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-12-22T17:59:51.097Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Cover critical parsing, networking, SPV, and wallet flows in tests; add regression tests for fixes; consider property tests with `proptest`

Applied to files:

dash-spv/tests/wallet_integration_test.rs
dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches

Applied to files:

dash-spv/tests/handshake_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/reputation_tests.rs
dash-spv/src/network/manager.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager

Applied to files:

dash-spv/tests/handshake_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/manager.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations

Applied to files:

dash-spv/tests/handshake_test.rs
dash-spv/src/network/manager.rs

📚 Learning: 2025-06-26T15:54:02.509Z

Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.

Applied to files:

dash-spv/tests/handshake_test.rs
dash-spv/tests/header_sync_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/manager.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-01T07:59:58.608Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function

Applied to files:

dash-spv/tests/handshake_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling

Applied to files:

dash-spv/tests/handshake_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/manager.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks

Applied to files:

dash-spv/tests/header_sync_test.rs
dash-spv/src/storage/mod.rs
dash-spv/src/network/manager.rs
dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-30T22:00:57.000Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*integration*.rs : Write integration tests for network operations

Applied to files:

dash-spv/tests/header_sync_test.rs
dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime

Applied to files:

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

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Tokio channels for inter-component message passing between async tasks

Applied to files:

dash-spv/src/storage/mod.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Follow a layered, trait-based architecture with clear separation of concerns across modules: client/, network/, storage/, sync/, validation/, wallet/, with trait-based abstractions for swappable implementations

Applied to files:

dash-spv/src/storage/mod.rs

📚 Learning: 2025-12-19T00:07:22.904Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data

Applied to files:

dash-spv/src/storage/mod.rs

📚 Learning: 2025-06-26T16:02:42.390Z

Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.

Applied to files:

dash-spv/src/storage/mod.rs
dash-spv/src/network/manager.rs

📚 Learning: 2025-12-22T17:59:51.097Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Unit tests should live alongside code with `#[cfg(test)]` annotation; integration tests use the `tests/` directory

Applied to files:

dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-12-22T17:59:51.097Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., `test_parse_address_mainnet`)

Applied to files:

dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-12-22T17:59:51.097Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Mark network-dependent or long-running tests with `#[ignore]` and run with `-- --ignored`

Applied to files:

dash-spv/src/network/reputation_tests.rs

📚 Learning: 2025-12-30T22:00:57.000Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Write unit tests for new functionality in Rust

Applied to files:

dash-spv/src/network/reputation_tests.rs

🧬 Code graph analysis (6)

dash-spv/tests/handshake_test.rs (2)

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

new (78-117)

dash-spv/src/client/config.rs (1)

new (210-217)

dash-spv/tests/header_sync_test.rs (2)

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

new (78-117)

dash-spv/src/storage/mod.rs (1)

new (99-143)

dash-spv/src/storage/mod.rs (2)

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

new (78-117)

clone (1036-1059)

dash-spv/src/storage/peers.rs (1)

open (57-63)

dash-spv/src/network/reputation_tests.rs (3)

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

new (78-117)

dash-spv/src/storage/mod.rs (2)

new (99-143)

open (52-52)

dash-spv/src/storage/peers.rs (1)

open (57-63)

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

dash-spv/src/storage/peers.rs (1)

open (57-63)

dash-spv/src/storage/mod.rs (2)

open (52-52)

new (99-143)

dash-spv/src/storage/peers.rs (3)

dash-spv/src/storage/io.rs (1)

atomic_write (24-57)

dash-spv/src/storage/mod.rs (4)

open (52-52)

persist (54-54)

persist (193-204)

new (99-143)

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

new (78-117)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)

GitHub Check: Windows / tools
GitHub Check: Ubuntu / ffi
GitHub Check: Ubuntu ARM / tools
GitHub Check: macOS / wallet
GitHub Check: macOS / ffi
GitHub Check: fuzz (hashes_sha512_256)
GitHub Check: MSRV Check (1.89)
GitHub Check: fuzz (hashes_json)
GitHub Check: fuzz (hashes_cbor)
GitHub Check: fuzz (hashes_sha256)
GitHub Check: fuzz (dash_outpoint_string)
GitHub Check: fuzz (dash_deserialize_block)
GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
GitHub Check: Pre-commit (macos-latest)
GitHub Check: Pre-commit (windows-latest)
GitHub Check: Pre-commit (ubuntu-latest)
GitHub Check: Thread Sanitizer
GitHub Check: Address Sanitizer
GitHub Check: conflict-check

🔇 Additional comments (17)

dash-spv/tests/handshake_test.rs (1)

76-77: LGTM! Proper test isolation with temporary storage.

The test now correctly uses a temporary directory for storage, ensuring test isolation and alignment with the new PersistentPeerStorage backend. The temp_dir lifetime is appropriate as it persists for the test duration.

dash-spv/src/network/reputation_tests.rs (2)

5-6: LGTM! Import aligns with new storage architecture.

The import correctly brings in PersistentStorage from the storage module, supporting the storage-backed persistence pattern.

66-73: LGTM! Test correctly migrated to storage-backed persistence.

The test properly:

Creates a temporary directory for isolation

Opens PersistentPeerStorage with the temp path

Uses storage handles for save_to_storage and load_from_storage

This aligns with the PR's goal of moving to storage-backed persistence.

dash-spv/src/storage/mod.rs (3)

8-12: LGTM! Module declarations for new storage components.

The new io and peers modules are correctly declared in the module tree.

43-43: LGTM! Public exports for peer storage types.

The PeerStorage trait and PersistentPeerStorage implementation are correctly exported, making them available to the network module.

90-90: LGTM! Peers storage properly integrated into DiskStorageManager.

The peers field is:

Correctly typed as Arc<RwLock<PersistentPeerStorage>>

Properly initialized in new()

Cloned for the background worker

Persisted in both the periodic worker loop and explicit persist() calls

This follows the same pattern as other storage components.

Also applies to: 133-133, 162-162, 179-179, 203-203

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

30-30: LGTM! Updated imports for storage-backed persistence.

The imports correctly bring in PeerStorage trait, PersistentPeerStorage implementation, and PersistentStorage trait for the new storage architecture.

42-42: LGTM! PeerNetworkManager migrated to PersistentPeerStorage.

The peer store field and initialization are correctly updated:

Field type changed from PeerStore to PersistentPeerStorage

Uses PersistentStorage::open() trait method for initialization

Reputation data is loaded on startup with graceful error handling

Also applies to: 84-90

741-743: LGTM! Reputation persistence uses new storage interface.

The save_to_storage calls in both the maintenance loop and shutdown path correctly pass the peer_store reference, aligning with the new &PersistentPeerStorage signature.

Also applies to: 1015-1017

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

8-8: LGTM! Updated imports for serde and storage.

Correctly imports Deserializer for custom deserialization and the new storage types from the storage module.

Also applies to: 15-15

80-92: LGTM! New constants and helper for reputation bounds.

The new constants properly define:

MAX_MISBEHAVIOR_SCORE (100) - ban threshold

MIN_MISBEHAVIOR_SCORE (-50) - best possible reputation

MAX_BAN_COUNT and MAX_ACTION_COUNT - guards against corrupted data

The default_instant() helper provides a consistent default for Instant fields.

94-134: LGTM! Defensive deserialization with clamping.

The custom deserializers provide robust handling of potentially corrupted or malicious persistence data:

clamp_peer_score: Clamps to valid score range with logging

clamp_peer_ban_count: Caps excessive ban counts

clamp_peer_connection_attempts: Limits connection attempts

This is good defensive programming for data loaded from disk.

137-171: LGTM! PeerReputation serde configuration.

The struct correctly:

Derives Serialize and Deserialize

Uses custom deserializers for clamped fields

Skips Instant-based fields that can't be serialized

Provides defaults for skipped fields via default_instant()

219-219: LGTM! Score clamping uses consistent bounds.

Both apply_decay and update_reputation now use the named constants MIN_MISBEHAVIOR_SCORE and MAX_MISBEHAVIOR_SCORE for consistent bounds enforcement.

Also applies to: 283-283

454-458: LGTM! Storage-backed persistence implementation.

The persistence methods correctly:

Accept &PersistentPeerStorage instead of file paths

Delegate to the storage trait methods

Include validation during load (connection count consistency, action count sanity checks)

One note on lines 483-485: Previously banned peers start with score.max(50), which means they're closer to the ban threshold. This is intentional defensive behavior to be more cautious with historically problematic peers.

Also applies to: 461-497

dash-spv/src/storage/peers.rs (2)

22-53: LGTM! Clean trait and struct design.

The trait design with immutable &self references aligns well with concurrent access patterns, and the helper methods provide clear path construction logic.

55-69: LGTM! Appropriate PersistentStorage implementation.

The no-op persist() is correctly documented as data is written immediately during save operations.

dash-spv/src/storage/peers.rs

github-actions · 2026-01-08T23:27:54Z

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

🧹 Nitpick comments (2)

dash-spv/src/storage/peers.rs (2)

149-160: Consider using spawn_blocking for consistency with load_peers.

While reputation JSON files are typically small, using spawn_blocking would maintain consistency with the load_peers implementation pattern and prevent potential executor blocking for larger reputation datasets.

♻️ Proposed refactor for consistency

 async fn load_peers_reputation(&self) -> StorageResult<HashMap<SocketAddr, PeerReputation>> {
     let reputation_file = self.peers_reputation_file();

     if !fs::try_exists(&reputation_file).await? {
         return Ok(HashMap::new());
     }

-    let json = fs::read_to_string(reputation_file).await?;
-    serde_json::from_str(&json).map_err(|e| {
-        StorageError::ReadFailed(format!("Failed to deserialize peers reputations: {e}"))
-    })
+    tokio::task::spawn_blocking(move || {
+        let json = std::fs::read_to_string(reputation_file)?;
+        serde_json::from_str(&json).map_err(|e| {
+            StorageError::ReadFailed(format!("Failed to deserialize peers reputations: {e}"))
+        })
+    })
+    .await
+    .map_err(|e| StorageError::ReadFailed(format!("Failed to load peers reputation: {e}")))?
 }

163-207: Add test coverage for reputation persistence.

The tests cover basic peer save/load operations well, but there's no coverage for save_peers_reputation and load_peers_reputation. Consider adding tests for:

Saving and loading peer reputations
Loading from empty reputation storage
Multiple peers and reputations

📝 Example test for reputation persistence

#[tokio::test]
async fn test_persistent_peer_storage_reputation() {
    let temp_dir = TempDir::new().expect("Failed to create temporary directory for test");
    let store = PersistentPeerStorage::open(temp_dir.path())
        .await
        .expect("Failed to open persistent peer storage");

    let addr: SocketAddr = "192.168.1.1:9999".parse().unwrap();
    let mut reputations = HashMap::new();
    reputations.insert(addr, PeerReputation::default()); // Adjust based on PeerReputation API

    store.save_peers_reputation(&reputations)
        .await
        .expect("Failed to save reputations");

    let loaded = store.load_peers_reputation()
        .await
        .expect("Failed to load reputations");
    
    assert_eq!(loaded.len(), 1);
    assert!(loaded.contains_key(&addr));
}

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a7925a and d45fe90.

📒 Files selected for processing (2)

dash-spv/src/storage/peers.rs
dash-spv/tests/header_sync_test.rs

🚧 Files skipped from review as they are similar to previous changes (1)

dash-spv/tests/header_sync_test.rs

🧰 Additional context used

📓 Path-based instructions (3)

dash-spv/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features

Files:

dash-spv/src/storage/peers.rs

dash-spv/src/storage/**/*.rs

📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)

Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory

Files:

dash-spv/src/storage/peers.rs

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with #[cfg(test)] annotation; integration tests use the tests/ directory
Use snake_case for function and variable names
Use UpperCamelCase for types and traits
Use SCREAMING_SNAKE_CASE for constants
Format code with rustfmt before commits; ensure cargo fmt --all is run
Run cargo clippy --workspace --all-targets -- -D warnings for linting; avoid warnings in CI
Prefer async/await via tokio for asynchronous operations

**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys

Files:

dash-spv/src/storage/peers.rs

🧠 Learnings (15)

📓 Common learnings

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches

📚 Learning: 2025-06-26T15:54:02.509Z

Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-01T07:59:58.608Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-01T07:59:58.608Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-06-26T16:02:42.390Z

Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-22T17:59:51.097Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Prefer `async`/`await` via `tokio` for asynchronous operations

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Tokio channels for inter-component message passing between async tasks

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-16T09:03:55.811Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations

Applied to files:

dash-spv/src/storage/peers.rs

📚 Learning: 2025-12-30T22:00:57.000Z

Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Use tokio runtime for async operations in Rust

Applied to files:

dash-spv/src/storage/peers.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)

GitHub Check: Windows / tools
GitHub Check: Windows / ffi
GitHub Check: macOS / ffi
GitHub Check: macOS / spv
GitHub Check: macOS / tools
GitHub Check: fuzz (hashes_sha512_256)
GitHub Check: fuzz (hashes_json)
GitHub Check: fuzz (hashes_sha512)
GitHub Check: fuzz (dash_deserialize_script)
GitHub Check: fuzz (hashes_ripemd160)
GitHub Check: fuzz (hashes_sha1)
GitHub Check: fuzz (hashes_cbor)
GitHub Check: fuzz (dash_deserialize_witness)
GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
GitHub Check: fuzz (dash_deser_net_msg)
GitHub Check: Address Sanitizer
GitHub Check: Pre-commit (windows-latest)
GitHub Check: Thread Sanitizer
GitHub Check: conflict-check

🔇 Additional comments (7)

dash-spv/src/storage/peers.rs (7)

1-16: LGTM: Clean import organization.

The imports are well-organized and all necessary for the functionality. The mix of sync (std::fs::File) and async (tokio::fs) types is appropriate for the spawn_blocking pattern used later.

18-33: LGTM: Well-designed trait interface.

The PeerStorage trait provides a clean separation between peer address storage and reputation tracking, with appropriate async signatures consistent with the storage layer patterns.

35-49: LGTM: Clean helper methods for path management.

The helper methods provide clear, centralized path construction for peer data and reputation files.

51-65: LGTM: Appropriate implementation of PersistentStorage.

The no-op persist() method is well-documented and makes sense since data is atomically written during save operations.

69-91: LGTM: Robust peer persistence with atomic writes.

The implementation correctly uses consensus encoding and atomic writes to safely persist peer data. The defensive parent directory check is good practice even though the path structure guarantees its existence.

93-128: Excellent use of spawn_blocking for sync I/O operations.

The implementation correctly offloads synchronous file I/O operations to a blocking thread pool using spawn_blocking, preventing the tokio executor from being blocked. The EOF handling is proper, and the silent filtering of invalid socket addresses (line 125) provides resilience against corrupted entries.

130-147: LGTM: Consistent reputation persistence pattern.

The implementation follows the same safe persistence pattern as save_peers, with appropriate JSON serialization and atomic writes. Pretty-printing provides debugging benefits with minimal overhead.

ZocoLini marked this pull request as draft December 30, 2025 16:59

github-actions bot added the merge-conflict The PR conflicts with the target branch. label Dec 30, 2025

ZocoLini changed the base branch from v0.41-dev to refactor/storage-manager-trait December 30, 2025 16:59

ZocoLini force-pushed the refactor/peer-persistence branch from 5d78f43 to 2d4550e Compare December 30, 2025 17:01

github-actions bot removed the merge-conflict The PR conflicts with the target branch. label Dec 30, 2025

ZocoLini mentioned this pull request Dec 30, 2025

Chore: reputation cleanup #328

Draft

ZocoLini force-pushed the refactor/storage-manager-trait branch from 2bc0490 to e40b953 Compare January 5, 2026 16:28

github-actions bot added the merge-conflict The PR conflicts with the target branch. label Jan 5, 2026

ZocoLini force-pushed the refactor/storage-manager-trait branch from e40b953 to 0248257 Compare January 5, 2026 16:44

Base automatically changed from refactor/storage-manager-trait to v0.42-dev January 8, 2026 01:54

ZocoLini added 7 commits January 8, 2026 21:43

peers persistance moved to the storage module

ddc3ef6

peers reputations storage system moven to the storage module

3662696

peer storage cleanup

b558806

peer reputation deserialization values clamp moved into serde pipeline

a301e7e

tests corrrectly use a temp dir

7271537

clippy warning fixed

cfd3138

gitignore updated

4a7925a

ZocoLini force-pushed the refactor/peer-persistence branch from 3ca989d to 4a7925a Compare January 8, 2026 21:54

github-actions bot removed the merge-conflict The PR conflicts with the target branch. label Jan 8, 2026

ZocoLini marked this pull request as ready for review January 8, 2026 21:55

coderabbitai bot reviewed Jan 8, 2026

View reviewed changes

dash-spv/src/storage/peers.rs Outdated Show resolved Hide resolved

dash-spv/src/storage/peers.rs Show resolved Hide resolved

dash-spv/src/storage/peers.rs Show resolved Hide resolved

reading and loading peers made not blocking

68750bc

ZocoLini force-pushed the refactor/peer-persistence branch from 732c3ac to 68750bc Compare January 8, 2026 22:22

github-actions bot added the merge-conflict The PR conflicts with the target branch. label Jan 8, 2026

Merge branch 'v0.42-dev' into refactor/peer-persistence

d45fe90

github-actions bot removed the merge-conflict The PR conflicts with the target branch. label Jan 8, 2026

coderabbitai bot reviewed Jan 8, 2026

View reviewed changes

ZocoLini marked this pull request as draft January 9, 2026 00:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor/peer persistence #327

Refactor/peer persistence #327

Uh oh!

ZocoLini commented Dec 30, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

github-actions bot commented Dec 30, 2025

Uh oh!

coderabbitai bot commented Dec 30, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

github-actions bot commented Jan 5, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 8, 2026

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Refactor/peer persistence #327

Are you sure you want to change the base?

Refactor/peer persistence #327

Uh oh!

Conversation

ZocoLini commented Dec 30, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

github-actions bot commented Dec 30, 2025

Uh oh!

coderabbitai bot commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

github-actions bot commented Jan 5, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 8, 2026

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ZocoLini commented Dec 30, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 30, 2025 •

edited

Loading