Uh oh!
There was an error while loading. Please reload this page.
feat: devnet support for dash-spv - #784
Merged
Merged
Conversation
The static height-0 devnet genesis block has a deterministic hash (verified by the existing `devnet_genesis_full_block` test). Returning `None` here caused `dash-spv` to fail with `"No known genesis hash for network"` when initializing storage for a devnet. Only the height-1 "devnet genesis" varies per devnet name, height 0 does not.
…m quorum `platform_type()` returned `LlmqtypeDevnet`, but Dash Core's devnet defaults use `LLMQ_DEVNET_PLATFORM` for `llmqTypePlatform` (see `chainparams.cpp:651`). This matches the standard devnet config `llmqplatform=llmq_devnet_platform`.
Allows targeting `Network::Devnet` from the `dash-spv` binary. Required for SPV-syncing a devnet, since previously only mainnet, testnet, and regtest were exposed.
Dash Core nodes on devnet check that incoming peers carry `devnet.<network-id>` in their user agent and disconnect otherwise (see `net_processing.cpp:3957-3967`). The `network-id` is `devnet-<name>` per `ArgsManager::GetDevNetName`, so the substring required for the `paloma` devnet is `devnet.devnet-paloma`. The new flag is required whenever `--network=devnet`, and the user agent is rebuilt as `/rust-dash-spv:<version>(devnet.devnet-<name>)/` so devnet peers accept the handshake.
Dash Core's `-llmqdevnetparams=<size>:<threshold>` lets a devnet adjust the `LLMQ_DEVNET` quorum size and threshold (`chainparams.cpp:UpdateLLMQDevnetParameters`). Without matching params, the SPV client picks the wrong masternodes when reconstructing the signing set, so ChainLock and legacy InstantSend signatures cannot be verified on devnets that use the override. `ClientConfig::llmq_devnet_params: Option<(u32, u32)>` carries the override and `DashSpvClient::new` applies it after `validate`. Internally this writes to a `OnceLock` in `dashcore::sml::llmq_type` that `LLMQType::params()` consults for `LlmqtypeDevnet`, mirroring Dash Core's chainparams-as-global model without exposing the setter beyond the lib. Only `LLMQ_DEVNET` is affected (matches Dash Core, the DIP0024 and platform devnet quorums are not adjusted by this flag). The `dash-spv` binary exposes the override as `--llmq-devnet-params=<size>:<threshold>`, valid only with `--network=devnet`.
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds devnet LLMQ runtime overrides, CLI flags and client config support for devnet parameters, a hardcoded devnet genesis hash, and applies configured overrides process-wide during client startup. ChangesDevnet LLMQ Configuration and Runtime Overrides
Sequence Diagram(s)sequenceDiagram
participant User as User / CLI
participant Args as Args parsing
participant ClientConfig as ClientConfig
participant LLMQ as sml::llmq_type
participant OnceLock as OnceLock store
User->>Args: supply --network=devnet + --devnet-name + --llmq-devnet-params
Args->>ClientConfig: build config with llmq_devnet_params
ClientConfig->>LLMQ: apply_global_overrides() calls set_llmq_devnet_params(size,threshold)
LLMQ->>OnceLock: set override once
LLMQ->>LLMQ: llmq_devnet_params() computes effective LLMQParams
ClientConfig->>DashSpvClient: proceed with validated & applied overrides
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## dev #784 +/- ##
==========================================
+ Coverage 72.24% 72.26% +0.01%
==========================================
Files 321 321 Lines 70370 70467 +97 ==========================================
+ Hits 50839 50922 +83 - Misses 19531 19545 +14
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/config.rs`:
- Around line 226-233: The comment and implementation disagree because
set_llmq_devnet_params currently uses a OnceLock that errors on any second set;
change apply_global_overrides (and/or set_llmq_devnet_params) so repeated calls
with the same llmq_devnet_params (size, threshold) are treated as idempotent:
when self.llmq_devnet_params is Some((size, threshold)) first check the existing
stored params via the OnceLock/get API (or equivalent accessor) and if a value
is already set compare it to (size, threshold) and return Ok(()) when they
match, but return Err(...) only when they conflict; only call OnceLock::set (or
perform initialization) when no value is present. Ensure you reference
apply_global_overrides, llmq_devnet_params, set_llmq_devnet_params and the
OnceLock-backed storage when making the change so multiple clients can apply
identical overrides without error.
In `@dash-spv/src/main.rs`:
- Around line 226-231: Validate and sanitize args.devnet_name before embedding
it into user_agent: ensure devnet_name (the value read from args.devnet_name and
used when building user_agent and passed to config.with_user_agent) contains
only allowed characters (e.g. restrict to a safe regex like alphanumeric plus -
and _ or at minimum reject control characters, whitespace and reserved
characters such as parentheses and slashes) and return an error (ok_or) when it
fails validation; update the code path that computes user_agent to perform this
validation/sanitization on devnet_name and only format the user_agent after the
value is confirmed safe.
In `@dash/src/network/constants.rs`:
- Around line 75-81: The Devnet genesis hash in the Network::Devnet match arm is
currently identical to Network::Regtest and will incorrectly gate validation;
update the Network::Devnet arm in constants.rs (the match handling
Network::Devnet / BlockHash construction) to use the correct devnet genesis hash
bytes instead of the regtest literal (or obtain it from the authoritative devnet
constant/source), keep the same reverse() + BlockHash::from_byte_array flow, and
ensure the provided hex decodes to 32 bytes so it no longer matches the Regtest
value.
In `@dash/src/sml/llmq_type/mod.rs`:
- Around line 211-230: Add three tests that exercise the process-wide override:
write test_valid_override to call set_llmq_devnet_params(size,threshold) with a
valid pair and then assert llmq_devnet_params() reflects size, min_size,
threshold and dkg_params.bad_votes_threshold; write
test_invalid_override_rejected to call set_llmq_devnet_params with invalid
values (e.g., threshold > size or zero) and assert it returns Err; write
test_second_set_fails to call set_llmq_devnet_params successfully then call it
again and assert the second call returns Err; because LLMQ_DEVNET_OVERRIDE is
process-global, run each test in its own process (spawn the test binary with
--exact <test_name> or use Command to re-exec the current test binary) so the
OnceLock is fresh for each case; reference functions/setter llmq_devnet_params,
set_llmq_devnet_params and static LLMQ_DEVNET_OVERRIDE and type LLMQParams when
locating code to assert fields.
- Around line 216-227: The override setter set_llmq_devnet_params currently
allows invalid tuples (size==0, threshold==0, threshold>size) which break quorum
logic; add input validation in set_llmq_devnet_params to reject size == 0 or
threshold == 0 and to reject threshold > size (return Err with a clear static
message), only call LLMQ_DEVNET_OVERRIDE.set when inputs pass validation; keep
llmq_devnet_params and LLMQ_DEVNET usage unchanged but rely on the validated
override to ensure size/min_size/threshold remain consistent.
🪄 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: 2a24a4f0-0204-4d03-a733-c3f324b3e9c0
📒 Files selected for processing (6)
dash-spv/src/client/config.rsdash-spv/src/client/lifecycle.rsdash-spv/src/main.rsdash/src/network/constants.rsdash/src/sml/llmq_type/mod.rsdash/src/sml/llmq_type/network.rs
Comment on lines
+226
to
+233
| /// Apply process-wide settings derived from this config. Idempotent for the | ||
| /// same values, returns an error if a conflicting setting was already applied. | ||
| pub(crate) fn apply_global_overrides(&self) -> Result<(), String> { | ||
| if let Some((size, threshold)) = self.llmq_devnet_params { | ||
| set_llmq_devnet_params(size, threshold).map_err(|e| e.to_string())?; | ||
| } | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
apply_global_overrides idempotency contract doesn’t match implementation.
The method comment says same-value calls are idempotent, but OnceLock::set fails on every second call regardless of value. This can break creating multiple clients in one process.
🤖 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-spv/src/client/config.rs` around lines 226 - 233, The comment and
implementation disagree because set_llmq_devnet_params currently uses a OnceLock
that errors on any second set; change apply_global_overrides (and/or
set_llmq_devnet_params) so repeated calls with the same llmq_devnet_params
(size, threshold) are treated as idempotent: when self.llmq_devnet_params is
Some((size, threshold)) first check the existing stored params via the
OnceLock/get API (or equivalent accessor) and if a value is already set compare
it to (size, threshold) and return Ok(()) when they match, but return Err(...)
only when they conflict; only call OnceLock::set (or perform initialization)
when no value is present. Ensure you reference apply_global_overrides,
llmq_devnet_params, set_llmq_devnet_params and the OnceLock-backed storage when
making the change so multiple clients can apply identical overrides without
error.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+211
to
+230
| /// Runtime override for `LLMQ_DEVNET` params, matching Dash Core's `-llmqdevnetparams`. | ||
| static LLMQ_DEVNET_OVERRIDE: OnceLock<(u32, u32)> = OnceLock::new(); | ||
| /// Override the `LLMQ_DEVNET` quorum size and threshold (matches Dash Core's | ||
| /// `-llmqdevnetparams=<size>:<threshold>`). May only be called once per process. | ||
| pub fn set_llmq_devnet_params(size: u32, threshold: u32) -> Result<(), &'static str> { | ||
| LLMQ_DEVNET_OVERRIDE.set((size, threshold)).map_err(|_| "LLMQ_DEVNET params already set") | ||
| } | ||
| /// Get the effective `LLMQ_DEVNET` params, applying any runtime override. | ||
| pub fn llmq_devnet_params() -> LLMQParams { | ||
| let mut params = LLMQ_DEVNET; | ||
| if let Some(&(size, threshold)) = LLMQ_DEVNET_OVERRIDE.get() { | ||
| params.size = size; | ||
| params.min_size = threshold; | ||
| params.threshold = threshold; | ||
| params.dkg_params.bad_votes_threshold = threshold; | ||
| } | ||
| params | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add focused tests for the new process-wide devnet override path.
Please add unit tests for: valid override application, invalid value rejection, and second-set behavior.
As per coding guidelines, "Write unit tests for new functionality".
🤖 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 211 - 230, Add three tests that
exercise the process-wide override: write test_valid_override to call
set_llmq_devnet_params(size,threshold) with a valid pair and then assert
llmq_devnet_params() reflects size, min_size, threshold and
dkg_params.bad_votes_threshold; write test_invalid_override_rejected to call
set_llmq_devnet_params with invalid values (e.g., threshold > size or zero) and
assert it returns Err; write test_second_set_fails to call
set_llmq_devnet_params successfully then call it again and assert the second
call returns Err; because LLMQ_DEVNET_OVERRIDE is process-global, run each test
in its own process (spawn the test binary with --exact <test_name> or use
Command to re-exec the current test binary) so the OnceLock is fresh for each
case; reference functions/setter llmq_devnet_params, set_llmq_devnet_params and
static LLMQ_DEVNET_OVERRIDE and type LLMQParams when locating code to assert
fields.
Uh oh!
There was an error while loading. Please reload this page.
The `apply_global_overrides` doc claimed idempotent behavior, but `OnceLock::set` rejected every second call regardless of value. Constructing multiple `DashSpvClient` instances with the same devnet config in one process therefore failed on the second call. Now the setter compares against the existing value and returns `Ok(())` when it matches, only erroring on a genuine conflict. Addresses CodeRabbit review comment on PR #784#784 (comment)
Add a single test that walks through valid initial set, idempotent reapplication of the same values, and rejection of a conflicting reapplication. The three checks share one test because `LLMQ_DEVNET_OVERRIDE` is a process-global `OnceLock` that cannot be reset between tests. Addresses CodeRabbit review comment on PR #784#784 (comment)
xdustinface
commented
May 27, 2026
CollaboratorAuthor
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dash/src/sml/llmq_type/mod.rs (1)
217-223: ⚡ Quick winUse a typed error for
set_llmq_devnet_params
dash/src/sml/llmq_type/mod.rsexposesdash::sml::llmq_type::set_llmq_devnet_params(size, threshold)as a public API returningResult<(), &'static str>, anddash-spv/src/client/config.rscurrently has to convert that viamap_err(|e| e.to_string())?. Since the SML layer already usesthiserror(e.g.,dash/src/sml/error.rs), add a smallthiserrorerror type for this setter (conflict/AlreadySet) and return it instead of&'static str.🤖 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 217 - 223, Create a typed error for set_llmq_devnet_params by adding a small thiserror-backed enum (e.g., enum LlmqDevnetError { #[error("LLMQ_DEVNET params already set to a different value")] AlreadySet }) in the sml error module or next to mod.rs, change the signature of set_llmq_devnet_params(size: u32, threshold: u32) -> Result<(), LlmqDevnetError>, and convert the existing string errors and map_err calls to return LlmqDevnetError::AlreadySet where appropriate (referencing LLMQ_DEVNET_OVERRIDE and the existing match arms/ set(...) map_err closure) so callers can handle a concrete error type instead of &'static str.
🤖 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/src/sml/llmq_type/mod.rs`:
- Around line 218-223: The current get()-then-set() sequence on
LLMQ_DEVNET_OVERRIDE is a TOCTOU race; instead attempt to set((size, threshold))
first and if set() fails read the stored value and accept it only if it equals
(size, threshold). Concretely, replace the match with: try
LLMQ_DEVNET_OVERRIDE.set((size, threshold)) and return Ok(()) on success; on
Err, call LLMQ_DEVNET_OVERRIDE.get() and compare the existing value to (size,
threshold) — return Ok(()) if equal, otherwise return the conflict Err. This
uses the atomic nature of set() as the first step and makes the idempotent path
safe under concurrent startup.
---
Nitpick comments:
In `@dash/src/sml/llmq_type/mod.rs`:
- Around line 217-223: Create a typed error for set_llmq_devnet_params by adding
a small thiserror-backed enum (e.g., enum LlmqDevnetError { #[error("LLMQ_DEVNET
params already set to a different value")] AlreadySet }) in the sml error module
or next to mod.rs, change the signature of set_llmq_devnet_params(size: u32,
threshold: u32) -> Result<(), LlmqDevnetError>, and convert the existing string
errors and map_err calls to return LlmqDevnetError::AlreadySet where appropriate
(referencing LLMQ_DEVNET_OVERRIDE and the existing match arms/ set(...) map_err
closure) so callers can handle a concrete error type instead of &'static str.
🪄 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: f4a84584-e61a-4d9a-a16a-74ca4970ba63
📒 Files selected for processing (1)
dash/src/sml/llmq_type/mod.rs
Uh oh!
There was an error while loading. Please reload this page.
shumkov
reviewed
May 27, 2026
Uh oh!
There was an error while loading. Please reload this page.
… tuple `Option<(u32, u32)>` was unreadable at call sites — `with_llmq_devnet_params(13, 9)` is one transposition away from a silent bug. Introduce a named struct in `dashcore::sml::llmq_type` so the size/threshold meaning is explicit everywhere it crosses an API boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
approved these changes
May 27, 2026
6 tasks
Uh oh!
There was an error while loading. Please reload this page.
This was referenced May 27, 2026
9 tasks
5 tasks
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
devnetin--network.Network::Devnetso storage can initialize.--devnet-name, embedded in the user agent asdevnet.devnet-<name>to pass Dash Core's devnet handshake check.Network::Devnetplatform quorum toLlmqtypeDevnetPlatform, matching Dash Core's default.ClientConfig::llmq_devnet_params(CLI--llmq-devnet-params=<size>:<threshold>) to mirror Dash Core's-llmqdevnetparamsoverride.Summary by CodeRabbit
New Features
Bug Fixes / Validation