feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim) - #4313
feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim)#4313bfoss765 wants to merge 23 commits into
Conversation
…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 #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 #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 #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 #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 (#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 (#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 (#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 #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 (#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 (#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 (#4204) 32 is allocated to `ErrorTransactionBuild` (#4247, also carried by #4256) in ERROR_CODE_REGISTRY.md (#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 #4268; 29 #4184; 31 #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 (#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>
…ency #4277 (merged into v4.2-dev) promoted `rand = "0.8"` from a dev-dependency to a mandatory entry in `[dependencies]`. This PR had added its own `rand = { version = "0.8", optional = true }` to the same table for the one-time Orchard key CSPRNG, and because the two lines sit in different parts of the table git merged both without a textual conflict — producing a manifest that cargo rejects outright: error: duplicate key --> packages/rs-platform-wallet/Cargo.toml:75:1 error: failed to load manifest for workspace member `.../packages/rs-platform-wallet` `cargo metadata` fails before any build starts, which is why the Kotlin SDK CI job died in the "Building rs-unified-sdk-jni" step rather than in the tests. `rand` is now unconditionally available, so this PR does not need to declare it at all: remove the optional duplicate and drop the now-invalid `dep:rand` from the `shielded` feature list (cargo rejects `dep:` on a non-optional dependency). `shielded::keys::generate_one_time_orchard_key` keeps using `OsRng` from the same crate at the same major version — no behaviour change. Verified with `cargo metadata`, `cargo check -p platform-wallet` (default and `--features shielded`) and `cargo check -p platform-wallet-ffi --features shielded`. Cargo.lock is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Orchard-secret `ScrubOnDrop(...)` wrapping added in the review-gate round left `keys.rs` with a `cargo fmt --check --all` drift (the `SpendingKey::from_zip32_seed(..).map_err(..)` argument was not re-wrapped to rustfmt's default layout). Purely cosmetic re-wrap; no behavior change. Restores a clean `cargo fmt --check --all` so the Formatting & Linting CI step passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ase merge) (#4204) The Kotlin SDK native-build CI (which compiles `refs/pull/4204/merge`, i.e. this PR merged into v4.2-dev) failed with: error[E0432]: unresolved import `rand` (shielded/keys.rs) Root cause: v4.2-dev advanced to remove `rand` from `[dependencies]` (it is now dev-only) and to drop `log` from `[dependencies]` entirely. Commit 806d198 had removed this PR's own `rand` declaration on the (now-false) premise that base provides `rand` unconditionally. The head still built because its merge-base copy of those lines was present, but the 3-way merge into the advanced base deletes them, leaving the PR's added lib code with no `rand`/`log`: * `shielded::keys::generate_one_time_orchard_key` uses `rand::OsRng` (shielded) * `identity::network::encrypted_document` uses `rand::OsRng` and the `log` facade (`log::debug!`/`log::warn!`) unconditionally Fix: declare `rand = "0.8"` and `log = "0.4"` as this PR's own `[dependencies]` inside the PR-authored comment block (a head-only region base does not have, so it survives the merge), and align the "Standard dependencies" `rand`/`log` lines to base's edited form so those regions merge without conflict or duplicate keys. Manifest-only; no code or feature-gate change. Verified by reproducing the exact CI merge locally (merge head into v4.2-dev tip 5bbd7c9) and building platform-wallet + platform-wallet-ffi with `shielded`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds one-time Orchard key generation, shielded invitation identity creation, resumable foreign-note scans, durable claim recovery, secure key handling, cross-language bindings, terminal error code 43 mappings, and signer-error message cleanup. ChangesShielded invitation identity creation
Cross-platform error handling
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🔵 Low · up to The new claim API accepts invalid wallet IDs without the same local length validation as neighboring methods, so malformed input may fail later in native code rather than immediately. The PR is otherwise mergeable with explicit owner awareness or a follow-up fix for this bounded validation issue. Sequence Diagram(s)sequenceDiagram
participant KotlinSDK
participant JNI
participant PlatformWalletFFI
participant PlatformWallet
participant ShieldedOperations
participant Network
KotlinSDK->>JNI: Create identity from one-time Orchard key
JNI->>PlatformWalletFFI: Validate and forward zeroizing inputs
PlatformWalletFFI->>PlatformWallet: Invoke wallet operation
PlatformWallet->>ShieldedOperations: Scan notes and execute claim
ShieldedOperations->>Network: Broadcast identity claim
Network-->>ShieldedOperations: Identity result or claim status
ShieldedOperations-->>PlatformWalletFFI: Identity ID or typed error
PlatformWalletFFI-->>KotlinSDK: Tagged result and diagnostic data
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
bfoss765
changed the title
|
⛔ Blockers found — Opus deferred (commit 122ba12) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4313 +/- ##
============================================
- Coverage 87.49% 87.36% -0.13%
============================================
Files 2672 2672
Lines 340400 340862 +462
============================================
- Hits 297819 297781 -38
- Misses 42581 43081 +500
🚀 New features to boost your workflow:
|
|
@bfoss765 I diagnosed the current I amended the introducing commit to remove only the unused
Replacement head: thepastaclaw@1ea5340 (branch I cannot update |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 4390cd7, all three carried-forward predecessor findings remain valid: two blocking claim recovery/classification defects and one full-history scan suggestion. The full current-PR range adds one genuinely new blocker—the unused direct log dependency fails the mandatory dependency audit; no predecessor finding is fixed, outdated, or deferred, and there are no exceptional out-of-scope follow-ups.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/security-auditor=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only)..
Validated blockers were found in the Codex precheck. Opus 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 - Opus: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1930-1933: Persist a recovery path before returning an unconfirmed claim
The one-time claim broadcasts the constructed transition at lines 1765-1774 without durably recording its serialized bytes, exact identity ID, selected nullifiers, submitted key bindings, or identity index. If execution succeeds but confirmation fails, this branch returns the only exact ID through the transient FFI/JNI result; process death or Kotlin cancellation during the synchronous native handoff can discard it, and `poke_sync_on_unconfirmed` has no foreign-key claim record to reconcile. A normal single-note retry cannot reproduce that ID because the randomly generated padding nullifier participates in it (`expected_identity_id` is `None` at lines 1708-1715), so the spent-nullifier preflight reaches terminal `ShieldedInviteAlreadyClaimed` at lines 3079-3089 even when this wallet's original transition created the identity. Persist sufficient pending-claim metadata before broadcast and automatically reconcile or re-drive the byte-identical transition after cancellation or restart.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1859-1871: Do not classify an applied chargeable fallback as retryable broadcast failure
This consensus-verdict branch recognizes an applied Type-20 chargeable fallback only when a separate nullifier query returns a positive spent status. `any_nullifier_spent_on_chain` maps `Ok(None)` and every transport, query, or proof error to `false` at lines 2896-2907, so a fallback that already consumed the invitation and credited the failure address can still be returned as `ShieldedBroadcastFailed`. Native code 16 and Kotlin explicitly describe that outcome as definitive non-execution and retryable, which is false after the fallback has applied. Even when the query succeeds, a collision on a submitted unique key other than MASTER leaves no identity under either the MASTER-key lookup or the transition's derived ID, causing the reconciler at lines 3092-3159 to return `ShieldedBroadcastUnconfirmed` instead of the terminal fallback result. Preserve unknown nullifier status separately from unspent status and classify the authenticated chargeable verdict without assuming the colliding key was MASTER.
In `packages/rs-platform-wallet/Cargo.toml`:
- [BLOCKING] packages/rs-platform-wallet/Cargo.toml:75: Remove the unused `log` dependency so cargo-machete passes
The exact reviewed head adds `log = "0.4"` as a direct runtime dependency, but no source in `rs-platform-wallet` imports or references the `log` facade; the nearby comment refers to an `identity::network::encrypted_document` module that is not present in the current crate. Both Rust CI workflows run `cargo machete`, and current PR comment 5199766972 confirms that this dependency causes the workspace test failure. The proposed replacement commit 1ea5340c7f998425e91e21eb05ec3fca9f0823eb removes it, but that commit is not the authoritative reviewed head. Remove `log` and its lockfile entry while retaining `rand`, which the current library code uses.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:835-860: Unfunded invitation keys force an unbounded full-history scan
Every syntactically valid foreign invitation key starts the proof-verified note stream at position zero with no cancellation token, chunk limit, or total-work budget. The only early exit is accumulating the requested denomination, so a valid but unfunded key downloads, verifies, and trial-decrypts the complete shielded history through the current tip. This attacker-controlled work grows with the pool and can be repeated to consume bandwidth, CPU, battery, memory, and a JNI worker; the supplied birth-height remains advisory only, and Kotlin coroutine cancellation cannot interrupt the synchronous native scan. Add native cancellation with a resumable work budget, an authenticated starting position, or another strict per-invitation bound.
| 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 foreign invitation key starts the proof-verified note stream at position zero with no cancellation token, chunk limit, or total-work budget. The only early exit is accumulating the requested denomination, so a valid but unfunded key downloads, verifies, and trial-decrypts the complete shielded history through the current tip. This attacker-controlled work grows with the pool and can be repeated to consume bandwidth, CPU, battery, memory, and a JNI worker; the supplied birth-height remains advisory only, and Kotlin coroutine cancellation cannot interrupt the synchronous native scan. Add native cancellation with a resumable work budget, an authenticated starting position, or another strict per-invitation bound.
source: ['codex']
There was a problem hiding this comment.
Deferred as a follow-up rather than fixed here: a strict per-invitation scan bound needs a product decision (a budget that stops short of the tip makes a deep-but-valid invite report unfunded), and native cancellation is a JNI-surface change beyond this PR. Partially mitigated by c6f4aa7: a retry of a broadcast claim now resumes from the persisted record and skips the transient rescan entirely, which removes the repeat-scan amplification path.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Fixed in 2dc71c9 — repeated foreign-key scans now resume from a process-local checkpoint (LRU-bounded, keyed by FVK hash). First scan per key stays full-history, which is the funds-safe direction: resume never advances past a partial chunk start_index (same rule as ShieldedChunkBatch::is_partial), so a resumed scan cannot miss a note a from-zero scan would find; every later scan pays only new tree growth. Native cancellation remains the acknowledged JNI-surface follow-up; this closes the repeat-amplification half.
There was a problem hiding this comment.
Resolved in 2dc71c9 — 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.
cargo-machete rejects the direct log dependency: no source in rs-platform-wallet uses the log facade (breadcrumbs go through tracing; the JNI layer bridges tracing, not log). rand stays — OsRng/RngCore back generate_one_time_orchard_key and the contact-info ephemeral keys. Addresses #4313 review finding f3fd60d83554. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ier status for one-time claims Two claim-lifecycle fixes for identity_create_from_one_time_key (#4313 review findings c0781f9d387f and 8d020115b274): Pending-claim record (persist-first, fail-closed). The claim now arms a persisted record — byte-exact transition, declared identity id, nullifiers, anchor — BEFORE broadcast, keyed deterministically by the one-time FVK under a reserved claim-records subwallet (ONE_TIME_CLAIM_RECORDS_ACCOUNT = u32::MAX, unreachable by the ZIP-32 hardened range and never visited by the spend-redrive sync pass). A retry after process death or JNI cancellation resumes from the record: spent notes reconcile against the DECLARED id (recoverable even for a padded single-note bundle, whose id embeds an unreproducible random dummy nullifier), unspent notes re-drive the byte-identical transition, and a definitively-rejected record with proven-unspent notes is cleared so a fresh build proceeds in the same call. Records clear on terminal outcomes (success / ShieldedInviteAlreadyClaimed) and survive Unconfirmed — the outcome whose retry needs them. Arming failure aborts before broadcast (Persistence error): nothing is consumed yet, and broadcasting without the record risks an unrecoverable ShieldedInviteAlreadyClaimed. Tri-state nullifier status. any_nullifier_spent_on_chain collapsed query errors, absent responses, and partial coverage to "unspent", letting an applied Type-20 chargeable fallback surface as ShieldedBroadcastFailed — documented to hosts as definitive non-execution and safe to retry. nullifier_spent_status now returns Spent/Unspent/Unknown; the consensus-verdict wait arm classifies ShieldedBroadcastFailed only on proven-Unspent, returns Unconfirmed on Unknown, and on proven-Spent hands the reconciler spend_finalized evidence so the nothing-found outcome is the terminal chargeable fallback / competing claim — correct even when the colliding unique key was not MASTER and no identity is findable under either probe. The pre-broadcast preflight still proceeds on Unknown (safe: the idempotent broadcast path reconciles via the NullifierAlreadySpent verdict). The broadcast/wait/classify tail is shared between the fresh and resume paths (broadcast_and_confirm_one_time_claim). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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 (6)
packages/rs-platform-wallet/src/wallet/platform_wallet.rs (1)
1386-1417: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared post-broadcast registration tail.
Lines 1386-1417 duplicate the tail of
shielded_identity_create_from_pool(lines 1291-1318). Only the log label differs. Extract a private helper that takes the identity,identity_index, and a&'static strlabel. This keeps the two flows from drifting.🤖 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/platform_wallet.rs` around lines 1386 - 1417, Extract the duplicated local identity registration logic from the current block and shielded_identity_create_from_pool into a private helper accepting the identity, identity_index, and a &'static str log label. Preserve the existing wallet-manager lookup, add_identity call, warning behavior, and flow-specific log labels, then replace both inline tails with helper calls.packages/rs-unified-sdk-jni/src/funding.rs (1)
936-971: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared tagged-payload packing tail.
Lines 936-971 duplicate lines 785-819 of
Java_..._shieldedIdentityCreateFromPool, including the unconfirmed-code check, the diagnostic capture, the result free, and the[tag] || id || diagnosticpacking. Extract one helper that takesenv, thePlatformWalletFFIResult, andout_id, and returns the packed array or a null pointer. This keeps the two claim paths from drifting on the unconfirmed contract.🤖 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-unified-sdk-jni/src/funding.rs` around lines 936 - 971, Extract the duplicated unconfirmed-result handling and tagged-payload packing from the current shielded identity claim path and Java_..._shieldedIdentityCreateFromPool into one shared helper. Have the helper accept JNIEnv, PlatformWalletFFIResult, and out_id, perform the unconfirmed check, diagnostic capture, result cleanup/error handling, and return the packed byte array pointer or null. Replace both inline implementations with calls to this helper so their unconfirmed contract remains identical.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt (1)
2298-2312: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSensitive Data Exposure (CWE-226)
Reachability: Internal · Exploitability: Theoretical
Add a
wipe()affordance toOneTimeOrchardKey.
spendingKeyis bearer spend authority. The transient 75-byte blob is cleared, but the returnedByteArrayremains in the heap until callers clear it or the JVM reclaims it. A named method would make this lifecycle explicit. The generatedtoString()does not print the key bytes.🤖 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/wallet/PlatformWalletManager.kt` around lines 2298 - 2312, Add a public wipe() method to OneTimeOrchardKey that securely clears both spendingKey and address in place using the appropriate ByteArray fill operation, making the transient key lifecycle explicit without changing equals or hashCode.packages/rs-platform-wallet/src/wallet/shielded/operations.rs (2)
4920-5178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
spend_finalizedterminal branch.
one_time_claim_evidence_testscoversrecovered_identity_matches_claimthoroughly, but no test exercisesrecover_executed_one_time_claimitself. Thespend_finalized == truebranch at Lines 3569-3584 encodes the fix for finding8d020115b274: when both lookups come up empty under a definitive verdict and proven-spent notes, the outcome must be terminalShieldedInviteAlreadyClaimed, notShieldedBroadcastUnconfirmed.That branch is reachable without a live network. A mock SDK drives both
fetch_identity_by_key_hash_with_retriesandfetch_identity_with_retriestoNone, which is exactly the nothing-found precondition.resume_drops_corrupt_record_and_rebuildsat Line 3959 already usesSdkBuilder::new_mock()in this file.Note that the test will sleep for the full retry cadence twice, once per lookup handle.
💚 Proposed test for both dispositions of the nothing-found outcome
/// Nothing found under either handle. `spend_finalized` decides between the /// terminal chargeable-fallback verdict and the retryable unconfirmed one /// (`#4204` review finding 8d020115b274). #[tokio::test] async fn nothing_found_terminal_only_when_spend_finalized() { let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); let expected_id = identity_id_from_nullifiers(&our_nullifiers()); let terminal = recover_executed_one_time_claim( &sdk, Some(OUR_MASTER_HASH), Some(expected_id), true, "test: definitive verdict with proven-spent notes", ) .await; assert!(matches!( terminal, Err(PlatformWalletError::ShieldedInviteAlreadyClaimed { .. }) )); let retryable = recover_executed_one_time_claim( &sdk, Some(OUR_MASTER_HASH), Some(expected_id), false, "test: spent nullifier without a definitive verdict", ) .await; assert!(matches!( retryable, Err(PlatformWalletError::ShieldedBroadcastUnconfirmed { .. }) )); }🤖 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 4920 - 5178, Add an async test in one_time_claim_evidence_tests that invokes recover_executed_one_time_claim with a mock SDK returning no identity from either lookup. Cover both spend_finalized values: assert true returns ShieldedInviteAlreadyClaimed and false returns ShieldedBroadcastUnconfirmed, using the existing identity_id_from_nullifiers helpers and required imports.
2154-2185: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the claim-record lifetime and either using or dropping
attempts.
arm_one_time_claim_recordwritesattempts: 0and no code path increments it. The claim path therefore has no attempt cap, unlike the spend-redrive path that readsMAX_REDRIVE_ATTEMPTS(referenced at Line 1404). Two consequences follow:
- A record whose
nullifier_spent_statusstaysUnknownis re-broadcast on every retry without bound.resume_one_time_claimclears the record only onUnspentplusShieldedBroadcastFailed(Lines 2312-2327).- A claim that is abandoned after arming leaves a permanent row under the reserved
ONE_TIME_CLAIM_RECORDS_ACCOUNTsubwallet. No TTL or pruning path exists, because the doc at Lines 2109-2114 excludes these rows from the spend-redrive sync pass.Neither breaks fund safety, since every re-broadcast is byte-identical. Increment
attemptsinresume_one_time_claimand clear the record past a cap, or remove the field from this construction so the redrive semantics are not implied.🤖 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 2154 - 2185, Update arm_one_time_claim_record and resume_one_time_claim to implement bounded retry semantics: increment PendingRedrive.attempts for each retry and clear the record once MAX_REDRIVE_ATTEMPTS is exceeded, including records left abandoned after arming where applicable. Preserve the existing byte-identical rebroadcast behavior and ensure terminal cleanup removes the reserved one-time-claim record.packages/rs-platform-wallet/src/wallet/shielded/keys.rs (1)
355-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a fully qualified rustdoc link for
RngCore::try_fill_bytes.
RngCoreis imported only insidegenerate_one_time_orchard_key, so the bare link produces a broken-link warning.🤖 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/keys.rs` at line 355, Update the rustdoc link in the documentation for generate_one_time_orchard_key to use a fully qualified path for RngCore::try_fill_bytes, avoiding reliance on the function-local import and eliminating the broken-link warning.
🤖 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/shielded_send.rs`:
- Line 1583: Update the documentation for the function containing
generate_one_time_orchard_key to remove the claim that it always succeeds; state
that it can return ErrorWalletOperation when key generation fails, including OS
entropy failure, so callers understand they must check the result.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 3542-3567: Update the Handle 2 error path around
recovered_identity_matches_claim to distinguish master_key_hash == None from a
mismatched submitted key. When no master key hash is available, return
ShieldedInviteAlreadyClaimed with a reason explaining that no valid MASTER
authentication key hash was submitted or could be derived; retain the existing
“another holder” reason only when a hash exists but the identity binding fails.
In `@packages/rs-unified-sdk-jni/src/funding.rs`:
- Around line 879-881: Update read_recipient43 to accept a field: &str parameter
and use it in all null and length validation exception messages instead of
hardcoding recipientRaw43. Update every existing read_recipient43 call site to
pass the appropriate field name, including changeAddressRaw43 for the
change-address call and recipientRaw43 for recipient calls.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- Around line 2298-2312: Add a public wipe() method to OneTimeOrchardKey that
securely clears both spendingKey and address in place using the appropriate
ByteArray fill operation, making the transient key lifecycle explicit without
changing equals or hashCode.
In `@packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- Around line 1386-1417: Extract the duplicated local identity registration
logic from the current block and shielded_identity_create_from_pool into a
private helper accepting the identity, identity_index, and a &'static str log
label. Preserve the existing wallet-manager lookup, add_identity call, warning
behavior, and flow-specific log labels, then replace both inline tails with
helper calls.
In `@packages/rs-platform-wallet/src/wallet/shielded/keys.rs`:
- Line 355: Update the rustdoc link in the documentation for
generate_one_time_orchard_key to use a fully qualified path for
RngCore::try_fill_bytes, avoiding reliance on the function-local import and
eliminating the broken-link warning.
In `@packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- Around line 4920-5178: Add an async test in one_time_claim_evidence_tests that
invokes recover_executed_one_time_claim with a mock SDK returning no identity
from either lookup. Cover both spend_finalized values: assert true returns
ShieldedInviteAlreadyClaimed and false returns ShieldedBroadcastUnconfirmed,
using the existing identity_id_from_nullifiers helpers and required imports.
- Around line 2154-2185: Update arm_one_time_claim_record and
resume_one_time_claim to implement bounded retry semantics: increment
PendingRedrive.attempts for each retry and clear the record once
MAX_REDRIVE_ATTEMPTS is exceeded, including records left abandoned after arming
where applicable. Preserve the existing byte-identical rebroadcast behavior and
ensure terminal cleanup removes the reserved one-time-claim record.
In `@packages/rs-unified-sdk-jni/src/funding.rs`:
- Around line 936-971: Extract the duplicated unconfirmed-result handling and
tagged-payload packing from the current shielded identity claim path and
Java_..._shieldedIdentityCreateFromPool into one shared helper. Have the helper
accept JNIEnv, PlatformWalletFFIResult, and out_id, perform the unconfirmed
check, diagnostic capture, result cleanup/error handling, and return the packed
byte array pointer or null. Replace both inline implementations with calls to
this helper so their unconfirmed contract remains identical.
🪄 Autofix
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: 99c733c6-800b-4aa1-a79c-5cde77535ab0
📒 Files selected for processing (20)
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/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/ShieldedDao.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/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/error.rspackages/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-unified-sdk-jni/src/funding.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Requesting changes to hold this PR while the invitation architecture is settled as a package with #4312 — sequencing, not implementation. To be explicit up front: the three earlier review blockers are fixed at head c6f4aa71dd and those fixes stand on their merits (the durable pending-claim record, the tri-state nullifier-status classification, the dependency cleanup). The hold is about what this PR is: the public claim API for invitations — FFI, JNI, error contract — and we are actively re-deciding the design it exposes.
1. The recovery semantics this API freezes may be about to simplify
The pending-claim record makes a crashed claim recoverable, but it is device-local: it does not survive device loss or a seed-restore onto a new phone. For a padded single-note claim, that residual hole is unfixable client-side — the identity id embeds a random dummy nullifier nobody can rederive, so a record-less retry hits a false terminal "already claimed" for the wallet's own identity.
#4312's two-note funding removes that entire failure class structurally: the id becomes derivable from seed + invitation secret alone, and this record demotes from correctness-critical to a crash-cache/rescan-skip optimization. If two-note (or its pre-split variant — see the #4312 review) becomes the standard funding layout, then the weight of this API's recovery machinery, its error contract (particularly the definitive-failure code-16 semantics and the already-claimed terminal cases the Kotlin layer documents), and what we promise host apps all change character. Freezing the FFI/JNI surface before the funding-layout decision risks shipping semantics we revise immediately after — the most expensive kind of API churn.
2. The deferred scan exposure is acceptable internally, not in a shipped API
The claim path scans the full shielded pool history for any syntactically valid foreign key — attacker-controlled work with no cancellation token, chunk budget, or authenticated starting position (the birth-height hint is advisory because the tree has no height→position oracle). We deliberately deferred this in review as a follow-up, which is right for an internal branch. It is not right for a public API surface that third-party apps will call with untrusted invite links: a valid-but-unfunded key is a repeatable bandwidth/CPU/battery drain primitive. The product decision (hard scan budget vs deep-valid-invite UX, plus native cancellation across JNI) should land before this surface does.
3. Design-freeze items worth deciding while we hold
- Claim-record keyspace: records live as
PendingRedriverows under a reservedu32::MAXsubwallet — sound (hardened ZIP-32 indices cannot collide; the sync redrive pass never visits it) but schema-by-convention. If the record survives the #4312 decision as an optimization, decide whether it should become a first-class store surface instead. - Recovery story for the legacy tail: already-funded single-note invites remain claimable indefinitely and are the one cohort the record permanently serves. The API docs should state plainly which recovery guarantees apply to which invite generation.
- Error-contract review: with two-note claims,
expected_identity_idis always derivable pre-build; several unconfirmed/terminal distinctions the current contract exposes collapse. Worth one deliberate pass over the surfaced error codes against the final design rather than incremental patches after release.
What unblocks this
The funding-layout decision on #4312, then one finalization pass here so the claim surface, its error contract, and the recovery guarantees ship coherently against the chosen design. The blocker fixes at c6f4aa71dd carry forward regardless — none of that work is wasted under any outcome.
QuantumExplorer
added
the
temp hold
The proposed-allocations table named the fork-era owners (#4184, #4185, #4204, #4247, #4256), all closed when the estate was recreated in-repository. Ownership now names the active successors (29 -> #4316, 32 -> #4310, 33 -> #4311, 34-36 -> #4308, 37 -> #4313, carriers updated), the no-code inventory is marked as the fork-era snapshot it is, and the provenance base is date-stamped instead of claiming to be current. Collision history keeps the fork-era numbers — it is record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The shielded-invite row both promised 42 to #4313-on-revival and named 42 the next allocatable integer, letting two contributors claim the same value. The held PR now explicitly holds nothing; it takes whatever the frontier is at revival, recording the claim here first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-time key generation The doc on platform_wallet_generate_one_time_orchard_key stated it always succeeds, but the function returns ErrorWalletOperation when the underlying generate_one_time_orchard_key fails (an OS entropy failure in try_fill_bytes). A caller trusting that line could skip the result check. State the real contract: re-rolling makes an INVALID key impossible, but the call itself can still fail — always check the result code. Addresses #4313 review thread at shielded_send.rs:1583 (CodeRabbit cr-comment 5b08c094f1096d57ab53741b).
…h is resolvable Handle 2 of recover_executed_one_time_claim reported every binding failure as 'belongs to another holder of the one-time key', but recovered_identity_matches_claim also fails closed when master_key_hash is None (no MASTER auth key submitted, or public_key_hash() errored for an unusual key type) — before it inspects any binding. In that case the key binding can never be established, which is not evidence of a competing holder. ShieldedInviteAlreadyClaimed is terminal, so this reason text is the only diagnostic the user gets for a permanently unclaimable invitation. Distinguish the None case: still terminal (a retry resubmits the same key set), but the reason now says ownership cannot be verified rather than misattributing the identity to another holder. The outcome doc gains the new cause. Addresses #4313 review thread at operations.rs:3567 (CodeRabbit cr-comment c96e9b63c7a921f8d43c57ac).
…-local resume checkpoint scan_notes_for_foreign_key (the L2-invitation claim path) restarted the proof-verified note stream at position zero on every call, with value coverage as the only early exit — so a syntactically valid but UNFUNDED invitation key (attacker-controlled input) forced a full-history download, verify, and trial-decrypt of the entire shielded pool on every attempt, repeatable at will (#4313 review finding d19c5cf84a9f). The tree exposes no height-to-position oracle (a chunk's block_height is the proof-tip height, not per-note inclusion height), so the invitation's birth-height hint cannot seed the scan start, and any budget that stops short of the tip would misreport a deep-but-valid invite as unfunded. Bound the REPEAT instead of the coverage: a process-local checkpoint keyed by sha256(domain-tag || one-time FVK) records how far the tree has been covered for each key plus the notes found on that covered prefix. The first scan for a key still covers the full history from position 0 (funds-safety: a resumed scan can never miss a note a from-zero scan would have found), and every later scan for the same key resumes past the immutable full chunks it already covered — one full-history scan per key per process, after which each retry pays only new tree growth plus the mutable buffer chunk. Mechanics: - The resume position advances past full chunks only, and is held AT a partial (buffer) chunk's start_index — the same resume rule the subwallet sync applies via ShieldedChunkBatch::is_partial — because that chunk can still receive notes. Buffer-chunk notes are never carried in the checkpoint, so the rescan cannot duplicate them. - The resume position is re-aligned DOWN to the on-chain MMR chunk boundary (CHUNK_SIZE) on use, so a resume can only over-scan, never skip. - Progress is checkpointed on every exit path, including a mid-scan stream error, so an interrupted retry resumes rather than restarting. - The map is LRU-bounded (8 keys); hostile key churn cannot pin memory, and an evicted key merely re-pays its own full scan. Deliberately process-local: no persisted state to invalidate. Native cancellation of the synchronous JNI scan remains a follow-up (it is a JNI-surface change); this closes the repeat-amplification path, complementing c6f4aa7 (broadcast-claim retries already skip the transient rescan via the durable pending-claim record). Adds unit tests for the checkpoint carry/drop rule and the map's take/save/evict semantics. Addresses #4313 review thread at sync.rs:860 (codex finding d19c5cf84a9f).
… errors A null/wrong-length changeAddressRaw43 was reported as recipientRaw43 — a parameter that entry point does not have. Give the helper a field parameter, mirroring read_id32. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/src/wallet/shielded/sync.rs`:
- Around line 863-871: Replace take_foreign_scan_checkpoint-based ownership with
per-key in-flight scan state so concurrent scan_notes_for_foreign_key callers
await the existing scan instead of restarting from position zero. Preserve or
monotonically update each key’s checkpoint when the scan completes, including
cancellation-safe cleanup, and never hold a std::sync::Mutex guard across an
await.
- Around line 945-949: Update foreign_scan_checkpoint_key and its callers in the
foreign scan flow to include sdk.network alongside the FVK, ensuring
process-global checkpoints are isolated per SDK network. Add a test that uses
the same FVK across two networks and verifies their checkpoints are not reused.
🪄 Autofix
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: d908b187-a55e-49e2-a6ea-8977773087d2
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/rs-unified-sdk-jni/src/funding.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-platform-wallet-ffi/src/shielded_send.rs
- packages/rs-platform-wallet/src/wallet/shielded/operations.rs
- packages/rs-unified-sdk-jni/src/funding.rs
| let checkpoint_key = foreign_scan_checkpoint_key(fvk); | ||
| let (mut found, resume_position) = match take_foreign_scan_checkpoint(&checkpoint_key) { | ||
| Some(cp) => (cp.notes, cp.resume_position), | ||
| None => (Vec::new(), 0), | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/rs-platform-wallet/src/wallet/shielded/sync.rs \
--items all \
--match 'foreign_scan_checkpoint_key|scan_notes_for_foreign_key'
rg -n -C 5 --glob '*.rs' '\bscan_notes_for_foreign_key\s*\(' packages
rg -n -C 5 --glob '*.rs' 'foreign_scan_checkpoint_key|FullViewingKey|to_bytes' \
packages/rs-platform-wallet/src/wallet/shielded/sync.rs
rg -n -C 4 --glob '*.rs' '\b(Network|network)\b' \
packages/rs-platform-wallet/src/wallet packages/rs-platform-wallet/src/managerRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=packages/rs-platform-wallet/src/wallet/shielded/sync.rs
printf '%s\n' '--- checkpoint definitions and scan method ---'
rg -n -C 12 'FOREIGN_SCAN_CHECKPOINTS|foreign_scan_checkpoint_key|take_foreign_scan_checkpoint|scan_notes_for_foreign_key' "$file"
printf '%s\n' '--- scan method callers ---'
rg -n -C 10 'scan_notes_for_foreign_key\s*\(' packages/rs-platform-wallet/src packages/rs-platform-wallet/tests 2>/dev/null || true
printf '%s\n' '--- SDK network/chain fields and constructors near shielded sync ---'
rg -n -C 6 'pub struct Sdk|struct Sdk|network:|Network::|chain|Chain' packages/rs-platform-wallet/src/wallet/shielded packages/rs-platform-wallet/src | head -n 500Repository: dashpay/platform
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SDK construction and network selection in platform-wallet ---'
rg -n -C 8 'Sdk::|Arc<dash_sdk::Sdk>|network\(\)|network:' packages/rs-platform-wallet/src \
| rg -n -C 4 'Sdk|sdk|network|Network' | head -n 500
printf '%s\n' '--- all direct SDK network APIs ---'
rg -n -C 5 '\bsdk\.[A-Za-z_]*network|\.network\(\)|get_network|Network::Mainnet|Network::Testnet|Network::Devnet' \
packages/rs-platform-wallet/src packages/rs-platform-wallet/tests 2>/dev/null | head -n 500
printf '%s\n' '--- foreign scan implementation and checkpoint save paths ---'
sed -n '937,1055p' packages/rs-platform-wallet/src/wallet/shielded/sync.rsRepository: dashpay/platform
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SDK network field and shielded scan body ---'
rg -n -C 5 'self\.sdk\.network|pub .*network|network:' packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs packages/rs-platform-wallet/src/wallet/platform_wallet.rs
sed -n '937,1065p' packages/rs-platform-wallet/src/wallet/shielded/sync.rs
printf '%s\n' '--- foreign-key derivation and SDK handoff ---'
rg -n -C 8 'foreign|one_time|FullViewingKey|IncomingViewingKey|from_seed|viewing_keys' \
packages/rs-platform-wallet/src/wallet/shielded/operations.rs \
packages/rs-platform-wallet/src/wallet/shielded/keys.rs \
packages/rs-platform-wallet/src/wallet/identity/network 2>/dev/null | head -n 600Repository: dashpay/platform
Length of output: 50372
Include the SDK network in the checkpoint key.
FOREIGN_SCAN_CHECKPOINTS is process-global, but foreign_scan_checkpoint_key hashes only the FVK. A checkpoint from one SDK network can be reused by another SDK with the same FVK, producing notes for the wrong chain. Include sdk.network in the key and add a cross-network test.
🤖 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/sync.rs` around lines 945 -
949, Update foreign_scan_checkpoint_key and its callers in the foreign scan flow
to include sdk.network alongside the FVK, ensuring process-global checkpoints
are isolated per SDK network. Add a test that uses the same FVK across two
networks and verifies their checkpoints are not reused.
There was a problem hiding this comment.
The checkpoint is incorrectly shared across networks. Including sdk.network prevents mainnet/testnet/devnet/regtest reuse, but it may not distinguish two separate devnets represented by the same Network::Devnet value; scope the cache to a coordinator/SDK chain instance or include a stable chain discriminator where available.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Candidate SDK and coordinator definitions ---'
fd -t f -e rs . packages | rg '(sdk|coordinator|wallet_lifecycle|platform_wallet)\.rs$' | head -n 120
printf '%s\n' '--- SDK type, fields, constructors, and network/chain accessors ---'
rg -n -C 5 --glob '*.rs' \
'(struct\s+Sdk\b|impl\s+Sdk\b|pub\s+(?:crate\s+)?(?:async\s+)?fn\s+.*(?:network|chain|genesis|identifier)|\bnetwork\s*:|\bchain\s*:|\bgenesis\s*:)' \
packages | head -n 900
printf '%s\n' '--- Coordinator state and construction paths ---'
rg -n -C 6 --glob '*.rs' \
'(struct\s+NetworkShieldedCoordinator\b|impl\s+NetworkShieldedCoordinator\b|NetworkShieldedCoordinator::new|shielded.*coordinator|coordinator.*shielded)' \
packages/rs-platform-wallet/src | head -n 900
printf '%s\n' '--- Foreign scan call sites and SDK configuration handoff ---'
rg -n -C 8 --glob '*.rs' \
'(scan_notes_for_foreign_key\s*\(|Arc<dash_sdk::Sdk>|dash_sdk::Sdk::|SdkConfig|Sdk::new)' \
packages/rs-platform-wallet/src | head -n 900Length of output: 201452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Coordinator identity-create methods and operation calls ---'
ast-grep outline packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs \
--items all \
--match 'identity_create_from_one_time_key|one_time|foreign'
rg -n -C 14 --glob '*.rs' \
'(identity_create_from_one_time_key\s*\(|scan_notes_for_foreign_key\s*\()' \
packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs \
packages/rs-platform-wallet/src/wallet/shielded/operations.rs \
packages/rs-platform-wallet/src/wallet/platform_wallet.rs
printf '%s\n' '--- Coordinator fields and constructor ---'
sed -n '89,165p' packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs
sed -n '365,430p' packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs
printf '%s\n' '--- One-time identity operation signature and scan call ---'
sed -n '1570,1725p' packages/rs-platform-wallet/src/wallet/shielded/operations.rsLength of output: 21962
@thepastaclaw, confirmed. The existing call path supports a coordinator-scoped cache:
PlatformWallet::identity_create_from_one_time_keyalready receives&Arc<NetworkShieldedCoordinator>.- It calls
operations::identity_create_from_one_time_key. - That operation calls
sync::scan_notes_for_foreign_key.
The safe fix is to replace FOREIGN_SCAN_CHECKPOINTS with a checkpoint cache owned by NetworkShieldedCoordinator. Pass that cache through the existing call path to scan_notes_for_foreign_key. This isolates checkpoints for separate devnet chains and avoids unsafe reuse of SDK pointer addresses.
The test should create two coordinator-owned caches, use the same FVK in both, save a checkpoint in the first cache, and verify that the second cache has no entry.
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/platform PR: 4313
File: packages/rs-platform-wallet/src/wallet/shielded/sync.rs:945-949
Timestamp: 2026-08-12T14:19:06.431Z
Learning: In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`, a foreign-key scan checkpoint must be scoped to a `NetworkShieldedCoordinator` or an equivalent stable chain instance. `dashcore::Network::Devnet` is not sufficient because separate devnet chains can use the same `Network::Devnet` value. Do not use an SDK allocation address as a process-global cache discriminator because allocator address reuse can cause cross-instance cache reuse after the SDK is dropped.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
There was a problem hiding this comment.
Fixed in 6668061 — implemented as the coordinator-scoped cache this thread converged on rather than a network-tagged key: the FOREIGN_SCAN_CHECKPOINTS static is removed and ForeignScanCheckpointCache is a field of NetworkShieldedCoordinator, flowing through the existing PlatformWallet::identity_create_from_one_time_key -> operations -> scan_notes_for_foreign_key path. One coordinator = one network + one tree store, so mainnet/testnet/devnet/regtest are isolated AND two distinct devnets under Network::Devnet get distinct caches; no SDK allocation address is used as a discriminator. The test is the exact requested shape: same FVK, checkpoint saved through one cache, the second cache has no entry (foreign_scan_checkpoints_do_not_cross_cache_instances).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
This was valid at the earlier head but is fixed in current code. FOREIGN_SCAN_CHECKPOINTS no longer exists: ForeignScanCheckpointCache is owned by NetworkShieldedCoordinator, so separate chains and separate devnet coordinator instances cannot reuse one another's checkpoints. The cross-cache isolation test verifies this.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The byte-exact pending record and tri-state nullifier classification fix the three prior blockers, but the recovery guarantee is still defeated by concurrent claims that can replace each other's record before either broadcast completes. The new process-global scan cache also crosses network boundaries, while the carried-forward first-scan work-bound issue remains unresolved.
Source: Codex reviewers gpt-5.6-sol (general), gpt-5.6-sol (security-auditor), gpt-5.6-sol (rust-quality), and gpt-5.6-sol (ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1681-1839: Serialize concurrent claims before replacing their recovery record
There is no per-invitation single-flight guard around the pending-record lookup, transition construction, arming, broadcast, and finalization. Two calls for the same FVK can both observe no record, and a single-note build gives each call a different random padding nullifier and therefore a different identity ID. `arm_one_time_claim_record` later uses `arm_redrive`, whose file-store implementation performs `INSERT OR REPLACE`, so the second call overwrites the first call's byte-exact recovery record. If the first transition executes and its result is lost, only the second transition's ID remains; recovery cannot bind the first identity, and either caller can also clear the shared row while the other is still active. The same lack of coordination makes `take_foreign_scan_checkpoint` remove scan progress while another same-key call starts from zero. Hold a process-wide per-FVK guard across the complete claim lifecycle, not merely around individual store or checkpoint map operations.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:851-857: Scope foreign-key scan checkpoints to the Platform chain
`FOREIGN_SCAN_CHECKPOINTS` is process-global, but its key hashes only the FVK. If that FVK is scanned through a large position on one network and then claimed through another SDK in the same process, the second scan reuses the foreign resume position and cached notes. It can skip a funded note at an earlier position on the actual network or attempt to use notes from the wrong tree. Include a stable chain identity in the key or move the cache into a network/coordinator-scoped owner. `sdk.network` separates mainnet, testnet, devnet, and regtest, but a coordinator or chain discriminator is also needed if multiple distinct devnets can coexist under `Network::Devnet`.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:984-1028: Unfunded invitation keys force an unbounded full-history scan
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3725876429)
The process-local checkpoint reduces repeat work only after a scan for the same FVK completes and remains among the eight cached entries. Every fresh, evicted, or post-restart invitation key still starts at position zero and consumes the proof-verified stream through the tip when it is unfunded or underfunded. There is no native cancellation input, chunk budget, total-work limit, or retained-note byte/count limit, so an untrusted caller can rotate valid keys and repeatedly consume bandwidth, proof-verification CPU, memory, battery, and synchronous JNI workers. Add native cancellation with a strict resumable work and retained-data budget, an authenticated starting position, or another bound that applies to the first scan for every invitation.
| if let Some(record) = | ||
| find_one_time_claim_record(store, claim_records_id, claim_record_key).await? | ||
| { | ||
| match resume_one_time_claim( | ||
| sdk, | ||
| store, | ||
| claim_records_id, | ||
| &record, | ||
| master_key_hash, | ||
| submitted_public_keys.clone(), | ||
| denomination, | ||
| ) | ||
| .await | ||
| { | ||
| OneTimeClaimResume::Resolved(result) => { | ||
| finalize_one_time_claim_record(store, claim_records_id, claim_record_key, &result) | ||
| .await; | ||
| return result; | ||
| } | ||
| // The stored transition is unusable (corrupt, or definitively | ||
| // rejected while its notes are provably unspent) — the record has | ||
| // been cleared; build a fresh claim below. | ||
| OneTimeClaimResume::RecordUnusable => {} | ||
| } | ||
| } | ||
|
|
||
| // Transient scan: re-derive the one-time key's note(s) from the network. | ||
| let discovered = super::sync::scan_notes_for_foreign_key(sdk, &fvk, &ivk, denomination).await?; | ||
| if discovered.is_empty() { | ||
| // No note decrypts under this key — nothing was funded to it (or the | ||
| // wallet hasn't synced far enough to see it yet). | ||
| return Err(PlatformWalletError::ShieldedNoUnspentNotes); | ||
| } | ||
|
|
||
| // Exact-equality selection over the transiently-scanned set: cover exactly | ||
| // `denomination`, gate on `denomination > predicted_fee`. Surfaces | ||
| // `ShieldedInsufficientBalance { available, required }` when the key's notes | ||
| // don't cover the denomination, mirroring the pool-funded neighbor. | ||
| let (selected_refs, total_input, predicted_fee) = | ||
| select_notes_for_denomination(&discovered, denomination, 2, num_keys, sdk.version())?; | ||
| let selected_notes: Vec<ShieldedNote> = selected_refs.into_iter().cloned().collect(); | ||
|
|
||
| info!( | ||
| denomination, | ||
| predicted_fee, | ||
| inputs = selected_notes.len(), | ||
| total_input, | ||
| keys = num_keys, | ||
| "IdentityCreateFromOneTimeKey" | ||
| ); | ||
|
|
||
| // Idempotent-retry preflight (no persisted record for this key). If this one-time key's | ||
| // selected note(s) are ALREADY spent on chain, a byte-identical claim already | ||
| // executed — so we must NOT rebuild+rebroadcast (that would only earn a | ||
| // `NullifierAlreadySpent` rejection). Everything checked here is re-derived | ||
| // from the invite the invitee holds: the one-time key → its note(s) via the | ||
| // transient scan above, and each note's real nullifier (`ShieldedNote.nullifier`, | ||
| // stamped `note.nullifier(fvk)` during the scan). If spent, recover the | ||
| // previously-created identity by the invitee's own re-derivable MASTER auth key | ||
| // hash (`discover_inner`'s unique-hash probe) and return it as success. | ||
| let selected_nullifiers: Vec<[u8; 32]> = selected_notes.iter().map(|n| n.nullifier).collect(); | ||
|
|
||
| // The id that an identity created by THIS claim must carry — the single | ||
| // handle that ties a recovered identity back to this claim's spend, and the | ||
| // reason a MASTER-key-hash hit alone is not evidence of a successful claim | ||
| // (see `recovered_identity_matches_claim`). | ||
| // | ||
| // Consensus derives the new identity id as `double_sha256` over the SORTED | ||
| // set of PUBLISHED action nullifiers (`derive_identity_id_from_actions`) and | ||
| // rejects a transition whose declared id differs, so this is a binding, not a | ||
| // guess. | ||
| // | ||
| // `None` for a single-spend claim: the builder pads to Orchard's 2-action | ||
| // minimum (`num_actions = spends.len().max(2)`) and the padding action's | ||
| // dummy nullifier is randomly generated per build, so it participates in the | ||
| // derivation but cannot be reproduced on a retry. With two or more real | ||
| // spends no padding is added and the published set is exactly | ||
| // `selected_nullifiers`. | ||
| let expected_identity_id = | ||
| (selected_notes.len() >= 2).then(|| identity_id_from_nullifiers(&selected_nullifiers)); | ||
|
|
||
| // Idempotent-retry preflight. If this one-time key's selected note(s) are | ||
| // ALREADY spent on chain, this claim can never execute — rebuilding and | ||
| // rebroadcasting would only earn a `NullifierAlreadySpent` rejection and burn | ||
| // a Halo 2 proof. Hand off to the reconciler, which decides between "this | ||
| // claim created that identity" (both bindings verified), "the invitation is | ||
| // gone" (terminal), and "executed but not yet indexed" (retryable). | ||
| // `Unknown` proceeds here — that is safe pre-broadcast: the idempotent | ||
| // broadcast path reconciles via the `NullifierAlreadySpent` verdict, so a | ||
| // transient query failure only costs a harmless rebuild. | ||
| if nullifier_spent_status(sdk, &selected_nullifiers).await == NullifierSpentStatus::Spent { | ||
| return recover_executed_one_time_claim( | ||
| sdk, | ||
| master_key_hash, | ||
| expected_identity_id, | ||
| false, | ||
| "the selected note's nullifier is already spent on chain (pre-broadcast preflight)", | ||
| ) | ||
| .await; | ||
| } | ||
|
|
||
| // Witness the selected notes against a Platform-recorded anchor from the | ||
| // shared, fully-marked commitment tree (identical probe to the pool op). | ||
| let (spends, anchor) = extract_spends_and_anchor(sdk, store, &selected_notes).await?; | ||
| let anchor_bytes = anchor.to_bytes(); | ||
|
|
||
| let build = build_identity_create_from_shielded_pool_transition( | ||
| public_keys, | ||
| denomination, | ||
| send_to_address_on_creation_failure, | ||
| spends, | ||
| change_address, | ||
| &fvk, | ||
| &ask, | ||
| anchor, | ||
| prover, | ||
| identity_signer, | ||
| [0u8; 36], | ||
| sdk.version(), | ||
| ) | ||
| .await | ||
| .map_err(|e| PlatformWalletError::ShieldedBuildError(e.to_string()))?; | ||
| // The spend-auth key's final use (the bundle build + spend-auth | ||
| // signatures above) is behind us — scrub it before the broadcast and | ||
| // result wait keep this frame alive across the network. | ||
| drop(ask); | ||
|
|
||
| let identity_id = build.identity_id; | ||
|
|
||
| // Re-assemble the transition from the PoP-signed keys + bundle params | ||
| // (preserving the per-key signatures) and broadcast. The broadcast/wait | ||
| // classification mirrors `identity_create_from_shielded_pool` verbatim, minus | ||
| // the note-reservation bookkeeping (there is no subwallet reservation to | ||
| // release — the spent notes belong to the foreign one-time key). | ||
| let st = sdk | ||
| .identity_create_from_shielded_pool_transition( | ||
| build.public_keys, | ||
| denomination, | ||
| send_to_address_on_creation_failure, | ||
| build.bundle, | ||
| ) | ||
| .map_err(|e| PlatformWalletError::ShieldedBuildError(e.to_string()))?; | ||
|
|
||
| // Persist the pending-claim record BEFORE the broadcast (#4204 review | ||
| // finding c0781f9d387f): once the transition leaves this process, the | ||
| // declared id — the only handle that recovers a padded single-note claim — | ||
| // must already be durable. Fail-closed: nothing has been consumed yet, so | ||
| // refusing to broadcast on a persistence failure is a clean, retryable | ||
| // stop; broadcasting without the record risks an unrecoverable | ||
| // `ShieldedInviteAlreadyClaimed` on the next attempt. | ||
| arm_one_time_claim_record( | ||
| store, | ||
| claim_records_id, | ||
| claim_record_key, | ||
| anchor_bytes, | ||
| &selected_nullifiers, | ||
| &st, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🔴 Blocking: Serialize concurrent claims before replacing their recovery record
There is no per-invitation single-flight guard around the pending-record lookup, transition construction, arming, broadcast, and finalization. Two calls for the same FVK can both observe no record, and a single-note build gives each call a different random padding nullifier and therefore a different identity ID. arm_one_time_claim_record later uses arm_redrive, whose file-store implementation performs INSERT OR REPLACE, so the second call overwrites the first call's byte-exact recovery record. If the first transition executes and its result is lost, only the second transition's ID remains; recovery cannot bind the first identity, and either caller can also clear the shared row while the other is still active. The same lack of coordination makes take_foreign_scan_checkpoint remove scan progress while another same-key call starts from zero. Hold a process-wide per-FVK guard across the complete claim lifecycle, not merely around individual store or checkpoint map operations.
source: ['codex', 'coderabbit']
There was a problem hiding this comment.
Fixed in 6668061 — coordinator-owned ForeignClaimGuards: identity_create_from_one_time_key now holds a per-FVK async mutex across the COMPLETE lifecycle (pending-record lookup, transient scan, transition construction, atomic arming, broadcast, finalization), so concurrent same-key claims serialize and the loser resumes the settled durable record instead of overwriting it through arm_redrive's INSERT-OR-REPLACE. The registry holds Weak handles — cancellation releases on drop, dead keys prune on the next acquisition — and lives on the SAME coordinator that owns the pending-record store it protects (one coordinator per network + tree store), so everything that can race one invitation's record serializes through one instance. The take-based checkpoint race is gone with it (see the sync.rs threads). Tests: same-key single-mutex identity, holder-blocks-second-caller, cancellation release, dead-entry prune. 865 platform-wallet lib tests green.
| fn foreign_scan_checkpoint_key(fvk: &grovedb_commitment_tree::FullViewingKey) -> [u8; 32] { | ||
| use dashcore::hashes::{sha256, Hash}; | ||
|
|
||
| let mut preimage = Vec::with_capacity(96 + 44); | ||
| preimage.extend_from_slice(b"platform-wallet:foreign-scan-checkpoint:v1"); | ||
| preimage.extend_from_slice(&fvk.to_bytes()); | ||
| sha256::Hash::hash(&preimage).to_byte_array() |
There was a problem hiding this comment.
🟡 Suggestion: Scope foreign-key scan checkpoints to the Platform chain
FOREIGN_SCAN_CHECKPOINTS is process-global, but its key hashes only the FVK. If that FVK is scanned through a large position on one network and then claimed through another SDK in the same process, the second scan reuses the foreign resume position and cached notes. It can skip a funded note at an earlier position on the actual network or attempt to use notes from the wrong tree. Include a stable chain identity in the key or move the cache into a network/coordinator-scoped owner. sdk.network separates mainnet, testnet, devnet, and regtest, but a coordinator or chain discriminator is also needed if multiple distinct devnets can coexist under Network::Devnet.
source: ['codex', 'coderabbit']
There was a problem hiding this comment.
Fixed in 6668061 — same fix as the CodeRabbit thread at sync.rs:949: the cache moved off the process-global static into NetworkShieldedCoordinator ownership (one coordinator = one network + one tree store), which isolates chains including distinct devnets sharing Network::Devnet — the stable chain discriminator falls out of instance identity instead of a key tag, so a resume position computed against one chain's tree can never skip an earlier funded note on another. Cross-instance isolation test included.
There was a problem hiding this comment.
Resolved in 6668061 — Scope foreign-key scan checkpoints to the Platform chain 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.
Carries #4380 (dashpay profile payment addresses, breaking dpp) + #4381 (rust-dashcore pin bump) + the shield-preflight family. Resolutions: - platform_wallet.rs / shielded_send.rs: false overlaps — keep BOTH our identity_create_from_one_time_key surface and v4.2-dev's shielded_shield_preflight/plan additions. - Error-code collision: v4.2-dev allocated 37-40 to the DPNS marketplace block and 41 to the shield-capacity shortfall, colliding with our ErrorShieldedInviteAlreadyClaimed = 37. Renumbered ours to 43 on every surface (Rust FFI enum, Kotlin arm + test pin, Swift raw value), the allocation the integration branch already ships in QA AARs (42 stays reserved to match it). - DashSdkError.kt / PlatformWalletResult.swift: keep both sides' new error classes/cases, ours renumbered and ordered after the v4.2-dev blocks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cope scan checkpoints to the coordinator Closes the two #4313 review clusters on the claim path: - ForeignClaimGuards (coordinator-owned): every identity_create_from_one_time_key holds a per-FVK async mutex across the COMPLETE lifecycle — pending-record lookup, transient scan, transition construction, atomic arming, broadcast, finalization (finding 979bbc2fcb3c). Concurrent same-key claims serialize instead of racing arm_one_time_claim_record's INSERT-OR-REPLACE and overwriting each other's byte-exact recovery row. Weak-handle registry: cancellation releases on drop, dead keys prune on the next acquisition. - ForeignScanCheckpointCache replaces the process-global FOREIGN_SCAN_CHECKPOINTS static: owned by NetworkShieldedCoordinator (one network + one tree store), so a resume position can never leak across chains — including two devnets sharing Network::Devnet (findings 6118148e4547 / cr-4d2aa8ce). load() clones instead of removing and save() is monotonic, so a claim cancelled mid-scan leaves the previous checkpoint intact instead of destroying it (finding cr-4808dde4); no sync mutex guard is ever held across an await. Tests: guard identity/serialization/cancellation-release/prune; cache load-no-remove, monotonic save, LRU eviction, and the cross-instance isolation shape CodeRabbit requested. 865 platform-wallet lib tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt (1)
1601-1611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
walletIdlength check.Every sibling method in this class validates
walletId.size == 32before the native call (seebindShieldedat Line 1357 andremoveWalletat Line 959). This method omits it. The JNIread_id32still rejects a wrong length, so the failure is safe, but it surfaces as a native exception instead of a local precondition.♻️ Proposed precondition
): ByteArray = teardownGate.op { + require(walletId.size == 32) { "walletId must be exactly 32 bytes, got ${walletId.size}" } require(oneTimeSk.size == 32) { "oneTimeSk must be 32 bytes, got ${oneTimeSk.size}" }🤖 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/wallet/PlatformWalletManager.kt` around lines 1601 - 1611, Add a local precondition in the teardown operation alongside the existing argument checks to require walletId.size == 32, matching sibling methods such as bindShielded and removeWallet, before the native call.
🤖 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/src/wallet/shielded/operations.rs`:
- Around line 2177-2200: Update purge_all_subwallets and purge_wallet to exclude
records keyed by ONE_TIME_CLAIM_RECORDS_ACCOUNT from destructive deletion.
Preserve those durable pending-claim rows during clear and unregister_wallet
while continuing to purge all other subwallet records as before.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- Around line 1601-1611: Add a local precondition in the teardown operation
alongside the existing argument checks to require walletId.size == 32, matching
sibling methods such as bindShielded and removeWallet, before the native call.
🪄 Autofix
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: ef76dc24-cd1c-4769-aab5-335a082cca3f
📒 Files selected for processing (21)
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/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/ShieldedDao.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/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/coordinator.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-unified-sdk-jni/src/funding.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt
- packages/rs-platform-wallet/src/wallet/shielded/mod.rs
- packages/rs-platform-wallet/Cargo.toml
- docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/ShieldedDao.kt
- packages/rs-platform-wallet/src/error.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt
- docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.kt
- packages/rs-platform-wallet/src/wallet/shielded/keys.rs
- packages/rs-unified-sdk-jni/src/funding.rs
| /// The synthetic ZIP-32 account index that keys durable one-time-claim records | ||
| /// in the [`ShieldedStore`]. | ||
| /// | ||
| /// Claim records reuse the store's persisted [`PendingRedrive`] rows (byte-exact | ||
| /// transition + nullifiers + anchor), but live under this reserved subwallet so | ||
| /// the spend-redrive sync pass — which iterates REAL Orchard accounts — never | ||
| /// re-broadcasts or prunes them; their lifecycle is owned entirely by | ||
| /// [`identity_create_from_one_time_key`]. ZIP-32 account indices are hardened | ||
| /// (`< 2^31`), so `u32::MAX` cannot collide with a real subwallet. | ||
| pub(super) const ONE_TIME_CLAIM_RECORDS_ACCOUNT: u32 = u32::MAX; | ||
|
|
||
| /// Deterministic record key for a one-time claim: every retry of the same | ||
| /// invitation re-derives the same key from the one-time FVK, which is exactly | ||
| /// what lets a retry find the record a crashed attempt left behind. Domain- | ||
| /// separated so it can never collide with an activity-entry id (sha256 of | ||
| /// visible output cmxs) sharing the `PendingRedrive.activity_id` keyspace. | ||
| fn one_time_claim_record_key(fvk: &grovedb_commitment_tree::FullViewingKey) -> [u8; 32] { | ||
| use dashcore::hashes::{sha256, Hash}; | ||
|
|
||
| let mut preimage = Vec::with_capacity(96 + 33); | ||
| preimage.extend_from_slice(b"platform-wallet:one-time-claim:v1"); | ||
| preimage.extend_from_slice(&fvk.to_bytes()); | ||
| sha256::Hash::hash(&preimage).to_byte_array() | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the store implementations that own the purge and redrive surfaces.
fd -e rs . packages/rs-platform-wallet/src/wallet/shielded --exec grep -ln 'purge_all_subwallets\|purge_wallet\|arm_redrive\|pending_redrives' {} \;
# Inspect the purge implementations and whether they filter by account index.
rg -n -C 12 --type=rs 'fn purge_all_subwallets|fn purge_wallet|fn clear_redrive|fn arm_redrive|fn pending_redrives' packages/rs-platform-wallet/src
# Check every consumer of the reserved account constant.
rg -n -C 6 --type=rs 'ONE_TIME_CLAIM_RECORDS_ACCOUNT' packages/rs-platform-wallet/srcRepository: dashpay/platform
Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- purge and redrive definitions ---'
rg -n -C 20 --type=rs \
'pub.*fn (purge_all_subwallets|purge_wallet)|fn (purge_all_subwallets|purge_wallet)|purge_all_subwallets|purge_wallet|arm_redrive|pending_redrives|clear_redrive' \
packages/rs-platform-wallet/src/wallet/shielded
printf '%s\n' '--- coordinator clear and unregister paths ---'
rg -n -C 25 --type=rs \
'fn clear|fn unregister_wallet|purge_all_subwallets|purge_wallet' \
packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs
printf '%s\n' '--- reserved account consumers ---'
rg -n -C 12 --type=rs 'ONE_TIME_CLAIM_RECORDS_ACCOUNT' \
packages/rs-platform-wallet/src/wallet/shieldedRepository: dashpay/platform
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- purge and redrive definitions ---'
rg -n -C 20 -g '*.rs' \
'pub.*fn (purge_all_subwallets|purge_wallet)|fn (purge_all_subwallets|purge_wallet)|purge_all_subwallets|purge_wallet|arm_redrive|pending_redrives|clear_redrive' \
packages/rs-platform-wallet/src/wallet/shielded
printf '%s\n' '--- coordinator clear and unregister paths ---'
rg -n -C 25 -g '*.rs' \
'fn clear|fn unregister_wallet|purge_all_subwallets|purge_wallet' \
packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs
printf '%s\n' '--- reserved account consumers ---'
rg -n -C 12 -g '*.rs' 'ONE_TIME_CLAIM_RECORDS_ACCOUNT' \
packages/rs-platform-wallet/src/wallet/shieldedRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- store purge method locations ---'
rg -n -g '*.rs' 'purge_all_subwallets|purge_wallet' \
packages/rs-platform-wallet/src/wallet/shielded/store.rs \
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs
printf '%s\n' '--- store trait and implementations ---'
rg -n -g '*.rs' 'trait ShieldedStore|impl ShieldedStore|fn arm_redrive|fn pending_redrives|fn clear_redrive' \
packages/rs-platform-wallet/src/wallet/shielded/store.rs \
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs
printf '%s\n' '--- relevant store source ---'
for file in packages/rs-platform-wallet/src/wallet/shielded/store.rs packages/rs-platform-wallet/src/wallet/shielded/file_store.rs; do
for line in $(rg -n -g '*.rs' 'purge_all_subwallets|purge_wallet' "$file" | cut -d: -f1); do
start=$((line - 25)); [ "$start" -lt 1 ] && start=1
end=$((line + 70))
sed -n "${start},${end}p" "$file"
done
doneRepository: dashpay/platform
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
store = Path("packages/rs-platform-wallet/src/wallet/shielded/store.rs").read_text()
file_store = Path("packages/rs-platform-wallet/src/wallet/shielded/file_store.rs").read_text()
def body(text, name):
match = re.search(
rf"fn {name}\s*\([^)]*\)[^{]*\{{(?P<body>.*?)\n\s*\}}",
text,
re.S,
)
if not match:
raise SystemExit(f"missing {name}")
return match.group("body")
in_memory_all = body(store, "purge_all_subwallets")
in_memory_wallet = body(store, "purge_wallet")
file_all = body(file_store, "purge_all_subwallets")
file_wallet = body(file_store, "purge_wallet")
assert "self.subwallets.clear()" in in_memory_all
assert "DELETE FROM shielded_pending_spends" in file_all
assert "self.subwallets.retain(|id, _| id.wallet_id != wallet_id)" in in_memory_wallet
assert "DELETE FROM shielded_pending_spends WHERE wallet_id = ?1" in file_wallet
print("InMemoryShieldedStore::purge_all_subwallets clears every subwallet.")
print("FileBackedShieldedStore::purge_all_subwallets deletes every durable pending-spend row.")
print("Both purge_wallet implementations remove all subwallet state for the wallet_id.")
print("A reserved account index is not excluded by either purge scope.")
PYRepository: dashpay/platform
Length of output: 336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
store = Path("packages/rs-platform-wallet/src/wallet/shielded/store.rs").read_text()
file_store = Path("packages/rs-platform-wallet/src/wallet/shielded/file_store.rs").read_text()
def method_region(text, name):
marker = f"fn {name}"
start = text.index(marker)
next_marker = text.find("\n fn ", start + len(marker))
return text[start:] if next_marker == -1 else text[start:next_marker]
in_memory_all = method_region(store, "purge_all_subwallets")
in_memory_wallet = method_region(store, "purge_wallet")
file_all = method_region(file_store, "purge_all_subwallets")
file_wallet = method_region(file_store, "purge_wallet")
assert "self.subwallets.clear()" in in_memory_all
assert "DELETE FROM shielded_pending_spends" in file_all
assert "self.subwallets.retain(|id, _| id.wallet_id != wallet_id)" in in_memory_wallet
assert "DELETE FROM shielded_pending_spends WHERE wallet_id = ?1" in file_wallet
print("InMemoryShieldedStore::purge_all_subwallets clears every subwallet.")
print("FileBackedShieldedStore::purge_all_subwallets deletes every durable pending-spend row.")
print("Both purge_wallet implementations remove all subwallet state for the wallet_id.")
print("Neither purge scope excludes ONE_TIME_CLAIM_RECORDS_ACCOUNT.")
PYRepository: dashpay/platform
Length of output: 243
Exclude ONE_TIME_CLAIM_RECORDS_ACCOUNT from destructive purges.
purge_all_subwallets deletes every durable pending-claim row, and purge_wallet deletes all rows for the wallet. Neither path excludes the reserved account. Therefore, clear and unregister_wallet can remove an unconfirmed claim record and make a padded claim unrecoverable.
🤖 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
2177 - 2200, Update purge_all_subwallets and purge_wallet to exclude records
keyed by ONE_TIME_CLAIM_RECORDS_ACCOUNT from destructive deletion. Preserve
those durable pending-claim rows during clear and unregister_wallet while
continuing to purge all other subwallet records as before.
There was a problem hiding this comment.
The destructive-purge race is valid, but unconditional exclusion of the reserved account is incomplete and conflicts with remove_wallet's full-wipe contract. clear and unregister_wallet use a lifecycle mutex that claims do not participate in, so they can delete an armed row while its transition is still broadcasting. The fix needs an admission fence that waits for active claims, or explicitly preserves records according to the lifecycle operation's semantics; it must also cover separate coordinators sharing the SQLite store.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- coordinator structure and lifecycle methods ---'
ast-grep outline packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs --items all --view expanded
rg -n -C 18 -g '*.rs' \
'\b(clear|unregister_wallet|remove_wallet|purge_all_subwallets|purge_wallet)\b' \
packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs \
packages/rs-platform-wallet/src/wallet
printf '%s\n' '--- ShieldedStore purge and redrive interfaces ---'
ast-grep outline packages/rs-platform-wallet/src/wallet/shielded/store.rs --items all --view expanded
rg -n -C 15 -g '*.rs' \
'trait ShieldedStore|fn (purge_all_subwallets|purge_wallet|arm_redrive|pending_redrives|clear_redrive)' \
packages/rs-platform-wallet/src/wallet/shielded/store.rs \
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs
printf '%s\n' '--- file-backed store opening and SQLite transaction/locking surfaces ---'
rg -n -C 12 -g '*.rs' \
'open_path|Connection|Transaction|transaction|BEGIN|busy_timeout|Mutex|purge_all_subwallets|purge_wallet' \
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs \
packages/rs-platform-wallet/src/wallet/shielded
printf '%s\n' '--- all purge call sites ---'
rg -n -C 10 -g '*.rs' \
'\.(purge_all_subwallets|purge_wallet)\s*\(|(purge_all_subwallets|purge_wallet)\s*\(' \
packages/rs-platform-wallet/srcLength of output: 201590
@thepastaclaw, the repository evidence confirms the clarification.
NetworkShieldedCoordinator::unregister_wallet()takes only its coordinator-locallifecyclemutex, then callsShieldedStore::purge_wallet().NetworkShieldedCoordinator::clear()takes the same local mutex, then callsShieldedStore::purge_all_subwallets()and resets the shared commitment tree.identity_create_from_one_time_key()does not acquirelifecycle.ForeignClaimGuardsis owned by oneNetworkShieldedCoordinator. It cannot coordinate another coordinator.FileBackedShieldedStore::open_path()opens independent SQLite connections to the same database path. The existingpending_connmutex protects only oneFileBackedShieldedStoreinstance.
Therefore, excluding ONE_TIME_CLAIM_RECORDS_ACCOUNT from purge_wallet() or purge_all_subwallets() is not a sufficient fix. It also changes the required full-wipe behavior of wallet removal and clear.
The race remains open. The fix must define lifecycle admission at the shared-store boundary. It must prevent a claim from progressing from record lookup through record arming while clear, unregister_wallet, or remove_wallet obtains destructive admission. It must also wait for claims that already hold admission.
The implementation must cover independent coordinators that use the same SQLite file. A coordinator-local tokio::sync::Mutex cannot satisfy that requirement. The design should not hold a SQLite write transaction during scanning, proof construction, broadcast, or confirmation waits.
🧠 Learnings used
Learnt from: thepastaclaw
Repo: dashpay/platform PR: 4313
File: packages/rs-platform-wallet/src/wallet/shielded/sync.rs:863-871
Timestamp: 2026-08-12T14:18:22.436Z
Learning: In the Rust shielded invitation claim flow, enforce per-FVK single-flight ownership across pending-record lookup, transition construction, atomic arming, broadcast, and finalization. Do not rely on a scan-only guard: concurrent claims must not overwrite the byte-exact durable recovery record. Also account for the fact that Kotlin coroutine cancellation does not currently abort the synchronous native scan.
You are interacting with an AI system.
cargo fmt --check --all gates the Rust workspace tests job; 1e78575 left this line over-width, failing the job in 51s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Status for a resolution pass — head `122ba12b2`. All five open threads addressed with inline replies:
Merge conflict with v4.2-dev resolved (`c74ff1859c`); `ErrorShieldedInviteAlreadyClaimed` renumbered 37→43 to avoid the new DPNS/shield-capacity codes. platform-wallet 865 + FFI 286 tests passing. Ready to resolve where you concur — thanks. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 122ba12, coordinator-scoped scan checkpoints correctly fix cross-chain reuse, but durable claim recovery remains blocked by three lifecycle gaps: claim serialization is coordinator-local, destructive purges can race an armed claim, and recovery is not bound to the original identity slot and keys. The first-scan work-bound issue remains intentionally deferred by the PR discussion.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1004-1027: Prevent Clear from deleting an armed claim before broadcast completes
Claim execution holds a per-FVK mutex, while `clear` and `unregister_wallet` hold the separate coordinator lifecycle mutex. After `arm_one_time_claim_record` releases the store lock, either purge path can acquire that lock and delete the reserved pending-claim row before or during broadcast. The transition may then execute after its only byte-exact recovery record has been erased, making a lost result or process failure unrecoverable for a padded single-note claim. Destructive lifecycle operations must wait for admitted claims, or preserve pending-claim records until those claims settle; the same admission boundary must also account for separate coordinators that share the durable store.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1592-1624: Bind a resumed claim to its original keys and identity slot
The pending record is selected only by wallet and FVK, and it persists neither `identity_index` nor the original key binding. On resume, the stored transition is rebroadcast while recovery receives the current call's `master_key_hash`, `submitted_public_keys`, and `denomination`; afterward the returned identity is registered at the current call's `identity_index`. A retry with different arguments can therefore classify the original identity as belonging to another holder and clear the record, backfill an empty proof result with keys that were not in the stored transition, or register the original identity under the wrong local HD slot. Persist the original slot and claim metadata, then reject mismatched retry arguments or derive the submitted keys from the serialized transition before broadcasting, finalizing, or registering anything.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1731-1733: Serialize concurrent claims before replacing their recovery record
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3767229122)
The new mutex serializes claims only within one `NetworkShieldedCoordinator`. Every `PlatformWalletManager` owns a separate coordinator slot, and separate managers can configure independent coordinators against the same network and SQLite path. Their `ForeignClaimGuards` are unrelated, and each file-backed store maintains its own in-memory `pending_redrives` map, so one coordinator does not even observe a row another coordinator inserted after opening. Two manager handles can therefore both find no record, build different padded transitions, and race the shared table's `INSERT OR REPLACE`, replacing the first transition's only byte-exact recovery record. Scope single-flight ownership to the durable store/chain rather than an individual coordinator, or claim the record atomically in SQLite and make lookup observe the durable row.
| super::shielded::operations::identity_create_from_one_time_key( | ||
| &self.sdk, | ||
| coordinator.store(), | ||
| coordinator.foreign_claim_guards(), | ||
| coordinator.foreign_scan_checkpoints(), | ||
| self.wallet_id, | ||
| one_time_sk, | ||
| funding_birth_height, | ||
| &change_address, | ||
| public_keys, | ||
| denomination, | ||
| send_to_address_on_creation_failure, | ||
| identity_signer, | ||
| &prover, | ||
| ) | ||
| .await?; | ||
|
|
||
| // Register the proof-verified identity in the local manager at its HD | ||
| // slot — the SAME tail as `shielded_identity_create_from_pool`. The | ||
| // broadcast already succeeded; a registration failure here is logged and | ||
| // swallowed (the identity exists on chain; the next sync heals the row). | ||
| { | ||
| let mut wm = self.wallet_manager.write().await; | ||
| match wm.get_wallet_info_mut(&self.wallet_id) { | ||
| Some(info) => { | ||
| if let Err(e) = info.identity_manager.add_identity( | ||
| identity, | ||
| identity_index, | ||
| self.wallet_id, | ||
| &self.persister, | ||
| ) { | ||
| tracing::warn!( | ||
| identity_index, |
There was a problem hiding this comment.
🔴 Blocking: Bind a resumed claim to its original keys and identity slot
The pending record is selected only by wallet and FVK, and it persists neither identity_index nor the original key binding. On resume, the stored transition is rebroadcast while recovery receives the current call's master_key_hash, submitted_public_keys, and denomination; afterward the returned identity is registered at the current call's identity_index. A retry with different arguments can therefore classify the original identity as belonging to another holder and clear the record, backfill an empty proof result with keys that were not in the stored transition, or register the original identity under the wrong local HD slot. Persist the original slot and claim metadata, then reject mismatched retry arguments or derive the submitted keys from the serialized transition before broadcasting, finalizing, or registering anything.
source: ['codex']
3 participants
Continues #4204 — moved from a fork branch to an in-repo branch (rebased onto v4.2-dev post-#4305) so maintainers can push changes directly, per review request. Full review history on #4204.
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