feat(rs-platform-wallet-ffi): expose devnet name and LLMQ_DEVNET override in spv_start#3758
Conversation
…ride in spv_start Devnet SPV sync needs two knobs the FFI didn't expose: - Dash Core devnet peers gate the inbound handshake on a `devnet.devnet-<name>` substring in the user agent (`net_processing.cpp:3957`). Without it, the SPV client cannot connect to any devnet peer. - `LLMQ_DEVNET` size/threshold must match the devnet's `-llmqdevnetparams=<size>:<threshold>` so the SPV client reconstructs the right signing sets for ChainLock / legacy InstantSend verification. Both are already library fields on `dash-spv`'s `ClientConfig` after rust-dashcore PR dashpay/rust-dashcore#784. This change wires them through the FFI and the Swift wrapper as pure plumbing — no decisions on either side. What changed: - `platform_wallet_manager_spv_start` gains three trailing params: `devnet_name: *const c_char`, `llmq_devnet_size: u32`, `llmq_devnet_threshold: u32`. `0`/`NULL` mean "leave default". Validates `(devnet_name set) <=> (network == Devnet)` and `(size > 0) <=> (threshold > 0)` and rejects size/threshold on non-devnet networks. When `devnet_name` is set, rebuilds the user agent as `/{base}(devnet.devnet-{name})/`, falling back to a `platform-wallet-ffi:<version>` base when the caller didn't supply one — mirrors the format the `dash-spv` binary uses. - `PlatformSpvStartConfig` gains `devnetName: String?`, `llmqDevnetSize: UInt32`, `llmqDevnetThreshold: UInt32` (all defaulted, so existing call sites compile unchanged). `startSpv(config:)` marshals them to the new trailing FFI params. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds SPV devnet-specific configuration support to both the Rust FFI module and Swift SDK wrapper. The Rust layer validates devnet parameters, rewrites user-agent strings for devnet identity, and populates LLMQ configuration. The Swift layer exposes these parameters through ChangesSPV Devnet and LLMQ Parameter Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "a8f988ec674ed853368bf44dff653391ae2dc2183128ae12e76778cef6a709fb"
)Xcode manual integration:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3758 +/- ##
============================================
- Coverage 87.32% 87.17% -0.16%
============================================
Files 2590 2607 +17
Lines 316977 319484 +2507
============================================
+ Hits 276809 278501 +1692
- Misses 40168 40983 +815
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Pure FFI plumbing for two devnet knobs (devnet_name and LLMQ_DEVNET size/threshold). The validation matrix is sensible and mirrors the documented contract. Three small input-edge-cases are worth tightening at the boundary; nothing blocks merge. The codex 'blocking' truncation claim could not be verified against the pinned dash-spv revision (58d61ea) and is dropped.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/spv.rs:237-248: Empty `devnet_name` passes the required-on-devnet check
The check on line 243 only rejects `NULL`. A caller passing an empty (but non-null) C string for `devnet_name` while `network == Devnet` satisfies validation, and the user agent is then rebuilt as `/{base}(devnet.devnet-)/`. That trailing `devnet.devnet-` substring will not match any real devnet's gating prefix, so the SPV client returns `ok()` but silently fails the handshake — precisely the failure mode this PR exists to prevent. Because the Swift wrapper takes `devnetName: String?` (not a validated enum), the FFI is the right place to enforce non-empty.
| let devnet_name_str = if devnet_name.is_null() { | ||
| None | ||
| } else { | ||
| Some(unwrap_result_or_return!(CStr::from_ptr(devnet_name).to_str()).to_string()) | ||
| }; | ||
|
|
||
| if net == crate::types::Network::Devnet && devnet_name_str.is_none() { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| "devnet_name is required when network=Devnet", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Empty devnet_name passes the required-on-devnet check
The check on line 243 only rejects NULL. A caller passing an empty (but non-null) C string for devnet_name while network == Devnet satisfies validation, and the user agent is then rebuilt as /{base}(devnet.devnet-)/. That trailing devnet.devnet- substring will not match any real devnet's gating prefix, so the SPV client returns ok() but silently fails the handshake — precisely the failure mode this PR exists to prevent. Because the Swift wrapper takes devnetName: String? (not a validated enum), the FFI is the right place to enforce non-empty.
| let devnet_name_str = if devnet_name.is_null() { | |
| None | |
| } else { | |
| Some(unwrap_result_or_return!(CStr::from_ptr(devnet_name).to_str()).to_string()) | |
| }; | |
| if net == crate::types::Network::Devnet && devnet_name_str.is_none() { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "devnet_name is required when network=Devnet", | |
| ); | |
| } | |
| let devnet_name_str = if devnet_name.is_null() { | |
| None | |
| } else { | |
| Some(unwrap_result_or_return!(CStr::from_ptr(devnet_name).to_str()).to_string()) | |
| }; | |
| if net == crate::types::Network::Devnet | |
| && devnet_name_str.as_deref().map_or(true, str::is_empty) | |
| { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "devnet_name is required (non-empty) when network=Devnet", | |
| ); | |
| } | |
| if net != crate::types::Network::Devnet && devnet_name_str.is_some() { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "devnet_name is only valid on devnet", | |
| ); | |
| } |
source: ['claude', 'codex']
| if let Some(name) = devnet_name_str.as_deref() { | ||
| let base = user_agent_str | ||
| .clone() | ||
| .unwrap_or_else(|| format!("platform-wallet-ffi:{}", env!("CARGO_PKG_VERSION"))); | ||
| user_agent_str = Some(format!("/{base}(devnet.devnet-{name})/")); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: User-agent rebuild double-wraps an already-BIP14-formatted base
If the caller supplies a base already in BIP14 form (e.g. "/myapp:1.0/"), the rewrite emits "//myapp:1.0/(devnet.devnet-name)/" — extra slashes that deviate from the /rust-dash-spv:VERSION(devnet.devnet-NAME)/ format the comment claims to mirror. Dash Core's substring filter still matches, so this is cosmetic, but normalizing the base avoids surprising peer-log fingerprints.
| if let Some(name) = devnet_name_str.as_deref() { | |
| let base = user_agent_str | |
| .clone() | |
| .unwrap_or_else(|| format!("platform-wallet-ffi:{}", env!("CARGO_PKG_VERSION"))); | |
| user_agent_str = Some(format!("/{base}(devnet.devnet-{name})/")); | |
| } | |
| if let Some(name) = devnet_name_str.as_deref() { | |
| let base = user_agent_str | |
| .clone() | |
| .unwrap_or_else(|| format!("platform-wallet-ffi:{}", env!("CARGO_PKG_VERSION"))); | |
| let base = base.trim_matches('/'); | |
| user_agent_str = Some(format!("/{base}(devnet.devnet-{name})/")); | |
| } |
source: ['claude']
| if (llmq_devnet_size > 0) ^ (llmq_devnet_threshold > 0) { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| "llmq_devnet_size and llmq_devnet_threshold must both be set or both zero", | ||
| ); | ||
| } | ||
| if llmq_devnet_size > 0 && net != crate::types::Network::Devnet { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| "llmq_devnet_params is only valid on devnet", | ||
| ); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: No threshold <= size check at the FFI boundary
The XOR check ensures both LLMQ params are zero or both nonzero, but accepts size=3, threshold=10. Downstream ClientConfig::validate() is expected to catch it, but enforcing at the FFI surface produces a direct ErrorInvalidParameter instead of a less direct downstream failure. Low priority — confirm whether spawn_in_background actually invokes validate() before applying the config; if it does, this is purely cosmetic.
| if (llmq_devnet_size > 0) ^ (llmq_devnet_threshold > 0) { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "llmq_devnet_size and llmq_devnet_threshold must both be set or both zero", | |
| ); | |
| } | |
| if llmq_devnet_size > 0 && net != crate::types::Network::Devnet { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "llmq_devnet_params is only valid on devnet", | |
| ); | |
| } | |
| if (llmq_devnet_size > 0) ^ (llmq_devnet_threshold > 0) { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "llmq_devnet_size and llmq_devnet_threshold must both be set or both zero", | |
| ); | |
| } | |
| if llmq_devnet_threshold > llmq_devnet_size { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "llmq_devnet_threshold must be <= llmq_devnet_size", | |
| ); | |
| } | |
| if llmq_devnet_size > 0 && net != crate::types::Network::Devnet { | |
| return PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | |
| "llmq_devnet_params is only valid on devnet", | |
| ); | |
| } |
source: ['claude']
2 participants
Issue being fixed or feature implemented
SPV-syncing a Dash devnet from the platform wallet currently doesn't work — the FFI's
platform_wallet_manager_spv_starthas no way to pass the two devnet-specific knobs that rust-dashcore PR dashpay/rust-dashcore#784 recently exposed onClientConfig(already pinned at58d61eain ourCargo.toml):devnet.devnet-<name>substring in the peer's user agent (net_processing.cpp:3957). Without it, the SPV client cannot connect to any devnet peer.LLMQ_DEVNETsize/threshold override. Mirrors Dash Core's-llmqdevnetparams=<size>:<threshold>. Without matching params, the SPV client reconstructs the wrong quorum and ChainLock / legacy InstantSend signatures fail to verify.This PR wires both through the FFI and the Swift wrapper as pure plumbing.
What was done?
Rust FFI (
rs-platform-wallet-ffi) —platform_wallet_manager_spv_startgains three trailing params:devnet_name: *const c_char—NULLmeans "not devnet".llmq_devnet_size: u32,llmq_devnet_threshold: u32— both0means "no override".Validations (returned as
ErrorInvalidParameter):devnet_namemust be set iffnetwork == Devnet.llmq_devnet_sizeandllmq_devnet_thresholdmust both be set or both zero.ClientConfig::validate()wording for consistency).When
devnet_nameis supplied, the user agent is rebuilt as/{base}(devnet.devnet-{name})/, falling back to aplatform-wallet-ffi:<crate version>base when the caller didn't pass one — mirrors the format thedash-spvbinary constructs inmain.rs. The LLMQ override is written intoClientConfig.llmq_devnet_params;DashSpvClient::newapplies it viaapply_global_overrides(idempotent for identical values).Swift wrapper (
SwiftDashSDK) —PlatformSpvStartConfiggainsdevnetName: String?,llmqDevnetSize: UInt32,llmqDevnetThreshold: UInt32, all defaulted so existing call sites (e.g.CoreContentView.swift) compile unchanged.startSpv(config:)marshalsdevnetNameto a C string with the samestrdup/defer freepattern asuserAgent, then passes the new trailing args to the FFI. Pure marshalling — no decisions on the Swift side (complies withpackages/swift-sdk/CLAUDE.md).Diff stat: 2 files changed, +93 / -3.
How Has This Been Tested?
cargo check -p platform-wallet-ffi— clean.cargo fmt --all -- --check— clean.cd packages/swift-sdk && ./build_ios.sh --target sim— succeeded end-to-end (regenerated the C header insideDashSDKFFI.xcframework, then ranxcodebuild buildonSwiftExampleAppwith-warnings-as-errors; reportsSwift/Xcode build succeeded).The regenerated header inside the xcframework carries the new signature:
Breaking Changes
None at the Swift level — every new field on
PlatformSpvStartConfigis defaulted (devnetName: nil,llmqDevnetSize: 0,llmqDevnetThreshold: 0), so existing call sites compile unchanged.The Rust
extern "C"signature does gain three trailing params, which is a C-ABI source-level break for any out-of-tree caller callingplatform_wallet_manager_spv_startdirectly. Perfeedback_breaking_change_scope,feat!:is reserved for consensus-breaking changes in this repo, so this lands under plainfeat:.Checklist:
Summary by CodeRabbit
Release Notes