feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim) - #4204
feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim)#4204bfoss765 wants to merge 33 commits into
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (61)
📝 WalkthroughWalkthroughThe PR implements policy-driven Android keystore aliases for identity private keys, durable pending-key repair tracking with Room v8 schema migration, structured signing error codes propagated across Kotlin/JNI/Rust/Swift, one-time Orchard shielded identity creation for invitation claims, and managed-identity outcome translation at the FFI boundary. ChangesKeystore policy and durable repair
Structured signing errors and platform-wallet error mappings
One-time Orchard shielded identity creation
Managed identity translation and documentation updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
7a450ea to
31e7bc1
Compare
|
🕓 Ready for review — 19 ahead in queue (commit 31e6a2f) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The one-time Orchard invitation API follows the existing Type-20 construction, but four blocking issues remain in proof-wait compatibility, ambiguous-broadcast recovery, bearer-key handling, and FFI panic safety. The foreign-key scan also permits untrusted unfunded invitations to trigger an uncancellable genesis-to-tip scan.
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— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 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/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1706-1709: Use the affected-state wait for the Type-20 claim
The current v4.1-dev proof contract introduced by 31c69cf793 marks IdentityCreateFromShieldedPool proofs as affected-state snapshots because they authenticate the resulting identity and nullifiers but cannot bind the complete Orchard request. That commit changed the pool-funded sibling to `wait_for_affected_state`, while this new path still calls the strict `wait_for_response`. After rebasing, every valid claim proof will therefore produce `ExecutionNotProved`, enter the ambiguous fallback, and may be reported unconfirmed despite successful execution. On this older head, the same call accepts a snapshot without making that weaker guarantee explicit. Rebase onto the current proof API and use the affected-state wait, matching the sibling Type-20 path.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1733-1737: Persist a recovery path before returning an unconfirmed claim
This branch returns `ShieldedBroadcastUnconfirmed` without persisting the byte-identical transition or any pending claim metadata. Unlike the pool-funded sibling at lines 1392-1407, there is no `PendingRedrive`; `poke_sync_on_unconfirmed` consequently starts a sync that has no record to process, and the foreign invitation notes are intentionally absent from every subwallet. If an ambiguously accepted transition was actually dropped, nothing safely rebroadcasts it. The Kotlin wrapper compounds this because `TeardownGate.withOp` uses prompt-cancellable `withContext(Dispatchers.IO)`: JNI cannot be interrupted, so cancellation can discard the tagged identity ID or typed unconfirmed exception after native broadcast completion. Persist a pending claim containing the identity ID/index, submitted-key metadata, and re-drivable transition before broadcasting, then reconcile it across cancellation and restart; an equivalent mechanism must safely rebroadcast and treat `NullifierAlreadySpent` as evidence that the original claim executed.
In `packages/rs-unified-sdk-jni/src/funding.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/funding.rs:829-833: Do not marshal the spending key through ordinary unsanitized buffers
`oneTimeSk` is bearer spend authority for a funded invitation, but the generic `read_id32` leaves both its intermediate `Vec<u8>` and returned `[u8; 32]` unsanitized. The FFI and wallet layers create further plain-array copies. The generation direction similarly leaves native `sk`, combined `out`, and Kotlin's 75-byte source blob containing the spending key until their memory is reused or garbage-collected. This repository already uses `read_key32_zeroizing` for JNI private keys and explicitly scrubs invitation bearer scalars in `identity/network/invitation.rs`. Carry this key through `Zeroizing` buffers where possible and explicitly wipe all transient native and JVM arrays after copying out the one intentionally caller-owned key.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1580-1590: OS RNG failure aborts before the JNI panic guard can run
`generate_one_time_orchard_key` calls `OsRng.fill_bytes`, whose rand_core implementation panics when `try_fill_bytes` reports an entropy-source failure. Because that panic occurs inside this `extern "C"` export, it cannot unwind to the outer JNI `guard`; Rust aborts at the C ABI boundary first. An operating-system RNG failure can therefore terminate the Android process instead of producing a Java/native error. Change the generator to use `try_fill_bytes` and propagate a `PlatformWalletFFIResult` error, or place an internal panic fence inside this C export.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:829-855: Unfunded invitation keys force an unbounded full-history scan
Every syntactically valid invitation key starts at position 0 and keeps fetching, proof-verifying, and trial-decrypting chunks until enough value is found or the complete shielded history is exhausted. An attacker can generate valid but unfunded invitation keys, so the value-based early exit provides no bound for hostile input. The supplied birth-height hint is ignored, and cancellation of the Kotlin coroutine cannot interrupt the blocking JNI operation. Add an authenticated or otherwise safe starting position, a maximum resumable scan budget, or another preflight mechanism that prevents one invitation from forcing an unlimited genesis-to-tip scan.
| let proof_result = match st | ||
| .wait_for_response::<StateTransitionProofResult>(sdk, None) | ||
| .await | ||
| { |
There was a problem hiding this comment.
🔴 Blocking: Use the affected-state wait for the Type-20 claim
The current v4.1-dev proof contract introduced by 31c69cf marks IdentityCreateFromShieldedPool proofs as affected-state snapshots because they authenticate the resulting identity and nullifiers but cannot bind the complete Orchard request. That commit changed the pool-funded sibling to wait_for_affected_state, while this new path still calls the strict wait_for_response. After rebasing, every valid claim proof will therefore produce ExecutionNotProved, enter the ambiguous fallback, and may be reported unconfirmed despite successful execution. On this older head, the same call accepts a snapshot without making that weaker guarantee explicit. Rebase onto the current proof API and use the affected-state wait, matching the sibling Type-20 path.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Use the affected-state wait for the Type-20 claim 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.
There was a problem hiding this comment.
Fixed on this branch in fb41ffb2 — "fix(shielded-invites): use wait_for_affected_state for the Type-20 claim (#4204)".
Per your direction, this was done after the rebase rather than by selectively introducing the symbol on the older head: the branch was rebased onto current v4.1-dev (4.1.0-rc.1), which carries the proof-contract commit 31c69cf. The pool-funded sibling picked up wait_for_affected_state from that rebase, and I switched the Type-20 one-time-key claim in operations.rs to match, so both Type-20 paths now move together under the new contract.
broadcast_shielded_spend intentionally stays on wait_for_response — it is a value spend, not an identity-create, so the affected-state relaxation does not apply to it.
Could you resolve this thread?
There was a problem hiding this comment.
Re-verified at head b6992a5dbc: this is fixed in code, and I believe the carry-forward is a stale match rather than a live finding.
The cite:
// packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1791
let proof_result = match st
.wait_for_affected_state::<StateTransitionProofResult>(sdk, None)
.awaitwait_for_response is not called anywhere on the Type-20 one-time-key claim path. The only wait_for_response remaining in this file is on broadcast_shielded_spend, which is a value spend rather than an identity-create, so the affected-state relaxation deliberately does not apply to it.
Both Type-20 paths now move together under the current proof contract: the pool-funded sibling is at operations.rs:1362 and this claim path at operations.rs:1791, both on wait_for_affected_state. The branch is rebased onto v4.1-dev (4.1.0-rc.1), which carries 31c69cf's proof-contract change, so the rebase-first sequencing you asked for was followed rather than selectively introducing the symbol on the older head.
Could you resolve this thread, or point me at the line you are still seeing wait_for_response on?
There was a problem hiding this comment.
Verified addressed — fix(shielded-invites): use wait_for_affected_state for the Type-20 claim (fb41ffb2bc on the reviewed head; c5cb3ae814 on the v4.2-dev rebase). The claim's result wait is now st.wait_for_affected_state::<StateTransitionProofResult>(sdk, None) (packages/rs-platform-wallet/src/wallet/shielded/operations.rs, identity_create_from_one_time_key), matching the pool-funded Type-20 sibling, with the affected-state rationale documented at the call site. The branch has also been rebased onto current v4.2-dev (post-#4268/#4258), so the call now targets the current proof contract; platform-wallet compiles and its 672-test lib suite passes on the rebased head 31e6a2f7f2.
| None => { | ||
| return Err(PlatformWalletError::ShieldedBroadcastUnconfirmed { | ||
| identity_id, | ||
| reason: wait_err.to_string(), | ||
| }); |
There was a problem hiding this comment.
🔴 Blocking: Persist a recovery path before returning an unconfirmed claim
This branch returns ShieldedBroadcastUnconfirmed without persisting the byte-identical transition or any pending claim metadata. Unlike the pool-funded sibling at lines 1392-1407, there is no PendingRedrive; poke_sync_on_unconfirmed consequently starts a sync that has no record to process, and the foreign invitation notes are intentionally absent from every subwallet. If an ambiguously accepted transition was actually dropped, nothing safely rebroadcasts it. The Kotlin wrapper compounds this because TeardownGate.withOp uses prompt-cancellable withContext(Dispatchers.IO): JNI cannot be interrupted, so cancellation can discard the tagged identity ID or typed unconfirmed exception after native broadcast completion. Persist a pending claim containing the identity ID/index, submitted-key metadata, and re-drivable transition before broadcasting, then reconcile it across cancellation and restart; an equivalent mechanism must safely rebroadcast and treat NullifierAlreadySpent as evidence that the original claim executed.
source: ['codex']
There was a problem hiding this comment.
Claim recovery is now implemented on this branch in 7bc8a845 ("feat(platform-wallet): idempotent one-time-key claim recovery"), packages/rs-platform-wallet/src/wallet/shielded/operations.rs.
Up front: this is not the design you specified, and I want to be explicit about that rather than claim the box is ticked. You asked for a persisted, subwallet-less PendingClaim record written before the first broadcast and replayed on restart. What landed instead recovers the same property by re-derivation, with no persisted record, no new table, and no on-disk migration. My argument is that persistence is unnecessary here — but the deviation is real and it is yours to judge.
Why nothing needs persisting: everything a recovery would have to store is already re-derivable from the invite the invitee is holding. The claim is deterministic in the one-time key, so a retry regenerates the same handles from scratch:
master_auth_public_key_hash()— the invitee's MASTER auth key hash. This is the unique Platform-indexed handle the identity is looked up by (the same unique-hash probediscover_inneruses), so the created identity is findable without knowing its id in advance.any_nullifier_spent_on_chain()— a proof-verifiedShieldedNullifierStatusespreflight. If the selected notes are already spent, we recover before rebuilding or rebroadcasting anything.recover_executed_one_time_claim()— the reconciler.NullifierAlreadySpentarms on both the broadcast and the wait paths route here; it resolves by master auth key hash first, then by the deterministically-derived identity id (fetch_identity_with_retries), both with bounded retries for DAPI indexing lag.
So the operation is idempotent: an already-executed claim reconciles to success instead of stranding the retry with a hard error, and it does so from the seed rather than from a record that could itself be lost, half-written, or out of sync with chain state.
Against your specific requirements:
- Fetch and verify the derived identity first; if absent, rebroadcast — inverted, deliberately. The nullifier preflight means we detect the already-executed case before rebuilding, so we never construct a second transition for a claim that already landed.
- If rebroadcast reports
NullifierAlreadySpent, fetch/verify the derived identity before declaring success — this is exactly what theNullifierAlreadySpentarms do. Success is only declared after an identity is actually fetched; it is never inferred from the error alone. - Never silently clear or rebuild a different transition — there is no record to clear, and the preflight prevents rebuilding a different transition.
- Surface a terminal conflict if the identity is still absent — this one differs. When neither handle resolves, it returns
ShieldedBroadcastUnconfirmedcarrying the derived id (retryable) rather than a terminal error. The reasoning: the dominant real cause of "executed but not yet resolvable" is DAPI indexing lag, and a terminal verdict there would strand a claim that is about to become resolvable, whereas a further retry reconciles cleanly once indexing catches up. The app already writes that id out and retries. If you'd rather this be terminal after the bounded retries, say so and I'll change it — it is a small, contained change.
Tests covering the NullifierAlreadySpent-means-executed classification are in the one_time_key_tests module in the same file.
Withdrawing the flag-gate offer: since the claim path is no longer un-recoverable, I'm withdrawing the offer to gate the claim API behind an experimental flag and defer recovery to a follow-up. That was contingent on shipping a claim path with no recovery story, which is no longer the case. Unless you'd still prefer the gate, I'd like to land this ungated.
Ready for re-review.
There was a problem hiding this comment.
Update at head b6992a5dbc. My previous reply on this thread argued that re-derivation made persistence unnecessary. That argument was wrong in an important way, and your follow-up findings found the hole before I did.
Re-derivation does give idempotency, but the handles it re-derives are not ownership evidence. A spent nullifier plus a MASTER-key-hash hit is satisfied by two on-chain outcomes in which this claim created nothing: the chargeable UnshieldAction fallback, and a competing holder of the same bearer key. Both are now fixed by requiring two bindings together — the id derived from this claim's published nullifiers, plus the on-chain MASTER auth key hash — with a distinct terminal ShieldedInviteAlreadyClaimed outcome when they fail. Details are on the two blocking threads.
Where that leaves your original requirement, honestly: it strengthens the guarantee for multi-spend claims and it narrows single-spend claims. The builder pads a one-action bundle to Orchard's 2-action minimum (num_actions = spends.len().max(2)) and the padding action's randomly generated dummy nullifier participates in the id derivation, so the original transition's id is not re-derivable on a retry. expected_identity_id is None there (operations.rs:1698), no binding can be established, and the claim now returns ShieldedInviteAlreadyClaimed rather than a recovered success.
Single-note is the common invite shape, so that is a real functional cost. And it is exactly the gap your persisted-PendingClaim design closes and re-derivation structurally cannot: a record written before the first broadcast carries the original transition's derived id (padding nullifier included) across the retry, which is the one piece of evidence that is unavailable by re-derivation. I withdraw the "persistence is unnecessary" argument.
I have not built persistence in this pass — the existing PendingRedrive mechanism is keyed by SubwalletId and the claim path has no subwallet, so it needs either a subwallet-less record or a new table plus migration, which is a bigger change than I wanted to fold into a correctness fix you are actively reviewing. I would rather land the false-success fix first and take the persistence work as its own change. Tell me which you prefer:
- Land this now, persistence as a follow-up before the feature ships, single-spend recovery stays terminal in the interim; or
- I build the persisted pending-claim record on this branch now and restore single-spend recovery under the same two-binding check.
Also still open from your original text: you asked for a terminal conflict when the identity is still absent. It currently returns ShieldedBroadcastUnconfirmed (retryable) only when the id is re-derivable and simply has not resolved yet, which I still think is right for DAPI indexing lag; the genuinely unresolvable cases are now terminal. Happy to make the bounded-retry exhaustion terminal too if you disagree.
There was a problem hiding this comment.
Addressed by the equivalent-mechanism clause, with one documented fail-closed residual (feat(platform-wallet): idempotent one-time-key claim recovery + anchored-note DAO queries, 7bc8a845c6 → rebased 76e50501bc, tightened by 197c1348a3 → 6c6c302ced).
The design replaces persisted pending-claim metadata with idempotent reconciliation re-derived entirely from the invitation the invitee holds: a retry's pre-broadcast preflight checks the selected notes' nullifiers on chain; unspent → the transition provably never landed and the rebuilt claim is safe to broadcast (the notes are foreign, no reservation exists to double-consume); spent → recover_executed_one_time_claim treats NullifierAlreadySpent as evidence of execution and recovers the identity under the dual binding (derived id + submitted master key), exactly the "equivalent mechanism must safely rebroadcast and treat NullifierAlreadySpent as evidence" clause. As of 31e6a2f7f2 the two post-build recovery arms also carry the exact built id (finding a00cee018e73), so in-call races recover even for padded single-note bundles.
Residual, documented at the expected_identity_id = None arm: a padded SINGLE-note claim whose outcome is lost between an accepted broadcast and result receipt (process death / cancellation) cannot be re-bound on a fresh retry — the padding nullifier is unreproducible — and terminal-fails closed as ShieldedInviteAlreadyClaimed rather than inventing a success on a bare key-hash match (the exact over-claim this thread's sibling finding prohibited). No double-pay is possible on any path; the created identity remains discoverable by the host's key-hash restore. If the gate holds that residual blocking, the remaining work is a persisted single-note claim record, which conflicts with the no-migration constraint this PR shipped under — flagging for an explicit maintainer call.
| let Some(wid) = read_id32(env, &wallet_id, "walletId") else { | ||
| return ptr::null_mut(); | ||
| }; | ||
| let Some(sk) = read_id32(env, &one_time_sk, "oneTimeSk") else { | ||
| return ptr::null_mut(); |
There was a problem hiding this comment.
🔴 Blocking: Do not marshal the spending key through ordinary unsanitized buffers
oneTimeSk is bearer spend authority for a funded invitation, but the generic read_id32 leaves both its intermediate Vec<u8> and returned [u8; 32] unsanitized. The FFI and wallet layers create further plain-array copies. The generation direction similarly leaves native sk, combined out, and Kotlin's 75-byte source blob containing the spending key until their memory is reused or garbage-collected. This repository already uses read_key32_zeroizing for JNI private keys and explicitly scrubs invitation bearer scalars in identity/network/invitation.rs. Carry this key through Zeroizing buffers where possible and explicitly wipe all transient native and JVM arrays after copying out the one intentionally caller-owned key.
source: ['codex']
There was a problem hiding this comment.
Fixed on this branch across two commits, the second of which closes the residual you flagged after the first partial fix:
58bc6092— the JNI input copy:one_time_skis read throughread_key32_zeroizing(mirroring the existingtransactions::read_key32_zeroizing), so the intermediateVec<u8>and the returned key are scrubbed on drop rather than going through the genericread_id32.deeda55c— the residual you correctly noted was not covered by the above: the claim path was still copying the bearer key into a plain[u8; 32]inrs-platform-wallet-ffiand passing it by value down the wallet layers. That copy is nowZeroizing<[u8; 32]>and moves into the wallet layer;identity_create_from_one_time_keytakesZeroizing<[u8; 32]>at both thePlatformWalletmethod and theoperationsfn, and the key is dereferenced only at the singleSpendingKey::from_bytesconsumption point.
The generation direction is covered too, which was the other half of your finding: the FFI generate path zeroizes the native sk after copying into out_sk_32 and holds the 75-byte out blob in Zeroizing, and PlatformWalletManager.generateOneTimeOrchardKey wipes the JVM byte array in a finally.
Net effect: no plain, unsanitized copy of the bearer spending key survives on either direction of the claim/generate path.
Note there is a separate open CodeRabbit comment (funding.rs#L1026-1029) asking for the same Zeroizing convention on the RNG-free generate/derive helpers — that is a distinct, narrower surface from this thread's claim/generate path and I'm tracking it separately.
Could you resolve this thread?
There was a problem hiding this comment.
Re-verified at head b6992a5dbc: fixed in code.
The claim path carries the bearer key in Zeroizing end to end, and nothing downstream re-copies it into a plain array:
- JNI input:
read_key32_zeroizing(the same helper used for the sibling secret), so the intermediateVec<u8>and the returned key are scrubbed on drop instead of going through the genericread_id32. - FFI -> wallet:
PlatformWallet::identity_create_from_one_time_keytakesone_time_sk: zeroize::Zeroizing<[u8; 32]>(platform_wallet.rs:1134) and the operations fn takes the same type (operations.rs:1584), by move. - Single consumption point:
SpendingKey::from_bytes(*one_time_sk)(operations.rs:1608) — the only place it is dereferenced.
The generation direction is now stronger than when I last replied. generate_one_time_orchard_key (keys.rs:283) returns Zeroizing<[u8; 32]> and wraps its in-loop draw, so a rejected draw is scrubbed too — previously a rejected 32-byte CSPRNG scalar dropped unscrubbed at the end of the iteration. Carrying the accepted key out still wrapped also covers the FFI export's early-return paths, which its explicit zeroize() missed; that call is now redundant and removed.
I also closed the narrower residual I said I was tracking separately (the RNG-free derive helper, CodeRabbit funding.rs#L1026-1029): orchardAddressFromSpendingKey reads its input through read_key32_zeroizing too, so there is no longer a plain-array path on either direction. One honest caveat: orchard_address_from_spending_key takes the key by value, so the caller-frame Zeroizing in platform_wallet_orchard_address_from_spending_key scrubs that frame only — the callee still makes its own transient copy. I documented that at the call site rather than overstating the fix.
Could you resolve this thread, or name the surface you are still seeing unsanitized?
There was a problem hiding this comment.
Resolved in this update — Do not marshal the spending key through ordinary unsanitized buffers 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.
There was a problem hiding this comment.
Verified addressed — fix(shielded-invites): zeroize the one-time bearer spending key end-to-end (deeda55cf0 → rebased 349c38fd5c) plus the CodeRabbit round (b6992a5dbc → 24122194f2). In packages/rs-unified-sdk-jni/src/funding.rs the claim path reads oneTimeSk through read_key32_zeroizing (JVM array scrubbed after copy-out, native buffer Zeroizing), and the generation direction carries the native sk, the combined 75-byte blob, and the address-derivation input in Zeroizing buffers with the JVM-side source arrays wiped. On the rebased head 31e6a2f7f2 the wallet layer additionally contains the derived non-zeroizing Orchard types themselves (ScrubOnDrop on SpendingKey/SpendAuthorizingKey, finding 1ee08ba70627), and orchard_address_from_spending_key now takes the scalar by reference so no plain-array copy is repeated at that boundary.
| pub unsafe extern "C" fn platform_wallet_generate_one_time_orchard_key( | ||
| out_sk_32: *mut u8, | ||
| out_address_43: *mut u8, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_sk_32); | ||
| check_ptr!(out_address_43); | ||
|
|
||
| let (sk, address) = generate_one_time_orchard_key(); | ||
| std::ptr::copy_nonoverlapping(sk.as_ptr(), out_sk_32, 32); | ||
| std::ptr::copy_nonoverlapping(address.as_ptr(), out_address_43, 43); | ||
| PlatformWalletFFIResult::ok() |
There was a problem hiding this comment.
🔴 Blocking: OS RNG failure aborts before the JNI panic guard can run
generate_one_time_orchard_key calls OsRng.fill_bytes, whose rand_core implementation panics when try_fill_bytes reports an entropy-source failure. Because that panic occurs inside this extern "C" export, it cannot unwind to the outer JNI guard; Rust aborts at the C ABI boundary first. An operating-system RNG failure can therefore terminate the Android process instead of producing a Java/native error. Change the generator to use try_fill_bytes and propagate a PlatformWalletFFIResult error, or place an internal panic fence inside this C export.
source: ['codex']
There was a problem hiding this comment.
Resolved in 5ba2794 — OS RNG failure aborts before the JNI panic guard can run 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.
There was a problem hiding this comment.
Fixed on this branch in 58bc6092 — "fix(shielded-invites): FFI RNG panic-safety + zeroize one-time spend key".
generate_one_time_orchard_key no longer calls OsRng::fill_bytes. It uses RngCore::try_fill_bytes and returns a typed PlatformWalletError::ShieldedKeyDerivation on an entropy-source failure, and the #[no_mangle] extern "C" export in shielded_send.rs maps that to ErrorWalletOperation instead of letting a panic abort across the C ABI. An OS RNG failure now surfaces as a normal native error to Java rather than terminating the Android process. Test call sites and callers were updated for the new Result return.
Could you resolve this thread?
There was a problem hiding this comment.
Re-verified at head b6992a5dbc: fixed in code.
generate_one_time_orchard_key no longer calls OsRng::fill_bytes:
// packages/rs-platform-wallet/src/wallet/shielded/keys.rs:298
rng.try_fill_bytes(sk_bytes.as_mut_slice()).map_err(|e| {
PlatformWalletError::ShieldedKeyDerivation(format!(
"OS RNG entropy source failed while generating a one-time Orchard key: {e}"
))
})?;An entropy-source failure is a typed Result error, and the #[no_mangle] extern "C" export in shielded_send.rs matches on it and returns a PlatformWalletFFIResult rather than letting a panic cross the C ABI. So an OS RNG failure surfaces to Java as a normal native error instead of aborting the Android process. The doc comment on the function records why try_fill_bytes is used rather than fill_bytes, so the constraint is not just implied by the call.
Could you resolve this thread, or point at the remaining panic path?
There was a problem hiding this comment.
Verified addressed — fix(shielded-invites): FFI RNG panic-safety + zeroize one-time spend key (58bc609223 → rebased bb83596987). generate_one_time_orchard_key (packages/rs-platform-wallet/src/wallet/shielded/keys.rs) now uses OsRng.try_fill_bytes and maps an entropy-source failure to a typed PlatformWalletError::ShieldedKeyDerivation, which the extern "C" export surfaces as a normal PlatformWalletFFIResult error — no panic can reach the C ABI boundary from this path, so an OS RNG failure can no longer abort the Android process. Every draw (rejected and accepted alike) is held in Zeroizing, and on 31e6a2f7f2 the accepted draw's derived SpendingKey is additionally scrub-contained.
| let prepared = PreparedIncomingViewingKey::new(ivk); | ||
| let stream = sync_shielded_notes_stream(sdk, &prepared, 0, None); | ||
| futures::pin_mut!(stream); | ||
|
|
||
| let mut found: Vec<ShieldedNote> = Vec::new(); | ||
| let mut total: u64 = 0; | ||
| while let Some(batch) = stream.next().await { | ||
| let batch = batch.map_err(|e| PlatformWalletError::ShieldedSyncFailed(e.to_string()))?; | ||
| for dn in batch.decrypted { | ||
| let value = dn.note.value().inner(); | ||
| let nullifier = dn.note.nullifier(fvk).to_bytes(); | ||
| found.push(ShieldedNote { | ||
| position: dn.position, | ||
| cmx: dn.cmx, | ||
| nullifier, | ||
| block_height: batch.block_height, | ||
| is_spent: false, | ||
| value, | ||
| note_data: serialize_note(&dn.note), | ||
| }); | ||
| total = total.saturating_add(value); | ||
| } | ||
| // A one-time key holds exactly its funding — stop once it's covered. | ||
| if total >= stop_at_value { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Unfunded invitation keys force an unbounded full-history scan
Every syntactically valid invitation key starts at position 0 and keeps fetching, proof-verifying, and trial-decrypting chunks until enough value is found or the complete shielded history is exhausted. An attacker can generate valid but unfunded invitation keys, so the value-based early exit provides no bound for hostile input. The supplied birth-height hint is ignored, and cancellation of the Kotlin coroutine cannot interrupt the blocking JNI operation. Add an authenticated or otherwise safe starting position, a maximum resumable scan budget, or another preflight mechanism that prevents one invitation from forcing an unlimited genesis-to-tip scan.
source: ['codex']
There was a problem hiding this comment.
Status at head b6992a5dbc: acknowledged, not addressed in this round.
I am not disputing it. The scan still starts at position 0 and bounds itself only by value coverage, and the birth-height hint is still advisory — operations.rs:1621-1626 logs it explicitly so it is observably dropped rather than silently ignored, which is documentation of the gap, not a fix for it.
This round was scoped to the four blocking recovery/key-handling findings, which are funds-adjacent and were reporting false ownership. A scan budget is a real change to the sync path with its own failure modes (a wrongly chosen bound turns a legitimate deep-funded invite into an unclaimable one), and I would rather not fold it into a correctness fix you are mid-review on.
On the substance, when we do take it: an authenticated starting position is the only one of your three options that is actually sound against hostile input, since a value-based early exit and a caller-supplied birth height are both attacker-controlled. That likely means the invite payload needs to carry a signed or otherwise bound funding position, which is a wire-format change to the invitation rather than a local guard — worth confirming you agree on the direction before I build it.
Happy to take it next, or to split it to its own issue if you would rather not hold this PR on it.
There was a problem hiding this comment.
Resolved in this update — Unfunded invitation keys force an unbounded full-history scan 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.
Addresses reviewer thepastaclaw's blocking findings on PR dashpay#4204. Two of the four blockers are fixed here; the other two are structural and reported back for a decision rather than guessed (crypto/money path). Blocker dashpay#4 (FFI RNG abort) — shielded_send.rs / keys.rs: `generate_one_time_orchard_key` used `OsRng::fill_bytes`, which panics on an OS entropy-source failure. It is called from a `#[no_mangle] extern "C"` export, so that panic aborts the process across the C ABI before any JNI panic guard can convert it. Switch to `RngCore::try_fill_bytes`, return a typed `PlatformWalletError::ShieldedKeyDerivation`, and have the FFI export map it to `ErrorWalletOperation` instead of aborting. Test call sites and callers updated for the new `Result` return. Blocker dashpay#3 (bearer spend key hygiene) — funding.rs: `oneTimeSk` is bearer spend authority but was marshalled via the generic `read_id32`, leaving its intermediate JNI `Vec<u8>` and returned `[u8; 32]` unsanitized. Add a `read_key32_zeroizing` helper (mirroring `transactions::read_key32_zeroizing`): the returned key is `Zeroizing` and the intermediate JNI copy is scrubbed. `sk` derefs to `[u8; 32]`, so the downstream `sk.as_ptr()` FFI call is unchanged. NOT fixed here (reported for decision): Blocker dashpay#1 (affected-state wait): `wait_for_affected_state` does not exist in this head's SDK, and the pool-funded sibling still uses `wait_for_response` on this branch. The reviewer's fix is predicated on rebasing onto the v4.1-dev proof API (31c69cf); it must be done in lockstep for both Type-20 paths. Blocker dashpay#2 (persist claim recovery record): the redrive mechanism is keyed by SubwalletId + activity entry and driven by the per-subwallet sync loop. Claim notes belong to a foreign one-time key tracked in no subwallet, so a correct fix needs a new subwallet-less pending-claim record + reconciliation path, not a reuse of `arm_redrive_record`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed fixes for two of the four blockers in
The other two need more than a minimal edit — flagging for direction:
The unbounded genesis-to-tip scan on an unfunded claim is noted as a separate follow-up (a bound needs a chosen scan-budget so legitimate funded claims don't return empty). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md (1)
674-675: 📐 Maintainability & Code Quality | 🔵 TrivialOptional: silence MD038 while preserving the prefix's trailing space.
markdownlint flags the space inside
`signer_error:key_unavailable: `. Since the trailing space is a meaningful part of the machine prefix, consider noting it in prose (e.g. "ends with a colon+space") instead of relying on the space inside the code span, so the lint passes without dropping the semantics.🤖 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 `@docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md` around lines 674 - 675, Update the documentation around the ProtocolError::Generic prefix to avoid placing a trailing space inside the inline code span, while preserving the prefix semantics by describing that it ends with a colon followed by a space in surrounding prose.Source: Linters/SAST tools
🤖 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 `@docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md`:
- Around line 57-69: Update the documentation text around the
`ProtocolError::Generic` prefix so the literal `signer_error:key_unavailable:`
code span contains no trailing space; describe or place the separator space
outside the backticks while preserving the prefix’s meaning.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- Around line 2673-2742: Update repairIdentityKeyDurably to coordinate the
durable repair with deleteWalletData and recordSigningKeyInvalidated through
withCallbackExclusion before invoking deriver.deriveAndStore(force = true),
preserving the existing breadcrumb, verification, persistence, and pending-state
behavior. Ensure native derivation calls are not executed while the callback
exclusion is held, following the established exclusion usage pattern.
In `@packages/rs-sdk-ffi/src/signer.rs`:
- Around line 179-188: Strip DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX from
signer error messages before surfacing them. Update both conversion paths—the
catch-all using error.to_string() and the ProtocolError implementation
formatting "DPP protocol error: {msg}"—to remove only this prefix while
preserving the existing error text and SigningKeyUnavailable mapping.
---
Nitpick comments:
In `@docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md`:
- Around line 674-675: Update the documentation around the
ProtocolError::Generic prefix to avoid placing a trailing space inside the
inline code span, while preserving the prefix semantics by describing that it
ends with a colon followed by a space in surrounding prose.
🪄 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: 54334ff6-0af1-44db-88ae-6cec9e5e8d8b
📒 Files selected for processing (59)
docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.mddocs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.mddocs/sdk/sdk-parity-manifest.jsonpackages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.ktpackages/kotlin-sdk/PARITY_SUMMARY.mdpackages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/8.jsonpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabaseMigrationTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/WalletStorageOwnershipTest.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/SignerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/PublicKeyDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PublicKeyEntity.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/IdentityKeyPrivateKeyDeriver.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyUnavailableException.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreKeyGenPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerCompletionCodeTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageUpgradeMatrixTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.ktpackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/keys.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/rs-sdk-ffi/src/signer.rspackages/rs-sdk-ffi/src/test_utils.rspackages/rs-sdk-ffi/src/token/claim.rspackages/rs-sdk-ffi/src/token/config_update.rspackages/rs-sdk-ffi/src/token/destroy_frozen_funds.rspackages/rs-sdk-ffi/src/token/emergency_action.rspackages/rs-sdk-ffi/src/token/freeze.rspackages/rs-sdk-ffi/src/token/mint.rspackages/rs-sdk-ffi/src/token/purchase.rspackages/rs-sdk-ffi/src/token/set_price.rspackages/rs-sdk-ffi/src/token/transfer.rspackages/rs-sdk-ffi/src/token/unfreeze.rspackages/rs-unified-sdk-jni/src/funding.rspackages/rs-unified-sdk-jni/src/signer.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift
|
Thanks for splitting the two structural items out rather than guessing on a fund-safety path. Direction from my side:
The RNG/extern-C abort fix looks correct on inspection. Current head |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes the OS-RNG abort and zeroizes the initial JNI claim-side key copy, but three in-scope blocking issues remain: the one-time Type-20 claim uses the wrong proof-wait contract for the current target, ambiguous claims have no durable recovery mechanism, and the bearer spending key still passes through several unsanitized temporary buffers. Unfunded invitation keys also retain an unbounded genesis-to-tip scan path; the valid inherited CodeRabbit observations are outside #4204's shielded-invitation scope.
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— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
5ba2794 to
fb41ffb
Compare
Addresses reviewer thepastaclaw's blocking findings on PR dashpay#4204. Two of the four blockers are fixed here; the other two are structural and reported back for a decision rather than guessed (crypto/money path). Blocker dashpay#4 (FFI RNG abort) — shielded_send.rs / keys.rs: `generate_one_time_orchard_key` used `OsRng::fill_bytes`, which panics on an OS entropy-source failure. It is called from a `#[no_mangle] extern "C"` export, so that panic aborts the process across the C ABI before any JNI panic guard can convert it. Switch to `RngCore::try_fill_bytes`, return a typed `PlatformWalletError::ShieldedKeyDerivation`, and have the FFI export map it to `ErrorWalletOperation` instead of aborting. Test call sites and callers updated for the new `Result` return. Blocker dashpay#3 (bearer spend key hygiene) — funding.rs: `oneTimeSk` is bearer spend authority but was marshalled via the generic `read_id32`, leaving its intermediate JNI `Vec<u8>` and returned `[u8; 32]` unsanitized. Add a `read_key32_zeroizing` helper (mirroring `transactions::read_key32_zeroizing`): the returned key is `Zeroizing` and the intermediate JNI copy is scrubbed. `sk` derefs to `[u8; 32]`, so the downstream `sk.as_ptr()` FFI call is unchanged. NOT fixed here (reported for decision): Blocker dashpay#1 (affected-state wait): `wait_for_affected_state` does not exist in this head's SDK, and the pool-funded sibling still uses `wait_for_response` on this branch. The reviewer's fix is predicated on rebasing onto the v4.1-dev proof API (31c69cf); it must be done in lockstep for both Type-20 paths. Blocker dashpay#2 (persist claim recovery record): the redrive mechanism is keyed by SubwalletId + activity entry and driven by the per-subwallet sync loop. Claim notes belong to a foreign one-time key tracked in no subwallet, so a correct fix needs a new subwallet-less pending-claim record + reconciliation path, not a reuse of `arm_redrive_record`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o-end (dashpay#4204) Reviewer (thepastaclaw) key-hygiene blocker: the one-time Orchard bearer spending key was copied into several plain, unsanitized buffers on both the claim and generate paths. Claim path — carry the key through `Zeroizing` from the FFI copy down through the wallet layers instead of leaking a plain `[u8; 32]` at each hop: - rs-platform-wallet-ffi: the `[u8; 32]` claim-key copy is now `Zeroizing<[u8; 32]>` and moves (not copies) into the wallet layer. - platform-wallet `identity_create_from_one_time_key` (both the PlatformWallet method and the operations fn) now take `Zeroizing<[u8; 32]>`; the key is scrubbed on drop, dereferenced only at the single `SpendingKey::from_bytes` consumption point. Generate path — wipe the transient native and JVM copies after handoff: - rs-platform-wallet-ffi: explicitly zeroize the native `sk` after copying it into the caller's `out_sk_32`. - rs-unified-sdk-jni: hold the JNI `sk` and the combined 75-byte `out` blob in `Zeroizing` buffers so both scrub on drop, including early returns. - kotlin-sdk `generateOneTimeOrchardKey`: wipe the 75-byte JVM blob in a `finally` once the two owned arrays have been sliced out. Validated: cargo build + cargo test -p platform-wallet (493 pass) + cargo fmt; :sdk:compileDebugKotlin succeeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aim (dashpay#4204) Reviewer (thepastaclaw) blocker: after rebasing onto v4.1-dev, the current proof contract (31c69cf) marks IdentityCreateFromShieldedPool proofs as affected-state snapshots — they authenticate the resulting identity and spent nullifiers but cannot bind the complete Orchard request. That commit switched the pool-funded sibling to wait_for_affected_state; the strict wait_for_response now yields ExecutionNotProved for every valid proof. The one-time-key claim path (identity_create_from_one_time_key) was still on the strict wait_for_response, so every valid claim proof would enter the ambiguous fallback and risk being reported unconfirmed despite executing. Switch it to wait_for_affected_state, matching the pool-funded sibling (the sibling already adopted it via the v4.1-dev rebase). Validated on the rebased v4.1.0-rc.1 base: cargo build (platform-wallet + rs-unified-sdk-jni) + cargo test -p platform-wallet (493 pass) + cargo fmt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@thepastaclaw update on the remaining items (branch rebased onto current v4.1-dev = 4.1.0-rc.1,
Note: this was a force-push (rebase). Backup of the pre-rebase branch is preserved locally. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commits correctly switch the one-time Type-20 claim to the affected-state proof contract and eliminate most transient bearer-key copies. Two in-scope blockers remain: ambiguous claims still have no durable recovery record, and the address-derivation plus Orchard conversion paths retain unsanitized spending-key copies. Unfunded invitations can also trigger a complete genesis-to-tip scan with no work or cancellation budget.
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— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
|
On-device verification (testnet, Samsung S22, build 11.10.16): the shielded-invite inviter path was exercised end-to-end — a shielded invitation was created successfully via the one-time Orchard key flow ( |
…red-note DAO queries (dashpay#4204) Shielded-invite claim recovery: an IdentityCreateFromOneTimeKey claim that has already executed on chain (its note nullifier is spent / broadcast or wait returns NullifierAlreadySpent) is now reconciled to success instead of stranding the retry with a hard error. Recovery re-derives everything from the invite the invitee already holds — no persisted record: - master_auth_public_key_hash(): the invitee's re-derivable MASTER auth key hash, the unique Platform-indexed handle the identity is looked up by (discover_inner's unique-hash probe). - any_nullifier_spent_on_chain(): proof-verified ShieldedNullifierStatuses preflight; if the selected notes are already spent, recover by key hash before rebuilding/rebroadcasting. - NullifierAlreadySpent arms on both broadcast and wait paths route to recover_executed_one_time_claim(), which recovers by key hash, then by the deterministically-derived identity id (fetch_identity_with_retries), and otherwise surfaces ShieldedBroadcastUnconfirmed carrying the derived id. Preserves the newer dashpay#4204 key-hygiene base already in this branch: the one-time spending key is still carried in Zeroizing<[u8;32]> and wait_for_affected_state is unchanged (Type-20 proof is affected-state). ShieldedDao: adds minUnspentAnchoredBlockHeight() and getUnspentAnchoredNotesByWallet() — read-only queries over existing shielded_notes columns (no schema change) backing the shielded-username anchor-confirmation gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt (1)
136-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
runCatchingalso swallows cancellation.
WalletStorage.migrateToPolicyAliasexplicitly rethrowsCancellationExceptionfor exactly this reason; the best-effort hook here silently absorbs it. Consider rethrowing cancellation while keeping other failures best-effort.🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt` at line 136, Update the best-effort hook invocation in KeystoreSigner to rethrow CancellationException while continuing to suppress other hook failures. Preserve the existing optional onSigningKeyInvalidated invocation and storageKey argument, but replace the unconditional runCatching behavior with cancellation-aware handling.packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.kt (1)
94-102: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the probing check off the main dispatcher.
probeIdentityKeyRecoverabilityperforms real RSA/AES Keystore decrypts (per the new KDoc inWalletStorage), unlike the cheapisPrivateKeyDecryptableit replaced.produceState's producer runs on the composition's dispatcher (main), so a wallet with several identity keys does N blocking Keystore operations on the UI thread. The repair path below already wraps inDispatchers.IO; the probe should too.♻️ Proposed change
- hasPrivateKey = runCatching { - container.walletStorage.probeIdentityKeyRecoverability(pubkeyHex) - }.getOrDefault(false), + hasPrivateKey = runCatching { + withContext(Dispatchers.IO) { + container.walletStorage.probeIdentityKeyRecoverability(pubkeyHex) + } + }.getOrDefault(false),🤖 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/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.kt` around lines 94 - 102, Update the hasPrivateKey probe in the produceState flow to execute probeIdentityKeyRecoverability on Dispatchers.IO, preserving the existing runCatching fallback to false. Keep the repair path and other capability checks unchanged.packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.kt (1)
58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest assumes the write landed on the auth-gated alias.
On a device/emulator without a secure lock screen,
storePrivateKeydegrades toKEYS_ALIAS_DEVICE_BOUND(resolveIdentityKeysWriteAlias), so deleting onlyKEYS_ALIAS_AUTH_GATEDleaves the blob decryptable and the finalassertFalsefails.WalletStorageOwnershipTestdocuments that CI enrolls a PIN, but deleting both policy aliases here would make the test independent of lock-screen state.🤖 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/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.kt` around lines 58 - 61, Update the cleanup in the instrumented test around keyStore.deleteEntry to delete both KeystoreManager.KEYS_ALIAS_AUTH_GATED and KeystoreManager.KEYS_ALIAS_DEVICE_BOUND, so the test removes whichever alias storePrivateKey selected through resolveIdentityKeysWriteAlias.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/IdentityKeyPrivateKeyDeriver.kt (1)
101-104: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueZeroize the derived halves when the arity check fails.
If
check(pair.size == 2)trips,scalaris still null, so the outerfinallyscrubs nothing and any derived bytes inpairstay in the heap. Cheap to close given this PR's zeroization goals.♻️ Proposed change
- check(pair.size == 2) { "keypair derive returned ${pair.size} elements" } + if (pair.size != 2) { + pair.forEach { it.fill(0) } + error("keypair derive returned ${pair.size} elements") + }🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/IdentityKeyPrivateKeyDeriver.kt` around lines 101 - 104, Update the keypair derivation flow around the arity check in IdentityKeyPrivateKeyDeriver so every derived byte array in pair is zeroized before the failed check propagates when pair.size is not 2. Preserve the existing scalar assignment and cleanup behavior for valid pairs, and ensure cleanup also occurs when the check throws.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt (1)
2677-2684: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDurable identity-key identifier writes loop per row without a transaction. Both sites fetch every
public_keysrow for a pubkey andupdatethem one by one, so a mid-loop failure leaves rows disagreeing about whether the private half is recorded — the very signal the restart reconstruction trusts.
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt#L2677-L2684: wrap the defaultpersistDurableIdentifierloop indatabase.withTransaction { … }.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt#L2769-L2773: wrap therecordSigningKeyInvalidatednull-out loop indatabase.withTransaction { … }so the invalidation signal lands all-or-nothing.🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt` around lines 2677 - 2684, Wrap the row-update loop in the default persistDurableIdentifier at packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt#L2677-L2684 with database.withTransaction { … }. Also wrap the null-out loop in recordSigningKeyInvalidated at packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt#L2769-L2773 with database.withTransaction { … } so each multi-row update is atomic.packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift (1)
849-856: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly this failure branch classifies the error; other signer failures stay
generic.
signPlatformAddressOnDemandandffiSignfailures still reportgenericeven when the underlyingKeychainSigner.Erroris.publicKeyNotFound/.privateKeyMissingFromKeychain, so hosts fall back to message sniffing for those paths. Routing everyKeychainSigner.ErrorthroughkeychainSignerCompletionErrorCode(for:)would make the discriminator uniform.🤖 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/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift` around lines 849 - 856, Update the error handling in signPlatformAddressOnDemand and ffiSign so every underlying KeychainSigner.Error is passed through keychainSignerCompletionErrorCode(for:) instead of always using the generic code. Preserve the existing localized error messages while ensuring .publicKeyNotFound and .privateKeyMissingFromKeychain receive the structured signingKeyUnavailable classification consistently.
🤖 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 `@docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md`:
- Around line 57-68: Remove the trailing separator space from the inline code
span containing signer_error:key_unavailable: in
docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md lines 57-68, placing the space
outside while preserving the literal prefix. Apply the same correction to the
ProtocolError::Generic description in
docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md lines 671-681.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- Around line 2821-2823: Update the probe handling in
reconstructPendingIdentityKeysFromPersistence so CancellationException
propagates instead of being swallowed by runCatching. Preserve the
false/unusable fallback for ordinary probe failures, while ensuring cancellation
exits the loop and prevents subsequent Room queries or pending-state
publication.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- Around line 885-901: Rename the unused exception bindings to _ in
probeOpensBlob for UserNotAuthenticatedException and GeneralSecurityException,
the rung-2 GeneralSecurityException catch, tryFormerRsaRecovery, and
opensUnderNonGatedDeviceBoundSibling. Apply these changes at
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:885-901,
580-594, and 634-643, and
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:470-479;
preserve the existing routing behavior.
In `@packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- Around line 133-152: Update the Swift managedIdentity wrapper’s result mapping
to preserve the distinction from platform_wallet_get_managed_identity: map
NotFound to identityNotFound only for a live wallet without the identity, and
map ErrorInvalidHandle to invalidHandle for stale or missing handles. Update the
related managedIdentity API documentation to describe both outcomes.
In `@packages/rs-unified-sdk-jni/src/funding.rs`:
- Around line 1026-1029: Update
packages/rs-unified-sdk-jni/src/funding.rs:1026-1029 to use read_key32_zeroizing
for spendingKey in the guard closure. Update
packages/rs-platform-wallet/src/wallet/shielded/keys.rs:284-302 so
generate_one_time_orchard_key returns Zeroizing<[u8; 32]> and each candidate
draw is Zeroizing, including rejected rerolls. Update
packages/rs-platform-wallet-ffi/src/shielded_send.rs:1628-1651 so
platform_wallet_orchard_address_from_spending_key stores the sk copy in
Zeroizing, preserving the existing generate export wipe behavior.
In `@packages/rs-unified-sdk-jni/src/signer.rs`:
- Around line 233-239: Update the Kotlin completeSign declaration in
SignerNative to include the error-message String parameter matching the Rust JNI
symbol, then update every KeystoreSigner call site to pass four arguments in the
correct order.
---
Nitpick comments:
In
`@packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.kt`:
- Around line 94-102: Update the hasPrivateKey probe in the produceState flow to
execute probeIdentityKeyRecoverability on Dispatchers.IO, preserving the
existing runCatching fallback to false. Keep the repair path and other
capability checks unchanged.
In
`@packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.kt`:
- Around line 58-61: Update the cleanup in the instrumented test around
keyStore.deleteEntry to delete both KeystoreManager.KEYS_ALIAS_AUTH_GATED and
KeystoreManager.KEYS_ALIAS_DEVICE_BOUND, so the test removes whichever alias
storePrivateKey selected through resolveIdentityKeysWriteAlias.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- Around line 2677-2684: Wrap the row-update loop in the default
persistDurableIdentifier at
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt#L2677-L2684
with database.withTransaction { … }. Also wrap the null-out loop in
recordSigningKeyInvalidated at
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt#L2769-L2773
with database.withTransaction { … } so each multi-row update is atomic.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/IdentityKeyPrivateKeyDeriver.kt`:
- Around line 101-104: Update the keypair derivation flow around the arity check
in IdentityKeyPrivateKeyDeriver so every derived byte array in pair is zeroized
before the failed check propagates when pair.size is not 2. Preserve the
existing scalar assignment and cleanup behavior for valid pairs, and ensure
cleanup also occurs when the check throws.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt`:
- Line 136: Update the best-effort hook invocation in KeystoreSigner to rethrow
CancellationException while continuing to suppress other hook failures. Preserve
the existing optional onSigningKeyInvalidated invocation and storageKey
argument, but replace the unconditional runCatching behavior with
cancellation-aware handling.
In `@packages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swift`:
- Around line 849-856: Update the error handling in signPlatformAddressOnDemand
and ffiSign so every underlying KeychainSigner.Error is passed through
keychainSignerCompletionErrorCode(for:) instead of always using the generic
code. Preserve the existing localized error messages while ensuring
.publicKeyNotFound and .privateKeyMissingFromKeychain receive the structured
signingKeyUnavailable classification consistently.
🪄 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: 6e6ab0ad-cf96-4932-909a-f2a3d2ef0061
📒 Files selected for processing (60)
docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.mddocs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.mddocs/sdk/sdk-parity-manifest.jsonpackages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.ktpackages/kotlin-sdk/PARITY_SUMMARY.mdpackages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/8.jsonpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabaseMigrationTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/WalletStorageOwnershipTest.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/SignerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/PublicKeyDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/ShieldedDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PublicKeyEntity.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/IdentityKeyPrivateKeyDeriver.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyUnavailableException.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreKeyGenPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerCompletionCodeTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageUpgradeMatrixTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.ktpackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/keys.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/rs-sdk-ffi/src/signer.rspackages/rs-sdk-ffi/src/test_utils.rspackages/rs-sdk-ffi/src/token/claim.rspackages/rs-sdk-ffi/src/token/config_update.rspackages/rs-sdk-ffi/src/token/destroy_frozen_funds.rspackages/rs-sdk-ffi/src/token/emergency_action.rspackages/rs-sdk-ffi/src/token/freeze.rspackages/rs-sdk-ffi/src/token/mint.rspackages/rs-sdk-ffi/src/token/purchase.rspackages/rs-sdk-ffi/src/token/set_price.rspackages/rs-sdk-ffi/src/token/transfer.rspackages/rs-sdk-ffi/src/token/unfreeze.rspackages/rs-unified-sdk-jni/src/funding.rspackages/rs-unified-sdk-jni/src/signer.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rs-sdk-ffi/src/token/unfreeze.rs
- packages/rs-sdk-ffi/src/token/emergency_action.rs
- packages/rs-platform-wallet/src/wallet/shielded/mod.rs
- packages/kotlin-sdk/PARITY_SUMMARY.md
- packages/rs-sdk-ffi/src/token/claim.rs
- packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.kt
|
All four blocking threads now have author replies at the current head Not re-arguing anything here — just requesting a revalidation pass so the gate re-evaluates those four threads against this head. @thepastaclaw please revalidate at @coderabbitai review |
…t.NotFound PlatformWalletFFIResultCode::NotFound (98) — the blanket Option→result miss (unknown wallet id, unmanaged identity, …) — previously collapsed into the top-level DashSdkError.NotFound shared with rs-sdk-ffi codes 7/8. It now maps to the new DashSdkError.PlatformWallet.NotFound, keeping 98 inside the wallet-error family in exact parity with Swift's PlatformWalletError.notFound, so callers can match a wallet-level absence without sniffing Generic codes. BREAKING: Kotlin hosts catching DashSdkError.NotFound from platform-wallet operations (e.g. rescanSpvFilters with an unknown wallet id) now receive DashSdkError.PlatformWallet.NotFound; rs-sdk-ffi codes 7/8 still map to the top-level NotFound. Dashpay's managed-identity local reads are unaffected — translateManagedIdentityNotFoundToZero intercepts the RAW code (dashpay#4051) before this mapping ever runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…as split The inherited dashpay#4172 instrumented tests hardcode the legacy KEYS_ALIAS and the single-alias API; after the alias split the default-policy storage writes under KEYS_ALIAS_AUTH_GATED, so simulating keypair replacement by deleting the legacy alias no longer invalidates anything (the CI failure shumkov diagnosed on storeIfAbsentRederivesWhenTheKeysAliasKeypairWasReplaced). - WalletStorageOwnershipTest: the replaced-keypair, rejected-blob, and no-regeneration probes delete/inspect KEYS_ALIAS_AUTH_GATED; the fingerprint-capture test uses encryptForIdentityKeysAlias (and pins the captured producing alias); the stale-invalidation-cleanup test uses the alias-parameterized deleteIdentityKeysAliasIfCurrentGeneration. - KeystoreSignerInstrumentedTest: canSignWith rejection simulates replacement of the policy alias. The five DashPayUnlockAndSyncTest / WalletManagerRoundTripTest failures from PR dashpay#4183's CI run were NOT test or SDK defects: the branch predated dashpay#4172's workflow hardening (screen_off_timeout / stayon / dismiss-keyguard + the hard deviceLocked=0 guard), so the emulator re-locked mid-run and every MASTER_ALIAS operation threw InvalidKeyException. Rebuilding on the current base inherits the fixed workflow; those tests are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity, and Room v8 shift - sdk-parity-manifest.json (+ regenerated PARITY_SUMMARY.md): new capability entries for the KeySecurityPolicy alias split (Swift not-applicable — iOS Keychain has no per-alias auth-parameter analog), the now-CONVERGENT platform-wallet code-98 typed mapping (was a divergence), the Kotlin-only durable pending-repair surface (Swift gap recorded), and the structured SigningKeyUnavailable discriminator (both hosts, unit-verification pointers on each). shared_symbols gains dash_sdk_sign_async_completion. - KOTLIN_SWIFT_SHARED_PARITY_SPEC.md: rationale for each divergence / convergence, the deprecated MESSAGE_MARKER fallback with its next-minor-release removal horizon, and the migration ladder update (v8 = derivation breadcrumbs; invitations shift to v9). - KOTLIN_MIGRATION_LEFTOVERS.md: marker-fallback removal and the on-device KeyPermanentlyInvalidatedException residual (unit-tier only via the fake seam, same as dashpay#4172). - Swift: testPlatformWalletNotFoundFFIResultMapping pins the convergent 98 mapping the manifest references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…closed auth window as recoverable probeIdentityKeyRecoverability treated UserNotAuthenticatedException as "recoverable" unconditionally. UNAE is thrown at cipher.init — before the ciphertext is examined — so after a Keystore loss + regeneration the fresh auth-gated key (whose window is closed in the steady state; it is only open ~30 s after an auth) reported a blob it can never open as healthy: the key-health sheet offered no repair while pendingIdentityKeys simultaneously listed the same key (dashpay#4060 round-2 finding 1). Prompt-free fix: UNAE counts as recoverable ONLY while the blob's stored write-time fingerprint still matches the recorded alias's CURRENT key (non-generating read). A mismatch disproves ownership → not recoverable → repair is offered; a fingerprint-matched blob behind a closed window stays recoverable (genuinely just needs auth). The legacy former-RSA rung keeps the documented residual (no fingerprint surface for the retained legacy key). New matrix rows pin both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ped signer code on the first attempt A KPIE escaping the sign path (retrieveKeyWithAuth — the biometric-gate retry only handles UserNotAuthenticatedException) previously fell into the signAsync catch-all and completed with SIGNER_ERROR_CODE_GENERIC: the host saw an opaque wallet-operation failure on the FIRST post-invalidation sign and only got the typed SigningKeyUnavailable on subsequent attempts (once the fingerprint gate reported the blob non-current). dashpay#4060 round-2 finding 2. - signWithStoredKey catches KPIE explicitly and completes with SIGNER_ERROR_CODE_KEY_UNAVAILABLE (marker text + invalidation reason kept in the message), so the typed code rides the wire immediately. - The signAsync catch-all classifies via the new pure completionErrorCodeFor(t) (KPIE → key-unavailable, everything else generic) as belt-and-braces for KPIE thrown outside the retrieval call. - KeystoreSignerCompletionCodeTest pins the classification (the full signer cannot be constructed on the JVM — its constructor creates a native handle — so the classification is a pure companion function). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… for legacy-alias keys A KeyPermanentlyInvalidatedException on a LEGACY-alias-backed key was invisible everywhere: the legacy Keystore aliases are read-only (no deletion boundary), so hasLegacyKeysKey() stays true forever — canSignWith keeps offering the key, the cheap capability check keeps reporting the blob usable, the restart reconstruction never seeds a repair slot, and every sign fails opaquely (dashpay#4060 round-2 finding 3). Fix without making the cheap check expensive (it is untouched): the sign path's KPIE classification now invokes an onSigningKeyInvalidated hook, wired by PlatformWalletManager to PlatformWalletPersistenceHandler.recordSigningKeyInvalidated, which nulls privateKeyKeychainIdentifier on the key's Room rows — the same durable signal the restart reconstruction reads — and re-runs the reconstruction so pendingIdentityKeys seeds after the FIRST failed sign (reason: "signing key permanently invalidated"), making repair reachable outside the health sheet. Best-effort: bookkeeping failure never eats the typed completion. Harmless for policy-alias keys (their generation-checked deletion already flips the fingerprint gate; this just accelerates the in-process seed). Pre-v8 rows without breadcrumbs still get the identifier null-out and seed once the next persist round back-fills their breadcrumbs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g-2 TOCTOU window Round-2 nits (dashpay#4060): - The MESSAGE_MARKER fallback's stated rationale — "old native library + new Kotlin partial builds" — was not viable: the sign-completion JNI arity change (3→4 args) makes that pairing a type-confused native call on every completion, so mixed artifacts are unsupported outright. The fallback's real purpose is the dashpay#4191 merge-order transition (whose marker-based classification predates the typed code 31) plus defense in depth for conversion paths that lose the machine prefix. KDoc/comments in DashSdkError, KeystoreSigner, DashSdkErrorTest, and the parity/leftovers docs now say so; removal horizon unchanged (next minor release). - retrievePrivateKey rung 2 gains a comment naming the accepted fingerprint-read→decrypt TOCTOU window (pre-existing dashpay#4172 parity): a concurrent rotation fails as a wrong-key crypto error into the recovery ladder — never stale plaintext. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…former-RSA rung
Round-3 verification finding: tryFormerRsaRecovery collapsed every
GeneralSecurityException to null ('not this key'), which swallowed a
KPIE from an invalidated pre-alias-split key — the signer's classifier
never saw it, so the typed completion and the durable repair seeding
(finding-3 hook) never fired for legacy blobs. KPIE now propagates like
UserNotAuthenticatedException, with a matrix row pinning the rethrow and
the post-regen recovery path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s and verify the pubkey before persisting Blocker 1 (shumkov): the repair path derived from caller-supplied indices (the example app passed the DPP key id as keyIndex), and its only check was probeIdentityKeyRecoverability — which proves the stored blob DECRYPTS, not that the derived key is the RIGHT one. A wrong index derives a different valid scalar that round-trips fine, clears pending state, and persists an unusable key. Fix: - IdentityKeyPrivateKeyDeriver: on the force (repair) path derive the KEYPAIR and verify the derived public key equals publicKeyData BEFORE any store; a mismatch throws IdentityKeyDerivationMismatchException without persisting. - PlatformWalletPersistenceHandler.repairIdentityKeyDurably: new orchestration that reads the derivation slot from the PERSISTED public_keys breadcrumbs (derivationIdentityIndex/derivationKeyIndex), never a caller index; a row without breadcrumbs fails the repair without clearing pending. Hoisted here (not the JVM-unconstructable manager) so it is unit-testable and shares the authoritative pendingIdentityKeys state. - PlatformWalletManager.repairIdentityKey: drop the identityIndex/keyIndex params and delegate; supply only the wallet-scoped probe. - WalletKeyHealthSheet: stop passing report.identityIndex / key.keyId. Tests (PlatformWalletPersistenceHandlerTest): correct breadcrumbs derive, verify, clear pending, and record the durable identifier (deriver called with the persisted 3/5, not a caller index); mismatched breadcrumbs are rejected (mismatch exception), nothing persisted, blob never probed, pending intact; a row without breadcrumbs fails with SigningKeyUnavailable and never derives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Blocker 3 (shumkov): in the repair path the Room privateKeyKeychainIdentifier update was wrapped in runCatching (swallowed on failure) while the pending state was cleared regardless. A failed durable write then resurrected the repair after restart (the reconstruction reads the persisted rows) while the live session believed it was done — session and restart disagreed. Fix: the durable write in repairIdentityKeyDurably now fails CLOSED — it runs through an injectable persistDurableIdentifier seam (default: the production public_keys write) and its exception propagates; markIdentityKeyRepaired only runs after the write commits. A failed durable write leaves pending intact and the repair retryable, so the session and a restart agree. Test: a repair whose derive + verify succeed but whose durable write throws propagates the failure and leaves the pending entry in place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…horitative signals Blocker 2 (shumkov): isNoSecureLockScreenKeyGenFailure classified any KeyStoreException whose message contained "generate_key" as a lock-screen rejection. But "generate_key" names the failing operation, not its cause — a transient KeyMint generation failure on a device WITH a lock screen carries it too, so a transient failure would silently and PERMANENTLY downgrade an AUTH_GATED key to DEVICE_BOUND instead of retrying. Fix: drop the bare "generate_key" substring match. Classification now rests only on the two authoritative signals — explicit lock-screen text, or the lock-screen rejection numeric code (internal Keystore 4 / KeyMint 10309, which the real android.security.KeyStoreException exposes via getNumericErrorCode()). Tests: the on-device shape now classifies via the numeric code (not the incidental generate_key text); two new negatives — a bare generate_key failure, and a transient generate_key failure under a key-gen ProviderException with a non-lock-screen numeric code — must NOT classify, so AUTH_GATED is retried, not downgraded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…olicy-alias migration Finding 4 (shumkov): migrateToPolicyAlias wrapped its encrypt + DataStore edit in runCatching, which swallows kotlin CancellationException along with genuine rewrite failures. A coroutine cancelled during the migration's suspend points was silently absorbed instead of unwinding — a structured-concurrency violation. Fix: replace runCatching with an explicit try/catch that rethrows CancellationException; only genuine rewrite failures stay best-effort (the recovered value is still returned; migration retries on the next read). Test: a cancellation landing at the migration encrypt (via the fake keystore's onNextPolicyEncrypt hook) now propagates out of retrievePrivateKey rather than being swallowed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fallback Finding 5 (shumkov): the DEVICE_BOUND documentation promised hardware-backed storage while the implementation permits a software AndroidKeyStore fallback (generation prefers StrongBox, falls back to the TEE, and finally to a software-backed AndroidKeyStore key on devices with no secure element — generateWithLockScreenDegradation never fails generation on a missing one). Align the docs: "device-bound" means non-exportable AndroidKeyStore, not a hardware-storage guarantee; backing is hardware-isolated only where the device provides it. AUTH_GATED shares the same backing characteristics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rom b2 line Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m_one_time_key), reconciled to base identity API Ports the L2-invitation CLAIM side from the b2 line (8008dc78b8): Verbatim grafts (byte-for-byte from b2, deps all present in base): - operations.rs: free fn identity_create_from_one_time_key (note-scan + Halo2 proof) and its supporting note-scan helper scan_notes_for_foreign_key (sync.rs), plus the one_time_key_tests module. - platform_wallet.rs: PlatformWalletManager::identity_create_from_one_time_key. - shielded_send.rs (FFI): platform_wallet_manager_shielded_identity_create_from_one_time_key (base's FFI-layer decode_identity_pubkeys/IdentityPubkeyFFI matches b2). Reconciled to base's API (NOT byte-for-byte): - funding.rs (JNI): decode_pubkeys_blob + hand-built IdentityPubkeyFFI literal (b2) -> decode_registration_pubkeys_blob + row.to_ffi() (base), plus base's tagged-payload return with ErrorShieldedBroadcastUnconfirmed handling. - Kotlin: IdentityKeyPreview.encodeForRegistration + withContext + raw return (b2) -> List<IdentityPubkey> via IdentityPubkeyCodec.encode + teardownGate.op + decodeShieldedCreatePayload (base), mirroring the tested inviter side. Pubkey-decode semantics preserved: identical key_id / pubkey bytes / order / count; role/read_only/contract-bounds source shifts from Rust-derived (b2) to caller-stamped blob (base) — base's authoritative pipeline-wide convention, already adopted by the tested inviter side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses reviewer thepastaclaw's blocking findings on PR dashpay#4204. Two of the four blockers are fixed here; the other two are structural and reported back for a decision rather than guessed (crypto/money path). Blocker dashpay#4 (FFI RNG abort) — shielded_send.rs / keys.rs: `generate_one_time_orchard_key` used `OsRng::fill_bytes`, which panics on an OS entropy-source failure. It is called from a `#[no_mangle] extern "C"` export, so that panic aborts the process across the C ABI before any JNI panic guard can convert it. Switch to `RngCore::try_fill_bytes`, return a typed `PlatformWalletError::ShieldedKeyDerivation`, and have the FFI export map it to `ErrorWalletOperation` instead of aborting. Test call sites and callers updated for the new `Result` return. Blocker dashpay#3 (bearer spend key hygiene) — funding.rs: `oneTimeSk` is bearer spend authority but was marshalled via the generic `read_id32`, leaving its intermediate JNI `Vec<u8>` and returned `[u8; 32]` unsanitized. Add a `read_key32_zeroizing` helper (mirroring `transactions::read_key32_zeroizing`): the returned key is `Zeroizing` and the intermediate JNI copy is scrubbed. `sk` derefs to `[u8; 32]`, so the downstream `sk.as_ptr()` FFI call is unchanged. NOT fixed here (reported for decision): Blocker dashpay#1 (affected-state wait): `wait_for_affected_state` does not exist in this head's SDK, and the pool-funded sibling still uses `wait_for_response` on this branch. The reviewer's fix is predicated on rebasing onto the v4.1-dev proof API (31c69cf); it must be done in lockstep for both Type-20 paths. Blocker dashpay#2 (persist claim recovery record): the redrive mechanism is keyed by SubwalletId + activity entry and driven by the per-subwallet sync loop. Claim notes belong to a foreign one-time key tracked in no subwallet, so a correct fix needs a new subwallet-less pending-claim record + reconciliation path, not a reuse of `arm_redrive_record`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o-end (dashpay#4204) Reviewer (thepastaclaw) key-hygiene blocker: the one-time Orchard bearer spending key was copied into several plain, unsanitized buffers on both the claim and generate paths. Claim path — carry the key through `Zeroizing` from the FFI copy down through the wallet layers instead of leaking a plain `[u8; 32]` at each hop: - rs-platform-wallet-ffi: the `[u8; 32]` claim-key copy is now `Zeroizing<[u8; 32]>` and moves (not copies) into the wallet layer. - platform-wallet `identity_create_from_one_time_key` (both the PlatformWallet method and the operations fn) now take `Zeroizing<[u8; 32]>`; the key is scrubbed on drop, dereferenced only at the single `SpendingKey::from_bytes` consumption point. Generate path — wipe the transient native and JVM copies after handoff: - rs-platform-wallet-ffi: explicitly zeroize the native `sk` after copying it into the caller's `out_sk_32`. - rs-unified-sdk-jni: hold the JNI `sk` and the combined 75-byte `out` blob in `Zeroizing` buffers so both scrub on drop, including early returns. - kotlin-sdk `generateOneTimeOrchardKey`: wipe the 75-byte JVM blob in a `finally` once the two owned arrays have been sliced out. Validated: cargo build + cargo test -p platform-wallet (493 pass) + cargo fmt; :sdk:compileDebugKotlin succeeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aim (dashpay#4204) Reviewer (thepastaclaw) blocker: after rebasing onto v4.1-dev, the current proof contract (31c69cf) marks IdentityCreateFromShieldedPool proofs as affected-state snapshots — they authenticate the resulting identity and spent nullifiers but cannot bind the complete Orchard request. That commit switched the pool-funded sibling to wait_for_affected_state; the strict wait_for_response now yields ExecutionNotProved for every valid proof. The one-time-key claim path (identity_create_from_one_time_key) was still on the strict wait_for_response, so every valid claim proof would enter the ambiguous fallback and risk being reported unconfirmed despite executing. Switch it to wait_for_affected_state, matching the pool-funded sibling (the sibling already adopted it via the v4.1-dev rebase). Validated on the rebased v4.1.0-rc.1 base: cargo build (platform-wallet + rs-unified-sdk-jni) + cargo test -p platform-wallet (493 pass) + cargo fmt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…red-note DAO queries (dashpay#4204) Shielded-invite claim recovery: an IdentityCreateFromOneTimeKey claim that has already executed on chain (its note nullifier is spent / broadcast or wait returns NullifierAlreadySpent) is now reconciled to success instead of stranding the retry with a hard error. Recovery re-derives everything from the invite the invitee already holds — no persisted record: - master_auth_public_key_hash(): the invitee's re-derivable MASTER auth key hash, the unique Platform-indexed handle the identity is looked up by (discover_inner's unique-hash probe). - any_nullifier_spent_on_chain(): proof-verified ShieldedNullifierStatuses preflight; if the selected notes are already spent, recover by key hash before rebuilding/rebroadcasting. - NullifierAlreadySpent arms on both broadcast and wait paths route to recover_executed_one_time_claim(), which recovers by key hash, then by the deterministically-derived identity id (fetch_identity_with_retries), and otherwise surfaces ShieldedBroadcastUnconfirmed carrying the derived id. Preserves the newer dashpay#4204 key-hygiene base already in this branch: the one-time spending key is still carried in Zeroizing<[u8;32]> and wait_for_affected_state is unchanged (Type-20 proof is affected-state). ShieldedDao: adds minUnspentAnchoredBlockHeight() and getUnspentAnchoredNotesByWallet() — read-only queries over existing shielded_notes columns (no schema change) backing the shielded-username anchor-confirmation gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
"same residual dashpay#4172 accepted" read ambiguously; say the residual was accepted in dashpay#4172. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ted the identity (dashpay#4204) A spent invitation nullifier proves only that *something* consumed the note. It does not prove that this claim's Type-20 transition created an identity, and recovery was treating "nullifier spent + an identity is findable under the submitted MASTER auth key hash" as a successful claim. Two real on-chain outcomes are reported as success by that rule: 1. The chargeable `UnshieldAction` fallback. When a submitted unique public-key hash is already registered, Type-20 finalizes the shielded spend as an `UnshieldTransitionAction` with `chargeable_failure: true` and creates NO identity, crediting the invitation value to the creation-failure address minus a penalty (rs-drive-abci .../identity_create_from_shielded_pool/state/ v0/mod.rs:62-128). A retry then saw the nullifier spent, fetched the *pre-existing* identity that owns the colliding key hash, and returned it as the claim's result. 2. A competing holder of the same bearer one-time key. The identity id is `double_sha256` over the SORTED published action nullifiers (`identity_id_from_nullifiers`) — derived from nullifiers only, never from identity keys. With two or more real spends no randomized padding action is added, so another holder of the same invite derives the SAME id under THEIR keys. The victim's retry fetched that foreign identity by the shared id and `platform_wallet.rs` registered it at the victim's identity index. Recovery is now gated on two independent bindings, both required (`recovered_identity_matches_claim`): - id binding — the identity's id equals the id derived from THIS claim's published nullifiers. Consensus re-derives and rejects a mismatch, so only a transition publishing exactly this nullifier set can carry that id. This is what rejects case 1. - key binding — the identity's ON-CHAIN key set carries this claim's submitted MASTER authentication key hash. This is what rejects case 2. The key binding is checked against the keys the fetch actually returned, so an identity fetched without public keys now fails closed instead of being topped up with locally-submitted keys that were never proven to exist on chain. Where the bindings cannot be established, recovery returns the new terminal `ShieldedInviteAlreadyClaimed` (FFI `ErrorShieldedInviteAlreadyClaimed` = 32) rather than a success or the retryable unconfirmed code. That includes the single-spend case: the builder pads a one-action bundle to Orchard's 2-action minimum (`num_actions = spends.len().max(2)`) and the padding action's RANDOM dummy nullifier participates in the id derivation, so the original id is not re-derivable on a retry and no candidate can be bound to the claim. Also: - The spent-nullifier preflight now hands off to the reconciler directly instead of falling through to rebuild+rebroadcast a transition that can only earn a `NullifierAlreadySpent` rejection (saves a Halo 2 proof build). - The generic wait-failure fallback applies the key binding too, but only when the bundle was NOT padded: a padded build's id embeds a locally generated dummy nullifier no other party can reproduce, so there the id alone is proof. Regression tests in `one_time_claim_evidence_tests` pin both attack scenarios plus the keyless-fetch, unre-derivable-id, absent-key-hash, wrong-purpose and different-nullifier-set cases. 7 of the 8 fail against the pre-fix rule (only the positive-acceptance case still passes), verified by reverting the predicate to the old accept-anything behavior.
…ene, message hygiene (dashpay#4204) Addresses the six open CodeRabbit threads. - `PlatformWalletPersistenceHandler.reconstructPendingIdentityKeysFromPersistence` wrapped a SUSPEND decryptability probe in `runCatching`, which catches `Throwable` and therefore swallowed `CancellationException`: a cancelled caller had the row misclassified as unusable and a spurious pending-repair entry published. Now rethrows cancellation and keeps `false` only for genuine probe failures, matching the convention this PR already established in `WalletStorage` ("NEVER swallow structured-concurrency cancellation"). CodeRabbit missed that `PlatformWalletManager` re-swallows one frame up in a bare `runCatching`; that site is fixed too, since fixing only the inner one would not have delivered the stated behavior. - Rename five unused `catch (e: ...)` bindings to `_` (detekt SwallowedException) in `WalletStorage` and `KeystoreManager`. Adjacent catches that `throw e` are deliberately untouched. - Carry the one-time bearer spending key through `Zeroizing` on the remaining generate/derive helpers: the JNI `orchardAddressFromSpendingKey` input now uses `read_key32_zeroizing` (matching `oneTimeSk`), and `generate_one_time_orchard_key` wraps its in-loop draw so REJECTED draws are scrubbed too and the accepted key travels out still wrapped — which also covers the FFI export's early-return paths that its explicit `zeroize()` missed (that call is now redundant and removed). Note `orchard_address_from_spending_key` takes the key BY VALUE, so the caller-frame `Zeroizing` in `platform_wallet_orchard_address_from_spending_key` scrubs that frame only; this is documented at the call site rather than overstated as eliminating the plaintext copy. - Strip the signer's internal machine prefix (`DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX`) from rendered messages on both conversion paths in `platform-wallet-ffi`. Both read the prefix to pick the typed code BEFORE stripping, so classification is unaffected, and the host-side fallback matcher keys on the human tail (`DashSdkError.MESSAGE_MARKER`), not the prefix. - Fix the markdownlint MD038 trailing-space-inside-code-span in `KOTLIN_MIGRATION_LEFTOVERS.md` and `KOTLIN_SWIFT_SHARED_PARITY_SPEC.md`. Also applies `cargo fmt` to the five pre-existing formatting violations in files this PR already owns, so `cargo fmt --check` passes clean.
…-> 37 and mirror it (dashpay#4204) 32 is allocated to `ErrorTransactionBuild` (dashpay#4247, also carried by dashpay#4256) in ERROR_CODE_REGISTRY.md (dashpay#4261). This variant took 32 without a registry row, so the two collide as a hard `E0081: discriminant value 32 assigned more than once` the moment both land — reproduced on a real integration merge, not hypothetical. 27-36 are all claimed (27 ErrorShutdownIncomplete via the merged dashpay#4268; 29 dashpay#4184; 31 dashpay#4183; 32/33 37 is the allocation frontier. The code was also unmirrored on BOTH hosts, which is the more dangerous half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its identity; Kotlin fell through to Generic(32), and in any tree carrying "shielded invite already claimed" as "reservation wallet mismatch". That matters on the claim-recovery path specifically — the error is raised from four sites in shielded/operations.rs, three inside the recovery function. Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal, inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift reservation comment the registry asked the next toucher to drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolves the two blocking review findings and the live suggestions on dashpay#4261. Blocking — advance dashpay#3968's reissue frontier to 38. Code 37 is allocated to dashpay#4204, so the branch-specific guidance in the non-conforming section and in the 27/28 detail section could still have sent dashpay#3968 to 37 and recreated the collision with ErrorShieldedInviteAlreadyClaimed. Both references now say 38+, and both state that the reserved 28 and 30 are not available either. Blocking — 28 and 30 were labelled RESERVED in the table and the frontier paragraph but "free" in the code-30 section and the collision history. Rule 1 lets a contributor claim a gap this file marks free, so the two labels gave two different allocation frontiers. Everything now says reserved-not-free and cites rule 1 explicitly. Open-PR inventory: verified 2026-08-03 against each PR's file list and the error.rs at its head. Adds dashpay#3417, dashpay#3549, dashpay#3992 and dashpay#4243 (all touch the crate without claiming an integer; dashpay#4243 maps new wallet errors onto the existing ErrorInvalidParameter). Removes dashpay#4240 and dashpay#4251, whose heads touch no file under this crate; removes dashpay#4258, merged on 2026-08-03; records dashpay#4264 as closed with its work carried by dashpay#4243. Also in this pass: - dashpay#4204's Swift mirror is recorded as PARTIALLY fixed. At d78b940 the raw case and its init(ffi:) arm exist, but PlatformWalletError has no typed case and its exhaustive init(result:) has no arm, so the Swift package does not compile as written. Rule 5's Swift clause is not yet satisfied there. - The preamble no longer claims duplicate discriminants are always silent. Two different names on one integer is an E0081 after a merge (that is how the code-32 collision surfaced); the silent case is a meaning moving to a new integer, or an un-updated host mirror. - Code 13 ErrorArithmeticOverflow does have an in-tree producer (shielded_send.rs); the row said it had none. - dashpay#3968 is no longer described as simply un-rebased: its head does contain the 2026-08-01 base, it is behind the current base, and a rebase alone fixes nothing because git sees no conflict — the branch must edit its own enum. - Provenance re-read on 2026-08-03 from GitHub rather than carried forward, now as a table with a note per PR. dashpay#4183 and dashpay#4184 were rebased onto 5d68612 today and keep 31 and 29 respectively; their stale in-enum reservation comments were corrected in the same rebase, so the code-30 section now lists dashpay#4204 as the only branch still carrying one. - markdownlint: MD018 (18 occurrences) and MD004 (mixed bullet styles) are both clean; MD013 is down to long table rows only.
…pplied-fallback verdict, terminal code at the FFI, Swift mirror, Orchard secret scrubbing (dashpay#4204) Five blocking findings from the 2026-08-03 gate run, fixed on the rebased head: * a00cee018e73 — the two POST-BUILD `NullifierAlreadySpent` recovery arms (broadcast + result wait) now pass `Some(identity_id)` — the id THIS transition committed — instead of the pre-build `expected_identity_id`, which is deliberately None for a padded single-note bundle. The SDK's broadcast retries internally, so an accepted-then-lost-ack first request legitimately yields NullifierAlreadySpent on the wire retry; with None the reconciler declared our own successfully created identity permanently lost. `expected_identity_id` remains for the pre-build preflight, where the randomized padding id is genuinely unavailable. * 8d020115b274 — the wait-path consensus-verdict arm no longer converts an APPLIED chargeable fallback into ShieldedBroadcastFailed (code 16, documented as definitive non-execution and retryable): a duplicate unique-key hash makes Type 20 apply the chargeable UnshieldAction — nullifiers consumed, fallback address credited minus the penalty — and its PaidConsensusError reaches the wait as a populated cause. The arm now verifies the selected nullifiers first; consumed notes route to the reconciler for the terminal claimed/fallback verdict (recovered success when this claim created the identity, terminal ShieldedInviteAlreadyClaimed for the fallback / a competing holder). * 7be05fde0d09 — the live claim FFI export routes ShieldedInviteAlreadyClaimed through the blanket From<PlatformWalletError> conversion (code 37) before the catch-all, which was flattening it to the generic ErrorWalletOperation (6) and made the terminal consumed-invitation discriminator unreachable from the one API that produces it. * 00b4b4d41758 — the Swift mirror is complete and compiles: public `PlatformWalletError.shieldedInviteAlreadyClaimed(String)` case, errorDescription coverage, and the `.errorShieldedInviteAlreadyClaimed` arm in `init(result:)` (the exhaustive switch previously rejected the new enum case). Verified with swiftc -parse. * 1ee08ba70627 — Orchard spend-authority representations are no longer left unscrubbed: a `ScrubOnDrop` guard (volatile per-byte overwrite + fence on every exit path, gated on `needs_drop` absence with a tripwire test) contains the non-zeroizing `SpendingKey` / `SpendAuthorizingKey` in the one-time-key claim (sk dropped right after derivation, ask right after the bundle build — neither survives the network awaits), in `OrchardKeySet::from_seed`, in the one-time keygen acceptance loop, and in `orchard_address_from_spending_key`, which now also takes the scalar BY REFERENCE so callers' Zeroizing buffers are not repeated as plain arrays at the boundary. platform-wallet 672/672, platform-wallet-ffi 228/228, JNI + FFI cargo check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d78b940 to
31e6a2f
Compare
… sites Three more live review findings, all verified against current PR heads. Rule 5 named a `PlatformWalletResultCode.init(result:)` that does not exist — `init(result:)` belongs to the downstream `PlatformWalletError`. As written, a contributor could add the Swift raw case and the typed error handling and still omit `PlatformWalletResultCode.init(ffi:)`, which is where the generated C constant is recognised; that switch has a `default:` yielding `.errorUnknown`, so the omission compiles and silently loses the code's identity before typed handling sees it. Rule 5 now enumerates all three Swift sites and says how each one fails: (1) the raw case, (2) the `init(ffi:)` arm — silent, and (3) `PlatformWalletError` + its `init(result:)` arm — a hard compile error, since that switch is exhaustive with no `default:`. That third failure is exactly what dashpay#4204 is sitting on at `d78b940a03`. dashpay#4196 is no longer blocked. Its head moved to `12492e8c54`, the restack onto dashpay#4185 is done, dashpay#4185's head `8813e98533` is an ancestor, the trio reads 34/35/36, and the PR is MERGEABLE against v4.2-dev. Verified the numeric references it owns were carried too: the `StaleReservationToken` KDoc and `fromPlatformWalletNative` mapping in `DashSdkError.kt` both read 34, and the V2 broadcast KDoc in `ManagedCoreWallet.kt` reads 34 with the rest symbolic. `PlatformWalletError::StaleReservation` refers to the code symbolically and never carried a number. The section is now a resolution rather than an open item; the account of why the restack was hard is kept, since that was the substance of the delay. The code-30 sweep was overstated. "No PR anywhere defines a code 30" is false for the surveyed heads — dashpay#4185 and dashpay#4256 both did; that was the allocation, not a competing claim. It now reads "no PR unrelated to dashpay#4185 defines a code 30", which is the claim that actually supports the conclusion. The list of branches carrying the stale consent-code reservation is corrected to dashpay#4183, dashpay#4204 and dashpay#4256's pre-renumber rationale (dashpay#4247 was never one of them). Provenance and the proposed table pick up dashpay#4196's new head. markdownlint MD018/MD004 remain at 0.
2 participants
What
Adds the one-time Orchard key shielded-invite API to the Kotlin SDK. Client-side only — no L2 protocol / consensus changes (nothing under
rs-dpp,rs-drive,dapi).generateOneTimeOrchardKey()/orchardAddressFromSpendingKey()+ theOneTimeOrchardKeytype: generate a one-time Orchard spending key and the raw address the inviter funds a note to.shieldedIdentityCreateFromOneTimeKey(...): a claimer, handed the one-time spending key, spends the funded note to create/top-up a shielded identity.Backing Rust:
rs-platform-wallet(shielded/keys.rs,operations.rs,sync.rs,platform_wallet.rs),rs-platform-wallet-ffi(shielded_send.rs),rs-unified-sdk-jni(funding.rs).The claim side consumes
decode_registration_pubkeys_blob+IdentityPubkeyCodec, both introduced by #4183. This branch is stacked on #4183, so until #4183 merges the diff below also contains #4183's changes. It will retarget to a clean diff once #4183 lands. Net-new files to review here:packages/rs-platform-wallet/src/wallet/shielded/keys.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-unified-sdk-jni/src/funding.rspackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/rs-platform-wallet/Cargo.toml(optionalranddep for theshieldedfeature)Security note — identity key roles
The claim path decodes registration pubkeys through base's
decode_registration_pubkeys_blob/row.to_ffi(), so key roles (purpose / security level) are caller-stamped (base's uniform registration convention) rather than derived in Rust. The role mapping is unchanged: key_id0 → AUTH/MASTER,1 → AUTH/CRITICAL,2 → AUTH/HIGH,3 → TRANSFER/CRITICAL. The reconciled JNI return also preserves the identity id on the unconfirmed-broadcast path.Validation
cargo test -p platform-wallet --features shielded— 624 lib tests + 3 new claim tests pass; inviter key-roundtrip tests pass.cargo build -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --features shielded— clean../gradlew :sdk:assemble— BUILD SUCCESSFUL (compileDebug/ReleaseKotlin).Consumer follow-up (tracked separately, not in this PR)
The Android wallet's
SdkShieldedUsernameCreation/SdkShieldedInviteCreationstill call the claim API withList<IdentityKeyPreview>; they need adapting toList<IdentityPubkey>(stamping the roles above) before the full wallet builds against this SDK.🤖 Generated with Claude Code
Summary by CodeRabbit