created benches with criterion and moved one performance test to it #277
Conversation
|
Warning Rate limit exceeded@ZocoLini has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdded a Criterion-based storage benchmark and supporting Cargo configuration, updated two documentation files to rename "Performance" references to "dash-spv/src", and removed a legacy performance test import and test function from the segmented storage tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
🧹 Nitpick comments (2)
dash-spv/benches/storage.rs (2)
47-80: Consider isolating benchmarks to avoid shared mutable state.The
storageinstance is created once (lines 47-57) and reused across both the "get" and "reverse_index" benchmarks (lines 59-67, 69-80). While this works for the current benchmark suite, it creates several concerns:
- The benchmarks depend on the setup phase completing successfully
- Benchmark isolation is compromised - if Criterion runs benchmarks in different orders or selectively, they may fail
- The
StorageManagertrait uses&mut selfmethods, suggesting potential for state mutations that could affect subsequent measurementsConsider refactoring to follow the same pattern as the "store" benchmark, where each benchmark iteration has isolated setup and teardown.
Example refactoring for the "get" benchmark:
- c.bench_function("storage/disk/get", |b| { - b.to_async(&rt).iter_batched( - || rand::random::<u32>() % NUM_ELEMENTS as u32, - async |height| { - let _ = storage.get_header(height).await.unwrap(); - }, - BatchSize::SmallInput, - ) - }); + c.bench_function("storage/disk/get", |b| { + b.to_async(&rt).iter_batched( + || async { + let height = rand::random::<u32>() % NUM_ELEMENTS as u32; + let mut storage = DiskStorageManager::new(temp_dir.path().to_path_buf()).await.unwrap(); + for chunk in headers.chunks(CHUNK_SIZE as usize) { + storage.store_headers(chunk).await.unwrap(); + } + (storage, height) + }, + |a| async { + let (storage, height) = a.await; + let _ = storage.get_header(height).await.unwrap(); + }, + BatchSize::LargeInput, + ) + });Note: This would include setup time in the benchmark, so you may need to adjust the approach or use a cached/pre-populated storage directory.
24-29: Consider reducing the dataset size for faster benchmark execution.The benchmark uses 260,000 headers (13,000 × 20), which creates a large dataset. While this provides realistic performance data, it may make the benchmarks slow to run during development.
Consider reducing to a smaller but still representative size (e.g., 50,000-100,000 headers) to speed up benchmark execution, especially since the sample size is already set to 10 iterations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
TEST_SUMMARY.md(1 hunks)UNIFIED_SDK.md(1 hunks)dash-spv/Cargo.toml(1 hunks)dash-spv/benches/storage.rs(1 hunks)dash-spv/tests/segmented_storage_test.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/tests/segmented_storage_test.rsdash-spv/benches/storage.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Write integration tests for network operations in Rust
Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate in RustIntegration tests use
tests/directory (e.g.,rpc-integration-test)
Files:
dash-spv/tests/segmented_storage_test.rs
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/segmented_storage_test.rsdash-spv/benches/storage.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/segmented_storage_test.rs
🧠 Learnings (24)
📓 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/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
📚 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:
TEST_SUMMARY.mddash-spv/Cargo.tomldash-spv/tests/segmented_storage_test.rsdash-spv/benches/storage.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Cover critical parsing, networking, SPV, and wallet flows with tests; add regression tests for fixes; consider property tests with `proptest`
Applied to files:
TEST_SUMMARY.md
📚 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 : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/Cargo.toml
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: When making changes to Rust FFI in dash-spv-ffi, rebuild with `cargo build --release` and run Swift tests to verify integration
Applied to files:
dash-spv/Cargo.tomldash-spv/tests/segmented_storage_test.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)
Applied to files:
dash-spv/Cargo.tomldash-spv/benches/storage.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/Cargo.tomldash-spv/tests/segmented_storage_test.rsdash-spv/benches/storage.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash-spv/Cargo.tomldash-spv/benches/storage.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/Cargo.toml
📚 Learning: 2025-08-21T04:45:50.436Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet/src/gap_limit.rs:291-295
Timestamp: 2025-08-21T04:45:50.436Z
Learning: The rust-dashcore project uses Rust 1.89 as evidenced by rust-version = "1.89" in dash-spv/Cargo.toml. Modern Rust features like Option::is_none_or (stabilized in 1.82) can be used safely.
Applied to files:
dash-spv/Cargo.toml
📚 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/Cargo.toml
📚 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/Cargo.toml
📚 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: Build dash-spv-ffi for iOS targets before running the unified SDK build process with `build_ios.sh`
Applied to files:
UNIFIED_SDK.md
📚 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/segmented_storage_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/segmented_storage_test.rs
📚 Learning: 2025-12-01T08:00:37.919Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:37.919Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize tests by functionality: BIP32 key derivation, mnemonic generation/validation, address generation/validation, path derivation, and PSBT serialization in separate test files
Applied to files:
dash-spv/tests/segmented_storage_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/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/segmented_storage_test.rsdash-spv/benches/storage.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/tests/segmented_storage_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/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/segmented_storage_test.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.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Keep Rust modules focused and organized
Applied to files:
dash-spv/benches/storage.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/*.rs : Keep test vectors deterministic in Rust test code
Applied to files:
dash-spv/benches/storage.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/*.rs : Write unit tests for new functionality in Rust
Applied to files:
dash-spv/benches/storage.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Write integration tests for network operations in Rust
Applied to files:
dash-spv/benches/storage.rs
🔇 Additional comments (6)
dash-spv/tests/segmented_storage_test.rs (1)
10-10: LGTM! Appropriate change for functional tests.The removal of
Instantand the performance test aligns with the PR objective of moving performance validation to Criterion benchmarks. The remaining tests correctly useDurationfor timeouts and focus on functional correctness rather than performance measurement.dash-spv/Cargo.toml (2)
69-71: LGTM! Correct Criterion benchmark configuration.The benchmark configuration is properly set up with
harness = false, which is the correct way to integrate Criterion benchmarks. The async_tokio feature in the dev-dependency (line 62) correctly supports async benchmarks.
62-62: Criterion 0.8.1 is the current latest stable version (released December 7, 2025). No security advisories exist for the Criterion crate.dash-spv/benches/storage.rs (3)
12-21: LGTM! Deterministic test data generation.The helper function creates deterministic test headers, which is appropriate for reproducible benchmarking.
31-45: LGTM! Proper benchmark isolation.The store benchmark correctly uses
iter_batchedwith per-iteration setup, ensuring each iteration benchmarks against a fresh storage instance. This provides accurate performance measurements.
87-93: Appropriate Criterion configuration.The sample size of 10 with a 1-second warm-up is reasonable for I/O-heavy benchmarks. This balances measurement accuracy with execution time.
| - Concurrent operations and thread safety | ||
| - Edge cases and error scenarios | ||
| - Performance considerations | ||
| - dash-spc/src considerations |
There was a problem hiding this comment.
Fix typo in the documentation.
The text reads "dash-spc/src considerations" but should likely be "dash-spv/src considerations" to match the project naming convention.
Apply this diff:
- - dash-spc/src considerations
+ - dash-spv/src considerations📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - dash-spc/src considerations | |
| - dash-spv/src considerations |
🤖 Prompt for AI Agents
In TEST_SUMMARY.md around line 83, the entry "dash-spc/src considerations" is a
typo; replace "dash-spc/src" with "dash-spv/src" so it matches the project
naming convention, and scan adjacent lines for any other occurrences to update
them for consistency.
UNIFIED_SDK.md
Outdated
| 2. **No Symbol Conflicts**: Shared dependencies included only once | ||
| 3. **Simplified Distribution**: Single XCFramework to manage | ||
| 4. **Better Performance**: Reduced memory footprint and faster load times | ||
| 4. **Better dash-spv/src**: Reduced memory footprint and faster load times |
There was a problem hiding this comment.
Clarify or revert the benefit heading.
The change from "Better Performance:" to "Better dash-spv/src:" doesn't make grammatical sense as a benefit statement. This appears to be an unintended edit.
Consider reverting to the original:
-4. **Better dash-spv/src**: Reduced memory footprint and faster load times
+4. **Better Performance**: Reduced memory footprint and faster load timesOr if the intent was to rename this section, please clarify what "Better dash-spv/src" means in this context.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 4. **Better dash-spv/src**: Reduced memory footprint and faster load times | |
| 4. **Better Performance**: Reduced memory footprint and faster load times |
🤖 Prompt for AI Agents
In UNIFIED_SDK.md around line 70, the benefit heading "Better dash-spv/src" is
grammatically incorrect or unintended; either revert it to the original "Better
Performance:" or replace it with a clear, meaningful heading that matches the
content (e.g., "Improved Performance:" or "Reduced Memory Footprint and Faster
Load Times:"). Update the heading text to be grammatically correct and
consistent with the listed benefits, and ensure any mentions of "dash-spv/src"
are clarified or removed if they were accidental.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
dash-spv/benches/storage.rs (1)
1-93: Add module-level documentation.The benchmark file lacks module-level documentation explaining what aspects of storage performance are being measured and why. This would help maintainers understand the benchmark's purpose and expected performance characteristics.
Add documentation at the top of the file:
+//! Criterion benchmarks for `DiskStorageManager` operations. +//! +//! Measures performance of: +//! - Storing headers in chunks +//! - Retrieving individual headers by height +//! - Reverse index lookups (hash → height) + use std::time::Duration;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
TEST_SUMMARY.md(1 hunks)UNIFIED_SDK.md(1 hunks)dash-spv/Cargo.toml(1 hunks)dash-spv/benches/storage.rs(1 hunks)dash-spv/tests/segmented_storage_test.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- dash-spv/Cargo.toml
- TEST_SUMMARY.md
- UNIFIED_SDK.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types with thiserror crate and propagate errors appropriately in Rust code
Use tokio runtime for async operations in Rust code
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format Rust code with cargo fmt before committing
Ensure clippy passes with all warnings as errors on all Rust code
**/*.rs: Each crate keeps sources insrc/; unit tests live alongside code with#[cfg(test)]
Maintain MSRV of 1.89 for Rust code
Format Rust code withrustfmt(seerustfmt.toml); runcargo fmt --allbefore commits
Lint Rust code withclippy; avoidunwrap()/expect()in library code; use error types (e.g.,thiserror)
Usesnake_casefor function and variable names in Rust
UseUpperCamelCasefor types and traits in Rust
UseSCREAMING_SNAKE_CASEfor constants in Rust
Follow mixed editions (2021/2024) and crate-specific idioms; prefer async viatokiowhere applicable
Unit tests should be placed near code with descriptive names (e.g.,test_parse_address_mainnet)
Use#[ignore]for network-dependent or long-running tests; run with-- --ignoredflag
Never commit secrets or real keys; avoid logging sensitive data in Rust code
Keep test vectors deterministic in Rust test code
Files:
dash-spv/tests/segmented_storage_test.rsdash-spv/benches/storage.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/tests/**/*.rs: Write integration tests for network operations in Rust
Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate in RustIntegration tests use
tests/directory (e.g.,rpc-integration-test)
Files:
dash-spv/tests/segmented_storage_test.rs
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/segmented_storage_test.rsdash-spv/benches/storage.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/segmented_storage_test.rs
🧠 Learnings (18)
📓 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/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: When making changes to Rust FFI in dash-spv-ffi, rebuild with `cargo build --release` and run Swift tests to verify integration
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Run pre-PR checks: `cargo fmt`, `cargo clippy`, `cargo test` (workspace). Update docs/CHANGELOG if user-facing
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
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.
📚 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/segmented_storage_test.rsdash-spv/benches/storage.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/tests/segmented_storage_test.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: When making changes to Rust FFI in dash-spv-ffi, rebuild with `cargo build --release` and run Swift tests to verify integration
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/benches/storage.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/segmented_storage_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/src/validation/**/*.rs : Implement three validation modes: ValidationMode::None (no validation), ValidationMode::Basic (structure and timestamp validation), and ValidationMode::Full (complete PoW and chain validation)
Applied to files:
dash-spv/tests/segmented_storage_test.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/segmented_storage_test.rsdash-spv/benches/storage.rs
📚 Learning: 2025-12-01T08:00:50.618Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: swift-dash-core-sdk/CLAUDE.md:0-0
Timestamp: 2025-12-01T08:00:50.618Z
Learning: Applies to swift-dash-core-sdk/**/*.rs : Implement new FFI functions in Rust with `#[no_mangle] extern "C"` annotation in dash-spv-ffi
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/benches/storage.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Applies to **/*.rs : Keep test vectors deterministic in Rust test code
Applied to files:
dash-spv/tests/segmented_storage_test.rsdash-spv/benches/storage.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 : Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Applied to files:
dash-spv/tests/segmented_storage_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/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/tests/segmented_storage_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/tests/segmented_storage_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/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/segmented_storage_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/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/segmented_storage_test.rsdash-spv/benches/storage.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Keep Rust modules focused and organized
Applied to files:
dash-spv/benches/storage.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/*.rs : Write unit tests for new functionality in Rust
Applied to files:
dash-spv/benches/storage.rs
📚 Learning: 2025-12-01T08:01:18.174Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T08:01:18.174Z
Learning: Organize workspace with crates: `dash`, `hashes`, `internals`, `dash-network`, `dash-spv`, `key-wallet`, `rpc-*`, utilities (`fuzz`, `test-utils`), and FFI crates (`*-ffi`)
Applied to files:
dash-spv/benches/storage.rs
📚 Learning: 2025-12-01T07:59:46.015Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:46.015Z
Learning: Applies to **/tests/**/*.rs : Write integration tests for network operations in Rust
Applied to files:
dash-spv/benches/storage.rs
🧬 Code graph analysis (1)
dash-spv/benches/storage.rs (2)
dash-spv/src/client/core.rs (1)
storage(169-171)dash-spv/tests/segmented_storage_test.rs (1)
create_test_header(15-24)
🔇 Additional comments (1)
dash-spv/tests/segmented_storage_test.rs (1)
10-10: LGTM: Appropriate removal of timing imports.Removing
Instantfrom imports is correct since performance measurements have been moved to Criterion benchmarks per the PR objectives.
| fn create_test_header(height: u32) -> Header { | ||
| Header { | ||
| version: Version::from_consensus(1), | ||
| prev_blockhash: BlockHash::all_zeros(), | ||
| merkle_root: dashcore_hashes::sha256d::Hash::all_zeros().into(), | ||
| time: height, | ||
| bits: CompactTarget::from_consensus(0x207fffff), | ||
| nonce: height, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated test helper to a shared module.
The create_test_header function is identical to the one in dash-spv/tests/segmented_storage_test.rs (lines 14-23). This duplication should be eliminated by extracting it to a shared test utilities module that both the tests and benchmarks can import.
Consider creating a dash-spv/tests/common/mod.rs or similar:
// In dash-spv/tests/common/mod.rs
use dashcore::{block::Version, BlockHash, CompactTarget, Header};
pub fn create_test_header(height: u32) -> Header {
Header {
version: Version::from_consensus(1),
prev_blockhash: BlockHash::all_zeros(),
merkle_root: dashcore_hashes::sha256d::Hash::all_zeros().into(),
time: height,
bits: CompactTarget::from_consensus(0x207fffff),
nonce: height,
}
}Then import it in both files:
// In benches and tests
mod common;
use common::create_test_header;🤖 Prompt for AI Agents
In dash-spv/benches/storage.rs around lines 12 to 21, the create_test_header
helper is duplicated elsewhere; extract it to a shared test utilities module
(e.g. dash-spv/tests/common/mod.rs) containing the create_test_header function,
then replace the local function with an import: add mod common; and use
common::create_test_header; in both dash-spv/benches/storage.rs and
dash-spv/tests/segmented_storage_test.rs so both files reuse the single
implementation.
| c.bench_function("storage/disk/store", |b| { | ||
| b.to_async(&rt).iter_batched( | ||
| || async { | ||
| DiskStorageManager::new(TempDir::new().unwrap().path().to_path_buf()).await.unwrap() | ||
| }, | ||
| |a| async { | ||
| let mut storage = a.await; | ||
|
|
||
| for chunk in headers.chunks(CHUNK_SIZE as usize) { | ||
| storage.store_headers(chunk).await.unwrap(); | ||
| } | ||
| }, | ||
| BatchSize::SmallInput, | ||
| ) | ||
| }); |
There was a problem hiding this comment.
Benchmark measures initialization overhead along with storage operations.
The storage/disk/store benchmark creates a new TempDir and DiskStorageManager for each iteration (line 34). This means the benchmark is measuring both initialization costs and storage operation costs, which may not provide an accurate measurement of pure storage performance.
If the intent is to benchmark only storage operations, consider setting up the storage manager once outside the benchmark loop, or document that this benchmark intentionally measures initialization + storage.
🤖 Prompt for AI Agents
In dash-spv/benches/storage.rs around lines 31 to 45 the benchmark currently
creates a new TempDir and DiskStorageManager inside the per-iteration setup,
which causes the benchmark to measure initialization overhead together with
storage operations; move the TempDir::new() and DiskStorageManager::new(...)
call out of the per-iteration closure and initialize the storage manager once
before calling c.bench_function so each iteration only runs the store_headers
loop (or, if you intend to measure init+store, add a clear comment above the
benchmark documenting that initialization is measured).
| c.bench_function("storage/disk/get", |b| { | ||
| b.to_async(&rt).iter_batched( | ||
| || rand::random::<u32>() % NUM_ELEMENTS, | ||
| async |height| { | ||
| let _ = storage.get_header(height).await.unwrap(); | ||
| }, | ||
| BatchSize::SmallInput, | ||
| ) | ||
| }); | ||
|
|
||
| c.bench_function("storage/disk/reverse_index", |b| { | ||
| b.to_async(&rt).iter_batched( | ||
| || { | ||
| let height = rand::random::<u32>() % NUM_ELEMENTS; | ||
| headers[height as usize].block_hash() | ||
| }, | ||
| async |hash| { | ||
| let _ = storage.get_header_height_by_hash(&hash).await.unwrap(); | ||
| }, | ||
| BatchSize::SmallInput, | ||
| ) | ||
| }); |
There was a problem hiding this comment.
Use deterministic inputs for reproducible benchmarks.
The benchmarks use rand::random() to generate heights (line 61) and hashes (lines 72-73), making the benchmark inputs non-deterministic. This violates the coding guideline to keep test vectors deterministic and may reduce benchmark reproducibility across runs.
As per coding guidelines, consider using a seeded RNG or a deterministic sequence:
+use rand::{rngs::StdRng, Rng, SeedableRng};
+
fn bench_disk_storage(c: &mut Criterion) {
const CHUNK_SIZE: u32 = 13_000;
const NUM_ELEMENTS: u32 = CHUNK_SIZE * 20;
+ const SEED: u64 = 42;
let rt = Builder::new_multi_thread().worker_threads(4).enable_all().build().unwrap();
let headers = (0..NUM_ELEMENTS).map(create_test_header).collect::<Vec<Header>>();
+ let mut rng = StdRng::seed_from_u64(SEED);
// ... storage setup ...
c.bench_function("storage/disk/get", |b| {
b.to_async(&rt).iter_batched(
- || rand::random::<u32>() % NUM_ELEMENTS,
+ || rng.gen::<u32>() % NUM_ELEMENTS,
async |height| {
let _ = storage.get_header(height).await.unwrap();
},
BatchSize::SmallInput,
)
});Committable suggestion skipped: line range outside the PR's diff.
Right now we have the performance checks as tests and executed at the same time in the same conditions what is not ideal. This is a first step into moving those performance checks to the benches folder under criterion.
Ideally we would be executing the important benches in CI/CD if we want those checks
Summary by CodeRabbit
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.