feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim) - #4313
Open
bfoss765 wants to merge 41 commits into
Open
feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim)#4313bfoss765 wants to merge 41 commits into
bfoss765 wants to merge 41 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>
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 6, 2026 01:52
Contributor
|
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bfoss765
changed the title
Collaborator
|
⛔ Blockers found — Opus deferred (commit 9000c1e) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4313 +/- ##
============================================
+ Coverage 84.39% 84.82% +0.43%
============================================
Files 2723 2757 +34
Lines 359568 364531 +4963
============================================
+ Hits 303450 309215 +5765
+ Misses 56118 55316 -802
🚀 New features to boost your workflow:
|
Collaborator
|
@bfoss765 I diagnosed the current I amended the introducing commit to remove only the unused
Replacement head: thepastaclaw@1ea5340 (branch I cannot update |
thepastaclaw
suggested changes
Aug 6, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
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.
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>
`AdmissionToken::new` drew its 16 bytes with `OsRng::fill_bytes`, whose
`rand_core` contract PANICS when the OS entropy source fails. Both
production callers sit inside futures reached from `#[no_mangle] extern "C"`
exports — the claim path through
`platform_wallet_manager_shielded_identity_create_from_one_time_key`, the
barrier path through the destructive lifecycle exports — and a panic there
is re-raised by `block_on_worker`'s `expect("tokio worker panicked")`, where
it cannot unwind across the C ABI and aborts the host process before the JNI
panic guard runs.
`generate_one_time_orchard_key` already avoids this with `try_fill_bytes`;
admission tokens now do the same. `AdmissionToken::generate()` is fallible
and reports `PlatformWalletError::Persistence` — the class both call sites
already map the rest of the admission step to, so the host sees one error
for "the admission could not be taken" regardless of which half failed.
The infallible paths are gone from production: `Default` is removed (it had
no callers) and `new()` is `#[cfg(test)]`-only, so nothing can reintroduce
the panic by reaching for a convenience constructor.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…4306) A syntactically valid but never-funded invitation key drove `scan_notes_for_foreign_key` genesis-to-tip: "no note yet" and "note further ahead" are indistinguishable mid-stream, so a hostile invite link cost an unbounded trial-decryption walk per claim attempt. The resume checkpoint (2dc71c9) bounded REPEATED scans, but the first attempt — and every attempt against a growing tree — had no stopping rule. Each attempt now consumes at most FOREIGN_SCAN_BATCH_BUDGET (128) stream batches (a batch is ≥ one 2048-note MMR chunk, so ≥ ~260k trial decryptions — generously past any realistic honest claim). Exhausting the budget before the value is covered checkpoints the position reached and returns the NEW retryable typed error `ShieldedForeignScanBudgetExhausted { scanned_through }` — attempts compound toward a genuinely deep note while each stays bounded, no matter what the link claims. A partial (buffer) batch is end-of-stream and never trips the budget, so an exhausted tree still returns Ok as before. The invite's birth-height hint cannot replace this bound: heights don't map to tree positions (no height→position oracle on this tree), and the hint arrives in the attacker-controlled link anyway. Surface: FFI code 44 `ErrorShieldedScanBudgetExhausted` (next frontier past the frozen 43) → Kotlin `DashSdkError.PlatformWallet.ShieldedScanBudgetExhausted` with `isRetryable = true` — the polarity is the entire host contract: render "still searching — retry", never "invalid/unfunded/already claimed". The consumption loop is extracted into a stream-generic `scan_foreign_stream_with_budget` so the budget/checkpoint behavior is unit-tested without a network: pause-checkpoints-and-resume, the partial-batch exemption, and the pre-existing error-path checkpointing. Claim admission already releases on every error exit, so the pause cannot wedge a lease. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 17, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 78e5969, admission-token entropy failures and cross-network checkpoint reuse are fixed, but three carried-forward claim recovery defects remain. The new scan budget also loses its retryable result at the actual claim FFI entry point, while lifecycle contention is similarly flattened to a non-retryable generic Kotlin error.
Source: reviewers codex/general=gpt-5.6-sol, codex/security-auditor=gpt-5.6-sol, codex/rust-quality=gpt-5.6-sol, and codex/ffi-engineer=gpt-5.6-sol; final verifier codex/verifier=gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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)
🔴 4 blocking | 🟡 1 suggestion(s)
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-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1034-1037: Preserve the retryable scan-budget error across the claim FFI
`ShieldedForeignScanBudgetExhausted` has a dedicated blanket conversion to native code 44, and Kotlin maps code 44 to retryable `ShieldedScanBudgetExhausted`. This entry point bypasses that conversion: only `ShieldedInviteAlreadyClaimed` reaches `e.into()`, while the catch-all converts budget exhaustion to generic `ErrorWalletOperation` code 6. A valid invitation whose note lies beyond the first 128 batches is therefore surfaced as a non-retryable generic failure even though its checkpoint requires another invocation to continue. Route this variant through the typed conversion and add an entry-point-level test that asserts code 44.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1034-1037: Preserve lifecycle-busy retryability across Kotlin
`PlatformWalletError::ShieldedLifecycleBusy` explicitly documents both contention outcomes as retryable, and a refused claim has not scanned, built, or broadcast anything. The same catch-all converts this variant to generic native code 6, which Kotlin maps to `PlatformWallet.Generic` with `isRetryable == false`. Ordinary contention with clear, unregister, or removal therefore loses its intended retry contract at the language boundary. Assign this variant a stable FFI result code and a corresponding retryable Kotlin error type instead of flattening it.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1717-1753: Serialize concurrent claims before replacing their recovery record
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3767229122)
The per-FVK guard is still coordinator-local, and `begin_claim_admission` only rejects destructive barriers; it permits multiple ordinary claim leases for the same wallet and invitation. Two coordinators or processes sharing one SQLite file can therefore both be admitted. Each file-backed store then performs the pending-record lookup through its separately hydrated in-memory `subwallets` map, so both can observe no record and construct different padded transitions. `arm_redrive_under_claim` subsequently uses `INSERT OR REPLACE` for the shared record key, allowing the later claim to replace the first transition's only byte-exact recovery record while the first is broadcasting. If the first transition executes and its result is lost, its randomized padded identity ID is no longer recoverable. Reserve the invitation's claim-record key atomically in SQLite, query the durable row, and make competing claimants resume or reject the existing reservation rather than replacing it.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:860-876: Prevent Clear from deleting an armed claim before broadcast completes
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3777986987)
The destructive-admission loop reaps expired claim leases before counting them, but renewal still protects only a freshly built claim's post-arm broadcast. When a pending record exists, `one_time_claim_admitted` awaits `resume_one_time_claim`, finalizes, and returns at operations.rs:1858-1875 before reaching the renewal loop at operations.rs:2028-2071. Resume may perform nullifier queries, repeated identity recovery, byte-identical rebroadcast, and an unbounded confirmation wait under only the initial five-minute lease. Once it expires, `clear` or `unregister_wallet` can observe no live claim and purge the recovery row while resume remains active. Run the renewal heartbeat around the complete admitted claim body, including pending-record recovery and rebroadcast.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1617-1621: Bind a resumed claim to its original keys and identity slot
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3771541508)
Resume now binds the denomination, master-key hash, and complete public-key set to the stored transition, but it explicitly does not bind `identity_index`. The public Rust, C, JNI, and Kotlin APIs accept the index independently from the key rows, so a retry can present the exact original keys with a different index, pass every transition-derived check, recover or rebroadcast the original claim, and register the identity under the retry's slot here. This is not harmless: `IdentityManager::add_identity` rejects duplicate identity IDs but inserts into the index map without rejecting an identity already occupying that slot, leaving incorrect HD metadata and stale side-index state. Persist the original identity index with the pending claim and reject a mismatch before network recovery, rebroadcast, or local registration.
…t the claim entry point `platform_wallet_manager_shielded_identity_create_from_one_time_key` routed only `ShieldedInviteAlreadyClaimed` through the blanket `From<PlatformWalletError>` conversion. Every other typed variant fell to the catch-all, which rewrites the code to the generic `ErrorWalletOperation` (6). `ShieldedForeignScanBudgetExhausted` is one of those. It has a blanket conversion to `ErrorShieldedScanBudgetExhausted` (44), which the Kotlin mirror maps to the RETRYABLE `PlatformWallet.ShieldedScanBudgetExhausted` — but the host never saw 44 from this entry point, only 6. A paused scan therefore rendered as a hard, non-retryable failure, which strands a genuinely funded invitation whose note sits deep in the tree: the scan had simply not looked far enough yet, and progress is checkpointed so the retry is cheap. Widen the pass-through arm to cover both typed variants, and split the classification out of the `unsafe extern "C"` body into `map_one_time_claim_result` so the code split is reachable from a unit test without a live manager handle — the same shape `map_spend_result` already uses for the spend entry points. `Some(identity_id)` is now the only channel that writes `out_identity_id`, so the "written on Success and on ErrorShieldedBroadcastUnconfirmed only" contract is decided in one place. Tests (both new, at the FFI entry point rather than at the blanket conversion): * `map_one_time_claim_result_pins_the_retryable_scan_budget_code` — 44, no identity id written, checkpoint position preserved in the message. * `map_one_time_claim_result_pins_the_terminal_and_unconfirmed_codes` — 43 writes no id, 17 does write one, unrelated variants still flatten. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ped retryable code (45) `ShieldedLifecycleBusy` had no FFI code of its own, so it reached hosts as the generic `ErrorWalletOperation` (6) — a non-retryable classification — through both the blanket conversion's catch-all and the claim entry point's. That is the wrong polarity in both of the variant's directions: * a one-time-key claim refused because a Clear / wallet removal holds destructive admission over its wallet, or because another claimant holds this invitation's claim-record key, scanned/built/broadcast nothing; * a Clear / wallet removal refused because in-flight claims did not drain purged nothing. Both are contended-lifecycle refusals that resolve by waiting a moment and retrying, and both consumed nothing. Surfaced as code 6 they read as a hard failure of the invitation itself. Allocate `ErrorShieldedLifecycleBusy = 45` — the next free integer past this PR's 44, taken from the registry frontier and NOT from a vacated gap (28, 30, 32 and 33 are RESERVED, not reissuable). Add its blanket conversion arm, add it to the claim entry point's pass-through arm, and mirror it in the Kotlin SDK as `DashSdkError.PlatformWallet.ShieldedLifecycleBusy` with `isRetryable = true` and a `45 ->` mapping — the same shape 44 -> `ShieldedScanBudgetExhausted` uses. The Swift mirror and the ERROR_CODE_REGISTRY.md row (the registry lives on docs/ffi-error-code-registry, #4318) are FOLLOW-UPS, not part of this commit. Tests: * Rust `shielded_invite_codes_are_pinned_at_43_through_45` — the numeric ABI, pinned like the marketplace block, since nothing checks it across the language boundary at compile time. * Rust `shielded_lifecycle_busy_maps_to_its_own_retryable_code` — both directions land on 45, Display payload preserved verbatim. * Kotlin `platformWalletCodesMapToPlatformWalletSubtree` — code 45 maps to the typed class and `isRetryable` is true, mirroring the code-44 assertions immediately above it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y, in SQLite
Concurrent-claim admission was only ever serialized per-FVK inside ONE
coordinator, by `ForeignClaimGuards`' async mutex. The claim LEASE next to
it is per-WALLET: it orders a claim against a purge and nothing else, so
it admits both claimants of one invitation quite happily.
That leaves the case the mutex cannot see. Two coordinators — or two
processes — open independent `FileBackedShieldedStore` connections to the
same SQLite file and share no in-process lock at all. Both then found no
pending record, built transitions with DIFFERENT padded identity ids (the
padding action's dummy nullifier is random per build), and the second
`arm_redrive_under_claim` — an `INSERT OR REPLACE` — silently overwrote
the first's byte-exact recovery row while the first's transition was
already on the wire. That row is the ONLY handle that recovers a padded
single-note claim, so the identity it created was stranded forever.
Reserve the invitation itself, where the contention actually is:
* new store method `reserve_one_time_claim_key`, contract: the
insert-if-absent and the read-back of the durable row are ONE atomic
step, and a live row is never overwritten. Returns `Acquired` (this
token owns the key; idempotent on re-entry by the same token) or
`Held { holder, expires_at }` read off the durable row — so exactly one
concurrent caller can see `Acquired`.
* `FileBackedShieldedStore`: a new `shielded_one_time_claim_reservation`
table keyed `(wallet_id, claim_record_key)`, written by
`INSERT ... ON CONFLICT DO NOTHING` inside `BEGIN IMMEDIATE`. SQLite
admits one writer at a time across every connection AND every process
on the file, so "exactly one `Acquired`" is a total order, not a
probability. `ON CONFLICT DO NOTHING`, never `OR REPLACE`: losing the
insert must leave the winner's row byte-for-byte untouched.
* `InMemoryShieldedStore`: the same semantics under its `&mut self` step.
* `arm_redrive_under_claim` now REFUSES, writing nothing, when a live
reservation covers `(wallet_id, activity_id)` under a different token —
in the same transaction that would have done the overwrite. This is
what makes the clobber structurally impossible rather than merely
unreachable. Ordinary spend redrives take no reservation, so the gate
is a no-op for them.
* the reservation is bound to its lease for its whole life: re-stamped by
`renew_claim_admission` and by `arm_redrive_under_claim`, released by
`end_claim_admission` in the same transaction as the lease, and
otherwise reaped by expiry — a claimant that died cannot hold an
invitation hostage past `CLAIM_LEASE_MS`.
At the wallet layer, `identity_create_from_one_time_key` takes the
reservation right after the lease and passes ownership down. A claimant
that did NOT win it may only RESUME or REFUSE, never replace: the durable
pending-record resume runs first and returns as usual, and if no record
exists yet the claim stops with the retryable
`ShieldedLifecycleBusy` before the transient scan — nothing scanned,
built or broadcast, note untouched. The in-process guard stays exactly
where it was, as the fast path.
Tests:
* `two_store_instances_cannot_both_claim_one_invitation` — two
`FileBackedShieldedStore` handles on ONE file, both admitted by the
per-wallet lease; one acquires, the loser is handed the winner's token,
the loser's arm is refused, and the winner's `st_bytes` survive
byte-for-byte across a cold reopen.
* `a_claim_key_reservation_lives_and_dies_with_its_lease` — idempotent
re-entry, renewal carries the hold past the original expiry, release
hands the invitation over immediately, a dead holder ages out.
* `the_claim_key_gate_leaves_unreserved_redrives_alone` — ordinary spend
redrives are unaffected.
* `only_one_claimant_acquires_an_invitations_claim_key` — the in-memory
backend gives the identical answer, since the wallet layer branches on
it and cannot tell the backends apart.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t and bind it to its identity slot Two review findings on the same code path — the RESUME branch of a one-time-key claim. They are grouped because their edits interleave in `identity_create_from_one_time_key` / `resume_one_time_claim`; each is described separately below. --- 1. The renewal heartbeat now wraps the COMPLETE admitted claim --- The 5-minute claim lease's renewal heartbeat wrapped only the fresh-build path's broadcast. The RESUME path returns before that is ever reached, so everything it does — pending-record lookup, nullifier queries, repeated identity recovery, re-broadcast of the stored transition, and an UNBOUNDED confirmation wait — ran under the initial lease alone. Resume is if anything the slower branch: it is the path a claim takes precisely because the previous attempt could not resolve quickly. Outrun the lease and it is reaped, at which point a concurrent purge counts zero live claims and deletes the very record the in-flight claim needs to recover. Hoist the heartbeat into the new `under_renewed_claim_lease`, wrapping the whole `one_time_claim_admitted` call. Both paths are now covered by construction, and there is no deeper place that could cover both — they only converge at that call site. The inner loop is gone; the broadcast is a plain await again. The claim-key reservation taken under the same token rides along, since `renew_claim_admission` re-stamps it in the same step. `CLAIM_LEASE_MS`'s doc no longer claims the protected window runs from the arm: it now bounds the gap between renewals, not the length of a claim. Tests: `a_resume_shaped_body_run_bare_lets_its_lease_lapse` (the bug — same body, no heartbeat, lease lapses), `the_heartbeat_keeps_a_resume_shaped_body_s_lease_live` (the fix, same body and clock, opposite outcome), and `the_heartbeat_also_holds_the_claim_key_reservation`. --- 2. `identity_index` is persisted with the pending claim and checked --- Ground truth first, because the PR carried two contradictory claims about this: the pending record did NOT persist the identity index. `PendingRedrive` had five fields (activity_id, anchor, nullifiers, st_bytes, attempts), the `shielded_pending_spends` table had no such column, and `identity_index` never even reached `shielded::operations` — it was consumed only in `platform_wallet.rs` after the claim returned. The in-tree doc on `resume_one_time_claim` said so explicitly: "Not directly bound: the caller's `identity_index` … It is bound *transitively*." That transitive argument — different slot implies different keys implies the key check catches it — breaks for a retry that presents the ORIGINAL keys at a different slot, and the consequence is not symmetric with a first attempt's. `IdentityManager::add_identity` rejects a duplicate identity id but inserts into an OCCUPIED slot without complaint, so such a retry silently displaces whatever identity the wallet tracked there. So persist it and check it: * `PendingRedrive` gains `identity_index: Option<u32>` — `None` for ordinary spend redrives, which register no identity. * `shielded_pending_spends` gains a NULLABLE `identity_index` column. This store versions its schema by `CREATE TABLE IF NOT EXISTS`, so the matching idempotent form for a new column is a `PRAGMA table_info` probe plus `ALTER TABLE ADD COLUMN`. No default and no back-fill: an existing record genuinely does not know its slot, and `NULL` says exactly that rather than manufacturing a value a resume would enforce. * `identity_index` is threaded from `PlatformWallet` down to `arm_one_time_claim_record`, and `one_time_claim_binding_mismatch` compares it first. It is the one field compared against the RECORD rather than re-derived from `st_bytes`, because the transition cannot witness a purely local DIP-9 placement — which is exactly why it has to be persisted. A mismatch is the existing typed `ShieldedClaimBindingMismatch`, refused before any network work with the record left intact for a correct retry. * A `None` (pre-migration) record skips the check and keeps the transitive binding it was written under, so an upgrade cannot strand a claim that is mid-flight across it. Tests: `a_resume_at_a_different_identity_index_is_refused` (everything else byte-identical — the case the transitive argument could not cover), `a_pre_migration_record_still_resumes_at_any_index`, `resuming_at_a_mismatched_identity_index_fails_closed` (end to end on the file store: arm at slot N, resume at N+1, typed refusal, record intact, slot still there after a cold reopen), `a_pre_migration_database_gains_the_identity_index_column`, and `a_claim_records_identity_index_survives_a_reopen`. `IdentityManager::add_identity`'s own occupied-slot tolerance is left alone deliberately — hardening it affects every identity registration path in the wallet, well outside this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 19, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 484233a, four of the five prior findings are fixed, but the pending-claim lookup still reads a stale per-store cache after reservation acquisition, so a second store can replace an unresolved recovery record. Two additional blockers remain: the correctness-critical recovery row is committed through WAL with synchronous=NORMAL, and wallet removal reports success after unregister skips its required purge; three scoped robustness issues also remain.
Source: reviewers codex/general=gpt-5.6-sol, codex/security-auditor=gpt-5.6-sol, codex/rust-quality=gpt-5.6-sol, and codex/ffi-engineer=gpt-5.6-sol; final verifier codex/verifier=gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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)
🔴 3 blocking | 🟡 3 suggestion(s)
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/file_store.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:106-107: Commit pending claim records with durable SQLite synchronization
`pending_conn` stores the byte-exact one-time-claim recovery record but is opened through `open_tuned_connection`, which configures WAL mode with `synchronous=NORMAL`. A NORMAL-mode WAL commit can return before the WAL is synchronized, so a system crash or power loss can discard the recent `arm_redrive_under_claim` insert after the transition has already been broadcast. Unlike the rebuildable commitment-tree data used to justify NORMAL, a single-note claim's row contains the randomized padded identity ID and cannot be reconstructed. Configure the recovery connection with `synchronous=FULL`, or use an equivalent durable commit/checkpoint protocol before broadcasting.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:194-206: Serialize the identity-index schema migration across processes
The identity-index migration probes for the column and performs `ALTER TABLE` as separate operations. Two processes opening the same pre-migration database can both observe the column as absent; after one adds it, the other's `ALTER TABLE` fails with a duplicate-column error and causes `open_path` to fail. Hold an IMMEDIATE transaction from the schema probe through the ALTER so the second opener probes only after the first migration commits.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:783-805: Do not complete wallet removal after skipping its shielded-state wipe
`unregister_wallet` clears the account, persister, and hydration registrations before acquiring destructive admission, then only logs when claims do not drain within the timeout. `remove_wallet_with_teardown` continues removing the wallet and reports success even though `purge_wallet` never ran. A long claim can therefore leave decrypted notes, watermarks, activity, and an unresolved pending-claim row on disk after the host was told the wallet was removed; the logged instruction to retry is ineffective because a second removal returns `WalletNotFound`. Make unregister/removal fallible and abort before deleting registrations when admission cannot be acquired, or make a guaranteed deferred purge part of the removal completion contract.
In `packages/rs-platform-wallet/src/wallet/shielded/keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/keys.rs:50-64: Constrain the unsafe scrub wrapper to audited secret types
`ScrubOnDrop<T>` is a safe generic wrapper whose `Drop` implementation overwrites any no-drop `T` bytewise. `needs_drop::<T>() == false` only proves that no destructor needs the old representation; it does not prove that an all-zero representation is valid for `T`. Because the tuple constructor is crate-visible, a future safe call site could wrap a reference, `NonZero*`, or another invariant-bearing no-drop type and make the unsafe block violate that type's validity requirements. Make the wrapper specific to the two audited Orchard key types, or require a private sealed unsafe marker implemented only for representations that have been audited for this operation.
In `packages/rs-platform-wallet-ffi/src/shielded_sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_sync.rs:445-454: Preserve lifecycle-busy on the clear FFI path
`NetworkShieldedCoordinator::clear` now returns `ShieldedLifecycleBusy` when claims do not drain, and this PR assigns that retryable condition native code 45. This entry point passes through only `ShutdownIncomplete`; lifecycle contention is rewritten as generic code 6, which Kotlin classifies as non-retryable. Swift also lacks code 45 in `PlatformWalletResultCode`, so passing it through without updating that mirror would become `errorUnknown`. Route `ShieldedLifecycleBusy` through the typed conversion here and add code 45 to the Swift result and error mappings.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2567-2571: Serialize concurrent claims before replacing their recovery record
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3767229122)
The SQLite reservation now prevents two active holders from arming the same invitation simultaneously, but this lookup still calls `pending_redrives`, whose file-backed implementation reads only the `subwallets` map hydrated when that store instance opened. If stores A and B opened before A armed a claim, A can return `ShieldedBroadcastUnconfirmed`, leave its durable row, and release its reservation. B then acquires the released reservation but cannot see A's row through its stale map, so it may build a different padded transition and replace the durable row through `arm_redrive_under_claim`. If A's transition executes, its randomized identity ID is no longer recoverable. Query the current SQLite pending row after reservation acquisition, or atomically return an existing pending row as part of reservation acquisition, and never arm or clear a claim based only on the startup-hydrated mirror.
…covery writes, fallible removal Six findings from the round-2 review on #4313. BLOCKING (r3767229122) — the pending-claim lookup after reservation acquisition read `pending_redrives`, which the file store serves from a mirror hydrated once at store OPEN. Store A could arm a claim, return ShieldedBroadcastUnconfirmed and release its reservation; store B then acquired the freed reservation, saw an empty mirror, built a DIFFERENT padded transition and replaced A's row — stranding A's randomized identity id if A's transition executed. `reserve_one_time_claim_key` now returns a `ClaimKeyReservationOutcome` carrying the durable pending row, read from SQLite inside the reservation's own BEGIN IMMEDIATE, and the claim path resumes that row instead of arming fresh. The row is also folded into the mirror so the rest of the claim agrees with disk. BLOCKING (file_store.rs:107) — the recovery connection now opens at synchronous=FULL; the commitment-tree connection keeps NORMAL. A commit that returns before the WAL is fsync'd could drop the claim record AFTER the broadcast, and that row is unreconstructable (its padded identity id exists nowhere else). The tree stays NORMAL because every row in it is chain-authenticated and rebuildable, and per-cmx fsync is what made a 1M-leaf build take minutes. BLOCKING (coordinator.rs:805) — `unregister_wallet` is now fallible and all-or-nothing. It takes destructive admission BEFORE clearing any registration and returns ShieldedLifecycleBusy on refusal, so a removal that cannot purge leaves the wallet intact and genuinely retryable instead of reporting success with the wallet's notes, watermarks and pending-claim row still on disk (and answering WalletNotFound on retry). `unregister_wallet_with` runs the caller's pre-teardown hook inside the critical section, which is where `mark_shielded_detached` has to go: set before the registries are cleared, never set when the removal aborts. The FFI remove path passes the busy refusal through as code 45 rather than flattening it to 6. SUGGESTION (file_store.rs:206) — the identity_index migration runs in one BEGIN IMMEDIATE (ordering two openers) and tolerates duplicate-column as benign (belt and braces). SUGGESTION (keys.rs:64) — `ScrubOnDrop<T>` is sealed behind a crate-private `ScrubbableSecret` marker implemented only for the two audited Orchard key types, so the byte-scrub can no longer be applied to a type it is not sound for. The needs_drop gate stays as defence in depth. SUGGESTION (shielded_sync.rs:445) — the `clear` FFI entry point passes ShieldedLifecycleBusy through as 45 instead of flattening it to 6, and the Swift mirror gains codes 44 and 45 (raw cases, init(ffi:) arms, typed PlatformWalletError cases, errorDescription, init(code:message:) arms) plus ErrorHandlingTests pinning both raw values. Tests: platform-wallet 902 passed (--features shielded), platform-wallet-ffi 292 passed. clippy and rustfmt clean on both crates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
that referenced
this pull request
Aug 19, 2026
… was stale The proposed table still said #4313 holds no number and a new code takes 43. False at #4313's head 0302b18: the revived branch defines and tests ErrorShieldedInviteAlreadyClaimed = 43, ErrorShieldedScanBudgetExhausted = 44 and ErrorShieldedLifecycleBusy = 45. Kotlin maps all three (typed cases, fromPlatformWalletNative arms, DashSdkErrorTest pins — all present at that commit's parent already), and Swift mirrors all three as of 0302b18 itself, which added 44/45's raw cases, init(ffi:) arms, typed cases with init(code:message:) arms, errorDescription coverage, and ErrorHandlingTests raw-value pins (43's Swift mirror predates it; its raw-value pin is Kotlin's). Record 43-45 as allocated by active #4313 with per-code rule-5 status, advance the frontier to 46, and update the three downstream frontier cites (the code-31 paragraph and both #3968 notes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
@QuantumExplorer — funding-layout decision posted on #4312 (#4312 (comment)): moving to a single-note invite + persist-before-broadcast. This PR's reservation/lease machinery slims down accordingly; a revised PR will be submitted if the design is approved. |
Brings the shielded-invite branch up to date with upstream v4.2-dev (#4451 masternode credit withdrawals, #4452, #4453, #4456, #4461). One conflict, in rs-platform-wallet/src/wallet/shielded/operations.rs: upstream #4451 moved carries_consensus_rejection() and broadcast_definitely_failed() out of operations.rs into the new shared crate::broadcast_outcome module (so masternode withdrawals can reuse them), while this branch had inserted its one-time-key claim machinery (NullifierSpentStatus, claim-evidence resolution) directly after those functions. Resolved by dropping the now-local duplicate of broadcast_definitely_failed() — its body is byte-identical to the moved copy, and the file already imports both helpers from crate::broadcast_outcome via upstream's auto-merged use line — and keeping this branch's one-time-key claim block in place. No semantic changes to either side. Verified: cargo check -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni clean; cargo test -p platform-wallet --features shielded wallet::shielded = 205 passed, 0 failed (includes the one_time_claim_evidence and note_selection suites). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 23, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head d1d707f, all six carried findings are fixed, and their targeted regression tests pass. Two blocking lifecycle failure paths remain: a failed purge leaves a detached wallet registered, and a claim continues after losing the lease that protects its recovery record; pending-connection poison handling is also a scoped robustness suggestion.
Source: Codex reviewers (general, security-auditor, rust-quality, and ffi-engineer) = gpt-5.6-sol; final verifier = gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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)
🔴 2 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/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:840-856: Do not abort removal after committing the shielded detach
`on_admitted()` has already set the wallet's irreversible `shielded_detached` flag, and the account, persister, and hydration registrations have been removed before `purge_wallet` runs. If that purge returns an I/O error, this branch returns `ShieldedStoreError`; `remove_wallet_with_teardown` propagates it before removing the wallet from either manager map. The caller therefore retains a registered wallet that cannot bind shielded state and whose coordinator registrations have already disappeared. The FFI flattens this store error to generic, non-retryable code 6, so the host has no contract to finish the partial removal. Keep the detach flag and registries unchanged until the fallible purge succeeds, or, once the detach has been committed, finish removing the wallet instead of returning it as a broken live wallet.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1923-1935: Do not continue an armed claim after its purge-protection lease is lost
Both `Ok(false)` and renewal errors are logged while the claim continues. Once the transition has been armed, that no longer preserves the recovery invariant: `renew_claim_admission` intentionally refuses to resurrect an expired lease, while destructive admission reaps expired leases before counting active claims. A forward wall-clock adjustment, repeated SQLite errors, or a long executor suspension can therefore make a live claim non-renewable; a concurrent clear or wallet removal can then count zero claims and delete the pending row while the transition is already on the wire. If the result is subsequently lost, the randomized padded identity ID cannot be reconstructed. The lifecycle protocol needs an armed-record ownership state that destructive admission cannot discard solely because a wall-clock lease expired, or another mechanism that fails closed whenever renewal can no longer prove ownership.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1175: Propagate pending-connection mutex poisoning through the store error
The newly added durable reservation and lifecycle methods acquire `pending_conn` with `.expect("pending_conn mutex")`, even though their surrounding SQLite operations return `FileShieldedStoreError` and the commitment-tree mutex already maps poisoning into that typed error. If a Rust caller catches a panic that occurred while this connection was locked, a later claim, purge, migration, or admission operation panics instead of returning through `ShieldedStore::Error`. When reached through an exported operation using `block_on_worker`, that secondary panic is re-raised by `expect("tokio worker panicked")` and can abort the host. Use a shared lock helper that maps `PoisonError` into `FileShieldedStoreError` for the new pending-connection paths.
QuantumExplorer
added a commit
that referenced
this pull request
Aug 24, 2026
…rage Review + CI follow-ups: * `ErrorMasternodeListUnavailable` moves 43 → 46: 43/44/45 are held by the in-flight shielded-invite error trio (#4313) across Rust, Kotlin and Swift, and the error-code registry (#4318) records 46 as the next allocatable value. Same renumber on the Swift raw case. * `InvitationPersistenceTests` capability pin gains the genuinely-attested `trackedMasternodes` bit (the handler wires the persist/load/free trio onto `PersistentTrackedMasternode`). * Storage Explorer covers `PersistentTrackedMasternode`: count row (scoped by its own networkRaw — tracked rows have no wallet join), list view, and a detail view showing the opaque Rust-owned snapshot document verbatim.
bfoss765
added a commit
that referenced
this pull request
Aug 24, 2026
…4356 must renumber 42: merged #4451 took the number active #4356 had claimed for ErrorAssetLockInputConflict — merged ABI wins, the open PR renumbers via the frontier. 46: #4465 initially minted 43 (held by #4313), was flagged in review, and renumbered to the frontier before merging — Rust and Swift together. Frontier moves to 47. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 tasks
Brings the shielded-invite branch up to date with upstream v4.2-dev (#4470 active-protocol-version shielded fees, #4472 shield credits to an external Orchard recipient, #4477, #4469 swift async shutdown). One conflict, in rs-platform-wallet/src/wallet/shielded/operations.rs: both sides appended a #[cfg(test)] module at the same insertion point — this branch's foreign_claim_guard_tests (single-flight claim lifecycle guard, #4313 review finding 979bbc2fcb3c) and upstream #4472's shield_recipient_tests (resolve_shield_recipient classification). Resolved by keeping BOTH modules in full, this branch's first, each under its own #[cfg(test)]. No code from either side dropped or altered. The FFI error-code seam needed no hand-merge: upstream #4469's ErrorMasternodeListUnavailable = 46 was allocated explicitly around this branch's 43/44/45 shielded-invite trio. Verified: cargo check -p platform-wallet --features shielded and platform-wallet-ffi --all-features clean; cargo test platform-wallet --features shielded --lib = 984 passed / 1 failed — shield_input_selection_tests::regression_reports_max_from_usable_suffix _not_total_account_balance, proven PRE-EXISTING on unmerged origin/v4.2-dev (1e26927): upstream's versioned-fee change dropped shield_fee_reserve_credits(LATEST) below the test's seeded 297_264_780 leading balance; the unmerged PR head passes it. platform-wallet-ffi = 330 passed / 0 failed; rs-unified-sdk-jni = 37 passed / 0 failed; kotlin-sdk :sdk:test = 353 tests x debug+release, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 7d9376a, three blocking recovery/lifecycle defects remain: purge failure can leave a detached wallet registered, an armed claim can continue after losing the lease protecting its recovery row, and successful claims delete that row before durable local registration. Two scoped robustness suggestions also remain; the alleged proof-trust defect is refuted because Drive's verifier derives and validates the exact identity, keys, and nullifiers before constructing the only possible result variant.
Source: Codex reviewers codex/general, codex/security-auditor, codex/rust-quality, and codex/ffi-engineer = gpt-5.6-sol; final verifier backend = gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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)
🔴 3 blocking | 🟡 1 suggestion(s)
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/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2353: Retain the recovery record until the identity is durably registered
`finalize_one_time_claim_record` clears the pending row for every successful result before control returns to `PlatformWallet::identity_create_from_one_time_key`, where the identity is added to the local manager and handed to the persister. The resume-success branch has the same ordering at lines 2157-2160. A process death in this gap, a missing wallet-manager entry, or a persistence failure loses the exact padded identity ID before the JNI caller receives it; `IdentityManager::add_identity` also logs and swallows `persister.store` errors. On a fresh single-note retry, `expected_identity_id` is `None`, and `recover_executed_one_time_claim` returns terminal `ShieldedInviteAlreadyClaimed` at lines 4193-4203 before attempting the master-key lookup. Keep the pending row until local registration is durably acknowledged, or introduce a persisted acknowledgement/reconciliation state that survives the native return and host persistence handoff.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2033-2045: Do not continue an armed claim after its purge-protection lease is lost
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3839256998)
Both `Ok(false)` and renewal errors only log while the admitted claim continues. The store intentionally refuses to resurrect an expired lease, while destructive admission reaps expired rows before counting active claims. A forward wall-clock adjustment, prolonged executor suspension, or repeated SQLite failure can therefore make a live armed claim non-renewable; concurrent clear or removal can then count no active claim and purge its byte-exact pending row while the transition is already on the wire. If the result is subsequently lost, the randomized padding nullifier makes the identity ID unrecoverable. Armed records need ownership that cannot be discarded solely because a wall-clock lease expires, or destructive admission must fail closed whenever ownership can no longer be established.
In `packages/rs-platform-wallet/src/wallet/shielded/keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/keys.rs:70-76: Make the secret-scrubbing marker an unsafe contract
`ScrubOnDrop::drop` performs raw volatile writes whose soundness depends on every marker implementation having a representation that may be overwritten bytewise without violating validity or resource ownership. The current two implementations satisfy that invariant, but `ScrubbableSecret` is a safe crate-visible trait, so a sibling module can add an implementation without acknowledging the unsafe obligation. Declare the trait and its implementations `unsafe` so every extension must explicitly accept and document the invariant relied upon by the `Drop` implementation.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:840-856: Do not abort removal after committing the shielded detach
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3839256993)
Destructive admission has already been acquired, `on_admitted()` has permanently marked the wallet shielded-detached, and the account, persister, and hydration registrations have been removed before `purge_wallet` runs. If that purge fails, this branch returns `ShieldedStoreError`; `remove_wallet_with_teardown` propagates it at manager/wallet_lifecycle.rs:803-805 before either manager map is cleaned up. The caller therefore retains a registered wallet that can no longer bind shielded state and whose coordinator registrations are gone. Once teardown is committed, complete manager removal even if the best-effort purge fails, or perform every fallible operation before setting the detach flag and clearing registrations.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1175: Propagate pending-connection mutex poisoning through the store error
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3839257001)
The new durable reservation and lifecycle methods acquire `pending_conn` with `.expect("pending_conn mutex")`, although their SQLite operations return `FileShieldedStoreError` and commitment-tree mutex poisoning is already mapped into that type. If a Rust caller catches a panic that occurred while this mutex was held, later claim, purge, migration, or admission operations panic instead of returning through `ShieldedStore::Error`. On an exported worker path, `block_on_worker` re-raises that panic and can abort the host at the C ABI. Use a shared pending-connection lock helper that maps `PoisonError` into `FileShieldedStoreError`.
| denomination, | ||
| ) | ||
| .await; | ||
| finalize_one_time_claim_record(store, claim_records_id, claim_record_key, &result).await; |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Retain the recovery record until the identity is durably registered
finalize_one_time_claim_record clears the pending row for every successful result before control returns to PlatformWallet::identity_create_from_one_time_key, where the identity is added to the local manager and handed to the persister. The resume-success branch has the same ordering at lines 2157-2160. A process death in this gap, a missing wallet-manager entry, or a persistence failure loses the exact padded identity ID before the JNI caller receives it; IdentityManager::add_identity also logs and swallows persister.store errors. On a fresh single-note retry, expected_identity_id is None, and recover_executed_one_time_claim returns terminal ShieldedInviteAlreadyClaimed at lines 4193-4203 before attempting the master-key lookup. Keep the pending row until local registration is durably acknowledged, or introduce a persisted acknowledgement/reconciliation state that survives the native return and host persistence handoff.
source: ['codex']
Collaborator
Author
There was a problem hiding this comment.
Fixed in da708c4d06 (branch head 347d5879a309c4a6c8b52730a5f297ff91acf362).
Took the first option — keep the pending row until local registration is durably acknowledged — expressed through the existing resume machinery rather than a second state beside it.
finalize_one_time_claim_record no longer clears on success. It now returns a OneTimeClaimRecoveryRecord for the caller to acknowledge, and clears only on the terminal ShieldedInviteAlreadyClaimed, where no retry can get anything from the record. Both orderings the finding names are covered: the fresh-build tail and the resume-success branch go through the same call.
operations::identity_create_from_one_time_key returns a OneTimeClaimOutcome { identity_id, identity, recovery_record }, so the row outlives the native return and the host persistence handoff. PlatformWallet::identity_create_from_one_time_key registers the identity and calls acknowledge_one_time_claim_registration — the only thing that drops the row — solely on durable success.
The swallowed persister.store error is closed too. IdentityManager::add_identity_persisted propagates it as PlatformWalletError::Persistence; the claim path uses that instead of add_identity, which keeps its log-and-swallow behaviour for its other callers and now delegates. The in-memory insert still stands on a persistence error — it is not rolled back — so the manager and location index stay consistent and the next sync can heal the host's row; the error reports only that the host has not durably acknowledged the identity. A registration failure is still not surfaced to the caller; it only withholds the acknowledgement.
That integrates with the existing machinery rather than duplicating it: the retained row is the same shielded_pending_spends record the reservation and lease already protect, carrying its identity_index. A retry finds it through reserve_one_time_claim_key's in-transaction read, resume_one_time_claim sees the notes spent, and recover_executed_one_time_claim gets Some(declared_id) — so it never reaches the expected_identity_id == None arm at :4193-4203.
Tests (operations.rs), both red before the change:
a_successful_claim_retains_its_record_until_registration_is_acknowledged— the row survives the resolution and is dropped only by the acknowledgement.a_claim_killed_before_acknowledgement_is_recoverable_after_a_restart— a claim whose transition carries a padded, non-re-derivable id resolves, the receipt is dropped without acknowledging (the process death), the store is reopened from disk as a relaunch does, and the record is still there carrying the declared id the retry needs. RED: the success path had cleared it, so the relaunch found nothing.
Cost of the retention is one stale row that the next claim of the same invitation resolves; the resume path is idempotent.
Known narrow window, called out rather than papered over: release_claim_admission still runs when the claim body returns, so the retained row outlives its lease and reservation during local registration. A concurrent clear/unregister_wallet in that window can delete it — but that only happens while the wallet is being wiped or removed, which is consistent with the removal. Happy to extend the lease across the acknowledgement instead if you would rather close it.
Suites: 1000/1001 lib with --features shielded (the one failure is pre-existing on the unmodified branch head), 758 without, 321 in platform-wallet-ffi.
Collaborator
There was a problem hiding this comment.
Resolved in da708c4 — Retain the recovery record until the identity is durably registered 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.
bfoss765
added a commit
that referenced
this pull request
Aug 26, 2026
…new collisions At #3968's current head 396977b the old 26/27/28 layout is gone: base merges (9d0dd5a on 08-20, c86d237/4dbf38f5da on 08-25) restored ErrorTransactionBroadcastRejected = 26 and inherited ErrorShutdownIncomplete = 27, curing the rule-3 renumber. But the 08-20 merge minted ErrorPersisterTransient = 42 / ErrorPersisterFatal = 43 below the frontier: 42 is now merged ABI (#4451's ErrorMasternodeWithdrawalUnconfirmed) and duplicated in #3968's own tree (E0081 in error.rs, duplicate Swift raw value 42), and 43 is active #4313's ErrorShieldedInviteAlreadyClaimed. Refresh the non-conforming table, its branch-state introduction, and the contested-section closer to the 42/43 state; both persister codes owe fresh frontier integers (48 as of 2026-08-26). The dated 2026-08-04 survey is untouched, per review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… steps, retain claim recovery records Addresses the five open review findings on #4313. coordinator.rs (b50fc7f161ed) — a `purge_wallet` failure after a removal's commit point no longer aborts the removal. Past `on_admitted` the wallet's irreversible detach flag is set and the registries are gone, so returning the store error made `remove_wallet_with_teardown` propagate before removing the wallet from either manager map: a live wallet that can never bind shielded state again, reported through an FFI code the host cannot retry. The removal now always completes and the unfinished purge is recorded in `deferred_purges`, retried by `finish_deferred_purges` from every sync pass, finished before any re-registration of the same id, and dropped by a successful `clear`. operations.rs (f58ed9d910d8) — a claim no longer proceeds to a chargeable step on a lease it cannot prove. The renewal heartbeat only logs a failed renewal, and `renew_claim_admission` refuses to resurrect an expired lease while destructive admission reaps expired leases before counting live claims, so a clock jump, repeated store errors or a long suspension could let a purge delete the pending row mid-flight. `assert_claim_lease_before_chargeable_step` re-proves ownership synchronously before both the fresh broadcast and the resume re-broadcast, failing closed and retryably on `Ok(false)` and on any store error, and re-stamping the lease it proves. operations.rs (325ce9fa8f84) — a successful claim now RETAINS its recovery record until the caller acknowledges durable local registration. Clearing on success dropped the only durable copy of a padded single-note claim's identity id before control returned through `identity_create_from_one_time_key`, so a process death, a missing wallet-manager entry or a swallowed `persister.store` error stranded the identity behind a terminal `ShieldedInviteAlreadyClaimed`. The op returns a `OneTimeClaimOutcome` carrying the record; the caller uses the new `IdentityManager::add_identity_persisted`, which propagates the persister's error, and calls `acknowledge_one_time_claim_registration` only on durable success. Until then a retry resumes the record and recovers by declared id. file_store.rs (ae228864b6d8) — the pending-connection lock sites map `PoisonError` into `FileShieldedStoreError` through a shared `lock_pending_conn` helper instead of `.expect`, so a panic caught while the lock was held no longer turns every later claim, purge, migration or admission call into a second panic that `block_on_worker` re-raises into the host. keys.rs (388cd8a854c4) — `ScrubbableSecret` is now an `unsafe` trait with the byte-scrubbing obligation documented, and both Orchard impls are `unsafe impl` with a SAFETY note. Tests: red-then-green for the three blockers — a post-commit purge failure that completes the removal and defers the purge, a re-registration that finishes a deferred purge first, a resume refused without a provable lease (which reached the broadcast and result-wait before the gate), and a claim killed before acknowledgement that is still recoverable after a store reopen. Suites: 992/9 with `--features shielded`, 750/9 without; the one failure (`regression_reports_max_from_usable_suffix_not_total_account_balance`) is pre-existing on the unmodified branch head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One conflict, in `rs-platform-wallet-ffi/src/shielded_send.rs`: both sides appended tests to the end of the same `#[cfg(test)]` module — this branch's one-time-claim result-code pins (scan-budget 44, terminal 43, unconfirmed) and dev's asset-lock shortfall pins (code 29, plus the resume sibling). They cover different helpers and neither supersedes the other, so both were kept. No duplicate helper symbols appeared in this merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…drain call site The dev merge widened read_recipient43 to take the JNI field name for its error message (#4361's merge); the coinjoin-drain call site introduced on this branch's side of the merge still passed two args, failing workspace compile in CI. Same cross-PR widening class as the gu2/gu3 fold fixes. rs-unified-sdk-jni: check --all-targets clean, 37 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
CI note on the red Swift SDK build + tests check: the failure is environmental, not from this branch. Both failing cases are in |
thepastaclaw
suggested changes
Aug 27, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Five blocking issues remain in the one-time claim lifecycle: an armed recovery record can lose purge protection after broadcast, registration can acknowledge before true durability, deferred purges are forgotten on restart, stale wallet handles can claim after removal, and claim-worker panics can abort across the C ABI. cargo check -p platform-wallet --features shielded --locked passes, but these recovery, teardown, and FFI defects can still strand an on-chain identity or retain shielded state after removal.
Source: Codex general, security-auditor, rust-quality, and ffi-engineer reviewers = gpt-5.6-sol; final verifier = gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was 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)
🔴 5 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:964-969: Do not forget deferred wallet purges across restart
A post-commit `purge_wallet` failure is recorded only in the coordinator's in-memory `deferred_purges` set. A newly opened coordinator starts with that set empty, and this early return then reports success without inspecting or purging any surviving SQLite rows. `register_locked` relies on this method, so re-registering the same wallet after restart can inherit decrypted notes, pending claims, activity, and a stale `last_synced_note_index`; because restore advances watermarks monotonically, the stale value can also make the re-added wallet skip historical notes. Persist a removal tombstone outside the failed purge operation, or make startup and registration detect and purge orphaned wallet state without depending on the in-memory set.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1591-1607: Refuse claims that begin after wallet removal commits
This claim entry point neither checks `shielded_detached` nor participates in the wallet generation's teardown guard. An FFI caller can resolve and retain the wallet and coordinator before removal, then begin executing after `unregister_wallet_with` has committed the detach, purged the store, released its destructive barrier, and removed the wallet from the manager. The stale handle can then acquire a fresh claim admission, arm a new pending row, and broadcast for a wallet the host has already removed. Its registration tail finds no manager entry, so the row remains without a deferred-purge marker. Re-check the detach state after admission is acquired, or fence the full claim with the removed generation's lifecycle gate.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1133-1156: Catch claim worker panics before crossing the C ABI
This new `extern "C"` export invokes `block_on_worker` directly. `block_on_worker` re-panics when its Tokio task panics, so a panic in proof generation, signing, wallet bookkeeping, or SDK processing reaches the non-unwind C ABI frame and aborts the host process. The JNI `guard` cannot catch it because that guard is on the far side of this C export. Other shielded exports in this file already move their bodies behind `catch_panic_to_code` wrappers for this reason. Wrap the complete claim export in a claim-specific panic guard and preserve an ambiguous/recoverable result contract because the panic may occur after broadcast.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2045-2063: Do not continue an armed claim after its purge-protection lease is lost
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3839256998)
The pre-broadcast assertion proves lease ownership only immediately before `broadcast_and_confirm_one_time_claim` starts. Once the transition is on the wire, this heartbeat still logs `Ok(false)` or a renewal error and continues waiting. A forward wall-clock jump, executor suspension longer than the lease, or repeated SQLite failure can therefore make the live claim non-renewable; `begin_destructive_admission` then reaps the expired lease and allows clear or removal to purge the byte-exact pending row while confirmation remains unresolved. If the result is subsequently lost, a padded single-note claim's randomized identity ID cannot be reconstructed. An armed unresolved record must continue blocking destructive admission independently of an expiring wall-clock lease.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1983-1988: Retain the recovery record until the identity is durably registered
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3856575755)
The claim releases its admission and invitation reservation here before `PlatformWallet::identity_create_from_one_time_key` registers the returned identity. A concurrent clear or removal can consequently purge the retained recovery row during that handoff. Even without that race, `add_identity_persisted` treats a successful `WalletPersister::store` as durable, although `PlatformWalletPersistence` explicitly allows `store` to buffer data and defines `flush` as the durability boundary unless `store_commits_inline()` is true; `NoPlatformPersistence` even returns success without persisting anything. The caller then acknowledges and deletes the recovery row. A crash before flush can lose both the durable identity registration and the only copy of a padded claim's exact ID. If `store` fails, the in-memory insertion also remains, so a same-session retry returns `IdentityAlreadyExists` before attempting persistence again. Keep purge protection through registration, require an actually durable backend and successful flush or inline-commit attestation before acknowledgement, and make the retry path idempotently re-persist an existing matching registration.
…llet removal, panic-guard the claim export Round-2 review fixes on #4313, all three on round 1's own changes. Do not forget deferred wallet purges across restart (finding 5ca995a0d2aa): round 1 recorded a post-commit `purge_wallet` failure only in the coordinator's in-memory `deferred_purges` set, so a process death lost the deferral and the orphaned rows were never purged — a re-registration of the same id could then inherit its notes, pending claims, activity and a stale `last_synced_note_index`, which restore only advances, making the re-added wallet skip historical notes. `unregister_wallet_with` now writes a DURABLE removal tombstone through the store (new `shielded_wallet_removal` table on the `synchronous=FULL` connection) past the commit point and BEFORE the purge it guards, and drops it only once that purge succeeds. Written before rather than on failure because a crash *during* the purge orphans the same rows and leaves no failure to react to. `NetworkShieldedCoordinator::new` seeds the in-memory set from that table, so a relaunched process starts knowing what an earlier one left unfinished, and `finish_deferred_purges` unions with it for tombstones a peer coordinator or process wrote after our open. The registration fast path deliberately stays lock-free — taking the store lock in `try_finish_deferred_purge` made a registration block behind an in-flight bind's transaction. Refuse claims that begin after wallet removal commits (finding 4a2c679745bb): the claim entry point checked neither `shielded_detached` nor the removal admission protocol, so a stale FFI handle could start after `unregister_wallet_with` had committed the detach, purged, and RELEASED its barrier — take a fresh admission, arm a pending row, and broadcast for a wallet the host had already removed. `PlatformWallet::identity_create_from_one_time_key` now pre-checks, and `operations::identity_create_from_one_time_key` takes the flag and RE-READS it under its own claim admission, which is what actually closes the race: the two admissions are mutually exclusive and totally ordered at the store, so a `false` read under our lease holds for the whole claim. Typed `WalletNotFound` — terminal for this wallet, and distinct from the invitation's terminal already-claimed outcome. The refusal releases its lease. Catch claim worker panics before crossing the C ABI (finding 945163f6ed5b): the new export called `block_on_worker` directly, which re-panics on a panicking Tokio task, so a panic in the scan, proving, signing or SDK processing reached the non-unwind `extern "C"` frame and aborted the host — the JNI guard sits on the far side. The body moves to a private `shielded_identity_create_from_one_time_key_inner` behind `catch_one_time_claim_panic`, mapping to the generic `ErrorUnknown`: every richer code this export uses promises something a panic cannot (17 says `out_identity_id` IS written, 43 is TERMINAL, 44/45 promise nothing was broadcast). The guidance keeps the ambiguity and points at the retained recovery record, which makes re-running the claim a resume rather than a second identity. Verification: 1003 pass with `--features shielded` (the single failure, `shield_input_selection_tests::regression_reports_max...`, fails identically on the unmodified branch head), 758 without, 324 in platform-wallet-ffi; `cargo check --all-targets` clean on rs-unified-sdk-jni and `cargo check --workspace --all-targets` clean with no warnings; clippy and fmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 27, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commit correctly rejects claims started through stale removed-wallet handles and prevents claim-worker panics from unwinding across the C ABI. Four blocking lifecycle/API issues remain: armed claims can lose purge protection after broadcast, recovery can be acknowledged before durable identity persistence, deferred purges can be bypassed during re-registration, and claim-panic recovery is exposed to Kotlin as non-retryable; the checkpoint cache also retains an unbounded number of notes per key.
Source: Codex reviewers codex/general, codex/security-auditor, codex/rust-quality, and codex/ffi-engineer = gpt-5.6-sol; final verifier = gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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)
🔴 4 blocking | 🟡 1 suggestion(s)
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-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:793-797: Preserve claim-panic recovery semantics in the Kotlin error type
The panic guard correctly prevents an unwind across the C ABI, but it reports an ambiguous, recoverable claim outcome as generic `ErrorUnknown` code 99. JNI offsets that to 1099, and `DashSdkError.fromPlatformWalletNative` has no claim-specific mapping for 99, so Kotlin exposes `PlatformWallet.Generic` with `isRetryable == false`. A host following the public typed retry contract can consequently release the identity slot or decline the recovery retry even though the transition may already be on chain and the retained row may be the only source of its padded identity ID. The recovery instructions embedded in the message are not a machine-readable replacement for the Kotlin error contract. Add a dedicated ambiguous-claim result code and map it through JNI to a Kotlin type that preserves the slot and permits a delayed recovery retry.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:896-905: Bound checkpoint size, not only the number of invitation keys
`FOREIGN_SCAN_CHECKPOINT_CAP` limits the cache to eight keys but does not limit each checkpoint's `Vec<ShieldedNote>`. The assumption that an invitation key receives only one or two notes is not enforced: an untrusted inviter can fund the published Orchard address with many small notes. Budgeted retries then retain every decrypted note below the resume position, and `load` clones the complete vector while the cached original remains live, temporarily doubling that entry's memory use. A malicious but valid invitation can therefore grow memory use with its note history despite the per-attempt scan-work budget. Add a retained-note or byte budget, or store a compact resumable representation whose memory use does not scale with every note owned by the foreign key.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2094-2112: Do not continue an armed claim after its purge-protection lease is lost
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3839256998)
The pre-broadcast assertion prevents a transition from first entering the network without a live lease, but it does not protect an already-broadcast transition. During the subsequent confirmation and recovery wait, this heartbeat logs `Ok(false)` or a renewal error and continues. Because destructive admission reaps expired leases before counting claims, a forward wall-clock adjustment, executor suspension beyond the lease window, or repeated SQLite failure can let clear or wallet removal purge the armed recovery row while the transition remains unresolved. If the result is then lost, a padded single-note claim's randomized identity ID cannot be reconstructed. An armed unresolved record must independently block destructive admission until it reaches a terminal or durably acknowledged state; a lease check performed only before broadcast cannot protect the post-broadcast interval.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2032-2033: Retain the recovery record until the identity is durably registered
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3856575755)
The claim releases its admission and invitation reservation here before `PlatformWallet::identity_create_from_one_time_key` registers and acknowledges the returned identity. A concurrent clear or removal can therefore purge the retained recovery row during that handoff. Registration also is not necessarily durable: `add_identity_persisted` calls only `WalletPersister::store`, while `PlatformWalletPersistence` permits `store` to buffer changes until `flush` and `NoPlatformPersistence` reports success without storing anything. Nevertheless, any successful `store` sets `registered = true` and immediately deletes the only durable copy of a padded claim's exact identity ID. If `store` fails, the in-memory insertion remains, so a same-session retry encounters `IdentityAlreadyExists` rather than retrying persistence. Keep purge protection through registration, require an attested durable backend plus either inline-commit confirmation or a successful flush before acknowledgement, and make matching existing registrations idempotently persistable.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:660-665: Do not forget deferred wallet purges across restart
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3867666255)
Re-registration still proceeds when the deferred purge cannot complete, explicitly allowing stale shielded state to enter the new bind. The durable tombstone is also not fail-closed: removal records it only after the irreversible detach and ignores a write failure, so a simultaneous purge failure leaves only the process-local set; startup hydration is best-effort; and `try_finish_deferred_purge` consults only that startup-seeded set, missing tombstones written later by a peer coordinator. A restart, transient tombstone-read failure, or immediate peer re-registration can therefore install a wallet over surviving pending claims and restored shielded state, including a stale monotonic watermark that can skip historical notes. Establish the tombstone before committing detach, durably check it during registration, and do not install the new registration until the required purge has succeeded.
Comment on lines
+793
to
+797
| catch_panic_to_code( | ||
| operation, | ||
| PlatformWalletFFIResultCode::ErrorUnknown, | ||
| ONE_TIME_CLAIM_PANIC_GUIDANCE, | ||
| body, |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Preserve claim-panic recovery semantics in the Kotlin error type
The panic guard correctly prevents an unwind across the C ABI, but it reports an ambiguous, recoverable claim outcome as generic ErrorUnknown code 99. JNI offsets that to 1099, and DashSdkError.fromPlatformWalletNative has no claim-specific mapping for 99, so Kotlin exposes PlatformWallet.Generic with isRetryable == false. A host following the public typed retry contract can consequently release the identity slot or decline the recovery retry even though the transition may already be on chain and the retained row may be the only source of its padded identity ID. The recovery instructions embedded in the message are not a machine-readable replacement for the Kotlin error contract. Add a dedicated ambiguous-claim result code and map it through JNI to a Kotlin type that preserves the slot and permits a delayed recovery retry.
source: ['codex']
Comment on lines
+896
to
+905
| fn load(&self, key: &[u8; 32]) -> Option<ForeignScanCheckpoint> { | ||
| let mut map = self | ||
| .entries | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| map.iter().position(|(k, _)| k == key).map(|i| { | ||
| let entry = map.remove(i); | ||
| let checkpoint = entry.1.clone(); | ||
| map.push(entry); | ||
| checkpoint |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Bound checkpoint size, not only the number of invitation keys
FOREIGN_SCAN_CHECKPOINT_CAP limits the cache to eight keys but does not limit each checkpoint's Vec<ShieldedNote>. The assumption that an invitation key receives only one or two notes is not enforced: an untrusted inviter can fund the published Orchard address with many small notes. Budgeted retries then retain every decrypted note below the resume position, and load clones the complete vector while the cached original remains live, temporarily doubling that entry's memory use. A malicious but valid invitation can therefore grow memory use with its note history despite the per-attempt scan-work budget. Add a retained-note or byte budget, or store a compact resumable representation whose memory use does not scale with every note owned by the foreign key.
source: ['codex']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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