feat(sdk): masternodes-by-voting-key lookup (contested-username voting post-cutover) - #4258
feat(sdk): masternodes-by-voting-key lookup (contested-username voting post-cutover)#4258bfoss765 wants to merge 4 commits into
Conversation
SPV-runtime accessor (current-tip MasternodeList via latest_masternode_list) -> platform-wallet-ffi platform_wallet_manager_masternodes_by_voting_key (IdentifierArray of proTxHashes) -> JNI masternodesByVotingKey (flat 32-byte rows, internal order) -> Kotlin external fun. Replaces the app's dashj getMasternodesByVotingKey dependency for contested-username voting post-cutover. Uses the new rust-dashcore MasternodeList::masternodes_by_voting_key helper (rev 4d927c15). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y_voting_key `MasternodeList::masternodes_by_voting_key` (with its unit test) is not yet in dashpay/rust-dashcore, so the 8 workspace git deps move from dashpay/rust-dashcore@70d4bf8e to bfoss765/rust-dashcore@4d927c15 for the lifetime of this PR. The fork rev is a strict fast-forward superset of the rev v4.2-dev already pins: 4 commits ahead, 0 behind, no divergence. Revert this commit once the helper lands upstream and re-pin to dashpay/rust-dashcore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds voting-key-based masternode lookup across the SPV runtime, platform-wallet FFI, JNI, and Kotlin SDK. It returns matching proTxHashes and adds validation, ownership, empty-state handling, and tests. ChangesMasternode voting-key lookup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant KotlinWalletManagerNative
participant UnifiedSdkJni
participant PlatformWalletFfi
participant SpvRuntime
KotlinWalletManagerNative->>UnifiedSdkJni: masternodesByVotingKey(votingKeyId)
UnifiedSdkJni->>PlatformWalletFfi: platform_wallet_manager_masternodes_by_voting_key
PlatformWalletFfi->>SpvRuntime: masternodes_by_voting_key_blocking
SpvRuntime-->>PlatformWalletFfi: matching proTxHashes
PlatformWalletFfi-->>UnifiedSdkJni: IdentifierArray
UnifiedSdkJni-->>KotlinWalletManagerNative: concatenated proTxHashes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
✅ Final review complete — no blockers (commit ce8233e) |
![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
🤖 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 `@packages/rs-platform-wallet-ffi/src/types.rs`:
- Around line 144-154: Update IdentifierArray::from_hashes to convert the hashes
Vec into a boxed slice and retain its exact allocation layout when transferring
ownership, rather than forgetting a Vec while storing only count. Update
platform_wallet_identifier_array_free to reconstruct the boxed slice via
std::slice::from_raw_parts_mut(...).into() using the stored count before
dropping it, ensuring creation and free paths use matching slice allocation
semantics.
In `@packages/rs-platform-wallet/src/spv/runtime.rs`:
- Around line 370-397: Add regression coverage for
masternodes_by_voting_key_blocking covering matching keys, no matches,
unavailable DML state, and preservation of returned internal byte order. Add FFI
and JNI tests verifying empty results and concatenated 32-byte rows, using the
existing test helpers and API symbols.
🪄 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 Plus
Run ID: a18dcb85-1c59-42c4-9d50-177c8b2b4c77
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/rs-platform-wallet-ffi/src/spv.rspackages/rs-platform-wallet-ffi/src/types.rspackages/rs-platform-wallet/src/spv/runtime.rspackages/rs-unified-sdk-jni/src/wallet_manager.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Rust-to-Kotlin lookup is structurally coherent, but three blocking issues remain: the returned hash buffer can be freed with the wrong allocation layout, one error path violates the documented initialized-out-parameter contract, and the workspace is pinned to an explicitly temporary personal fork containing unrelated wallet changes. The new runtime and FFI/JNI byte contracts also lack regression coverage.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(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/types.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/types.rs:144-154: Preserve the exact allocation layout for returned hash arrays
`from_hashes` leaks a `Vec<[u8; 32]>` but exports only its pointer and length. The paired free function reconstructs it with `Vec::from_raw_parts(items, count, count)`, which is valid only when the original capacity equals `count`. That invariant is false on the production path: collecting one match from the filtered masternode map produces a vector with spare capacity, and the same-layout mapping in the runtime preserves it. JNI unconditionally invokes the free function, so an ordinary one-result lookup can deallocate with a different layout than was allocated, causing undefined behavior. Convert the vector to an exact-length boxed slice before transferring ownership.
In `packages/rs-platform-wallet-ffi/src/spv.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/spv.rs:176-180: Initialize the output before validating the input pointer
`check_ptr!(voting_key_id)` can return `ErrorNullPointer` before `*out_array` receives the empty sentinel. A valid but initially uninitialized output therefore remains garbage on this error path, despite the function's explicit guarantee that errors leave the output initialized. A C or Swift caller performing unconditional cleanup can pass that garbage to `platform_wallet_identifier_array_free`, which may attempt to free an arbitrary pointer. Validate `out_array`, publish the sentinel, and only then validate the input pointer.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Remove the temporary personal-fork dependency pin before merge
All eight workspace rust-dashcore dependencies are redirected to `bfoss765/rust-dashcore`. The pinned revision is four commits beyond the previous Dash-owned revision and includes 528 changed lines of unrelated owner-tagged-reservation and asset-lock behavior in addition to the lookup helper. The PR description explicitly says this pin is temporary and must not merge as-is; upstream rust-dashcore PR #916 is still open and currently mixes these changes. Re-pin `Cargo.toml` and `Cargo.lock` to a Dash-owned revision containing the helper, or implement the small voting-key filtering loop locally until a suitable upstream revision exists.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:370-397: Add regression coverage for the new lookup boundaries
The fork's helper test covers filtering, multi-match, and no-match behavior, but this PR has no tests for its newly introduced layers. Add coverage for unavailable DML state and preservation of internal proTxHash byte order in the runtime, plus FFI/JNI coverage for the empty sentinel and concatenation of multiple 32-byte rows. These are the contracts consumed by Kotlin and are not exercised by the reported `cargo check`.
| pub fn from_hashes(hashes: Vec<[u8; 32]>) -> Self { | ||
| let count = hashes.len(); | ||
| if count == 0 { | ||
| return Self::empty(); | ||
| } | ||
|
|
||
| let mut items = hashes; | ||
| let ptr = items.as_mut_ptr(); | ||
| std::mem::forget(items); | ||
|
|
||
| Self { items: ptr, count } |
There was a problem hiding this comment.
🔴 Blocking: Preserve the exact allocation layout for returned hash arrays
from_hashes leaks a Vec<[u8; 32]> but exports only its pointer and length. The paired free function reconstructs it with Vec::from_raw_parts(items, count, count), which is valid only when the original capacity equals count. That invariant is false on the production path: collecting one match from the filtered masternode map produces a vector with spare capacity, and the same-layout mapping in the runtime preserves it. JNI unconditionally invokes the free function, so an ordinary one-result lookup can deallocate with a different layout than was allocated, causing undefined behavior. Convert the vector to an exact-length boxed slice before transferring ownership.
| pub fn from_hashes(hashes: Vec<[u8; 32]>) -> Self { | |
| let count = hashes.len(); | |
| if count == 0 { | |
| return Self::empty(); | |
| } | |
| let mut items = hashes; | |
| let ptr = items.as_mut_ptr(); | |
| std::mem::forget(items); | |
| Self { items: ptr, count } | |
| let mut items = hashes.into_boxed_slice(); | |
| let ptr = items.as_mut_ptr(); | |
| std::mem::forget(items); | |
| Self { items: ptr, count } |
source: ['codex']
There was a problem hiding this comment.
Fixed in b1abb82.
Confirmed the invariant really is false on the production path rather than only in principle. The masternodes_by_voting_key collect goes through a filter, so its size_hint lower bound is 0 and the vector grows by doubling: a one-match lookup gives len 1, capacity 4. I ran it under an allocator that records the Layout each pointer was allocated with and compares it at dealloc — the old path allocates 128 bytes and frees as 32. JNI invokes the free unconditionally, so that is reachable from an ordinary single-result lookup.
Took your into_boxed_slice suggestion, and applied it to new as well: new now delegates to from_hashes, and the free reclaims a Box<[[u8; 32]]> of the same length, so creation and release are the same layout by construction rather than by a capacity coincidence. One ownership contract instead of two.
Tests: a round trip built through the same filtered collect, pinning capacity > len as an explicit precondition so the test keeps exercising the mismatch if the producer changes; plus a multi-row byte-for-byte round trip and the empty-sentinel case.
There was a problem hiding this comment.
Resolved in b1abb82 — Preserve the exact allocation layout for returned hash arrays no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| check_ptr!(voting_key_id); | ||
| check_ptr!(out_array); | ||
| // Publish the empty sentinel before any fallible work so an error return | ||
| // never leaves the out-param holding uninitialized stack bytes. | ||
| *out_array = IdentifierArray::empty(); |
There was a problem hiding this comment.
🔴 Blocking: Initialize the output before validating the input pointer
check_ptr!(voting_key_id) can return ErrorNullPointer before *out_array receives the empty sentinel. A valid but initially uninitialized output therefore remains garbage on this error path, despite the function's explicit guarantee that errors leave the output initialized. A C or Swift caller performing unconditional cleanup can pass that garbage to platform_wallet_identifier_array_free, which may attempt to free an arbitrary pointer. Validate out_array, publish the sentinel, and only then validate the input pointer.
| check_ptr!(voting_key_id); | |
| check_ptr!(out_array); | |
| // Publish the empty sentinel before any fallible work so an error return | |
| // never leaves the out-param holding uninitialized stack bytes. | |
| *out_array = IdentifierArray::empty(); | |
| check_ptr!(out_array); | |
| *out_array = IdentifierArray::empty(); | |
| check_ptr!(voting_key_id); |
source: ['codex']
There was a problem hiding this comment.
Fixed in b1abb82, exactly as suggested — check_ptr!(out_array) and the sentinel now come before every other guard.
Added a regression test that pre-poisons the out-param the way an uninitialized C local looks (non-null pointer, non-zero count) and asserts the (null, 0) sentinel survives a null voting_key_id, plus one for the null-out_array rejection. I verified the test actually catches this by restoring the old guard order first — it fails with got items=0xdeadbeef, count=9, i.e. precisely the arbitrary pointer a cleanup-on-error caller would have handed to platform_wallet_identifier_array_free.
While in this function: it also had a pre-existing rustfmt violation that was failing CI's format check independently of these fixes, so that's corrected in the same commit.
There was a problem hiding this comment.
Resolved in b1abb82 — Initialize the output before validating the input pointer no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| dashcore = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } | ||
| dash-network-seeds = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } | ||
| dash-spv = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } | ||
| key-wallet = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } | ||
| key-wallet-ffi = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } | ||
| key-wallet-manager = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } | ||
| dash-network = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } | ||
| dashcore-rpc = { git = "https://github.com/bfoss765/rust-dashcore", rev = "4d927c155b6740ba7e1f271144217f7e82b69990" } |
There was a problem hiding this comment.
🔴 Blocking: Remove the temporary personal-fork dependency pin before merge
All eight workspace rust-dashcore dependencies are redirected to bfoss765/rust-dashcore. The pinned revision is four commits beyond the previous Dash-owned revision and includes 528 changed lines of unrelated owner-tagged-reservation and asset-lock behavior in addition to the lookup helper. The PR description explicitly says this pin is temporary and must not merge as-is; upstream rust-dashcore PR #916 is still open and currently mixes these changes. Re-pin Cargo.toml and Cargo.lock to a Dash-owned revision containing the helper, or implement the small voting-key filtering loop locally until a suitable upstream revision exists.
source: ['codex']
There was a problem hiding this comment.
Fixed in ce8233e — the pin is gone entirely.
All eight workspace rust-dashcore dependencies are back on the Dash-owned revision 70d4bf8e36057c58e02d56769a6e9760f701dd06, which is exactly what v4.2-dev pins. Cargo.toml and Cargo.lock no longer appear in this PR's diff at all, and grep bfoss765 over the full PR diff returns nothing.
I took the second option you offered rather than waiting for upstream. rust-dashcore#916 carries the helper only as the last of four commits, is CHANGES_REQUESTED, conflicts against dev, and its own reviewer has asked for it to be split — so waiting on it was open-ended.
The filter is now local. A 14-line private free function in packages/rs-platform-wallet/src/spv/runtime.rs, over the engine's current-tip MasternodeList. I checked both fields it reads (key_id_voting, pro_reg_tx_hash) are public on 70d4bf8e before relying on them. It also folds in the ProTxHash -> [u8; 32] internal-byte-order conversion the caller was previously doing itself, so masternodes_by_voting_key_blocking comes out net shorter than it was with the helper.
Tested. Four new unit tests cover multi-match, single-match, no-match and the empty-list case. Against the Dash-owned pin: cargo test -p platform-wallet --lib is 497 passed / 0 failed, and cargo check -p platform-wallet-ffi -p rs-unified-sdk-jni is clean.
Tracked, so the duplication doesn't get orphaned: #4262 records why the local copy exists, names the exact function to delete and the upstream call to restore in its place. That issue number is cited in a doc comment directly above the function, so anyone reading the code finds the plan, and I left a note on rust-dashcore#916 so whoever merges it sees the downstream cleanup it enables.
There was a problem hiding this comment.
Resolved in ce8233e — Remove the temporary personal-fork dependency pin before merge no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| pub fn masternodes_by_voting_key_blocking(&self, voting_key_id: &PubkeyHash) -> Vec<[u8; 32]> { | ||
| // Clone the engine `Arc` out while holding the client lock, then drop | ||
| // it before reading the engine — same ordering as `connected_peers`. | ||
| let engine = { | ||
| let client_guard = self.client.blocking_read(); | ||
| let Some(client) = client_guard.as_ref() else { | ||
| return Vec::new(); | ||
| }; | ||
| match client.masternode_list_engine().ok() { | ||
| Some(engine) => engine, | ||
| None => return Vec::new(), | ||
| } | ||
| }; | ||
|
|
||
| let engine_guard = engine.blocking_read(); | ||
| let Some(list) = engine_guard.latest_masternode_list() else { | ||
| return Vec::new(); | ||
| }; | ||
|
|
||
| list.masternodes_by_voting_key(voting_key_id) | ||
| .into_iter() | ||
| .map(|pro_tx| { | ||
| let mut out = [0u8; 32]; | ||
| out.copy_from_slice(pro_tx.as_ref()); | ||
| out | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add regression coverage for the new lookup boundaries
The fork's helper test covers filtering, multi-match, and no-match behavior, but this PR has no tests for its newly introduced layers. Add coverage for unavailable DML state and preservation of internal proTxHash byte order in the runtime, plus FFI/JNI coverage for the empty sentinel and concatenation of multiple 32-byte rows. These are the contracts consumed by Kotlin and are not exercised by the reported cargo check.
source: ['coderabbit']
…with Two blockers from the dashpay#4258 review. (a) `IdentifierArray::from_hashes` leaked a `Vec<[u8; 32]>` but exported only `(ptr, len)`, while `platform_wallet_identifier_array_free` reconstructed it as `Vec::from_raw_parts(items, count, count)` — valid only when the original capacity happened to equal `count`. On the production path it does not: `masternodes_by_voting_key` collects through a `filter`, whose `size_hint` lower bound is 0, so an ordinary one-match lookup yields len 1 / capacity 4. Measured with a layout-checking allocator, that allocates 128 bytes and frees as 32 — undefined behaviour, and JNI invokes the free unconditionally. Both constructors now hand over an exact-length boxed slice (`into_boxed_slice`), and the free path reclaims it as the same `Box<[[u8; 32]]>`, so creation and release use identical layouts regardless of what capacity the producer had. `new` delegates to `from_hashes` so there is a single ownership contract rather than two. (b) `check_ptr!(voting_key_id)` returned before the out-param sentinel was published, so a null input pointer left `*out_array` holding the caller's uninitialized stack — breaking the documented initialized-out contract and handing a cleanup-on-error C/Swift caller an arbitrary pointer to free. The `out_array` guard and sentinel are hoisted above every other guard. Tests: a filtered-collect round trip pinning the capacity != len precondition, a multi-row byte-for-byte round trip, the empty sentinel, and both null-pointer paths. The sentinel test was confirmed to fail against the old guard order. 209 lib tests pass; also fixes a pre-existing rustfmt violation in this function that had CI's format check red. The Cargo.toml fork pin (blocker 3) is deliberately untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
All eight workspace rust-dashcore dependencies were redirected at bfoss765/rust-dashcore solely to pick up `MasternodeList::masternodes_by_voting_key`. That revision also carried ~528 lines of unrelated owner-tagged-reservation and asset-lock behaviour, and the upstream PR carrying the helper (dashpay/rust-dashcore#916) is CHANGES_REQUESTED, conflicting against dev, and has been asked to split — so the pin had no bounded lifetime. Revert the pin to the Dash-owned revision v4.2-dev already tracks (70d4bf8e36057c58e02d56769a6e9760f701dd06) and implement the filter locally in `spv/runtime.rs` as a private free function over the engine's current-tip MasternodeList. It reads only long-standing public SML fields (`key_id_voting`, `pro_reg_tx_hash`, both present on that revision), and folds in the `ProTxHash -> [u8; 32]` internal-byte-order conversion the caller previously did itself, so `masternodes_by_voting_key_blocking` gets shorter rather than longer. Four unit tests cover multi-match, single-match, no-match and the empty-list case. Swapping back to the upstream helper once dashpay#916 lands is tracked by dashpay#4262, cited in a doc comment at the duplication site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rs-platform-wallet/src/spv/runtime.rs (1)
371-392: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a test for the "DML unavailable" early-return paths.
The new tests exercise
masternodes_by_voting_key(the pure filter) for multi-match, single-match, no-match, and empty-list cases.masternodes_by_voting_key_blockingitself has three additional early-return branches — client not started,masternode_list_engine()unavailable, andlatest_masternode_list()returningNone— that are not covered by any test. The prior review round asked for "unavailable DML state" coverage specifically; this remains open at the blocking-accessor level. Add a test that builds anSpvRuntimewithout callingstart()and assertsmasternodes_by_voting_key_blockingreturns an emptyVec.🤖 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 `@packages/rs-platform-wallet/src/spv/runtime.rs` around lines 371 - 392, Add a test for masternodes_by_voting_key_blocking using an SpvRuntime constructed without start(), and assert it returns an empty Vec when the client is unavailable. Place it alongside the existing masternode voting-key tests; no production changes are needed.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/spv/runtime.rs (1)
477-510: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared proTxHash byte-copy.
masternodes_by_voting_key(lines 505-507) copiespro_reg_tx_hash.as_ref()into a[u8; 32]the same waymasternode_validity_snapshot_blockingdoes at lines 346-347. Extracting a small helper, e.g.fn pro_tx_hash_bytes(entry: &MasternodeListEntry) -> [u8; 32], removes this duplication and keeps the internal-byte-order convention in one place.🤖 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 `@packages/rs-platform-wallet/src/spv/runtime.rs` around lines 477 - 510, Extract the duplicated proTxHash byte-copy logic from masternodes_by_voting_key and masternode_validity_snapshot_blocking into a shared pro_tx_hash_bytes helper accepting a MasternodeListEntry and returning [u8; 32]. Replace both local copy implementations with the helper, preserving the existing internal-byte-order behavior.
🤖 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.
Duplicate comments:
In `@packages/rs-platform-wallet/src/spv/runtime.rs`:
- Around line 371-392: Add a test for masternodes_by_voting_key_blocking using
an SpvRuntime constructed without start(), and assert it returns an empty Vec
when the client is unavailable. Place it alongside the existing masternode
voting-key tests; no production changes are needed.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/spv/runtime.rs`:
- Around line 477-510: Extract the duplicated proTxHash byte-copy logic from
masternodes_by_voting_key and masternode_validity_snapshot_blocking into a
shared pro_tx_hash_bytes helper accepting a MasternodeListEntry and returning
[u8; 32]. Replace both local copy implementations with the helper, preserving
the existing internal-byte-order behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e9338254-353c-4494-a3cc-05e8577cd733
📒 Files selected for processing (3)
packages/rs-platform-wallet-ffi/src/spv.rspackages/rs-platform-wallet-ffi/src/types.rspackages/rs-platform-wallet/src/spv/runtime.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
All three prior blocking findings and the fork-pin issue are confirmed fixed at head ce8233e: IdentifierArray now allocates and frees an exact-length boxed slice on both paths (verified in types.rs), the FFI out-parameter sentinel is published before the input-pointer guard (verified in spv.rs), and the temporary personal-fork Cargo pin is fully reverted with a net-zero Cargo.toml/Cargo.lock diff since the PR base, replaced by a locally-implemented, well-documented, and tested voting-key filter. One suggestion-level test-coverage gap remains only partially closed (the blocking accessor's unavailable-client/engine/list branches and the JNI/Kotlin round trip are still untested), and a new suggestion surfaced in this pass: the new native binding has no public Kotlin wrapper on PlatformWalletManager, so SDK consumers outside the kotlin-sdk module cannot actually call it through supported API despite the PR's stated goal of surfacing this lookup to Kotlin.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— security-auditor (completed)
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt:496-516: Expose masternodesByVotingKey through the public PlatformWalletManager API
WalletManagerNative is declared `internal object` (line 23), and masternodesByVotingKey is added here as a raw `external fun` with no corresponding public wrapper in PlatformWalletManager.kt. Every other native call in this class — createWallet, removeWallet, platformAddressSyncStart, identitySyncStart, shieldedSyncStart, trackedIdentityRecoveryAssetLocks, etc. — has a matching public `suspend fun` on PlatformWalletManager that validates input, dispatches via `withContext(Dispatchers.IO)`, and calls through `mapNativeErrors`. This lookup breaks that pattern: it is unreachable from outside the kotlin-sdk module (managerHandle is also private on PlatformWalletManager), and even the raw binding bypasses the IO-dispatch convention even though the underlying Rust accessor documents itself as blocking via two tokio::RwLock::blocking_read calls. Since this PR's stated purpose is surfacing the lookup to Kotlin/dash-wallet consumers, the feature isn't actually usable through supported API without this wrapper.
| external fun spvIsRunning(managerHandle: Long): Boolean | ||
| external fun spvStop(managerHandle: Long) | ||
|
|
||
| /** | ||
| * The proTxHashes of every masternode in the current-tip deterministic | ||
| * masternode list whose voting-key hash matches the 20-byte [votingKeyId] | ||
| * (hash160 of a voting public key), as a flat `byte[]` of concatenated | ||
| * 32-byte proTxHashes (internal byte order) — the caller splits into | ||
| * 32-byte rows. Replaces dashj's | ||
| * `MasternodeListManager.getMasternodesByVotingKey(votingKeyId)` used by | ||
| * contested-username voting. Returns an EMPTY (non-null) `byte[]` when the | ||
| * masternode list hasn't synced (SPV client not running / DML unavailable) | ||
| * or no masternode uses the key; throws only on a structural FFI error. | ||
| * JNI symbol: | ||
| * `Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_masternodesByVotingKey`; | ||
| * bridges `platform_wallet_manager_masternodes_by_voting_key`. | ||
| */ | ||
| external fun masternodesByVotingKey( | ||
| managerHandle: Long, | ||
| votingKeyId: ByteArray, | ||
| ): ByteArray |
There was a problem hiding this comment.
🟡 Suggestion: Expose masternodesByVotingKey through the public PlatformWalletManager API
WalletManagerNative is declared internal object (line 23), and masternodesByVotingKey is added here as a raw external fun with no corresponding public wrapper in PlatformWalletManager.kt. Every other native call in this class — createWallet, removeWallet, platformAddressSyncStart, identitySyncStart, shieldedSyncStart, trackedIdentityRecoveryAssetLocks, etc. — has a matching public suspend fun on PlatformWalletManager that validates input, dispatches via withContext(Dispatchers.IO), and calls through mapNativeErrors. This lookup breaks that pattern: it is unreachable from outside the kotlin-sdk module (managerHandle is also private on PlatformWalletManager), and even the raw binding bypasses the IO-dispatch convention even though the underlying Rust accessor documents itself as blocking via two tokio::RwLock::blocking_read calls. Since this PR's stated purpose is surfacing the lookup to Kotlin/dash-wallet consumers, the feature isn't actually usable through supported API without this wrapper.
| external fun spvIsRunning(managerHandle: Long): Boolean | |
| external fun spvStop(managerHandle: Long) | |
| /** | |
| * The proTxHashes of every masternode in the current-tip deterministic | |
| * masternode list whose voting-key hash matches the 20-byte [votingKeyId] | |
| * (hash160 of a voting public key), as a flat `byte[]` of concatenated | |
| * 32-byte proTxHashes (internal byte order) — the caller splits into | |
| * 32-byte rows. Replaces dashj's | |
| * `MasternodeListManager.getMasternodesByVotingKey(votingKeyId)` used by | |
| * contested-username voting. Returns an EMPTY (non-null) `byte[]` when the | |
| * masternode list hasn't synced (SPV client not running / DML unavailable) | |
| * or no masternode uses the key; throws only on a structural FFI error. | |
| * JNI symbol: | |
| * `Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_masternodesByVotingKey`; | |
| * bridges `platform_wallet_manager_masternodes_by_voting_key`. | |
| */ | |
| external fun masternodesByVotingKey( | |
| managerHandle: Long, | |
| votingKeyId: ByteArray, | |
| ): ByteArray | |
| suspend fun masternodesByVotingKey(votingKeyId: ByteArray): List<ByteArray> = withContext(Dispatchers.IO) { | |
| require(votingKeyId.size == 20) { "votingKeyId must be exactly 20 bytes" } | |
| val flat = mapNativeErrors { WalletManagerNative.masternodesByVotingKey(managerHandle, votingKeyId) } | |
| check(flat.size % 32 == 0) { "masternodesByVotingKey returned a non-multiple-of-32 buffer" } | |
| flat.toList().chunked(32).map { it.toByteArray() } | |
| } |
source: ['codex']
2 participants
Why
The wallet's contested-username voting feature asks "which masternode(s) can this voting key vote for?" Today that answer comes from dashj's
MasternodeListManager.getMasternodesByVotingKey(votingKeyId). Post-cutover the app no longer runs dashj's masternode-list manager, so that list comes back empty and voting silently stops working.Nothing new has to be synced to fix this: the SPV engine already syncs the full SML, and every entry already carries both
key_id_votingandpro_reg_tx_hash. This PR just surfaces the lookup that was already possible over data we already hold — Rust → FFI → JNI → Kotlin.What
The chain, one layer per hop:
SpvRuntime::masternodes_by_voting_key_blockingpackages/rs-platform-wallet/src/spv/runtime.rsplatform_wallet_manager_masternodes_by_voting_keypackages/rs-platform-wallet-ffi/src/spv.rsIdentifierArray::from_hashespackages/rs-platform-wallet-ffi/src/types.rsJava_..._masternodesByVotingKey+read_id20packages/rs-unified-sdk-jni/src/wallet_manager.rsexternal fun masternodesByVotingKeypackages/kotlin-sdk/.../ffi/WalletManagerNative.ktThe runtime accessor reads the current tip via
latest_masternode_list()and delegates to the newMasternodeList::masternodes_by_voting_keyhelper in rust-dashcore.Byte contract
pro_reg_tx_hash.as_ref()convention as the existingmasternode_validity_snapshot_blocking. Kotlin receives oneByteArrayand splits it into 32-byte rows.IdentifierArrayis reused as the transport for raw hashes (these are proTxHashes, not platformIdentifiers — hence the separatefrom_hashesconstructor, with the same leak/free ownership contract asnew). Freed viaplatform_wallet_identifier_array_free.*out_arraybefore any fallible work, so an error return can never leave the out-param holding uninitialized stack bytes.Blocking, like the sibling
*_blockingaccessors: it takes the client + enginetokio::RwLocks viablocking_readand must run off the async runtime. Lock ordering (clone the engineArcout under the client lock, drop it, then read the engine) matchesconnected_peers.The helper this PR calls,
MasternodeList::masternodes_by_voting_key(with a unit test), is not yet in dashpay/rust-dashcore. It currently lives only on the fork.So the second commit here repoints all 8 workspace rust-dashcore git deps:
dashpay/rust-dashcore@70d4bf8e36057c58e02d56769a6e9760f701dd06(whatv4.2-devpins)bfoss765/rust-dashcore@4d927c155b6740ba7e1f271144217f7e82b69990The fork rev is a strict fast-forward superset of the rev
v4.2-devalready pins — 4 commits ahead, 0 behind, no divergence. For full transparency, those 4 commits are the voting-key helper, acargo fmt, and two owner-tagged-reservation changes related to platform#4185 — i.e. the pin drags in slightly more than just the helper.This pin is temporary and should not merge as-is. The intended sequence is: land
masternodes_by_voting_keyupstream in dashpay/rust-dashcore, then revert the pin commit and re-pin to an upstream rev. Happy to hold this PR until that lands, or to split the pin out — reviewer's call.Verification
cargo check -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni— clean (only a pre-existing workspacedefault-featuresmanifest warning that is also present onv4.2-dev).App wiring
The consuming side is already written and running on
dashpay/dash-wallet@feat/kotlin-sdk-phase1; this PR is the SDK half it binds against.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes