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 27 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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 shielded-invite claim API can defer invalid wallet identifiers to a later JNI failure instead of rejecting them at the SDK boundary. The PR is otherwise mergeable with explicit owner awareness and a follow-up to add the missing length validation. 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 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 dd73cdb) |
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% 85.21% -2.29%
============================================
Files 2672 2712 +40
Lines 340400 356304 +15904
============================================
+ Hits 297819 303608 +5789
- Misses 42581 52696 +10115
🚀 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.
There was a problem hiding this comment.
This was valid at the commented revision but is fixed at the current head. ForeignScanCheckpointCache is now a field of NetworkShieldedCoordinator rather than a process-global static, so separate SDK chains—including distinct devnets represented by the same network enum—use separate cache instances. The cross-cache isolation test verifies that a checkpoint saved for an FVK in one coordinator is absent from another.
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
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']
There was a problem hiding this comment.
Fixed in 847f0a1 — the resume now DERIVES its binding from record.st_bytes and demotes the call's arguments to assertions, refusing outright when they disagree.
I evaluated both options you offered and went with derive-plus-reject rather than persisting the metadata:
- The transition already is the binding. Its
public_keysare exactly what the binding signature committed to and itsdenominationis the value that leaves the pool, so deriving needs no record-schema change and — unlike a separately persisted copy — cannot drift from what actually went on the wire. Resuming means re-broadcasting those bytes, so the identity that results belongs to those keys, and reading them from anywhere else is the bug. - Persisting would mean adding a field to
PendingRedrive(shared with the ordinary spend-redrive path, where it is meaningless) plus anALTER TABLEonshielded_pending_spends, for a value that is already recoverable.
What happens now. resume_one_time_claim rebuilds submitted_public_keys, the MASTER auth key hash and denomination from the deserialized transition. one_time_claim_binding_mismatch compares them against the caller's, and any disagreement returns the new PlatformWalletError::ShieldedClaimBindingMismatch — before the spent-nullifier probe and before the re-broadcast. Nothing is resubmitted, no proof is burned, and finalize_one_time_claim_record does not clear on this variant, so the record survives for a retry that presents the original arguments. Past the gate the derived values are what drive recovery, the empty-proof-result backfill and the broadcast, so the transition stays the source of truth even if the check is ever relaxed.
Key comparison is whole-set and by content, not by id — IdentityPublicKey's Eq covers purpose, security level, key type, read-only, contract bounds and key data — so the dangerous shape (same key ids, swapped key material, which is what would register a foreign identity at this wallet's slot) is caught. Pure reordering is not a mismatch; both sides are BTreeMaps keyed by key id.
identity_index, explicitly. It is a purely local DIP-9 placement and appears nowhere in the transition, so there is nothing in st_bytes to derive it from. It is bound transitively: the identity's keys are derived from the wallet seed at that slot, so a retry naming a different slot presents different keys and is refused by the check above. That leaves exactly one uncovered case — a caller pairing slot i with keys derived at slot j — which violates the same caller contract a first attempt relies on and mis-slots identically. So the resume path is now no weaker than a fresh claim, which is the strongest guarantee available without persisting the slot. This is stated in the resume_one_time_claim docs rather than left implicit; if you want it airtight rather than transitive I will add the persisted slot and the migration.
Tests (one_time_claim_evidence_tests):
claim_binding_is_recoverable_from_the_stored_transition— the derive path: a serialize/deserialize round-trip reproduces the exact key set, the denomination and the master key hash.matching_retry_arguments_resumeandkey_order_is_not_a_binding_mismatch.mismatched_retry_arguments_are_refused_per_field— key set (asserting first that the swap keeps the key ids identical, so ids alone would not have caught it), master hash, denomination.mismatched_retry_refuses_without_broadcasting_or_clearing_the_record— end to end throughresume_one_time_claimover a realFileBackedShieldedStore, with a bare mock SDK carrying no expectations: reaching the nullifier probe or the re-broadcast would surface as something other than this error. Asserts the pending record is still there and byte-identical afterwards.
880 tests green; cargo fmt --check --all and cargo clippy --all-targets --all-features --locked -- --no-deps -D warnings clean.
… aliases cargo clippy --workspace -D warnings failed the Rust workspace tests job in 51s on clippy::type_complexity at the ForeignClaimGuards entries field (introduced by 6668061). Name the guard handle and the registry row so the field reads as Vec<ClaimGuardEntry>; no behaviour change.
Note on the
|
…nsition The durable pending-claim record is found by wallet id and one-time FVK alone, so nothing about the lookup says which identity the earlier attempt was creating. `resume_one_time_claim` nevertheless re-broadcast the STORED transition while handing recovery, the empty-proof-result backfill and the result classification this call's `master_key_hash`, `submitted_public_keys` and `denomination` — and the caller then registered the returned identity at this call's `identity_index`. A retry with different arguments could therefore classify the original identity as another holder's and clear the record (permanently stranding a padded single-note claim, whose declared id embeds a random dummy nullifier and exists nowhere else), backfill an empty proof result with keys that were never in the transition, or register the original identity at the wrong local HD slot (#4313, finding 195efdd4ae21). The binding is now DERIVED from `record.st_bytes` rather than taken from the call: the transition's `public_keys` are exactly what the binding signature committed to and its `denomination` is the value that leaves the pool, so the transition is the authoritative statement of what was submitted. Deriving needs no record-schema change and — unlike a separately persisted copy — cannot drift from what actually went on the wire. The caller's arguments are demoted to assertions. Any disagreement in the key set (compared whole, by id AND content, so a same-ids key-material swap is caught), the MASTER auth key hash that idempotent recovery probes Platform with, or the denomination fails closed with the new `PlatformWalletError::ShieldedClaimBindingMismatch` — checked BEFORE the spent-nullifier probe and the re-broadcast, so a mismatched retry burns no proof, makes no chargeable resubmission, and leaves the record intact for a retry that presents the original arguments. Key ORDER is not a mismatch; both sides are `BTreeMap`s keyed by key id. `identity_index` is not in the transition and cannot be derived from it. It is bound transitively: the identity's keys are DIP-9-derived at that slot, so a retry naming a different slot presents different keys and is refused. The one uncovered case — a caller pairing slot i with keys derived at slot j — violates the same contract a FIRST attempt relies on and mis-slots identically, so the resume path is now no weaker than a fresh claim. This is stated explicitly in the fn docs rather than left implicit. Tests: the derive path (a serialize/deserialize round-trip reproduces the exact key set, denomination and master key hash), matching-args resume, key order not being a mismatch, per-field refusal, and an end-to-end refusal through `resume_one_time_claim` over a real `FileBackedShieldedStore` that asserts the pending record survives untouched and that no network work was reached. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…te boundary `purge_wallet` runs `DELETE FROM shielded_pending_spends WHERE wallet_id = ?1` and `purge_all_subwallets` deletes unfiltered, so `clear` / `unregister_wallet` / `remove_wallet` could delete the pending-claim record of a one-time-key claim whose transition was still broadcasting — and for a padded single-note bundle that record is the ONLY handle to the created identity (its declared id embeds a per-build random dummy nullifier), so the identity is then unrecoverable. Nothing in the process could order the two. The destructive paths take the coordinator's `lifecycle` mutex; `identity_create_from_one_time_key` takes none of it, and could not usefully be made to: `FileBackedShieldedStore::open_path` opens independent SQLite connections to the same file, so two coordinators — or two processes — share the records but no in-process lock, and the per-FVK `ForeignClaimGuards` are owned by one coordinator. Excluding `ONE_TIME_CLAIM_RECORDS_ACCOUNT` from the purges was considered and rejected: it does not stop a purge racing an arming claim, and it breaks `remove_wallet`'s full-wipe contract. Admission therefore moves to the only thing the two sides actually share — the store — as five `ShieldedStore` methods over a `shielded_lifecycle_admission` table in the same SQLite file: * a claim takes a LEASE, refused if a barrier already covers its wallet; * a destructive operation installs a BARRIER, which blocks new leases and reports the leases already live in scope, then waits for that count to reach zero; * the claim arms its record UNDER its lease (`arm_redrive_under_claim`), which re-checks and re-stamps the lease in the same atomic step, leaving no gap between "still admitted" and "record written". Correctness: both entry points are single `BEGIN IMMEDIATE` transactions, and SQLite admits one writer at a time across every connection and every process on the file, so they are totally ordered — lease first means the barrier counts it and the purge waits; barrier first means the lease is refused. There is no interleaving in which both proceed. `InMemoryShieldedStore` holds the same table in memory, where the shared object is the store behind one `RwLock` and the write guard supplies the same order. No transaction is held across scanning, proof construction, broadcast, or a confirmation wait. Failure is closed in both directions. `clear()` propagates `ShieldedLifecycleBusy` — its contract is that the host wipes its own rows only on `Ok`, so reporting success over an untouched store would desynchronize them. `unregister_wallet` skips the purge and warns, matching its existing contract, which already tolerates a purge that did not happen. A claim refused admission has scanned, built and broadcast nothing. Leases and barriers carry expiries because a holder can die (process kill, cancelled JNI call) with no chance to release. Expiry is a liveness backstop only: it never removes a LIVE admission, so it cannot let a purge delete a record under a running claim — it only bounds how long a dead claim blocks wallet removal and a dead purge blocks claims. The claim body is split into `one_time_claim_admitted` purely so the lease is released on every exit, including the many `?` paths, without threading a release through each. Tests: the cross-INSTANCE cases are the ones that matter, and each opens two `FileBackedShieldedStore`s on one file — barrier in A refuses a claim in B, a live lease in A is counted by B's barrier, and an armed record survives a concurrent purge attempt and is only wiped once the claim releases (proving the fence is a fence, not an exemption). Plus: arming refuses and writes nothing without a lease (verified durably through a cold reopen), expired leases stop blocking, scope matches the operation, and coordinator-level `clear()` / `unregister_wallet` refusal-and-retry under a paused clock. Reverting either half of the fence — the barrier check or the drain wait — fails four of them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dmission The refusal path returns without committing and relies on rusqlite's `Transaction` drop to roll back; the comment claimed an explicit rollback. Also spells out that the reap performed earlier in the same transaction is rolled back with it, which is harmless because the next admission call reaps again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head dd73cdb, the coordinator-scoped checkpoint cache correctly prevents cross-network reuse, but durable claim recovery still has three blocking gaps: independent coordinators can replace each other's recovery record, an expired lease can permit a purge while its claim remains live, and the recovered identity can be registered under a different local identity index. The first scan of each fresh, evicted, or post-restart unfunded invitation key also remains unbounded.
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
3 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:856-876: Prevent Clear from deleting an armed claim before broadcast completes
The SQLite fence stops protecting a live claim five minutes after `arm_redrive_under_claim`, because that operation re-stamps the lease once and no later network phase renews it. Every destructive-admission poll reaps expired rows before counting live claims, so after that deadline `clear` or `unregister_wallet` can see zero claims and purge the armed recovery row while the original future is still running. The post-arm path can exceed five minutes: it broadcasts, waits using `wait_for_affected_state(..., None)`, and may perform two nested four-attempt identity recovery loops whose individual fetches have their own request retries; suspension or a forward wall-clock adjustment has the same effect. Renew the lease throughout all post-arm broadcast, confirmation, and recovery phases, or use durable ownership semantics where a fixed wall-clock expiry alone cannot authorize deletion while the holder remains active.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1717-1748: Serialize concurrent claims before replacing their recovery record
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3767229122)
The per-FVK mutex is owned by one `NetworkShieldedCoordinator`, while `begin_claim_admission` only fences claims against destructive operations. It permits multiple claim leases for the same wallet/FVK. Two coordinators or processes sharing one SQLite path can therefore acquire independent leases, and each `FileBackedShieldedStore::pending_redrives` lookup reads its separately rehydrated in-memory map rather than querying the current SQLite row. Both claims can observe no record, construct different padded transitions, and call `arm_redrive_under_claim`; its durable write uses `INSERT OR REPLACE` for the same claim-record key, so the second claim replaces the first claim's only byte-exact recovery record. Reserve the claim-record identity atomically at the SQLite boundary, reject or resume an existing durable reservation, and make lookup observe the durable row rather than a stale per-instance cache.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1613-1621: Bind a resumed claim to its original keys and identity slot
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3771541508)
The stored-transition checks now bind the key set, master-key hash, and denomination, but `identity_index` is still neither persisted nor passed into `resume_one_time_claim`. The public Rust, FFI, JNI, and Kotlin APIs accept the index independently from caller-supplied key rows and do not verify that those rows were derived from that slot. A retry can therefore present the exact original keys and denomination with a different `identity_index`, pass `one_time_claim_binding_mismatch`, recover or rebroadcast the stored transition, and register the resulting identity under the retry's slot here. Persist the original identity index with the claim record and reject a mismatched retry index before any recovery, broadcast, or local registration.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:1024-1032: Unfunded invitation keys force an unbounded full-history scan
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3725876429)
The coordinator-owned checkpoint only reduces repeat work after a scan of the same FVK has progressed and while that entry remains among the eight cached keys. A fresh, evicted, or post-restart syntactically valid invitation still begins at position zero and consumes the proof-verified note stream through the current tip when it is unfunded or underfunded. Because the public claim API accepts untrusted invitation material and provides no native cancellation input, per-call chunk or work budget, retained-note budget, or authenticated starting position, callers can rotate valid unfunded keys to repeatedly consume bandwidth, proof-verification CPU, memory, battery, and synchronous JNI workers. Add native cancellation and a strict resumable first-scan work bound, or another funds-safe bound suitable for untrusted invitation input.
| let started = tokio::time::Instant::now(); | ||
| loop { | ||
| // Re-taking the barrier each pass also REFRESHES its expiry, so a | ||
| // long drain cannot let the barrier lapse and admit a new claim. | ||
| let live = { | ||
| let mut store = self.store.write().await; | ||
| store | ||
| .begin_destructive_admission( | ||
| scope, | ||
| token, | ||
| admission_now_ms(), | ||
| DESTRUCTIVE_BARRIER_MS, | ||
| ) | ||
| .map_err(|e| { | ||
| crate::error::PlatformWalletError::ShieldedStoreError(format!( | ||
| "{operation}: could not take lifecycle admission: {e}" | ||
| )) | ||
| })? | ||
| }; | ||
| if live == 0 { | ||
| return Ok(token); |
There was a problem hiding this comment.
🔴 Blocking: Prevent Clear from deleting an armed claim before broadcast completes
The SQLite fence stops protecting a live claim five minutes after arm_redrive_under_claim, because that operation re-stamps the lease once and no later network phase renews it. Every destructive-admission poll reaps expired rows before counting live claims, so after that deadline clear or unregister_wallet can see zero claims and purge the armed recovery row while the original future is still running. The post-arm path can exceed five minutes: it broadcasts, waits using wait_for_affected_state(..., None), and may perform two nested four-attempt identity recovery loops whose individual fetches have their own request retries; suspension or a forward wall-clock adjustment has the same effect. Renew the lease throughout all post-arm broadcast, confirmation, and recovery phases, or use durable ownership semantics where a fixed wall-clock expiry alone cannot authorize deletion while the holder remains active.
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