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 11 commits into
Conversation
|
Warning Review limit reached
Next review available in: 58 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 (22)
📝 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
|
⛔ Blockers found — Sonnet deferred (commit 4efecd5) |
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 #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 #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 |
|
Renumbered |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 217-224: Update the stale error-code allocation documentation
preceding the code-37 comment to match the current ERROR_CODE_REGISTRY.md
assignments: identify code 27 as allocated to ErrorShutdownIncomplete and code
28 as vacated but RESERVED, rather than reserving both for deferred-payment
errors. Keep the surrounding error-code declarations unchanged.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 1834-1854: Update the ownership check in the
IdentityCreateFromOneTimeKey flow around recovered_identity_matches_claim to
validate the fetched identity against the builder-derived identity_id as well as
expected_identity_id. Ensure single-spend builds with expected_identity_id unset
still recognize an identity whose id matches identity_id, and only return
ShieldedInviteAlreadyClaimed when neither derived id matches and the master-key
check fails.
🪄 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: 08091b16-4257-4350-a92f-6e7b1aa747cc
📒 Files selected for processing (17)
docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.mddocs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.mdpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.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/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/src/error.rspackages/rs-platform-wallet/src/wallet/shielded/keys.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-sdk-ffi/src/signer.rspackages/rs-unified-sdk-jni/src/funding.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/rs-platform-wallet-ffi/src/dashpay.rs
- docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md
- docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
- packages/rs-sdk-ffi/src/signer.rs
- packages/rs-platform-wallet-ffi/src/shielded_send.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt
| /// Code 37 — the next free integer per the allocation frontier in | ||
| /// `ERROR_CODE_REGISTRY.md` (dashpay/platform#4261). This variant briefly | ||
| /// held 32, which is allocated to `ErrorTransactionBuild` | ||
| /// (dashpay/platform#4247, also carried by #4256); the two collided as an | ||
| /// `E0081` the moment both were merged. 27-36 are all claimed (27 | ||
| /// `ErrorShutdownIncomplete`, merged via #4268; 29 #4184; 31 #4183; 32/33 | ||
| /// #4247/#4256; 34-36 the #4185 deferred-token trio), and 28/30 are vacated | ||
| /// but RESERVED, so 37 is the only correct allocation. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the error-code allocation comments.
The note at Lines 217-224 says code 27 is allocated to ErrorShutdownIncomplete and code 28 is reserved. The preceding note at Lines 176-184 still says codes 27-28 are reserved for deferred-payment errors. Replace the stale note with the current allocation from ERROR_CODE_REGISTRY.md. Contradictory registry comments can cause a future FFI code collision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-ffi/src/error.rs` around lines 217 - 224, Update
the stale error-code allocation documentation preceding the code-37 comment to
match the current ERROR_CODE_REGISTRY.md assignments: identify code 27 as
allocated to ErrorShutdownIncomplete and code 28 as vacated but RESERVED, rather
than reserving both for deferred-payment errors. Keep the surrounding error-code
declarations unchanged.
| if expected_identity_id.is_some() | ||
| && !recovered_identity_matches_claim( | ||
| &identity, | ||
| expected_identity_id, | ||
| master_key_hash, | ||
| ) | ||
| { | ||
| warn!( | ||
| derived_id = %identity_id, | ||
| "IdentityCreateFromOneTimeKey: an identity exists at this claim's \ | ||
| derived id but does not carry the submitted master auth key; another \ | ||
| holder of the same one-time key claimed the invitation first" | ||
| ); | ||
| return Err(PlatformWalletError::ShieldedInviteAlreadyClaimed { | ||
| reason: format!( | ||
| "identity {identity_id} was created from this invitation's notes \ | ||
| but does not carry the submitted master authentication key, so it \ | ||
| belongs to another holder of the one-time key: {wait_err}" | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the builder's identity-id derivation with identity_id_from_nullifiers.
fd -t f 'identity_create_from_shielded_pool.rs' packages/rs-dpp | xargs rg -n -C 6 'num_actions|identity_id|derive_identity_id_from_actions|nullifier'
rg -n -C 6 'fn identity_id_from_nullifiers' packages/rs-dppRepository: dashpay/platform
Length of output: 14672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the reviewed file and the relevant identity-create bridge code.
echo "== candidate files =="
fd -t f '^operations\.rs$|^identity_create_from_shielded_pool_transition\.mod\.rs$|^identity_create_from_shielded_pool_transition\.rs$' packages/rs-platform-wallet packages/rs-dpp
echo
echo "== reviewed section =="
sed -n '1780,1870p' packages/rs-platform-wallet/src/wallet/shielded/operations.rs | cat -n
echo
echo "== shielded identity-create transition helpers =="
sed -n '1,110p' packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs | cat -n
echo
echo "== derive_identity_id implementation nearby =="
sed -n '110,180p' packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs | cat -n
echo
echo "== identity_id extraction in platform-wallet bridge =="
rg -n -C 8 'expected_identity_id|identity_id_from_nullifiers|derived_identity_id|selected_nullifiers|build\.identity_id|ShieldedInviteAlreadyClaimed|recovered_identity_matches_claim' packages/rs-platform-wallet/src/wallet/shielded/operations.rsRepository: dashpay/platform
Length of output: 42523
Check builder-derived identity-id binding before reporting claim ownership.
The identity here is fetched by identity_id from build.identity_id, but the fallback compares only the master key against expected_identity_id. In single-spend builds expected_identity_id is None, so recovered_identity_matches_claim() returns false even when the same padded bundle-derived identity exists under identity_id; this reports a valid claim as ShieldedInviteAlreadyClaimed.
Fetch with the claim-recoverable id instead, or compare identity.id() against both identity_id and expected_identity_id before classifying the outcome. For unpadded bundles, identity_id and expected_identity_id both derive from the same sorted selected_nullifiers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs` around lines
1834 - 1854, Update the ownership check in the IdentityCreateFromOneTimeKey flow
around recovered_identity_matches_claim to validate the fetched identity against
the builder-derived identity_id as well as expected_identity_id. Ensure
single-spend builds with expected_identity_id unset still recognize an identity
whose id matches identity_id, and only return ShieldedInviteAlreadyClaimed when
neither derived id matches and the master-key check fails.
…keystore-qa4 Brings the ErrorShieldedInviteAlreadyClaimed 32 -> 37 renumber + host mirrors, clearing the E0081 this integration hit against qa3's ErrorReservationWalletMismatch = 32. Conflicts (4) all unions — kept BOTH sides, since this subset does not carry dashpay#4185/dashpay#4256: the deferred-token trio stays at qa3's 27/28/32 and 37 is added alongside. Swift's inherited comment was rewritten to describe THIS tree's numbering rather than the 34-36 layout those branches introduce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried forward: all six prior findings remain in scope and unresolved at the exact head, including five blockers and the unbounded-scan suggestion. Genuinely new in the latest delta: adding Swift result code 37 without completing the exhaustive public error mapping makes the Swift SDK source fail compilation. The PR therefore still requires changes before merge.
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)
🔴 6 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1606-1615: Do not marshal the spending key through ordinary unsanitized buffers
The JNI and outer C buffers use `Zeroizing`, but dereferencing `one_time_sk` creates an ordinary `[u8; 32]` and Orchard's `Copy`, non-zeroizing `SpendingKey`; deriving `ask` creates a non-zeroizing `SpendAuthorizingKey`. Orchard 0.14.1 defines these types without `Zeroize` or a scrubbing `Drop`, so their complete spend-authority representations are not scrubbed after use and remain in the long-lived async claim frame through network work. The address helper repeats the by-value `SpendingKey` representation at `wallet/shielded/keys.rs:243-251`. Preserve zeroizing ownership across avoidable raw-array boundaries and provide explicit scrubbing or equivalent containment for every unavoidable Orchard secret representation after its final use.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1762-1766: Use the built identity ID when reconciling single-note broadcast races
The transition has already been built here, so `identity_id` is the exact ID committed by this transition, including the randomized padding nullifier used for a single-note bundle. Passing the pre-build `expected_identity_id`, which is intentionally `None` for that bundle shape, makes `recover_executed_one_time_claim` immediately return terminal `ShieldedInviteAlreadyClaimed`. The SDK broadcast method retries requests, so an initial accepted request followed by a lost acknowledgement can legitimately produce `NullifierAlreadySpent` on a retry; this path then reports the wallet's successfully created identity as permanently lost. Pass `Some(identity_id)` here and in the equivalent wait-time arm at lines 1801-1805, reserving `expected_identity_id` for pre-build or restart reconciliation where the randomized ID is genuinely unavailable.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1809-1810: Do not classify an applied chargeable fallback as retryable broadcast failure
A duplicate unique-key hash makes Type 20 apply an `UnshieldAction` with `chargeable_failure: true`: Drive consumes the invitation nullifiers, credits the fallback address minus the penalty, and records a `PaidConsensusError`. That consensus error reaches the SDK wait path as a `StateTransitionBroadcastError` with a populated cause, so this arm converts an already-applied outcome into `ShieldedBroadcastFailed`. Kotlin documents code 16 as definitive non-execution and marks it retryable, which is false after the fallback has consumed the invitation. Detect the chargeable fallback cause, or verify the selected nullifiers before applying the generic failure classification, and return a terminal claimed/fallback outcome instead.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:998-1001: Preserve the terminal invite-claimed code at the claim FFI boundary
The blanket `From<PlatformWalletError>` conversion now maps `ShieldedInviteAlreadyClaimed` to native code 37, and the latest delta adds corresponding Kotlin and Swift result-code mirrors. This live claim export bypasses that conversion: its catch-all maps the variant to `ErrorWalletOperation` code 6. JNI consequently emits the generic wallet-operation error, so callers cannot observe the terminal consumed-invitation discriminator and the new code-37 mapping remains unreachable from the API that produces it. Route this variant through the blanket conversion before the catch-all.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:96: Complete the Swift error mirror for result code 37
This delta adds `.errorShieldedInviteAlreadyClaimed`, and the C-enum conversion can now produce it, but the exhaustive `switch result.code` at lines 318-355 has no matching case. Swift rejects that switch as non-exhaustive, so the SDK source does not compile once this enum case is present. The public `PlatformWalletError` enum also lacks a `shieldedInviteAlreadyClaimed` case, leaving nowhere to preserve the terminal semantics. Add the public error case, include it in `errorDescription`, and map `.errorShieldedInviteAlreadyClaimed` in `init(result:)`.
| // Derive the Orchard key material from the one-time spending key. `from_bytes` | ||
| // returns a `CtOption`; an invalid scalar means the caller handed us a | ||
| // non-key, which is a hard input error. | ||
| let sk: SpendingKey = Option::from(SpendingKey::from_bytes(*one_time_sk)).ok_or_else(|| { | ||
| PlatformWalletError::ShieldedKeyDerivation( | ||
| "one-time spending key is not a valid Orchard SpendingKey".to_string(), | ||
| ) | ||
| })?; | ||
| let fvk = FullViewingKey::from(&sk); | ||
| let ask = SpendAuthorizingKey::from(&sk); |
There was a problem hiding this comment.
🔴 Blocking: Do not marshal the spending key through ordinary unsanitized buffers
The JNI and outer C buffers use Zeroizing, but dereferencing one_time_sk creates an ordinary [u8; 32] and Orchard's Copy, non-zeroizing SpendingKey; deriving ask creates a non-zeroizing SpendAuthorizingKey. Orchard 0.14.1 defines these types without Zeroize or a scrubbing Drop, so their complete spend-authority representations are not scrubbed after use and remain in the long-lived async claim frame through network work. The address helper repeats the by-value SpendingKey representation at wallet/shielded/keys.rs:243-251. Preserve zeroizing ownership across avoidable raw-array boundaries and provide explicit scrubbing or equivalent containment for every unavoidable Orchard secret representation after its final use.
source: ['codex']
| return recover_executed_one_time_claim( | ||
| sdk, | ||
| master_key_hash, | ||
| expected_identity_id, | ||
| &format!("broadcast returned NullifierAlreadySpent: {e}"), |
There was a problem hiding this comment.
🔴 Blocking: Use the built identity ID when reconciling single-note broadcast races
The transition has already been built here, so identity_id is the exact ID committed by this transition, including the randomized padding nullifier used for a single-note bundle. Passing the pre-build expected_identity_id, which is intentionally None for that bundle shape, makes recover_executed_one_time_claim immediately return terminal ShieldedInviteAlreadyClaimed. The SDK broadcast method retries requests, so an initial accepted request followed by a lost acknowledgement can legitimately produce NullifierAlreadySpent on a retry; this path then reports the wallet's successfully created identity as permanently lost. Pass Some(identity_id) here and in the equivalent wait-time arm at lines 1801-1805, reserving expected_identity_id for pre-build or restart reconciliation where the randomized ID is genuinely unavailable.
source: ['codex']
There was a problem hiding this comment.
Fixed in 31e6a2f: both post-build NullifierAlreadySpent recovery arms now pass Some(<built identity id>) to the reconciler; expected_identity_id is reserved for the pre-build path only.
| Err(dash_sdk::Error::StateTransitionBroadcastError(e)) if e.cause.is_some() => { | ||
| return Err(PlatformWalletError::ShieldedBroadcastFailed(e.to_string())); |
There was a problem hiding this comment.
🔴 Blocking: Do not classify an applied chargeable fallback as retryable broadcast failure
A duplicate unique-key hash makes Type 20 apply an UnshieldAction with chargeable_failure: true: Drive consumes the invitation nullifiers, credits the fallback address minus the penalty, and records a PaidConsensusError. That consensus error reaches the SDK wait path as a StateTransitionBroadcastError with a populated cause, so this arm converts an already-applied outcome into ShieldedBroadcastFailed. Kotlin documents code 16 as definitive non-execution and marks it retryable, which is false after the fallback has consumed the invitation. Detect the chargeable fallback cause, or verify the selected nullifiers before applying the generic failure classification, and return a terminal claimed/fallback outcome instead.
source: ['codex']
There was a problem hiding this comment.
Fixed in 31e6a2f: the wait-path consensus-verdict arm now verifies the selected nullifiers first; consumed notes route to the reconciler (terminal claimed/fallback, or recovered success) instead of the retryable ShieldedBroadcastFailed.
| Err(e) => PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorWalletOperation, | ||
| format!("shielded identity-create-from-one-time-key failed: {e}"), | ||
| ), |
There was a problem hiding this comment.
🔴 Blocking: Preserve the terminal invite-claimed code at the claim FFI boundary
The blanket From<PlatformWalletError> conversion now maps ShieldedInviteAlreadyClaimed to native code 37, and the latest delta adds corresponding Kotlin and Swift result-code mirrors. This live claim export bypasses that conversion: its catch-all maps the variant to ErrorWalletOperation code 6. JNI consequently emits the generic wallet-operation error, so callers cannot observe the terminal consumed-invitation discriminator and the new code-37 mapping remains unreachable from the API that produces it. Route this variant through the blanket conversion before the catch-all.
| Err(e) => PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorWalletOperation, | |
| format!("shielded identity-create-from-one-time-key failed: {e}"), | |
| ), | |
| Err(e @ PlatformWalletError::ShieldedInviteAlreadyClaimed { .. }) => e.into(), | |
| Err(e) => PlatformWalletFFIResult::err( | |
| PlatformWalletFFIResultCode::ErrorWalletOperation, | |
| format!("shielded identity-create-from-one-time-key failed: {e}"), | |
| ), |
source: ['codex']
There was a problem hiding this comment.
Fixed in 31e6a2f: the claim FFI export routes ShieldedInviteAlreadyClaimed through the blanket conversion (code 37) ahead of the catch-all — see shielded_send.rs ~line 1000.
| /// (dashpay/platform#4261): 32 belongs to `errorTransactionBuild` | ||
| /// (#4247/#4256), 34-36 to the #4185 deferred-token trio, and 28/30 are | ||
| /// vacated-but-reserved. | ||
| case errorShieldedInviteAlreadyClaimed = 37 |
There was a problem hiding this comment.
🔴 Blocking: Complete the Swift error mirror for result code 37
This delta adds .errorShieldedInviteAlreadyClaimed, and the C-enum conversion can now produce it, but the exhaustive switch result.code at lines 318-355 has no matching case. Swift rejects that switch as non-exhaustive, so the SDK source does not compile once this enum case is present. The public PlatformWalletError enum also lacks a shieldedInviteAlreadyClaimed case, leaving nowhere to preserve the terminal semantics. Add the public error case, include it in errorDescription, and map .errorShieldedInviteAlreadyClaimed in init(result:).
source: ['codex']
There was a problem hiding this comment.
Fixed in 31e6a2f: added the PlatformWalletError case, errorDescription, and init(result:) arm (PlatformWalletResult.swift lines 103/307/332/379); the switch is exhaustive again and swiftc -parse passes.
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.
d78b940 to
31e6a2f
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 #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>
…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>
…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>
… 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.
…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 #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>
…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>
31e6a2f to
4efecd5
Compare
Re-verified the whole document against the CURRENT `origin/v4.2-dev` (`97904ed2fc`), not the `f53e5eef0a` the review comment cited and not the `5d68612a45` this file was last compiled against. `ErrorSigningKeyUnavailable = 31` is merged ABI. It landed in `189a3abb1c` (dashpay#4183, stacked on dashpay#4191) together with its Rust C-facing discriminant and complete Swift and Kotlin mirrors — the raw case, the `init(ffi:)` arm, the typed `PlatformWalletError` case with its `init(result:)` arm, and Kotlin's `31 -> PlatformWallet.SigningKeyUnavailable`. Leaving it under "Proposed allocations", whose preamble explicitly permits renumbering, contradicted rule 3. Moved to the merged table. Four PRs merged into `v4.2-dev` on 2026-08-04 and this file still treated all four as open: dashpay#4191 (`0e2282b586`), dashpay#4183 (`189a3abb1c`), dashpay#4277 (`6704a41a85`), dashpay#4251 (`7afc8a8ff3`). Only dashpay#4183 claimed an integer; the other three claimed none, and dashpay#4277 is now recorded as the merged precedent for "touches error.rs but allocates nothing" (it routes TxMetadataPayloadTooLarge onto the existing ErrorInvalidParameter). Dependent sections updated so nothing implies 31 may still move: the frontier breakdown (unchanged at 38), the proposed table, the inherited-code table (31 is trunk now, not an inheritable claim), the collision-history bullet list, the no-new-code open-PR inventory, the 31-vs-33 note (collapsing 31 is no longer available; only dashpay#4256's 33 is still open), and the survey provenance plus the PR-heads-of-record table. Also refreshed, because a re-dated provenance section must not carry claims that are now false: dashpay#4204's and dashpay#4256's Swift mirror gaps are both closed, and the stale ErrorAssetLockCrossDomainConsentRequired comments are gone from every branch that carried them. Every discriminant, mirror, PR state, and SHA above was read from git or the GitHub API on 2026-08-04. The four merge SHAs were confirmed ancestors of `97904ed2fc`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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