feat(dash-spv): consolidate devnet config into DevnetConfig struct#788
Merged
Conversation
`From<u8> for LLMQType` was missing the `107 => LlmqtypeDevnetPlatform` arm, so wire decoders silently mapped Platform quorum messages to `LlmqtypeUnknown`. Add it so devnet Platform routing can be observed by SPV when overrides are exercised.
Mirror Dash Core's `-llmqchainlocks`, `-llmqinstantsenddip0024`, and `-llmqplatform` flags with process-global `OnceLock` setters and getters. Wire them into `NetworkLLMQExt::{chain_locks_type, isd_llmq_type, platform_type}` so the `Network::Devnet` arms honor any active override.
Setters reject types outside Dash Core's devnet allowlist (`50_60`, `60_75`, `400_60`, `400_85`, `100_67`, `devnet`, `devnet_dip0024`, `devnet_platform`) and enforce the rotation invariants Core applies in `chainparams.cpp`: ChainLocks must not rotate, InstantSend DIP24 must rotate, Platform has no rotation constraint. Setters are idempotent on identical re-set and error on conflict, matching the existing `set_llmq_devnet_params` contract.
`enabled_llmq_types(Devnet)` expands to all eight devnet-registered LLMQ types so DKG window scheduling tracks any legal routing target.
`devnet_llmq_type_from_name` parses the Dash-Core name set, accepting `llmq_dev_platform` as a defensive alias for this crate's local spelling of `LLMQ_DEV_PLATFORM`.
Replace `ClientConfig.llmq_devnet_params` and the free-floating `--devnet-name` CLI flag with a single `Option<DevnetConfig>` field on `ClientConfig`. The struct collects every devnet-only setting in one place and surfaces the routing overrides added in the previous commit (`-llmqchainlocks`, `-llmqinstantsenddip0024`, `-llmqplatform`). `ClientConfig::validate` collapses to a biconditional: `devnet.is_some() == (network == Network::Devnet)`. The devnet user-agent suffix (`/rust-dash-spv:VERSION(devnet.devnet-NAME)/`) moves off `main.rs` and onto `DevnetConfig::user_agent`, so library consumers building a devnet client get the same handshake string without re-implementing it. `main.rs` gains `--llmq-chainlocks`, `--llmq-instantsend-dip0024`, `--llmq-platform` flags and extracts `build_client_config(&Args, PathBuf)` so the `Args` → `ClientConfig` mapping is unit-testable.
`config_test.rs` adds round-trip, user-agent format, table-driven validation matrix, empty-name rejection, and a single-test forwarding check that exercises all four `apply_global_overrides` slots in one shot. The forwarding test owns the process-global `OnceLock`s for the whole binary, comment block calls that out so future tests don't accidentally collide. `main.rs` gains a `tests` module that drives the extracted `build_devnet_config`, `build_client_config`, and `parse_llmq_devnet_params` helpers via clap's `try_parse_from`: covers missing `--devnet-name`, devnet flags rejected on non-devnet networks, minimal devnet build, full five-flag compose, parse-error shapes, and unknown quorum-name rejection.
Hoist three duplicate `use crate::sml::llmq_type::network::NetworkLLMQExt;` lines out of individual lifecycle tests in `dash/src/sml/llmq_type/mod.rs` into the `mod tests` module top, and pull `use dashcore::sml::llmq_type::{...}` out of `test_apply_global_overrides_forwards_all_slots` in `dash-spv/src/client/config_test.rs`.
Replace inline fully-qualified paths in `dash-spv/src/main.rs`: `dash_spv::ValidationMode` and `dash_spv::LLMQType::*` become normal imports; `tempfile::TempDir::new()` in the test module uses `use tempfile::TempDir;` instead.
Trim cross-referencing comments that named sibling tests or caller flows. Fold `test_devnet_llmq_type_from_name_rejects_unknown_names` into `test_devnet_llmq_type_from_name`. Swap a `;` clause separator for `.` in a `DevnetConfig` doc comment.
Contributor
📝 WalkthroughWalkthroughThis PR adds a complete devnet LLMQ routing override system. It introduces ChangesDevnet LLMQ Routing Override System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR spans multiple files across two crates with heterogeneous logic: new struct definition, configuration integration with validation, process-global override state with constraint enforcement, network routing updates, CLI parsing and composition, and comprehensive multi-layered testing. The changes introduce new concepts (DevnetConfig, devnet override setters/getters) and require understanding their interaction across module boundaries, though the implementation is straightforward and well-tested. Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dash/src/sml/llmq_type/mod.rs (1)
304-330: 💤 Low valueMinor: TOCTOU race can produce misleading error message (acceptable for CLI use).
If two threads concurrently call a setter with the same valid value, one will succeed but the other may receive an error stating "already set to a different value" due to the gap between
slot.get()andslot.set(). The actual behavior is correct (value gets set properly), but the error message is misleading in this edge case.Given this is intended for single-threaded CLI configuration at startup, this is acceptable. If library-level concurrent access becomes a use case, consider rechecking the value inside the
Errbranch ofslot.set().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dash/src/sml/llmq_type/mod.rs` around lines 304 - 330, The TOCTOU can make set_devnet_routing_override return a misleading "already set to a different value" when slot.set() fails but the concurrent setter wrote the same llmq_type; modify the Err branch of slot.set() so that on set error you immediately recheck slot.get() — if Some(&existing) == llmq_type return Ok(()) else keep returning Err(format!("-{} already set to a different value", flag_name)); this touches the OnceLock usage in set_devnet_routing_override and uses slot.get()/slot.set() and the LLMQType comparison to disambiguate the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dash-spv/src/client/devnet.rs`:
- Around line 79-105: Replace ad-hoc Result<(), String> error handling in
DevnetConfig::validate and DevnetConfig::apply_global_overrides
(dash-spv/src/client/devnet.rs) with a typed thiserror enum (e.g., DevnetError)
that has variants for validation failures (EmptyName, ContainsSlash) and for
each override failure wrapping the underlying dashcore error (e.g.,
LlmqParamsError(#[from] dashcore::LlmqParamsError), ChainLocksTypeError(#[from]
dashcore::ChainLocksTypeError), ...). Change
DevnetConfig::apply_global_overrides to return Result<(), DevnetError> and
remove map_err(|e| e.to_string()) by returning the underlying error via the From
conversions; update DevnetConfig::validate to return Result<(), DevnetError>.
Then update ClientConfig::validate and ClientConfig::apply_global_overrides
(dash-spv/src/client/config.rs) to return a new higher-level ConfigError (also a
thiserror enum) that can wrap DevnetError (via #[from]) so callers can
pattern-match on structured errors instead of strings.
---
Nitpick comments:
In `@dash/src/sml/llmq_type/mod.rs`:
- Around line 304-330: The TOCTOU can make set_devnet_routing_override return a
misleading "already set to a different value" when slot.set() fails but the
concurrent setter wrote the same llmq_type; modify the Err branch of slot.set()
so that on set error you immediately recheck slot.get() — if Some(&existing) ==
llmq_type return Ok(()) else keep returning Err(format!("-{} already set to a
different value", flag_name)); this touches the OnceLock usage in
set_devnet_routing_override and uses slot.get()/slot.set() and the LLMQType
comparison to disambiguate the error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90b58fe2-c543-493a-9f6f-bc1ba50b04f4
📒 Files selected for processing (8)
dash-spv/src/client/config.rsdash-spv/src/client/config_test.rsdash-spv/src/client/devnet.rsdash-spv/src/client/mod.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash/src/sml/llmq_type/mod.rsdash/src/sml/llmq_type/network.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #788 +/- ##
==========================================
+ Coverage 72.21% 72.44% +0.22%
==========================================
Files 321 322 +1
Lines 70467 70898 +431
==========================================
+ Hits 50890 51361 +471
+ Misses 19577 19537 -40
|
QuantumExplorer
approved these changes
May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #786. Groups every devnet-only
ClientConfigknob (name,llmq_params, three LLMQ routing overrides) into oneOption<DevnetConfig>field. Validation becomes one biconditional:devnet.is_some() == (network == Network::Devnet).Lands the three Dash Core routing flags (
-llmqchainlocks,-llmqinstantsenddip0024,-llmqplatform) with rotation invariants enforced perchainparams.cpp: ChainLocks must not rotate, ISD24 must rotate. Setters accept all 8 devnet-registered LLMQ types, andenabled_llmq_types(Devnet)expands to match so DKG scheduling tracks any legal override target.Summary by CodeRabbit
Release Notes
New Features
Improvements