chore: cleanup unused config parameters #346
Conversation
Just cleans up all currently unused config parameters.
📝 WalkthroughWalkthroughThis pull request removes 22 configuration fields from ClientConfig, including timeout settings, request concurrency limits, rate-limit controls, and QRInfo-related options. Configuration values are moved from the public API to hardcoded defaults within implementation code, with corresponding test updates and removals reflecting the eliminated fields. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 0
🧹 Nitpick comments (2)
dash-spv/src/sync/filters/manager.rs (2)
113-119: Hardcoded defaults look reasonable.The values are sensible for CFHeaders synchronization:
- 3 retries provides fault tolerance without excessive delays
- 50 concurrent requests balances throughput with resource usage
- 30s timeout is appropriate for network operations
Consider documenting these constants or extracting them to named constants for clarity.
♻️ Optional: Extract to named constants for documentation
+/// Default maximum retries for CFHeader requests +const DEFAULT_MAX_CFHEADER_RETRIES: u32 = 3; +/// Default maximum concurrent CFHeader requests +const DEFAULT_MAX_CONCURRENT_CFHEADER_REQUESTS: usize = 50; +/// Default timeout for CFHeader requests +const DEFAULT_CFHEADER_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); + impl<S: StorageManager, N: NetworkManager> FilterSyncManager<S, N> { pub fn new(config: &ClientConfig, received_filter_heights: SharedFilterHeights) -> Self { Self { // ... - max_cfheader_retries: 3, + max_cfheader_retries: DEFAULT_MAX_CFHEADER_RETRIES, // ... - max_concurrent_cfheader_requests: 50, + max_concurrent_cfheader_requests: DEFAULT_MAX_CONCURRENT_CFHEADER_REQUESTS, // ... - cfheader_request_timeout: Duration::from_secs(30), + cfheader_request_timeout: DEFAULT_CFHEADER_REQUEST_TIMEOUT, } } }
44-44: Remove unused_configfield to eliminate unnecessary memory overhead.The
_configfield is initialized but never read after construction. All CFHeaders configuration values (retry limits, concurrency limits, timeouts) are hardcoded at initialization rather than being sourced from theClientConfigobject. Removing this field would reduce memory footprint and eliminate confusion about unused configuration.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
dash-spv/src/client/config.rsdash-spv/src/client/config_test.rsdash-spv/src/sync/filters/manager.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_simple_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/integration_real_node_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/simple_header_test.rs
💤 Files with no reviewable changes (5)
- dash-spv/tests/peer_test.rs
- dash-spv/tests/chainlock_simple_test.rs
- dash-spv/tests/block_download_test.rs
- dash-spv/src/client/config_test.rs
- dash-spv/src/client/config.rs
🧰 Additional context used
📓 Path-based instructions (7)
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/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/sync/filters/manager.rsdash-spv/tests/simple_header_test.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/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/sync/filters/manager.rsdash-spv/tests/simple_header_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
**/*integration*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write integration tests for network operations
Files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
Files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
dash-spv/src/sync/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Files:
dash-spv/src/sync/filters/manager.rs
🧠 Learnings (14)
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/sync/filters/manager.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_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/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : 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/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_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/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_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/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/simple_header_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/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/simple_header_test.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*integration*.rs : Write integration tests for network operations
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/header_sync_test.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurations
Applied to files:
dash-spv/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_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/tests/integration_real_node_test.rsdash-spv/tests/qrinfo_integration_test.rsdash-spv/tests/simple_header_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/qrinfo_integration_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/src/sync/filters/manager.rsdash-spv/tests/simple_header_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/src/sync/filters/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Tokio channels for inter-component message passing between async tasks
Applied to files:
dash-spv/tests/simple_header_test.rs
🧬 Code graph analysis (2)
dash-spv/tests/header_sync_test.rs (1)
dash-spv/src/client/config.rs (1)
new(115-122)
dash-spv/tests/simple_header_test.rs (1)
dash-spv/src/client/config.rs (1)
new(115-122)
⏰ 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). (18)
- GitHub Check: Ubuntu ARM / core
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Thread Sanitizer
- GitHub Check: Address Sanitizer
🔇 Additional comments (12)
dash-spv/src/sync/filters/manager.rs (1)
11-14: LGTM - Import cleanup and Duration addition.The import reorganization is clean, and adding
std::time::Durationis necessary for the newcfheader_request_timeoutfield.dash-spv/tests/integration_real_node_test.rs (7)
76-79: LGTM - Simplified client configuration.The test correctly removes the explicit connection timeout and relies on default behavior. The test-level timeout via
tokio::time::timeoutin the sync operations provides adequate protection against hangs.
118-121: Configuration change consistent with PR cleanup.Same pattern applied correctly - validation mode is preserved while connection timeout is removed.
191-194: LGTM.
342-344: LGTM.
402-404: LGTM.
475-477: LGTM.
539-541: LGTM.dash-spv/tests/simple_header_test.rs (1)
47-49: LGTM - Consistent with PR cleanup.The test correctly removes the explicit connection timeout configuration while preserving the test-level timeout for the sync operation. The test's error detection logic for "Header does not connect" remains intact.
dash-spv/tests/qrinfo_integration_test.rs (2)
10-11: LGTM - Import cleanup after test removal.The import list is correctly updated to only include types used by the remaining tests. The removed
Duration,ClientConfig, andNetworkimports corresponded to the deleted config builder tests.
104-116: Remaining tests provide good coverage for QRInfo message structure.The retained tests verify QRInfo and GetQRInfo message creation, serialization, and skip list modes - which are the core functionality tests. The removed config tests were testing now-deleted configuration options.
dash-spv/tests/header_sync_test.rs (1)
266-266: LGTM - Configuration simplified.The test correctly removes the explicit connection timeout while retaining the validation mode. The
Durationimport is still used by performance assertions elsewhere in the file.
Just cleans up all currently unused config parameters.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.