Chore: config storage improvement #366
Conversation
📝 WalkthroughWalkthroughRefactored storage initialization architecture: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dash-spv/tests/block_download_test.rs (1)
117-122: TempDir is immediately dropped, deleting the directory before use.
TempDir::new().unwrap()creates a temporary directory, but since it's not bound to a variable, it's dropped immediately after.path()is called. WhenTempDirdrops, it deletes the directory, leavingstorage_pathpointing to a non-existent path.🐛 Proposed fix
The
TempDirmust be bound to a variable that lives for the duration of the test. However, sincecreate_test_config()is a helper function, you'll need to refactor to return both the config and the TempDir, or create TempDir within each test:Option 1 - Return both from helper:
-fn create_test_config() -> ClientConfig { - ClientConfig::testnet() - .without_masternodes() - .with_validation_mode(dash_spv::types::ValidationMode::None) - .with_storage_path(TempDir::new().unwrap().path()) -} +fn create_test_config() -> (ClientConfig, TempDir) { + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::testnet() + .without_masternodes() + .with_validation_mode(dash_spv::types::ValidationMode::None) + .with_storage_path(temp_dir.path()); + (config, temp_dir) +}Then update callers:
-let config = create_test_config(); +let (config, _temp_dir) = create_test_config();Option 2 - Create TempDir in each test that needs storage persistence:
let temp_dir = TempDir::new().unwrap(); let config = ClientConfig::testnet() .without_masternodes() .with_validation_mode(dash_spv::types::ValidationMode::None) .with_storage_path(temp_dir.path());dash-spv/src/client/mod.rs (1)
85-93:TempDiris dropped immediately, causing the storage path to become invalid.On line 89,
TempDir::new().unwrap().path()creates a temporary directory, extracts its path, and then immediately drops theTempDir. WhenTempDiris dropped, it deletes the directory. This means thestorage_pathin the config points to a non-existent directory.Additionally, there's an inconsistency: line 92-93 still uses
DiskStorageManager::with_temp_dir()which creates its own separate temp directory, so the storage path configured in line 89 is never actually used by this test.🔧 Proposed fix - retain TempDir for its lifetime
#[tokio::test] async fn client_exposes_shared_wallet_manager() { + let temp_dir = TempDir::new().unwrap(); let config = ClientConfig::mainnet() .without_filters() .without_masternodes() .with_mempool_tracking(MempoolStrategy::FetchAll) - .with_storage_path(TempDir::new().unwrap().path()); + .with_storage_path(temp_dir.path()); let network_manager = MockNetworkManager::new(); - let storage = - DiskStorageManager::with_temp_dir().await.expect("Failed to create tmp storage"); + let storage = DiskStorageManager::new(&config).await.expect("Failed to create tmp storage");
🤖 Fix all issues with AI agents
In `@dash-spv/benches/storage.rs`:
- Around line 37-40: The TempDir is created inline so it drops immediately and
the path no longer exists when DiskStorageManager::new is called; fix by binding
the TempDir to a variable that remains in scope for the storage lifetime (e.g.,
let tmp = TempDir::new().unwrap(); let config =
ClientConfig::testnet().with_storage_path(tmp.path()); then pass that config
into DiskStorageManager::new and ensure tmp is kept alive until after the
storage is dropped), applying the same change for both occurrences to keep the
temporary directory alive while DiskStorageManager is in use.
In `@dash-spv/src/client/config.rs`:
- Around line 83-84: The default storage_path is network-agnostic and can cause
cross-network data mixing; update ClientConfig construction so the default
storage_path includes the network name (e.g., "./dash-spv-storage/{network}")
when building non-mainnet configs. Modify ClientConfig::new (and/or the
ClientConfig::testnet() and ClientConfig::regtest() helpers) to derive
storage_path from the chosen network/ValidationMode and set the storage_path
field accordingly instead of PathBuf::from("./dash-spv-storage"), ensuring each
network gets a unique subdirectory.
In `@dash-spv/src/client/mod.rs`:
- Around line 111-118: The TempDir is created inline and dropped immediately
(TempDir::new().unwrap().path()) so the directory is deleted before
DiskStorageManager::new(&config) uses it; fix by creating and binding the
TempDir to a local variable (e.g., tmp_dir) that outlives the config, then pass
tmp_dir.path() (or clone to a PathBuf if with_storage_path requires ownership)
into ClientConfig::with_storage_path so the temporary directory remains
available when DiskStorageManager::new is called; update the test so tmp_dir
stays in scope until after DiskStorageManager::new(&config).await completes,
referencing ClientConfig::testnet(), with_storage_path, and
DiskStorageManager::new to find the locations to change.
In `@dash-spv/tests/edge_case_filter_sync_test.rs`:
- Around line 152-159: The TempDir used to set the storage path is being dropped
immediately after .path(), causing the directory to be removed before
DiskStorageManager::new runs; in test_no_invalid_getcfheaders_at_tip preserve
the TempDir by binding it to a variable (e.g., temp_dir) that lives for the
duration of the test and use temp_dir.path() when constructing the ClientConfig
(created via ClientConfig::new(Network::Dash).with_storage_path(...)); ensure
the temp_dir variable remains in scope until after
DiskStorageManager::new(&config) completes so the directory still exists.
- Around line 110-117: The TempDir is being dropped immediately because you call
TempDir::new().unwrap().path() inline when building ClientConfig, invalidating
the storage path before DiskStorageManager::new runs; preserve the TempDir by
binding it to a variable that lives for the duration of the test (e.g., let
tmp_dir = TempDir::new().unwrap()) and then pass tmp_dir.path() into
ClientConfig::with_storage_path (or set config.storage_path) so that the
temporary directory remains valid while calling FilterSyncManager::new and
DiskStorageManager::new in test_filter_sync_at_tip_edge_case.
In `@dash-spv/tests/filter_header_verification_test.rs`:
- Around line 177-181: The test is dropping the TempDir immediately by passing
TempDir::new().unwrap().path() into ClientConfig, so the directory is deleted
before DiskStorageManager::new() runs; instead create and bind the TempDir to a
local variable (e.g., let tmp_dir = TempDir::new().unwrap()) and pass
tmp_dir.path() into ClientConfig::new(Network::Dash).with_storage_path(...) so
the TempDir lives for the lifetime of the storage and subsequent calls like
DiskStorageManager::new() and storage operations can use the existing directory;
apply the same pattern to other occurrences mentioned (around lines 342-345,
518-521, 615-618).
In `@dash-spv/tests/header_sync_test.rs`:
- Around line 25-27: The TempDir is being created inline and dropped
immediately, so keep the TempDir handle alive for the duration of the test by
creating a local variable (e.g., let temp_dir = TempDir::new().expect(...)) and
pass temp_dir.path() into ClientConfig::with_storage_path; this ensures the
temporary directory is not removed prematurely and remains managed (while
DiskStorageManager::new() can still recreate contents) so test artifacts are
cleaned up when temp_dir goes out of scope.
In `@dash-spv/tests/peer_test.rs`:
- Around line 18-22: create_test_config currently drops the TempDir after
extracting a PathBuf so the temporary directory is deleted before tests run,
causing DiskStorageManager::new to fail; fix by keeping the TempDir alive for
the test lifetime — either change create_test_config to return (ClientConfig,
TempDir) so the caller holds the TempDir, or change create_test_config to accept
a TempDir parameter and set ClientConfig.storage_path from that TempDir.path();
update call sites to hold the TempDir until the test completes and reference
ClientConfig.storage_path as before.
In `@dash-spv/tests/wallet_integration_test.rs`:
- Around line 18-27: The TempDir is being dropped immediately because you call
TempDir::new().unwrap().path() inline; bind the TempDir to a local variable
(e.g., let temp_dir = TempDir::new().unwrap();) and pass temp_dir.path() into
ClientConfig::testnet().without_filters().with_storage_path(...) so the
temporary directory stays alive for the test lifetime; update the setup where
ClientConfig::testnet(), PeerNetworkManager::new, and DiskStorageManager::new
are used to reference that temp_dir variable (consistent with
DiskStorageManager::with_temp_dir()).
🧹 Nitpick comments (3)
dash-spv-ffi/src/config.rs (1)
316-334: Documentation may be outdated for non-optional storage_path.The doc comment at lines 319-320 states "If null or no data directory is set, returns an FFIString with null pointer", but since
storage_pathis now a requiredPathBuf(non-optional), the "no data directory is set" case no longer applies. The function will now return anFFIStringcontaining the default path (likely empty) rather than a null pointer whenstorage_pathhasn't been explicitly configured.Consider updating the doc comment to reflect the new behavior:
📝 Suggested documentation update
/// Gets the data directory path from the configuration /// /// # Safety /// - `config` must be a valid pointer to an FFIClientConfig or null -/// - If null or no data directory is set, returns an FFIString with null pointer +/// - If null, returns an FFIString with null pointer +/// - Otherwise returns the configured storage path (may be empty if not explicitly set) /// - The returned FFIString must be freed by the caller using `dash_spv_ffi_string_destroy`dash-spv/src/client/config.rs (1)
209-214: Prefer domain-specific error types for validation failures.Consider returning a ValidationError/StorageError instead of String so callers can pattern-match and propagate reliably. As per coding guidelines, ...
dash-spv/src/storage/mod.rs (1)
142-148: Keep the temp directory alive during tests.
with_temp_dirdropsTempDirimmediately, so its Drop may delete the directory while the storage is still in use, making test cleanup nondeterministic. Consider returning theTempDiralongside the storage, or usinginto_path()to keep it alive for the storage lifetime.♻️ Possible adjustment (keeps path alive)
#[cfg(test)] pub async fn with_temp_dir() -> StorageResult<Self> { use tempfile::TempDir; let temp_dir = TempDir::new()?; - Self::new(&ClientConfig::testnet().with_storage_path(temp_dir.path())).await + let path = temp_dir.into_path(); + Self::new(&ClientConfig::testnet().with_storage_path(path)).await }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
dash-spv-ffi/src/client.rsdash-spv-ffi/src/config.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/benches/storage.rsdash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/config.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/network/manager.rsdash-spv/src/storage/mod.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/reverse_index_test.rsdash-spv/tests/segmented_storage_debug.rsdash-spv/tests/segmented_storage_test.rsdash-spv/tests/simple_segmented_test.rsdash-spv/tests/storage_consistency_test.rsdash-spv/tests/storage_test.rsdash-spv/tests/wallet_integration_test.rs
💤 Files with no reviewable changes (6)
- dash-spv/tests/storage_test.rs
- dash-spv/tests/simple_segmented_test.rs
- dash-spv/tests/reverse_index_test.rs
- dash-spv/tests/segmented_storage_test.rs
- dash-spv/tests/segmented_storage_debug.rs
- dash-spv/tests/storage_consistency_test.rs
🧰 Additional context used
📓 Path-based instructions (7)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys
Use proper error types (thiserror) and propagate errors appropriately
Use tokio runtime for async operations
Use conditional compilation for optional features
Use proptest for property-based testing where appropriate
Always validate inputs from untrusted sources
Never log or expose private keys
Format code using cargo fmt
Ensure clippy passes with all features enabled and -D warnings flag
**/*.rs: Keep unit tests in the source code alongside implementation using#[cfg(test)]modules
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtfollowing therustfmt.tomlconfiguration; runcargo fmt --allbefore commits
Avoidunwrap()andexpect()in library code; use explicit error types (e.g.,thiserror)
Usetokiofor async/await implementations
Run clippy with warnings-as-errors for linting:cargo clippy --workspace --all-targets -- -D warnings
Maintain Minimum Supported Rust Version (MSRV) of 1.89
Follow crate-specific idioms for mixed Rust editions (2021/2024)
Files:
dash-spv/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv-ffi/src/client.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/examples/filter_sync.rsdash-spv/tests/block_download_test.rsdash-spv-ffi/src/config.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.rs
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use trait-based abstractions for swappable implementations (e.g.,NetworkManager,StorageManager)
Use async/await throughout the codebase instead of callback-based patterns
Use domain-specific error types (NetworkError,StorageError,SyncError,ValidationError,SpvError) for error handling
Use Tokio channels for inter-component message passing in async architecture
Runcargo fmt --checkto verify code formatting before committing
Runcargo clippy --all-targets --all-features -- -D warningsto catch potential bugs and style issues
Write unit tests in-module and integration tests in thetests/directory following the test organization strategy
UseArc<dyn TraitName>for trait objects in Rust to support runtime polymorphism
Files:
dash-spv/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv/examples/filter_sync.rsdash-spv/tests/block_download_test.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.rs
**/*test*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Write descriptive test names (e.g.,
test_parse_address_mainnet)
Files:
dash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/config_test.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rs
dash-spv-ffi/src/**/*.rs
📄 CodeRabbit inference engine (dash-spv-ffi/CLAUDE.md)
dash-spv-ffi/src/**/*.rs: Use#[no_mangle] extern "C"attribute when implementing new FFI functions in Rust
All FFI types must have corresponding_destroy()functions for explicit memory management
Rust strings must be returned as*const c_charwith caller responsibility to free usingdash_string_free
Input strings in FFI functions are*const c_char(borrowed, not freed by C caller)
Add cbindgen annotations for complex types in FFI functions
Use thread-local storage for error propagation viadash_spv_ffi_get_last_error()function
Files:
dash-spv-ffi/src/client.rsdash-spv-ffi/src/config.rs
**/*-ffi/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*-ffi/**/*.rs: Exercise careful memory safety handling at FFI boundaries
Be careful with FFI memory management
Files:
dash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv-ffi/src/config.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement
StorageManagertrait for storage backends and store headers in 10,000-header segments with index files
Files:
dash-spv/src/storage/mod.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 fallback to DNS seeds when no explicit peers are configured
Implement configurable timeouts with recovery mechanisms in network operations
Files:
dash-spv/src/network/manager.rs
🧠 Learnings (38)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Implement `StorageManager` trait for storage backends and store headers in 10,000-header segments with index files
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Implement `StorageManager` trait for storage backends and store headers in 10,000-header segments with index files
Applied to files:
dash-spv/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv-ffi/src/client.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv/examples/filter_sync.rsdash-spv/tests/block_download_test.rsdash-spv-ffi/src/config.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/**/*.rs : Use trait-based abstractions for swappable implementations (e.g., `NetworkManager`, `StorageManager`)
Applied to files:
dash-spv/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv-ffi/src/client.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/examples/filter_sync.rsdash-spv/tests/block_download_test.rsdash-spv-ffi/src/config.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.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/benches/storage.rsdash-spv-ffi/src/client.rsdash-spv/tests/wallet_integration_test.rsdash-spv/examples/filter_sync.rsdash-spv/tests/block_download_test.rsdash-spv-ffi/src/config.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/**/*.rs : Run `cargo fmt --check` to verify code formatting before committing
Applied to files:
dash-spv/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv-ffi/src/client.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/examples/filter_sync.rsdash-spv/tests/block_download_test.rsdash-spv-ffi/src/config.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/**/*.rs : Run `cargo clippy --all-targets --all-features -- -D warnings` to catch potential bugs and style issues
Applied to files:
dash-spv/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv-ffi/src/client.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/examples/filter_sync.rsdash-spv-ffi/src/config.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.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/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv-ffi/src/client.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/examples/filter_sync.rsdash-spv/tests/block_download_test.rsdash-spv/src/main.rsdash-spv/src/storage/mod.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/edge_case_filter_sync_test.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Use sequential phase-based synchronization via `SyncManager` for organized sync coordination
Applied to files:
dash-spv/benches/storage.rsdash-spv/src/client/mod.rsdash-spv/examples/filter_sync.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.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/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv-ffi/src/client.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/examples/filter_sync.rsdash-spv-ffi/src/config.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/**/*.rs : Write unit tests in-module and integration tests in the `tests/` directory following the test organization strategy
Applied to files:
dash-spv/benches/storage.rsdash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/tests/block_download_test.rsdash-spv/src/storage/mod.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rs
📚 Learning: 2025-02-25T06:13:52.858Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 51
File: dash/src/sml/masternode_list/scores_for_quorum.rs:50-73
Timestamp: 2025-02-25T06:13:52.858Z
Learning: ScoreHash is a cryptographic hash in the rust-dashcore library, and therefore does not need collision handling when used as a key in collections like BTreeMap due to the extremely low probability of collisions.
Applied to files:
dash-spv/benches/storage.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/src/validation/**/*.rs : Implement configurable validation modes (`ValidationMode::None`, `ValidationMode::Basic`, `ValidationMode::Full`)
Applied to files:
dash-spv/tests/filter_header_verification_test.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/tests/block_download_test.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/**/*.rs : Use domain-specific error types (`NetworkError`, `StorageError`, `SyncError`, `ValidationError`, `SpvError`) for error handling
Applied to files:
dash-spv/tests/filter_header_verification_test.rsdash-spv/examples/filter_sync.rsdash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/src/network/manager.rsdash-spv/src/client/config.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/peer_test.rsdash-spv/tests/edge_case_filter_sync_test.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/filter_header_verification_test.rsdash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/tests/block_download_test.rsdash-spv/src/storage/mod.rsdash-spv/tests/chainlock_simple_test.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/tests/filter_header_verification_test.rsdash-spv/src/client/mod.rsdash-spv/examples/filter_sync.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/examples/simple_sync.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 : Input strings in FFI functions are `*const c_char` (borrowed, not freed by C caller)
Applied to files:
dash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv-ffi/src/config.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 `#[no_mangle] extern "C"` attribute when implementing new FFI functions in Rust
Applied to files:
dash-spv-ffi/src/client.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 : All FFI types must have corresponding `_destroy()` functions for explicit memory management
Applied to files:
dash-spv-ffi/src/client.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 : Add cbindgen annotations for complex types in FFI functions
Applied to files:
dash-spv-ffi/src/client.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic fallback to DNS seeds when no explicit peers are configured
Applied to files:
dash-spv-ffi/src/client.rsdash-spv/src/client/mod.rsdash-spv/tests/block_download_test.rsdash-spv/src/network/manager.rsdash-spv/src/client/config.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/examples/spv_with_wallet.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rs
📚 Learning: 2026-01-16T14:54:24.093Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-16T14:54:24.093Z
Learning: Applies to **/*test*.rs : Write descriptive test names (e.g., `test_parse_address_mainnet`)
Applied to files:
dash-spv/src/client/config_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rs
📚 Learning: 2026-01-16T14:54:24.093Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-16T14:54:24.093Z
Learning: Applies to **/*.rs : Keep unit tests in the source code alongside implementation using `#[cfg(test)]` modules
Applied to files:
dash-spv/src/client/config_test.rs
📚 Learning: 2026-01-16T14:53:56.692Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T14:53:56.692Z
Learning: Applies to **/rpc-integration-test/**/*.rs : Write integration tests for network operations
Applied to files:
dash-spv/src/client/config_test.rsdash-spv/src/client/mod.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms in network operations
Applied to files:
dash-spv/src/client/mod.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.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/src/client/mod.rsdash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.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/src/client/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 : Separate immutable structures (`Account`, `Wallet`) containing only identity information from mutable wrappers (`ManagedAccount`, `ManagedWalletInfo`) with state management
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv-ffi/tests/test_wallet_manager.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/c_tests/**/*.{c,h} : Add corresponding C tests in `tests/c_tests/` for each new FFI function
Applied to files:
dash-spv-ffi/tests/test_wallet_manager.rs
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
dash-spv-ffi/tests/test_wallet_manager.rs
📚 Learning: 2025-06-15T16:42:18.187Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet-ffi/src/lib_tests.rs:41-48
Timestamp: 2025-06-15T16:42:18.187Z
Learning: In key-wallet-ffi, the HDWallet::derive_xpriv method returns Result<String, KeyWalletError>, not an ExtPrivKey wrapper. When unwrapped, it yields a String that can have .is_empty() called on it.
Applied to files:
dash-spv-ffi/tests/test_wallet_manager.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 : Rust strings must be returned as `*const c_char` with caller responsibility to free using `dash_string_free`
Applied to files:
dash-spv-ffi/tests/test_wallet_manager.rsdash-spv-ffi/src/config.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement state machines using `SyncState` enum to drive synchronization flow
Applied to files:
dash-spv/examples/filter_sync.rsdash-spv/src/lib.rsdash-spv/src/storage/mod.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/edge_case_filter_sync_test.rsdash-spv/examples/simple_sync.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase instead of callback-based patterns
Applied to files:
dash-spv/src/main.rsdash-spv/src/lib.rsdash-spv/examples/spv_with_wallet.rs
📚 Learning: 2026-01-16T14:54:24.093Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-16T14:54:24.093Z
Learning: This codebase is not intended for consensus-critical validation; do not rely on exact Dash Core consensus behavior
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2026-01-16T14:54:10.809Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2026-01-16T14:54:10.809Z
Learning: Applies to dash-spv/**/Cargo.toml : Maintain Minimum Rust Version (MSRV) of 1.89 and ensure all dependencies compile with `cargo check --all-features`
Applied to files:
dash-spv/src/client/config.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 : Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/tests/peer_test.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
dash-spv/tests/peer_test.rs
📚 Learning: 2026-01-16T14:53:56.692Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T14:53:56.692Z
Learning: Test both mainnet and testnet configurations
Applied to files:
dash-spv/tests/peer_test.rs
🧬 Code graph analysis (17)
dash-spv/benches/storage.rs (2)
dash-spv/src/client/config.rs (2)
testnet(117-119)new(102-109)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/filter_header_verification_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv-ffi/src/client.rs (3)
dash-spv/src/client/config.rs (1)
new(102-109)dash-spv/src/network/manager.rs (1)
new(78-128)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/src/client/config_test.rs (1)
test-utils/src/helpers.rs (1)
path(126-128)
dash-spv/tests/wallet_integration_test.rs (2)
dash-spv/src/client/config.rs (2)
testnet(117-119)new(102-109)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv-ffi/tests/test_wallet_manager.rs (1)
dash-spv-ffi/src/config.rs (1)
dash_spv_ffi_config_set_data_dir(70-88)
dash-spv/examples/filter_sync.rs (3)
dash-spv/src/client/config.rs (2)
mainnet(112-114)new(102-109)dash-spv/src/network/manager.rs (1)
new(78-128)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/block_download_test.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv-ffi/src/config.rs (1)
dash-spv-ffi/src/types.rs (2)
new(24-32)new(297-311)
dash-spv/src/main.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/src/storage/mod.rs (1)
dash-spv/src/client/config.rs (2)
testnet(117-119)regtest(122-124)
dash-spv/tests/chainlock_simple_test.rs (2)
dash-spv/src/network/manager.rs (1)
new(78-128)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/examples/spv_with_wallet.rs (2)
dash-spv/src/client/config.rs (2)
testnet(117-119)new(102-109)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/header_sync_test.rs (2)
dash-spv/src/client/config.rs (2)
new(102-109)regtest(122-124)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/peer_test.rs (2)
dash-spv/src/client/config.rs (1)
new(102-109)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/tests/edge_case_filter_sync_test.rs (3)
dash-spv/src/client/config.rs (1)
new(102-109)dash-spv/src/network/manager.rs (1)
new(78-128)dash-spv/src/storage/mod.rs (1)
new(97-140)
dash-spv/examples/simple_sync.rs (1)
dash-spv/src/storage/mod.rs (1)
new(97-140)
⏰ 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). (20)
- GitHub Check: Ubuntu / core
- GitHub Check: Windows / spv
- GitHub Check: Ubuntu ARM / core
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: Address Sanitizer
- GitHub Check: Thread Sanitizer
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (macos-latest)
🔇 Additional comments (33)
dash-spv/src/network/manager.rs (1)
82-83: LGTM!The simplified
data_dirinitialization directly fromconfig.storage_pathis cleaner and aligns with the config-driven storage refactor. Removing the fallback to"."is appropriate sincestorage_pathis now a required field inClientConfig.dash-spv/src/client/config_test.rs (1)
58-58: LGTM!The assertion correctly reflects that
storage_pathis now a non-optionalPathBufrather thanOption<PathBuf>.dash-spv-ffi/tests/test_wallet_manager.rs (2)
24-28: LGTM!Good test hygiene:
TempDiris bound to a local variable (temp_dir) that lives for the entire test scope, ensuring the directory exists throughout test execution. The FFI path handling viaCStringis correct.
61-65: LGTM!Consistent with the first test's pattern for setting up storage configuration before client creation.
dash-spv/tests/block_download_test.rs (1)
329-329: Correct API usage, but depends on thecreate_test_config()fix above.The config-based
DiskStorageManager::new(&config)call is correct. Once theTempDirlifetime issue increate_test_config()is fixed, this test should work as expected.dash-spv-ffi/src/client.rs (1)
155-160: No action required. The code properly handles default storage paths throughClientConfig::default(), which initializesstorage_pathto"./dash-spv-storage". FFI callers who omitdash_spv_ffi_config_set_data_dir()receive this sensible default, not an invalid or uninitialized path. The removal of fallback logic is correct—defaults should be centralized inClientConfigrather than duplicated across the FFI layer.Likely an incorrect or invalid review comment.
dash-spv/tests/chainlock_simple_test.rs (2)
34-40: Config-driven storage setup looks consistent.Nice alignment with the new config-based storage path flow.
Also applies to: 81-87
47-48: DiskStorageManager constructor update is correct.Passing
&configmatches the new API.Also applies to: 94-95
dash-spv/src/main.rs (1)
323-329: Storage manager initialization is aligned with the new API.Looks good with
DiskStorageManager::new(&config).dash-spv-ffi/src/config.rs (1)
70-88: LGTM!The change correctly updates the FFI setter to assign directly to the non-optional
PathBuffield. The.into()conversion from&strtoPathBufis idiomatic and correct. Error handling for invalid UTF-8 is properly preserved.dash-spv/examples/simple_sync.rs (1)
19-28: LGTM!The example correctly demonstrates the new config-driven storage initialization pattern. The
with_storage_pathbuilder method is used to configure the storage path, andDiskStorageManager::new(&config)properly derives the path from the configuration. This aligns well with the centralized configuration approach introduced in this PR.dash-spv/examples/filter_sync.rs (1)
25-33: LGTM!The filter sync example correctly adopts the new config-driven storage initialization pattern, consistent with the changes in
simple_sync.rs. The storage path is configured via the builder method and consumed byDiskStorageManager::new(&config).dash-spv/src/lib.rs (1)
27-33: Doc example matches the new config-based storage init.Looks consistent with the updated DiskStorageManager::new(&config) usage.
dash-spv/examples/spv_with_wallet.rs (2)
20-22: Builder-style config setup looks good.Keeps configuration immutable while still flexible.
28-28: Storage manager construction is aligned with the config-driven API.dash-spv/tests/header_sync_test.rs (3)
14-14: No comment.
34-34: Storage initialization follows the new config-driven API.
90-91: Regtest setup is consistent with config-driven storage.dash-spv/tests/peer_test.rs (10)
36-36: No comment.
42-42: Storage manager init aligned with config-based API.
78-78: No comment.
86-86: Storage manager init aligned with config-based API.
92-94: Client construction with cloned config looks good.
111-111: Storage manager init aligned with config-based API.
140-140: No comment.
149-149: Storage manager init aligned with config-based API.
174-174: No comment.
184-184: Storage manager init aligned with config-based API.dash-spv/src/client/config.rs (2)
37-38: No comment.
139-140: Builder-style storage path setter looks good.dash-spv/src/storage/mod.rs (3)
35-35: No review needed.
97-139: EnsureClientConfigis validated before use.
DiskStorageManager::newnow trustsconfig.storage_pathdirectly; if callers skip validation, you can get late IO errors or inconsistent paths. Consider invokingconfig.validate()here (if cheap) or documenting/adding a debug assertion that the config is validated.
421-462: Test updates align with the config-driven constructor.Nice, consistent migration to
DiskStorageManager::new(&config)across the storage tests.Also applies to: 465-518, 521-557, 559-576, 578-604
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| storage_path: PathBuf::from("./dash-spv-storage"), | ||
| validation_mode: ValidationMode::Full, |
There was a problem hiding this comment.
Default storage_path is network-agnostic; risk of cross-network data mixing.
If callers use ClientConfig::testnet()/regtest() without overriding storage_path, multiple networks will share the same on-disk state directory. That can corrupt or misinterpret chain data. Consider deriving a network-specific default (e.g., ./dash-spv-storage/{network}) in ClientConfig::new or when constructing defaults for non-mainnet.
🤖 Prompt for AI Agents
In `@dash-spv/src/client/config.rs` around lines 83 - 84, The default storage_path
is network-agnostic and can cause cross-network data mixing; update ClientConfig
construction so the default storage_path includes the network name (e.g.,
"./dash-spv-storage/{network}") when building non-mainnet configs. Modify
ClientConfig::new (and/or the ClientConfig::testnet() and
ClientConfig::regtest() helpers) to derive storage_path from the chosen
network/ValidationMode and set the storage_path field accordingly instead of
PathBuf::from("./dash-spv-storage"), ensuring each network gets a unique
subdirectory.
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Do you really want to remove all those test files?
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
|
yep, and for a couple of reasons
|
|
Hm ok, probably better to just drop all the tests first separate PR, feels a bit out of topic? |
|
yeah, i can do that, but given the state of the repo i would rather prefer to clean as fast as we can |
I like cleaning up, but i don't like to mix unrelated things into PRs. I almost get a heart attack when i sometimes see unrelated comments being removed or unrelated formatting changes so how should i deal with thousands of lines of unrelated removals? ;) One way to get faster is having clear scoped PRs to begin with that don't lead to many questions or confusion! |
… is built using the config
ZocoLini
force-pushed
the
chore/config-storage-improvement
branch
from
31a910a to
c60ae8c
Compare
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
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/tests/peer_test.rs (1)
195-195: Assertion message contradicts the expected value.The assertion expects
MAX_PEERSto equal3, but the message states "Default max peers should be 12". This is confusing and likely a copy-paste error or outdated message.🔧 Proposed fix
- assert_eq!(MAX_PEERS, 3, "Default max peers should be 12"); + assert_eq!(MAX_PEERS, 3, "Default max peers should be 3");
♻️ Duplicate comments (9)
dash-spv/src/client/config.rs (1)
83-83: Default storage_path is network-agnostic; risk of cross-network data mixing.If callers use
ClientConfig::testnet()/regtest()without overridingstorage_path, multiple networks will share the same on-disk state directory. That can corrupt or misinterpret chain data. Consider deriving a network-specific default (e.g.,./dash-spv-storage/{network}) inClientConfig::newor when constructing defaults for non-mainnet.dash-spv/src/storage/mod.rs (1)
142-148: Keep theTempDiralive for the storage lifetime.Line 146 drops
TempDirat the end ofwith_temp_dir, deleting the directory whileDiskStorageManagerstill uses it. That can lead to flaky tests or writes failing against a removed path. Consider returning theTempDirhandle alongside the storage so it stays in scope.🧩 Suggested fix
- pub async fn with_temp_dir() -> StorageResult<Self> { + pub async fn with_temp_dir() -> StorageResult<(TempDir, Self)> { use tempfile::TempDir; let temp_dir = TempDir::new()?; - Self::new(&ClientConfig::testnet().with_storage_path(temp_dir.path())).await + let storage = + Self::new(&ClientConfig::testnet().with_storage_path(temp_dir.path())).await?; + Ok((temp_dir, storage)) }dash-spv/src/client/mod.rs (1)
85-89: HoldTempDirin a variable to avoid immediate cleanup.Line 85 and Line 111 create
TempDirinline, so it’s dropped at the end of the expression. That deletes the directory beforeDiskStorageManager::new(&config)runs and also prevents automatic cleanup. Bind it to a local variable and keep it in scope for the test duration.🔧 Suggested fix
- let config = ClientConfig::mainnet() + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::mainnet() .without_filters() .without_masternodes() .with_mempool_tracking(MempoolStrategy::FetchAll) - .with_storage_path(TempDir::new().unwrap().path()); + .with_storage_path(temp_dir.path()); @@ - let config = ClientConfig::testnet() + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::testnet() .without_filters() .without_masternodes() .with_mempool_tracking(MempoolStrategy::FetchAll) - .with_storage_path(TempDir::new().unwrap().path()); + .with_storage_path(temp_dir.path());Also applies to: 111-115
dash-spv/benches/storage.rs (1)
36-39: Keep the temporary directory alive during the benchmark.Line 37 and Line 52 create
TempDirinline, so it’s dropped immediately. That removes the directory before storage use and prevents cleanup. Keep theTempDirhandle alive for the duration of each benchmark run.🔧 Suggested fix
b.to_async(&rt).iter_batched( || async { - let config = - ClientConfig::testnet().with_storage_path(TempDir::new().unwrap().path()); - DiskStorageManager::new(&config).await.unwrap() + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::testnet().with_storage_path(temp_dir.path()); + let storage = DiskStorageManager::new(&config).await.unwrap(); + (temp_dir, storage) }, |a| async { - let mut storage = a.await; + let (_temp_dir, mut storage) = a.await; for chunk in headers.chunks(CHUNK_SIZE as usize) { storage.store_headers(chunk).await.unwrap(); } }, BatchSize::SmallInput, ) }); - let config = ClientConfig::testnet().with_storage_path(TempDir::new().unwrap().path()); + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::testnet().with_storage_path(temp_dir.path()); let mut storage = rt.block_on(async { let mut storage = DiskStorageManager::new(&config).await.unwrap(); for chunk in headers.chunks(CHUNK_SIZE as usize) { storage.store_headers(chunk).await.unwrap(); } storage });Also applies to: 52-55
dash-spv/tests/filter_header_verification_test.rs (1)
177-180: Avoid inlineTempDircreation to preserve directory lifetime.Line 178, Line 342, Line 518, and Line 615 create
TempDirinline, so it drops immediately and deletes the directory before storage uses it. Keep theTempDirhandle in scope for each test.🔧 Suggested pattern
- let config = ClientConfig::new(Network::Dash).with_storage_path(TempDir::new().unwrap().path()); + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::new(Network::Dash).with_storage_path(temp_dir.path());Also applies to: 342-345, 518-521, 615-618
dash-spv/tests/header_sync_test.rs (1)
25-27: KeepTempDiralive for the duration of the test.Line 25 creates
TempDirinline, so it drops immediately and deletes the directory before storage init. Store it in a local variable.🔧 Suggested fix
- let config = ClientConfig::new(Network::Dash) - .with_validation_mode(ValidationMode::Basic) - .with_storage_path(TempDir::new().expect("Failed to create tmp dir").path()); + let temp_dir = TempDir::new().expect("Failed to create tmp dir"); + let config = ClientConfig::new(Network::Dash) + .with_validation_mode(ValidationMode::Basic) + .with_storage_path(temp_dir.path());dash-spv/tests/peer_test.rs (1)
18-29:TempDiris dropped immediately, invalidating the storage path before tests run.On line 21,
TempDir::new().unwrap().path().to_path_buf()creates a temporary directory that is dropped immediately after extracting its path. The directory is deleted beforeDiskStorageManager::new(&config)can use it, causing tests to fail or operate on non-existent/recreated paths.Return the
TempDiralongsideClientConfigto keep it alive for the test's duration:🔧 Proposed fix
-fn create_test_config(network: Network) -> ClientConfig { +fn create_test_config(network: Network) -> (ClientConfig, TempDir) { + let temp_dir = TempDir::new().unwrap(); let mut config = ClientConfig::new(network); - - config.storage_path = TempDir::new().unwrap().path().to_path_buf(); - + config.storage_path = temp_dir.path().to_path_buf(); config.validation_mode = ValidationMode::Basic; config.enable_filters = false; config.enable_masternodes = false; config.max_peers = 3; config.peers = vec![]; // Will be populated by DNS discovery - config + (config, temp_dir) }Then update all call sites to hold the
TempDir:let (config, _temp_dir) = create_test_config(Network::Testnet);dash-spv/tests/edge_case_filter_sync_test.rs (2)
110-117:TempDirdropped immediately, invalidating the storage path.On line 112,
TempDir::new().unwrap().path()creates a temporary directory that is dropped immediately after the builder chain completes. The directory is deleted beforeDiskStorageManager::new(&config)on line 117 can use it.🔧 Proposed fix
#[tokio::test] async fn test_filter_sync_at_tip_edge_case() { - let config = ClientConfig::new(Network::Dash).with_storage_path(TempDir::new().unwrap().path()); - + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::new(Network::Dash).with_storage_path(temp_dir.path()); + let received_heights = Arc::new(Mutex::new(HashSet::new())); let mut filter_sync = FilterSyncManager::new(&config, received_heights); let mut storage = DiskStorageManager::new(&config).await.expect("Failed to create tmp storage");
152-159: SameTempDirlifetime issue in second test.Line 154 has the identical problem:
TempDir::new().unwrap().path()drops theTempDirimmediately after the builder chain, deleting the directory beforeDiskStorageManager::new(&config)on line 159 can use it.🔧 Proposed fix
#[tokio::test] async fn test_no_invalid_getcfheaders_at_tip() { - let config = ClientConfig::new(Network::Dash).with_storage_path(TempDir::new().unwrap().path()); - + let temp_dir = TempDir::new().unwrap(); + let config = ClientConfig::new(Network::Dash).with_storage_path(temp_dir.path()); + let received_heights = Arc::new(Mutex::new(HashSet::new())); let mut filter_sync = FilterSyncManager::new(&config, received_heights); let mut storage = DiskStorageManager::new(&config).await.expect("Failed to create tmp storage");
🧹 Nitpick comments (1)
dash-spv/src/client/config.rs (1)
209-215: Side-effect invalidate()may be surprising.
validate()now creates the storage directory viacreate_dir_all. While ensuring the path exists is useful, performing I/O in a method named "validate" is unconventional—callers typically expect validation to be read-only. Consider either renaming tovalidate_and_prepare()or moving directory creation to the storage initialization phase (which already callscreate_dir_allinDiskStorageManager::new).
Summary by CodeRabbit
Release Notes
with_storage_path()) for simplified chaining and setup.✏️ Tip: You can customize this high-level summary in your review settings.