Repository navigation
Conversation
…rom b2 line Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m_one_time_key), reconciled to base identity API Ports the L2-invitation CLAIM side from the b2 line (8008dc78b8): Verbatim grafts (byte-for-byte from b2, deps all present in base): - operations.rs: free fn identity_create_from_one_time_key (note-scan + Halo2 proof) and its supporting note-scan helper scan_notes_for_foreign_key (sync.rs), plus the one_time_key_tests module. - platform_wallet.rs: PlatformWalletManager::identity_create_from_one_time_key. - shielded_send.rs (FFI): platform_wallet_manager_shielded_identity_create_from_one_time_key (base's FFI-layer decode_identity_pubkeys/IdentityPubkeyFFI matches b2). Reconciled to base's API (NOT byte-for-byte): - funding.rs (JNI): decode_pubkeys_blob + hand-built IdentityPubkeyFFI literal (b2) -> decode_registration_pubkeys_blob + row.to_ffi() (base), plus base's tagged-payload return with ErrorShieldedBroadcastUnconfirmed handling. - Kotlin: IdentityKeyPreview.encodeForRegistration + withContext + raw return (b2) -> List<IdentityPubkey> via IdentityPubkeyCodec.encode + teardownGate.op + decodeShieldedCreatePayload (base), mirroring the tested inviter side. Pubkey-decode semantics preserved: identical key_id / pubkey bytes / order / count; role/read_only/contract-bounds source shifts from Rust-derived (b2) to caller-stamped blob (base) — base's authoritative pipeline-wide convention, already adopted by the tested inviter side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses reviewer thepastaclaw's blocking findings on PR #4204. Two of the four blockers are fixed here; the other two are structural and reported back for a decision rather than guessed (crypto/money path). Blocker #4 (FFI RNG abort) — shielded_send.rs / keys.rs: `generate_one_time_orchard_key` used `OsRng::fill_bytes`, which panics on an OS entropy-source failure. It is called from a `#[no_mangle] extern "C"` export, so that panic aborts the process across the C ABI before any JNI panic guard can convert it. Switch to `RngCore::try_fill_bytes`, return a typed `PlatformWalletError::ShieldedKeyDerivation`, and have the FFI export map it to `ErrorWalletOperation` instead of aborting. Test call sites and callers updated for the new `Result` return. Blocker #3 (bearer spend key hygiene) — funding.rs: `oneTimeSk` is bearer spend authority but was marshalled via the generic `read_id32`, leaving its intermediate JNI `Vec<u8>` and returned `[u8; 32]` unsanitized. Add a `read_key32_zeroizing` helper (mirroring `transactions::read_key32_zeroizing`): the returned key is `Zeroizing` and the intermediate JNI copy is scrubbed. `sk` derefs to `[u8; 32]`, so the downstream `sk.as_ptr()` FFI call is unchanged. NOT fixed here (reported for decision): Blocker #1 (affected-state wait): `wait_for_affected_state` does not exist in this head's SDK, and the pool-funded sibling still uses `wait_for_response` on this branch. The reviewer's fix is predicated on rebasing onto the v4.1-dev proof API (31c69cf); it must be done in lockstep for both Type-20 paths. Blocker #2 (persist claim recovery record): the redrive mechanism is keyed by SubwalletId + activity entry and driven by the per-subwallet sync loop. Claim notes belong to a foreign one-time key tracked in no subwallet, so a correct fix needs a new subwallet-less pending-claim record + reconciliation path, not a reuse of `arm_redrive_record`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…o-end (#4204) Reviewer (thepastaclaw) key-hygiene blocker: the one-time Orchard bearer spending key was copied into several plain, unsanitized buffers on both the claim and generate paths. Claim path — carry the key through `Zeroizing` from the FFI copy down through the wallet layers instead of leaking a plain `[u8; 32]` at each hop: - rs-platform-wallet-ffi: the `[u8; 32]` claim-key copy is now `Zeroizing<[u8; 32]>` and moves (not copies) into the wallet layer. - platform-wallet `identity_create_from_one_time_key` (both the PlatformWallet method and the operations fn) now take `Zeroizing<[u8; 32]>`; the key is scrubbed on drop, dereferenced only at the single `SpendingKey::from_bytes` consumption point. Generate path — wipe the transient native and JVM copies after handoff: - rs-platform-wallet-ffi: explicitly zeroize the native `sk` after copying it into the caller's `out_sk_32`. - rs-unified-sdk-jni: hold the JNI `sk` and the combined 75-byte `out` blob in `Zeroizing` buffers so both scrub on drop, including early returns. - kotlin-sdk `generateOneTimeOrchardKey`: wipe the 75-byte JVM blob in a `finally` once the two owned arrays have been sliced out. Validated: cargo build + cargo test -p platform-wallet (493 pass) + cargo fmt; :sdk:compileDebugKotlin succeeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aim (#4204) Reviewer (thepastaclaw) blocker: after rebasing onto v4.1-dev, the current proof contract (31c69cf) marks IdentityCreateFromShieldedPool proofs as affected-state snapshots — they authenticate the resulting identity and spent nullifiers but cannot bind the complete Orchard request. That commit switched the pool-funded sibling to wait_for_affected_state; the strict wait_for_response now yields ExecutionNotProved for every valid proof. The one-time-key claim path (identity_create_from_one_time_key) was still on the strict wait_for_response, so every valid claim proof would enter the ambiguous fallback and risk being reported unconfirmed despite executing. Switch it to wait_for_affected_state, matching the pool-funded sibling (the sibling already adopted it via the v4.1-dev rebase). Validated on the rebased v4.1.0-rc.1 base: cargo build (platform-wallet + rs-unified-sdk-jni) + cargo test -p platform-wallet (493 pass) + cargo fmt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…red-note DAO queries (#4204) Shielded-invite claim recovery: an IdentityCreateFromOneTimeKey claim that has already executed on chain (its note nullifier is spent / broadcast or wait returns NullifierAlreadySpent) is now reconciled to success instead of stranding the retry with a hard error. Recovery re-derives everything from the invite the invitee already holds — no persisted record: - master_auth_public_key_hash(): the invitee's re-derivable MASTER auth key hash, the unique Platform-indexed handle the identity is looked up by (discover_inner's unique-hash probe). - any_nullifier_spent_on_chain(): proof-verified ShieldedNullifierStatuses preflight; if the selected notes are already spent, recover by key hash before rebuilding/rebroadcasting. - NullifierAlreadySpent arms on both broadcast and wait paths route to recover_executed_one_time_claim(), which recovers by key hash, then by the deterministically-derived identity id (fetch_identity_with_retries), and otherwise surfaces ShieldedBroadcastUnconfirmed carrying the derived id. Preserves the newer #4204 key-hygiene base already in this branch: the one-time spending key is still carried in Zeroizing<[u8;32]> and wait_for_affected_state is unchanged (Type-20 proof is affected-state). ShieldedDao: adds minUnspentAnchoredBlockHeight() and getUnspentAnchoredNotesByWallet() — read-only queries over existing shielded_notes columns (no schema change) backing the shielded-username anchor-confirmation gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ted the identity (#4204) A spent invitation nullifier proves only that *something* consumed the note. It does not prove that this claim's Type-20 transition created an identity, and recovery was treating "nullifier spent + an identity is findable under the submitted MASTER auth key hash" as a successful claim. Two real on-chain outcomes are reported as success by that rule: 1. The chargeable `UnshieldAction` fallback. When a submitted unique public-key hash is already registered, Type-20 finalizes the shielded spend as an `UnshieldTransitionAction` with `chargeable_failure: true` and creates NO identity, crediting the invitation value to the creation-failure address minus a penalty (rs-drive-abci .../identity_create_from_shielded_pool/state/ v0/mod.rs:62-128). A retry then saw the nullifier spent, fetched the *pre-existing* identity that owns the colliding key hash, and returned it as the claim's result. 2. A competing holder of the same bearer one-time key. The identity id is `double_sha256` over the SORTED published action nullifiers (`identity_id_from_nullifiers`) — derived from nullifiers only, never from identity keys. With two or more real spends no randomized padding action is added, so another holder of the same invite derives the SAME id under THEIR keys. The victim's retry fetched that foreign identity by the shared id and `platform_wallet.rs` registered it at the victim's identity index. Recovery is now gated on two independent bindings, both required (`recovered_identity_matches_claim`): - id binding — the identity's id equals the id derived from THIS claim's published nullifiers. Consensus re-derives and rejects a mismatch, so only a transition publishing exactly this nullifier set can carry that id. This is what rejects case 1. - key binding — the identity's ON-CHAIN key set carries this claim's submitted MASTER authentication key hash. This is what rejects case 2. The key binding is checked against the keys the fetch actually returned, so an identity fetched without public keys now fails closed instead of being topped up with locally-submitted keys that were never proven to exist on chain. Where the bindings cannot be established, recovery returns the new terminal `ShieldedInviteAlreadyClaimed` (FFI `ErrorShieldedInviteAlreadyClaimed` = 32) rather than a success or the retryable unconfirmed code. That includes the single-spend case: the builder pads a one-action bundle to Orchard's 2-action minimum (`num_actions = spends.len().max(2)`) and the padding action's RANDOM dummy nullifier participates in the id derivation, so the original id is not re-derivable on a retry and no candidate can be bound to the claim. Also: - The spent-nullifier preflight now hands off to the reconciler directly instead of falling through to rebuild+rebroadcast a transition that can only earn a `NullifierAlreadySpent` rejection (saves a Halo 2 proof build). - The generic wait-failure fallback applies the key binding too, but only when the bundle was NOT padded: a padded build's id embeds a locally generated dummy nullifier no other party can reproduce, so there the id alone is proof. Regression tests in `one_time_claim_evidence_tests` pin both attack scenarios plus the keyless-fetch, unre-derivable-id, absent-key-hash, wrong-purpose and different-nullifier-set cases. 7 of the 8 fail against the pre-fix rule (only the positive-acceptance case still passes), verified by reverting the predicate to the old accept-anything behavior.
…ene, message hygiene (#4204) Addresses the six open CodeRabbit threads. - `PlatformWalletPersistenceHandler.reconstructPendingIdentityKeysFromPersistence` wrapped a SUSPEND decryptability probe in `runCatching`, which catches `Throwable` and therefore swallowed `CancellationException`: a cancelled caller had the row misclassified as unusable and a spurious pending-repair entry published. Now rethrows cancellation and keeps `false` only for genuine probe failures, matching the convention this PR already established in `WalletStorage` ("NEVER swallow structured-concurrency cancellation"). CodeRabbit missed that `PlatformWalletManager` re-swallows one frame up in a bare `runCatching`; that site is fixed too, since fixing only the inner one would not have delivered the stated behavior. - Rename five unused `catch (e: ...)` bindings to `_` (detekt SwallowedException) in `WalletStorage` and `KeystoreManager`. Adjacent catches that `throw e` are deliberately untouched. - Carry the one-time bearer spending key through `Zeroizing` on the remaining generate/derive helpers: the JNI `orchardAddressFromSpendingKey` input now uses `read_key32_zeroizing` (matching `oneTimeSk`), and `generate_one_time_orchard_key` wraps its in-loop draw so REJECTED draws are scrubbed too and the accepted key travels out still wrapped — which also covers the FFI export's early-return paths that its explicit `zeroize()` missed (that call is now redundant and removed). Note `orchard_address_from_spending_key` takes the key BY VALUE, so the caller-frame `Zeroizing` in `platform_wallet_orchard_address_from_spending_key` scrubs that frame only; this is documented at the call site rather than overstated as eliminating the plaintext copy. - Strip the signer's internal machine prefix (`DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX`) from rendered messages on both conversion paths in `platform-wallet-ffi`. Both read the prefix to pick the typed code BEFORE stripping, so classification is unaffected, and the host-side fallback matcher keys on the human tail (`DashSdkError.MESSAGE_MARKER`), not the prefix. - Fix the markdownlint MD038 trailing-space-inside-code-span in `KOTLIN_MIGRATION_LEFTOVERS.md` and `KOTLIN_SWIFT_SHARED_PARITY_SPEC.md`. Also applies `cargo fmt` to the five pre-existing formatting violations in files this PR already owns, so `cargo fmt --check` passes clean.
…-> 37 and mirror it (#4204) 32 is allocated to `ErrorTransactionBuild` (#4247, also carried by #4256) in ERROR_CODE_REGISTRY.md (#4261). This variant took 32 without a registry row, so the two collide as a hard `E0081: discriminant value 32 assigned more than once` the moment both land — reproduced on a real integration merge, not hypothetical. 27-36 are all claimed (27 ErrorShutdownIncomplete via the merged #4268; 29 #4184; 31 #4183; 32/33 37 is the allocation frontier. The code was also unmirrored on BOTH hosts, which is the more dangerous half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its identity; Kotlin fell through to Generic(32), and in any tree carrying "shielded invite already claimed" as "reservation wallet mismatch". That matters on the claim-recovery path specifically — the error is raised from four sites in shielded/operations.rs, three inside the recovery function. Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal, inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift reservation comment the registry asked the next toucher to drop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pplied-fallback verdict, terminal code at the FFI, Swift mirror, Orchard secret scrubbing (#4204) Five blocking findings from the 2026-08-03 gate run, fixed on the rebased head: * a00cee018e73 — the two POST-BUILD `NullifierAlreadySpent` recovery arms (broadcast + result wait) now pass `Some(identity_id)` — the id THIS transition committed — instead of the pre-build `expected_identity_id`, which is deliberately None for a padded single-note bundle. The SDK's broadcast retries internally, so an accepted-then-lost-ack first request legitimately yields NullifierAlreadySpent on the wire retry; with None the reconciler declared our own successfully created identity permanently lost. `expected_identity_id` remains for the pre-build preflight, where the randomized padding id is genuinely unavailable. * 8d020115b274 — the wait-path consensus-verdict arm no longer converts an APPLIED chargeable fallback into ShieldedBroadcastFailed (code 16, documented as definitive non-execution and retryable): a duplicate unique-key hash makes Type 20 apply the chargeable UnshieldAction — nullifiers consumed, fallback address credited minus the penalty — and its PaidConsensusError reaches the wait as a populated cause. The arm now verifies the selected nullifiers first; consumed notes route to the reconciler for the terminal claimed/fallback verdict (recovered success when this claim created the identity, terminal ShieldedInviteAlreadyClaimed for the fallback / a competing holder). * 7be05fde0d09 — the live claim FFI export routes ShieldedInviteAlreadyClaimed through the blanket From<PlatformWalletError> conversion (code 37) before the catch-all, which was flattening it to the generic ErrorWalletOperation (6) and made the terminal consumed-invitation discriminator unreachable from the one API that produces it. * 00b4b4d41758 — the Swift mirror is complete and compiles: public `PlatformWalletError.shieldedInviteAlreadyClaimed(String)` case, errorDescription coverage, and the `.errorShieldedInviteAlreadyClaimed` arm in `init(result:)` (the exhaustive switch previously rejected the new enum case). Verified with swiftc -parse. * 1ee08ba70627 — Orchard spend-authority representations are no longer left unscrubbed: a `ScrubOnDrop` guard (volatile per-byte overwrite + fence on every exit path, gated on `needs_drop` absence with a tripwire test) contains the non-zeroizing `SpendingKey` / `SpendAuthorizingKey` in the one-time-key claim (sk dropped right after derivation, ask right after the bundle build — neither survives the network awaits), in `OrchardKeySet::from_seed`, in the one-time keygen acceptance loop, and in `orchard_address_from_spending_key`, which now also takes the scalar BY REFERENCE so callers' Zeroizing buffers are not repeated as plain arrays at the boundary. platform-wallet 672/672, platform-wallet-ffi 228/228, JNI + FFI cargo check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ency #4277 (merged into v4.2-dev) promoted `rand = "0.8"` from a dev-dependency to a mandatory entry in `[dependencies]`. This PR had added its own `rand = { version = "0.8", optional = true }` to the same table for the one-time Orchard key CSPRNG, and because the two lines sit in different parts of the table git merged both without a textual conflict — producing a manifest that cargo rejects outright: error: duplicate key --> packages/rs-platform-wallet/Cargo.toml:75:1 error: failed to load manifest for workspace member `.../packages/rs-platform-wallet` `cargo metadata` fails before any build starts, which is why the Kotlin SDK CI job died in the "Building rs-unified-sdk-jni" step rather than in the tests. `rand` is now unconditionally available, so this PR does not need to declare it at all: remove the optional duplicate and drop the now-invalid `dep:rand` from the `shielded` feature list (cargo rejects `dep:` on a non-optional dependency). `shielded::keys::generate_one_time_orchard_key` keeps using `OsRng` from the same crate at the same major version — no behaviour change. Verified with `cargo metadata`, `cargo check -p platform-wallet` (default and `--features shielded`) and `cargo check -p platform-wallet-ffi --features shielded`. Cargo.lock is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Orchard-secret `ScrubOnDrop(...)` wrapping added in the review-gate round left `keys.rs` with a `cargo fmt --check --all` drift (the `SpendingKey::from_zip32_seed(..).map_err(..)` argument was not re-wrapped to rustfmt's default layout). Purely cosmetic re-wrap; no behavior change. Restores a clean `cargo fmt --check --all` so the Formatting & Linting CI step passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ase merge) (#4204) The Kotlin SDK native-build CI (which compiles `refs/pull/4204/merge`, i.e. this PR merged into v4.2-dev) failed with: error[E0432]: unresolved import `rand` (shielded/keys.rs) Root cause: v4.2-dev advanced to remove `rand` from `[dependencies]` (it is now dev-only) and to drop `log` from `[dependencies]` entirely. Commit 806d198 had removed this PR's own `rand` declaration on the (now-false) premise that base provides `rand` unconditionally. The head still built because its merge-base copy of those lines was present, but the 3-way merge into the advanced base deletes them, leaving the PR's added lib code with no `rand`/`log`: * `shielded::keys::generate_one_time_orchard_key` uses `rand::OsRng` (shielded) * `identity::network::encrypted_document` uses `rand::OsRng` and the `log` facade (`log::debug!`/`log::warn!`) unconditionally Fix: declare `rand = "0.8"` and `log = "0.4"` as this PR's own `[dependencies]` inside the PR-authored comment block (a head-only region base does not have, so it survives the merge), and align the "Standard dependencies" `rand`/`log` lines to base's edited form so those regions merge without conflict or duplicate keys. Manifest-only; no code or feature-gate change. Verified by reproducing the exact CI merge locally (merge head into v4.2-dev tip 5bbd7c9) and building platform-wallet + platform-wallet-ffi with `shielded`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. Walkthrough
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. Pre-merge checks:
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description Check | |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title "feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim)" matches the main change. The PR adds one-time Orchard key support to the Kotlin SDK, covering key generation, a… |
Full details: Title check
-
Explanation
The title "feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim)" matches the main change. The PR adds one-time Orchard key support to the Kotlin SDK, covering key generation, address derivation, and shielded identity creation for inviter and claim flows. The summary's changes span the Kotlin SDK, JNI, FFI, and platform-wallet Rust layers, which the title covers at a high level. The title does not name the Rust or FFI layers, but it does not need to list every detail.
Finishing Touches
-
Generate docstrings
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
port/v4.1/shielded-invites
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c31b73d7-b812-43bf-9299-c2ee40e0d710
📒 Files selected for processing (1)
-
packages/rs-platform-wallet/src/wallet/shielded/operations.rs
🚧 Files skipped from review as they are similar to previous changes (1)
-
- packages/rs-platform-wallet/src/wallet/shielded/operations.rs
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID:
c31b73d7-b812-43bf-9299-c2ee40e0d710
📒 Files selected for processing (1)
-
packages/rs-platform-wallet/src/wallet/shielded/operations.rs
🚧 Files skipped from review as they are similar to previous changes (1)
-
- packages/rs-platform-wallet/src/wallet/shielded/operations.rs
-
bfoss765
changed the title
|
⛔ Final review complete — 6 blocking finding(s) (commit b42bc95) · triage: critical |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v5.1-dev #4313 +/- ##
============================================
- Coverage 87.00% 84.43% -2.58%
============================================
Files 2773 2748 -25
Lines 357184 371717 +14533
============================================
+ Hits 310780 313844 +3064
- Misses 46404 57873 +11469
🚀 New features to boost your workflow:
|
|
@bfoss765 I diagnosed the current I amended the introducing commit to remove only the unused
Replacement head: thepastaclaw@1ea5340 (branch I cannot update |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 4390cd7, all three carried-forward predecessor findings remain valid: two blocking claim recovery/classification defects and one full-history scan suggestion. The full current-PR range adds one genuinely new blocker—the unused direct log dependency fails the mandatory dependency audit; no predecessor finding is fixed, outdated, or deferred, and there are no exceptional out-of-scope follow-ups.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/security-auditor=gpt-5.6-sol(completed); codex/ffi-engineer=gpt-5.6-sol(completed); verifier=codex/verifier=gpt-5.6-sol(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only)..
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1930-1933: Persist a recovery path before returning an unconfirmed claim
The one-time claim broadcasts the constructed transition at lines 1765-1774 without durably recording its serialized bytes, exact identity ID, selected nullifiers, submitted key bindings, or identity index. If execution succeeds but confirmation fails, this branch returns the only exact ID through the transient FFI/JNI result; process death or Kotlin cancellation during the synchronous native handoff can discard it, and `poke_sync_on_unconfirmed` has no foreign-key claim record to reconcile. A normal single-note retry cannot reproduce that ID because the randomly generated padding nullifier participates in it (`expected_identity_id` is `None` at lines 1708-1715), so the spent-nullifier preflight reaches terminal `ShieldedInviteAlreadyClaimed` at lines 3079-3089 even when this wallet's original transition created the identity. Persist sufficient pending-claim metadata before broadcast and automatically reconcile or re-drive the byte-identical transition after cancellation or restart.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1859-1871: Do not classify an applied chargeable fallback as retryable broadcast failure
This consensus-verdict branch recognizes an applied Type-20 chargeable fallback only when a separate nullifier query returns a positive spent status. `any_nullifier_spent_on_chain` maps `Ok(None)` and every transport, query, or proof error to `false` at lines 2896-2907, so a fallback that already consumed the invitation and credited the failure address can still be returned as `ShieldedBroadcastFailed`. Native code 16 and Kotlin explicitly describe that outcome as definitive non-execution and retryable, which is false after the fallback has applied. Even when the query succeeds, a collision on a submitted unique key other than MASTER leaves no identity under either the MASTER-key lookup or the transition's derived ID, causing the reconciler at lines 3092-3159 to return `ShieldedBroadcastUnconfirmed` instead of the terminal fallback result. Preserve unknown nullifier status separately from unspent status and classify the authenticated chargeable verdict without assuming the colliding key was MASTER.
In `packages/rs-platform-wallet/Cargo.toml`:
- [BLOCKING] packages/rs-platform-wallet/Cargo.toml:75: Remove the unused `log` dependency so cargo-machete passes
The exact reviewed head adds `log = "0.4"` as a direct runtime dependency, but no source in `rs-platform-wallet` imports or references the `log` facade; the nearby comment refers to an `identity::network::encrypted_document` module that is not present in the current crate. Both Rust CI workflows run `cargo machete`, and current PR comment 5199766972 confirms that this dependency causes the workspace test failure. The proposed replacement commit 1ea5340c7f998425e91e21eb05ec3fca9f0823eb removes it, but that commit is not the authoritative reviewed head. Remove `log` and its lockfile entry while retaining `rand`, which the current library code uses.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:835-860: Unfunded invitation keys force an unbounded full-history scan
Every syntactically valid foreign invitation key starts the proof-verified note stream at position zero with no cancellation token, chunk limit, or total-work budget. The only early exit is accumulating the requested denomination, so a valid but unfunded key downloads, verifies, and trial-decrypts the complete shielded history through the current tip. This attacker-controlled work grows with the pool and can be repeated to consume bandwidth, CPU, battery, memory, and a JNI worker; the supplied birth-height remains advisory only, and Kotlin coroutine cancellation cannot interrupt the synchronous native scan. Add native cancellation with a resumable work budget, an authenticated starting position, or another strict per-invitation bound.
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>
github-actions
Bot
removed
the
bot-review-skipped
…d-invites Conflict notes: - FFI codes: v5.1-dev took 48 (ErrorAssetLockInputContested) and the frontier now stands at 61, so ErrorShieldedClaimUnconfirmed moves 48 -> 61 in error.rs, ERROR_CODE_REGISTRY.md, DashSdkError.kt, the Swift mirror and the Rust/Kotlin/Swift pin tests. 43-45 are unchanged (still recorded for this PR in the registry). - PendingRedrive carries both identity_index (this branch) and the identity_nonce_finalized / identity_user_abandoned flags (v5.1-dev); the pending_spends schema, rehydrate, read_pending_row and arm paths persist all three. - Rehydrate keeps v5.1-dev's fail-closed recovery classification and this branch's poison-mapped pending_conn lock and shared hydrate_pending_row. - Resume paths use PlatformDeserializableTrusted (StateTransition no longer implements PlatformDeserializable on v5.1-dev). - docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md and docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md: took v5.1-dev's deletion; this branch only reworded two lines in them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…te[] read fails read_opt_bytes hardcoded "surplusOutput byte[] was invalid", but the one-time-claim entry points also use it for fallbackAddress. Take the argument name from the caller, as read_id32 and read_recipient43 do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ode 62 A one-time-key claim retry whose arguments differ from the transition the earlier attempt submitted is refused fail-closed: nothing is rebroadcast and the pending claim record is kept for a rerun with the original arguments. The claim entry point's catch-all flattened it to the generic ErrorWalletOperation (6), which Kotlin maps to the non-retryable PlatformWallet.WalletOperation, so hosts abandoned a recoverable claim. Allocate 62 from the registry frontier, map the variant in the blanket conversion and pass it through the claim entry point's typed arm, and mirror it in Kotlin (PlatformWallet.ShieldedClaimBindingMismatch, not retryable unchanged) and Swift (raw case, init(ffi:), typed case, errorDescription), with pins in each layer's tests and the registry row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…binds over them finish_deferred_purges computed the union of the in-memory and durable removal tombstones, then handed each id to a helper that returned at once for ids missing from the in-memory set. A tombstone a peer coordinator wrote after this one opened was therefore counted forever and never purged. register_locked used the same gate and, when a required purge failed, installed the new binding anyway over the removed wallet's surviving notes, pending claims and stale advance-only watermark. The purge attempt is now unconditional and reports Done / Busy / Failed; finish_deferred_purges runs it for every id in the durable union. register_locked checks the in-memory set and, when nothing is registered for the id, the durable tombstones, and refuses the registration (ShieldedLifecycleBusy while a claim holds the purge off, ShieldedStoreError otherwise) until the purge and the tombstone clear have succeeded. The bind FFI keeps ShieldedLifecycleBusy typed (45) instead of flattening it to 6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… removal on its own Destructive admission counted live claim leases only. A lease is reaped at expires_at and cannot be renewed back, so a clock jump, a suspended process or a run of renewal failures past CLAIM_LEASE_MS made a claim whose transition was already broadcast invisible to clear()/unregister_wallet, which then deleted the only row holding its padded identity id. begin_destructive_admission now also counts every durable claim record armed under ONE_TIME_CLAIM_RECORDS_ACCOUNT in scope (in-memory and file store; the file store reads the durable table so a peer store's record is seen). The record leaves only on a terminal claim outcome or a durably acknowledged registration, which is what releases the block. Addresses #4313 review thread r3839256998. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…durable, retryable registration The registration tail released the claim's recovery record as soon as persister.store returned Ok. store may buffer until flush (store_commits_inline() defaults to false), so a crash could lose both the identity and the only copy of its padded id. And a store that failed after the in-memory insert made every same-session retry hit IdentityAlreadyExists, so persistence was never retried and the record could never be released. - New IdentityManager::add_or_repersist_identity: idempotent for an identity already registered at exactly this wallet and slot (re-hands its changeset to the persister); any other location is still IdentityAlreadyExists. - The tail moves to PlatformWallet::register_one_time_claim_identity and flushes (outside the wallet-manager lock) unless store_commits_inline(); the record is released only after both store and flush succeed. NoPlatformPersistence flushes Ok and so releases, since nothing would ever acknowledge more. Addresses #4313 review thread r3895896338. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
read_pending_row treated a claim row with v5.1-dev's identity-debit recovery flags set as corrupt and reported no record. The claim would then rebuild and overwrite the only copy of a possibly-broadcast transition's padded identity id. The flags never apply to a claim record, so they are now logged and ignored; resume still re-validates the stored transition against the retry's arguments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…split Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
|
@QuantumExplorer, the funding-layout decision request for this PR and #4312 is here: #4312 (comment). The PastaClaw blockers on this PR are addressed at |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of eccc65f confirms ten deduplicated blocking issues involving durable registration, recovery-record retirement, lifecycle fencing and mobile contracts; the prior code-62 mapping finding is fixed. No builds or tests were run: current exact-head checks show policy/title checks passing and PR Hygiene pending, but no build/test results. The newer base conflicts in ERROR_CODE_REGISTRY.md; three verified non-blocking observations were omitted to keep the inline findings within the ten-comment budget.
🔴 10 blocking
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Validate purge ownership in the same transaction as deletion
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1334-1342
This deletion does not validate either the removal generation or the destructive admission that authorized it. Two concrete interleavings make that destructive: finish_deferred_purges can snapshot wallet W, pause while register_locked finishes W's removal and restores a new binding, then resume and unconditionally erase the replacement ledger and ordinary pending-spend rows. Separately, a process can obtain destructive admission with zero claims, suspend beyond the 60-second barrier deadline, and resume after a peer has reaped the barrier and armed a new claim; purge_wallet or purge_all_subwallets still deletes that protected recovery row. Counting armed records only during the earlier admission transaction does not protect the later mutation. Make store-level deletion conditional on current destructive ownership and protected-claim checks, and atomically consume the applicable removal tombstone when performing deferred deletion. Serialize the coordinator's corresponding in-memory cleanup with installation so a stale snapshot cannot erase a replacement registration. Add interleaving coverage for both re-registration and an expired destructive token.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 14: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in shielded/operations.rs, shielded/keys.rs, and shielded/file_store.rs implement spending invitation-funded notes, handling one-time Orchard secret keys, and durable claim recovery, directly changing funds movement and key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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:1334-1342: Validate purge ownership in the same transaction as deletion
This deletion does not validate either the removal generation or the destructive admission that authorized it. Two concrete interleavings make that destructive: finish_deferred_purges can snapshot wallet W, pause while register_locked finishes W's removal and restores a new binding, then resume and unconditionally erase the replacement ledger and ordinary pending-spend rows. Separately, a process can obtain destructive admission with zero claims, suspend beyond the 60-second barrier deadline, and resume after a peer has reaped the barrier and armed a new claim; purge_wallet or purge_all_subwallets still deletes that protected recovery row. Counting armed records only during the earlier admission transaction does not protect the later mutation. Make store-level deletion conditional on current destructive ownership and protected-claim checks, and atomically consume the applicable removal tombstone when performing deferred deletion. Serialize the coordinator's corresponding in-memory cleanup with installation so a stale snapshot cannot erase a replacement registration. Add interleaving coverage for both re-registration and an expired destructive token.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1938-1951: Provide a terminating recovery path for armed claim holds
Every armed claim now blocks wallet removal and coordinator-wide Clear indefinitely, but some supported states have no release path. For example, an inviter can spend the bearer note to another shielded address after the victim's preflight. The victim arms its record, then receives NullifierAlreadySpent; every subsequent resume sees spent notes, calls recovery with spend_finalized=false, finds no matching identity and returns unconfirmed forever. A lost verdict after a non-MASTER-key chargeable fallback has the same result. Losing the invitation link also prevents entry into the only claim reconciliation path. Ordinary sync does not process the reserved claim account, and identity-debit enumeration/abandonment explicitly excludes nonempty-nullifier records. Registration failures worsen this: register_one_time_claim_identity logs store/flush failures and returns (), so the public method reports success even though a retained row still requires another invitation retry. Add Rust-owned reconciliation from persisted claim data and an explicit user-acknowledged abandonment/removal path. Surface incomplete registration unless automatic reconciliation will finish it; do not expire or silently discard potentially live records.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:701-707: Do not report damaged claim metadata as an absent recovery record
A bad anchor width or malformed nullifier metadata returns Ok(None) even when st_bytes still contains the valid Type-20 transition and its exact padded identity ID. reserve_one_time_claim_key presents that None to the caller as permission to take the fresh-claim path. This survives reopening because rehydrate_pending_spends deliberately permits damaged ancillary metadata to be skipped when the bytes identify a note-spending transition, including IdentityCreateFromShieldedPool. If the original single-note claim executed, the fresh scan finds its note spent, cannot reconstruct the random padding nullifier and reports terminal ShieldedInviteAlreadyClaimed instead of recovering the identity retained in st_bytes. Meanwhile, the durable row still counts toward the teardown hold. Return a corruption error while preserving the record, or reconstruct and validate redundant metadata from its stored transition; do not equate unreadable metadata with absence. Cover valid Type-20 bytes with malformed ancillary columns.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1140-1151: Persist removal intent before committing the detach
on_admitted has already irreversibly detached the wallet and its registrations have been removed before this durable tombstone write. If storage fails after admission, both record_wallet_removal and the subsequent purge_wallet can fail, yet removal still returns success. Only deferred_purges remembers the unfinished wipe, and that set disappears on restart. Startup and re-registration then have no tombstone identifying the surviving pending-spend rows as residue from a removed wallet. The existing purge-only failure injection leaves tombstone writes working and does not exercise this sequence. Persist removal intent while destructive admission is held but before invoking on_admitted or mutating registrations; if that prerequisite fails, release admission and leave the wallet intact. Once intent is durable, post-detach purge failures can safely use the existing deferred-completion contract.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:4189-4193: Require authenticated terminal evidence before deleting a claim record
The outcomes authorizing irreversible cleanup are weaker than this recovery record requires. On resume, status was queried before rebroadcast, while ShieldedBroadcastFailed can come from broadcast_definitely_failed treating Unavailable as definitive. That classifier explicitly documents delivery followed by a lost acknowledgement, including an earlier accepted SDK retry hidden by the final transport error. This branch can therefore delete the original padded identity ID and rebuild while its transition is executing. The terminal cleanup path has another instance of the same problem: the wait-side consensus cause is accepted from DAPI before proof verification, but the wallet passes spend_finalized=true when it sees that payload plus spent nullifiers. Both identity-fetch helpers collapse query/proof failures into None, so an endpoint can relay a successful claim, fabricate an error verdict and fail the identity queries, causing terminal cleanup of the successful claim's record. Keep exact records for ambiguous delivery and unknown lookup outcomes. Require authenticated terminal-state evidence, rather than transport classification or an RPC error payload, before retiring them.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3063-3070: Fence claim progression and deletion by reservation and record generation
A caller that received ClaimKeyReservation::Held still enters resume_one_time_claim here; owns_claim_key is checked only after resume returns RecordUnusable. Resume can rebroadcast and delete the row, and assert_claim_lease_before_chargeable_step verifies only that caller's wallet lease, not its invitation reservation. With two coordinators sharing SQLite, both can resume old record R1: owner A rejects and clears R1, builds replacement R2 and arms it under the same activity_id; non-owner B's delayed rejection of R1 then calls clear_redrive and deletes R2. The SQL delete checks only wallet/account/activity coordinates, so A can broadcast R2 after its only recovery record has disappeared. Make progression exclusive at the store-owned invitation reservation boundary, and make retirement an atomic compare-and-delete of the expected transition or generation. Apply the same fencing to finalization and registration acknowledgement, not only arming. Add a cross-store test that delays old-record cleanup until after replacement arming.
In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/manager.rs:880-885: Preserve Swift signing keys when native removal is refused
This change makes native removal fail before mutation when shielded destructive admission cannot be obtained, but Swift's deleteWallet still erases all per-identity Keychain entries and wallet-owned identity private keys before calling platform_wallet_manager_remove_wallet (PlatformWalletManager.swift:2678–2689). A busy refusal, or a store error while acquiring the new admission, now leaves the native wallet registered and its persisted identity rows intact after Swift has deleted their signing material. Swift's active-operation gate cannot prevent an unresolved durable claim record or an admission I/O failure. Update the consumer alongside this new fallible contract: obtain successful Rust-owned removal before deleting Keychain material, while retaining Keychain cleanup before the SwiftData cascade. A refused removal must leave both native state and host signing keys intact.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:1963-1967: Keep claim domain validation in its Rust owner
The new wrapper independently enforces a positive denomination and a nonempty registration key set instead of only marshalling the request. The Rust claim operation already rejects empty public keys, the DPP identity-create builder repeats that rule, and Rust note selection/DPP own denomination validity. The C claim shim adds another key-count copy at shielded_send.rs:1258. These are domain rules, not buffer-safety checks. The book's 'Validation lives once' rule explicitly prohibits SDK/FFI copies of DPP range and count validation, and 'FFI and mobile SDKs hold no business logic' requires these wrappers to marshal rather than decide. The Kotlin checks also bypass mapNativeErrors with IllegalArgumentException. Remove the duplicated domain checks and use the owning Rust operation's result; retain only pointer, buffer and signed-to-unsigned conversion checks needed to represent the request safely at the ABI boundary.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:1982-1983: Preserve resume semantics for ordinary unconfirmed claims
The claim uses the pool-create decoder, but those operations have different recovery contracts. Ordinary broadcast/result-wait ambiguity becomes native code 17, JNI encodes tag 1, and decodeShieldedCreatePayload throws ShieldedCreateUnconfirmed. That type inherits isRetryable=false and appends 'do NOT retry'. For this operation, rerunning the same invitation claim is precisely how resume_one_time_claim safely replays the persisted transition and completes registration. The reserved claim account is not part of ordinary background redrive, so poking sync cannot replace that retry. A host following the published Kotlin contract can therefore leave an undelivered claim permanently pending, with its armed row also blocking teardown. Code 61 covers caught panics only and does not repair this normal network-error path. Carry a claim-specific, resume-capable ambiguous outcome through Rust/C/JNI/Kotlin while preserving the known identity ID and original slot.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1870-1873: Retain the recovery record until the identity is durably registered
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r3895896338)
The flush, matching-slot re-persistence and armed-record fence fix those portions of the original finding, but this predicate still acknowledges identity persistence that did not happen. NoPlatformPersistence returns success without retaining anything. More importantly, a supported FFIPersister callback table can omit on_persist_identities_fn, silently skip the identity changeset, and nevertheless report store_commits_inline() merely because on_changeset_end_fn exists; an absent flush callback also returns success. The recovery row is then deleted before the C/JNI caller receives the result, without another durable copy of the padded identity ID. A crash during that handoff leaves a single-note retry taking the no-record, already-claimed terminal path. An explicit memory-only exception does not cover incompletely wired hosts or acknowledge receipt by the caller. Require an attested identity-write/restoration capability before starting this claim, or retain a durable result until an explicit handoff is acknowledged. Refusing unsupported configurations before spending avoids creating an indefinite hold.
| // A failure to WRITE the tombstone cannot abort the removal either — | ||
| // everything above it is already committed — so it is logged and the | ||
| // in-memory deferral below still stands. That degrades to exactly the | ||
| // pre-fix behaviour for this one wallet, which is the best available | ||
| // when the store will not accept any write at all. | ||
| if let Err(e) = self.store.write().await.record_wallet_removal(wallet_id) { | ||
| tracing::error!( | ||
| wallet_id = %hex::encode(wallet_id), | ||
| error = %e, | ||
| "Failed to record the shielded removal tombstone; the removal stands, but an \ | ||
| unfinished purge will not survive a restart" | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Persist removal intent before committing the detach
on_admitted has already irreversibly detached the wallet and its registrations have been removed before this durable tombstone write. If storage fails after admission, both record_wallet_removal and the subsequent purge_wallet can fail, yet removal still returns success. Only deferred_purges remembers the unfinished wipe, and that set disappears on restart. Startup and re-registration then have no tombstone identifying the surviving pending-spend rows as residue from a removed wallet. The existing purge-only failure injection leaves tombstone writes working and does not exercise this sequence. Persist removal intent while destructive admission is held but before invoking on_admitted or mutating registrations; if that prerequisite fails, release admission and leave the wallet intact. Once intent is durable, post-detach purge failures can safely use the existing deferred-completion contract.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Agreed this is real. Deferred to one design pass over the claim-record state machine and removal lifecycle, once the funding layout is decided (#4312 (comment)). Agreed on the ordering (tombstone durable before detach). It will be fixed together with purge ownership as one change: intent → admission → detach → purge, with the store checking ownership at delete time.
There was a problem hiding this comment.
Combining the ordering and purge-ownership fixes makes sense, but intent → admission needs adjusting: acquire destructive admission, durably write intent, then detach and purge. Otherwise a refused admission can leave a removal tombstone for a wallet that was never removed. This removal contract is part of this PR, so the combined fix needs to land here rather than become a post-merge follow-up.
| "one-time claim resume: stored transition is definitively rejected and its notes \ | ||
| are unspent; dropping the record and rebuilding" | ||
| ); | ||
| return; | ||
| clear_one_time_claim_record(store, claim_records_id, record.activity_id).await; | ||
| return OneTimeClaimResume::RecordUnusable; |
There was a problem hiding this comment.
🔴 Blocking: Require authenticated terminal evidence before deleting a claim record
The outcomes authorizing irreversible cleanup are weaker than this recovery record requires. On resume, status was queried before rebroadcast, while ShieldedBroadcastFailed can come from broadcast_definitely_failed treating Unavailable as definitive. That classifier explicitly documents delivery followed by a lost acknowledgement, including an earlier accepted SDK retry hidden by the final transport error. This branch can therefore delete the original padded identity ID and rebuild while its transition is executing. The terminal cleanup path has another instance of the same problem: the wait-side consensus cause is accepted from DAPI before proof verification, but the wallet passes spend_finalized=true when it sees that payload plus spent nullifiers. Both identity-fetch helpers collapse query/proof failures into None, so an endpoint can relay a successful claim, fabricate an error verdict and fail the identity queries, causing terminal cleanup of the successful claim's record. Keep exact records for ambiguous delivery and unknown lookup outcomes. Require authenticated terminal-state evidence, rather than transport classification or an RPC error payload, before retiring them.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Agreed this is real. Deferred to one design pass over the claim-record state machine and removal lifecycle, once the funding layout is decided (#4312 (comment)). The rule will be that only a proof-verified terminal state retires a record, never a transport classification or an unverified DAPI error.
There was a problem hiding this comment.
That is the right retirement rule, with terminal evidence tied to the exact stored claim and its outcome. A proof that the nullifiers are spent is not sufficient by itself: it does not distinguish successful identity creation from a fallback or competing spend, and failed identity queries must remain unknown. I agree with that fix direction, but this claim-record safety rule needs to be implemented before this PR merges.
The finding stands on this commit until that change is pushed; the push gets a full review.
| // store's startup-hydrated mirror (#4313 review finding r3767229122). | ||
| if let Some(record) = pending_record { | ||
| match resume_one_time_claim( | ||
| sdk, | ||
| store, | ||
| claim_records_id, | ||
| &record, | ||
| admission, |
There was a problem hiding this comment.
🔴 Blocking: Fence claim progression and deletion by reservation and record generation
A caller that received ClaimKeyReservation::Held still enters resume_one_time_claim here; owns_claim_key is checked only after resume returns RecordUnusable. Resume can rebroadcast and delete the row, and assert_claim_lease_before_chargeable_step verifies only that caller's wallet lease, not its invitation reservation. With two coordinators sharing SQLite, both can resume old record R1: owner A rejects and clears R1, builds replacement R2 and arms it under the same activity_id; non-owner B's delayed rejection of R1 then calls clear_redrive and deletes R2. The SQL delete checks only wallet/account/activity coordinates, so A can broadcast R2 after its only recovery record has disappeared. Make progression exclusive at the store-owned invitation reservation boundary, and make retirement an atomic compare-and-delete of the expected transition or generation. Apply the same fencing to finalization and registration acknowledgement, not only arming. Add a cross-store test that delays old-record cleanup until after replacement arming.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Agreed this is real. Deferred to one design pass over the claim-record state machine and removal lifecycle, once the funding layout is decided (#4312 (comment)). If one-note is accepted, the second claimant is refused at reservation. If two-note is chosen, we add compare-and-delete by generation.
There was a problem hiding this comment.
Refusing a second claimant at reservation would fix the currently held-reservation path, but single-note funding does not eliminate stale-owner deletion. An old owner can pause past lease expiry, a new owner can replace the record under the same key, and the old owner's delayed cleanup can still delete that replacement. The generation or expected-transition check therefore needs to protect retirement and registration acknowledgement regardless of note count; those calls currently carry only the record coordinates.
| let armed_claim_records: i64 = tx | ||
| .query_row( | ||
| "SELECT COUNT(*) FROM shielded_pending_spends \ | ||
| WHERE account_index = ?1 AND (?2 IS NULL OR wallet_id = ?2)", | ||
| rusqlite::params![ | ||
| super::operations::ONE_TIME_CLAIM_RECORDS_ACCOUNT, | ||
| scope_bytes | ||
| ], | ||
| |row| row.get(0), | ||
| ) | ||
| .map_err(|e| { | ||
| FileShieldedStoreError::Storage(format!("count armed claim records: {e}")) | ||
| })?; | ||
| let live = live.max(0) + armed_claim_records.max(0); |
There was a problem hiding this comment.
🔴 Blocking: Provide a terminating recovery path for armed claim holds
Every armed claim now blocks wallet removal and coordinator-wide Clear indefinitely, but some supported states have no release path. For example, an inviter can spend the bearer note to another shielded address after the victim's preflight. The victim arms its record, then receives NullifierAlreadySpent; every subsequent resume sees spent notes, calls recovery with spend_finalized=false, finds no matching identity and returns unconfirmed forever. A lost verdict after a non-MASTER-key chargeable fallback has the same result. Losing the invitation link also prevents entry into the only claim reconciliation path. Ordinary sync does not process the reserved claim account, and identity-debit enumeration/abandonment explicitly excludes nonempty-nullifier records. Registration failures worsen this: register_one_time_claim_identity logs store/flush failures and returns (), so the public method reports success even though a retained row still requires another invitation retry. Add Rust-owned reconciliation from persisted claim data and an explicit user-acknowledged abandonment/removal path. Surface incomplete registration unless automatic reconciliation will finish it; do not expire or silently discard potentially live records.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Agreed this is real. Deferred to one design pass over the claim-record state machine and removal lifecycle, once the funding layout is decided (#4312 (comment)). The fix is an explicit, acknowledged abandon call (new public API, which we asked the maintainer about) plus Rust-owned reconciliation from the persisted claim data.
There was a problem hiding this comment.
An explicitly acknowledged abandon API plus Rust-owned reconciliation from the persisted record is the right recovery shape. Reconciliation needs to work without the invitation link and retry incomplete local registration, or the public claim call needs to surface that incomplete registration instead of returning success. That addresses this concern as a proposed fix, but the terminating recovery path needs to ship with the holds this PR introduces.
The finding stands on this commit until that change is pushed; the push gets a full review.
| require(denomination > 0) { "denomination must be positive, got $denomination" } | ||
| require(fallbackAddress.size == 21) { | ||
| "fallbackAddress must be 21 bytes, got ${fallbackAddress.size}" | ||
| } | ||
| require(keys.isNotEmpty()) { "keys must not be empty" } |
There was a problem hiding this comment.
✅ Resolved at
b686ca9c; see the replies below.
🔴 Blocking: Keep claim domain validation in its Rust owner
The new wrapper independently enforces a positive denomination and a nonempty registration key set instead of only marshalling the request. The Rust claim operation already rejects empty public keys, the DPP identity-create builder repeats that rule, and Rust note selection/DPP own denomination validity. The C claim shim adds another key-count copy at shielded_send.rs:1258. These are domain rules, not buffer-safety checks. The book's 'Validation lives once' rule explicitly prohibits SDK/FFI copies of DPP range and count validation, and 'FFI and mobile SDKs hold no business logic' requires these wrappers to marshal rather than decide. The Kotlin checks also bypass mapNativeErrors with IllegalArgumentException. Remove the duplicated domain checks and use the owning Rust operation's result; retain only pointer, buffer and signed-to-unsigned conversion checks needed to represent the request safely at the ABI boundary.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 6dabafe. Kotlin no longer checks denomination or keys; Rust does. Only buffer sizes and the Long→u64 sign check remain. I kept the C identity_pubkeys_count == 0 check, because 5 other FFI entry points have the same guard, so removing it should be a crate-wide change.
There was a problem hiding this comment.
The Kotlin change addresses that part of the finding. The other FFI entry points do not require retaining this new claim shim's count guard: with a non-null, correctly aligned pointer and count zero, decode_identity_pubkeys produces an empty map, which the Rust claim operation already rejects. Please remove this guard locally; cleaning up the older entry points can remain a separate change.
There was a problem hiding this comment.
Resolved (re-reviewed at b686ca9c): Your Kotlin cleanup leaves only buffer and signed-to-unsigned representation checks in this wrapper. The remaining JNI helper policy is a separate boundary covered by 1f83a122ad81, not a reason to keep this Kotlin finding open.
…bsent read_pending_row returned "no record" for a claim row whose anchor or nullifier columns could not be decoded. The claim then took the fresh path, although the row's st_bytes may hold the only copy of an executed claim's padded identity id, so a fresh scan ends at a false "already claimed". It now returns RecoveryCorrupted and keeps the row; the reservation transaction rolls back with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
shieldedIdentityCreateFromOneTimeKey decoded an ambiguous outcome as the pool path's ShieldedCreateUnconfirmed, which is non-retryable and says "do NOT retry". For a claim, re-running the same claim is exactly how the persisted transition is replayed and registration finished. The claim now decodes that outcome to the retryable ShieldedClaimUnconfirmed, which gains an optional identityId (set here, null for the code-61 panic case). Also drops the wrapper's copies of Rust's domain rules (positive denomination, non-empty key set); only buffer sizes and the Long -> u64 sign check remain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…keys deleteWallet erased the Keychain identity keys before calling platform_wallet_manager_remove_wallet, which can now refuse (an unresolved shielded claim, or a store error taking destructive admission). A refusal left the native wallet and its identities registered with their keys gone. Native removal now runs first; the Keychain sweep still runs before the SwiftData cascade so a retry can find the identity ids. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against head 8f465f7: three prior blockers are fixed, while six remain around binding-layer validation and durable claim/removal lifecycle correctness; two additional suggestions are confirmed. This was a static review only, and git diff --check passed. Current CI still has Kotlin SDK validation, Rust workspace runner preparation, and the Swift frozen-schema check pending, so completed build/test validation for this head is not yet available.
🔴 6 blocking | 🟡 2 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Validate purge ownership in the same transaction as deletion
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1338-1343
This DELETE receives no destructive-admission token and does not recheck ownership or protected records. Admission counting occurred in an earlier committed transaction, and its barrier expires after 60 seconds. A remover can obtain zero-live admission, be suspended past that deadline, and resume after another store instance has reaped the barrier, admitted a claim, and armed/broadcast its recovery record. The unconditional DELETE then removes that new record. Counting armed records during admission does not close this check-to-delete window, and the coordinator-local locks cannot order independent SQLite connections. Pass the admission token into the destructive operation and validate its live ownership, scope, and current claim blockers inside the same IMMEDIATE transaction as deletion. Apply the same contract to whole-store purges and deferred cleanup.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in shielded/operations.rs, shielded/keys.rs, and shielded/file_store.rs implement spending invitation-funded notes, temporary secret-key handling, and persistent claim recovery, directly changing funds movement and key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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:1338-1343: Validate purge ownership in the same transaction as deletion
This DELETE receives no destructive-admission token and does not recheck ownership or protected records. Admission counting occurred in an earlier committed transaction, and its barrier expires after 60 seconds. A remover can obtain zero-live admission, be suspended past that deadline, and resume after another store instance has reaped the barrier, admitted a claim, and armed/broadcast its recovery record. The unconditional DELETE then removes that new record. Counting armed records during admission does not close this check-to-delete window, and the coordinator-local locks cannot order independent SQLite connections. Pass the admission token into the destructive operation and validate its live ownership, scope, and current claim blockers inside the same IMMEDIATE transaction as deletion. Apply the same contract to whole-store purges and deferred cleanup.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1942-1955: Provide a terminating recovery path for armed claim holds
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147976)
Counting durable claim records independently of leases correctly prevents unsafe purges, but some supported outcomes cannot release this hold through the public API. If another bearer spends the invitation after preflight, resume observes spent nullifiers, calls recovery with spend_finalized=false, and returns unconfirmed indefinitely when no matching identity exists. Losing the invitation link also prevents invoking the only claim-resume entry point. The existing identity-debit abandonment API explicitly excludes these nonempty-nullifier records, and background sync does not reconcile the reserved claim account. Consequently, wallet removal and coordinator-wide Clear can remain permanently busy after the claimant has stopped or restarted. Registration store/flush failures compound this: the claim reports success while retaining a blocking row that another invocation must resolve. Add Rust-owned recovery using persisted claim data and an explicitly acknowledged abandonment/removal route. Preserve the fail-closed hold rather than silently expiring potentially live records.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1269-1274: Keep claim domain validation in its Rust owner
The Kotlin cleanup removes its duplicated domain checks, but this C shim still independently rejects an empty registration key set, and the new JNI claim entry point still rejects denomination == 0 with its denomination <= 0 guard. Neither rule is required for marshaling: an empty vector is representable safely, and zero is representable as u64. The owning wallet operation already rejects empty public keys, while the DPP builder validates the protocol-versioned denomination set. The book's validation-single-owner and thin-binding rules explicitly prohibit these duplicate count/range decisions, even when they provide an earlier error. Remove the C nonempty-key and JNI zero-denomination checks and propagate the owner's result, retaining pointer/buffer checks and rejection of negative Java values that cannot represent the Rust input.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:2741-2746: Preserve typed claim-recovery corruption through the mobile boundary
The damaged-metadata fix now returns FileShieldedStoreError::RecoveryCorrupted, but this map_err immediately renders it into PlatformWalletError::Persistence. The claim FFI mapper then emits generic ErrorWalletOperation (6), so Kotlin receives WalletOperation rather than the existing ShieldedRecoveryCorrupted type for code 56; Swift likewise loses the typed corruption result. The row is safely retained, but hosts cannot distinguish damaged recovery state requiring repair from an ordinary operation failure. Preserve the structured store error through the owning operation, then pass the recovery variants through map_one_time_claim_result using the existing result-code allocation.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3064-3075: Fence claim progression and deletion by reservation and record generation
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147972)
The ownership refusal at line 3122 protects fresh construction, not this earlier resume path: a caller receiving ClaimKeyReservation::Held still resumes the owner's record. Resume can rebroadcast and delete, while its chargeable-step check verifies only the caller's wallet lease. Two coordinators can therefore resume R1 concurrently: owner A clears rejected R1 and arms replacement R2, then non-owner B's delayed R1 failure clears the same coordinates and deletes R2. clear_one_time_claim_record ultimately calls a coordinate-only delete, and the registration receipt likewise contains no expected transition or generation. The atomic reservation/read and foreign-reservation check during arming do not protect these later deletions. Require invitation ownership for mutating progression, and make cleanup, terminal finalization, and registration acknowledgement atomic compare-and-delete operations against the expected record generation or exact transition.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:4180-4193: Require authenticated terminal evidence before deleting a claim record
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147966)
The Unspent observation used here predates rebroadcast. ShieldedBroadcastFailed can then come from broadcast_definitely_failed treating Unavailable as definitive, although that helper explicitly documents that an earlier SDK attempt may have delivered the transition before its acknowledgement was lost. This branch can therefore delete the original padded identity ID while that transition is pending or executing. Another retirement route has the same authority problem: the SDK returns wait-side error envelopes before proof verification, yet the claim combines a populated error cause with spent nullifiers to set spend_finalized=true. Both identity lookup helpers collapse transport/proof failures into None, allowing failed lookups to become terminal ShieldedInviteAlreadyClaimed and clear the row. Retain the exact record when delivery or lookup results are unknown; retirement must use authenticated terminal evidence sufficient to resolve the stored transition, not a stale snapshot, transport classification, or unverified error envelope.
In `packages/rs-platform-wallet/src/wallet/shielded/keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/keys.rs:56: Track secret zeroization in the Orchard owner
ScrubOnDrop compensates for the lack of a safe secret-zeroization API by overwriting private Orchard representations from Platform. The pinned Orchard keys and RedPallas wrapper expose no Zeroize implementation or explicit scrubbing destructor, and the current dashified branch still lacks that API; the open issue/PR searches found no zeroization tracking. This representation-dependent workaround should have a linked upstream follow-up in dashpay/orchard, which owns these fields and can maintain their scrubbing contract as their layout evolves. Track a minimal safe-zeroization implementation and replacement of the local raw-memory workaround. A linked upstream issue or PR is sufficient; this does not require removing the current containment before upstream support exists.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1140-1151: Persist removal intent before committing the detach
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147958)
on_admitted() has already irreversibly detached the wallet and its coordinator registrations have been removed before this fallible tombstone write. If storage becomes unwritable after admission, both record_wallet_removal and purge_wallet can fail, but unregister_wallet_with still returns success. The host can then delete its wallet and signing-key records while only the process-local deferred set remembers the unfinished native wipe. Restart loses that set, and the startup/registration tombstone readers cannot discover a marker that was never committed. The existing purge-only failure injection deliberately leaves tombstone writes working, so it does not cover this sequence. Persist removal intent while destructive admission is held, before invoking the detach hook or changing registrations; if that prerequisite fails, release admission and leave the wallet intact. Best-effort post-detach cleanup is safe once its intent is durable.
| if identity_pubkeys_count == 0 { | ||
| return PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| "`identity_pubkeys_count` must be >= 1", | ||
| ); | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
b42bc950; see the replies below.
🔴 Blocking: Keep claim domain validation in its Rust owner
The Kotlin cleanup removes its duplicated domain checks, but this C shim still independently rejects an empty registration key set, and the new JNI claim entry point still rejects denomination == 0 with its denomination <= 0 guard. Neither rule is required for marshaling: an empty vector is representable safely, and zero is representable as u64. The owning wallet operation already rejects empty public keys, while the DPP builder validates the protocol-versioned denomination set. The book's validation-single-owner and thin-binding rules explicitly prohibit these duplicate count/range decisions, even when they provide an earlier error. Remove the C nonempty-key and JNI zero-denomination checks and propagate the owner's result, retaining pointer/buffer checks and rejection of negative Java values that cannot represent the Rust input.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality)
There was a problem hiding this comment.
Fixed in 4534f73. The empty-key check is removed from the claim shim, and JNI now only rejects negative denominations. The older entry points are left for a separate change.
There was a problem hiding this comment.
Still applies (re-reviewed at b686ca9c): Your removal of the direct C and denomination guards is correct, but the new claim call still reaches check_registration_invariants through decode_registration_pubkeys_blob and rejects empty rows inside JNI. Selecting a structural-only decoder for this call closes the remaining duplication without changing the older registration entry points.
There was a problem hiding this comment.
Resolved (re-reviewed at b42bc950): The claim C empty-key guard is removed, and JNI rejects only negative denominations that cannot represent the Rust input. The structural-only decoder also removes the indirect registration-policy check from this route.
| .map_err(|e| { | ||
| // Fail closed. Proceeding without knowing who owns the key is | ||
| // exactly the clobber this reservation exists to prevent. | ||
| PlatformWalletError::Persistence(format!( | ||
| "one-time claim: could not reserve the invitation's claim-record key: {e}" | ||
| )) |
There was a problem hiding this comment.
✅ Resolved at
b686ca9c; see the replies below.
🟡 Suggestion: Preserve typed claim-recovery corruption through the mobile boundary
The damaged-metadata fix now returns FileShieldedStoreError::RecoveryCorrupted, but this map_err immediately renders it into PlatformWalletError::Persistence. The claim FFI mapper then emits generic ErrorWalletOperation (6), so Kotlin receives WalletOperation rather than the existing ShieldedRecoveryCorrupted type for code 56; Swift likewise loses the typed corruption result. The row is safely retained, but hosts cannot distinguish damaged recovery state requiring repair from an ordinary operation failure. Preserve the structured store error through the owning operation, then pass the recovery variants through map_one_time_claim_result using the existing result-code allocation.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in b686ca9. Reservation corruption stays ShieldedRecoveryCorrupted and passes through as code 56. Tests are on both the Rust and FFI sides.
There was a problem hiding this comment.
Resolved (re-reviewed at b686ca9c): Your b686ca9 fix preserves the structured corruption variant through reservation handling and the claim FFI mapper's code 56. The Rust and FFI assertions cover the classification boundaries; I verified them statically without executing tests.
| /// (What no bound can rule out is the caller having made further copies — | ||
| /// the guard contains the representation it owns; avoiding stray copies is | ||
| /// the call site's job.) | ||
| pub(crate) struct ScrubOnDrop<T: ScrubbableSecret>(pub(crate) T); |
There was a problem hiding this comment.
🟡 Suggestion: Track secret zeroization in the Orchard owner
ScrubOnDrop compensates for the lack of a safe secret-zeroization API by overwriting private Orchard representations from Platform. The pinned Orchard keys and RedPallas wrapper expose no Zeroize implementation or explicit scrubbing destructor, and the current dashified branch still lacks that API; the open issue/PR searches found no zeroization tracking. This representation-dependent workaround should have a linked upstream follow-up in dashpay/orchard, which owns these fields and can maintain their scrubbing contract as their layout evolves. Track a minimal safe-zeroization implementation and replacement of the local raw-memory workaround. A linked upstream issue or PR is sufficient; this does not require removing the current containment before upstream support exists.
source: gpt-6.1-sol (phase2-reviewer: general)
|
Purge ownership (summary item, file_store.rs:1334-1342): agreed. Deferred along with the removal-intent ordering finding, as one removal-lifecycle change. This round:
|
…s to Rust The one-time-key claim shim no longer rejects an empty key set, and the JNI entry point only rejects negative denominations. The wallet operation and the DPP builder own those rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ption A RecoveryCorrupted error from the claim-key reservation now stays typed as ShieldedRecoveryCorrupted, and the claim FFI mapper passes it through as code 56 instead of flattening it to ErrorWalletOperation (6). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified head b686ca9: the corruption mapping, Kotlin unconfirmed-result handling, and Swift deletion ordering are fixed, but eight blocking issues remain in claim recovery, removal, persistence, JNI validation, and standalone native initialization. This was static verification only; diff whitespace checks passed, and current CI exposes no build/test results, with PR Hygiene pending. The supplied base-drift evidence identifies conflicts in coordinator.rs and operations.rs; the overlapping base changes do not resolve these findings.
🔴 8 blocking | 🟡 2 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Validate purge ownership in the same transaction as deletion
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1338-1343
purge_wallet unconditionally deletes pending rows without receiving or checking the destructive-admission token. Admission and its armed-record count occurred in an earlier transaction, and the barrier expires after DESTRUCTIVE_BARRIER_MS. If the remover pauses after admission, another store can reap the barrier and admit, arm, and broadcast a new claim before this DELETE resumes. The new armed-record hold cannot protect that claim because deletion never rechecks admission or protected records. purge_all_subwallets has the same gap, and begin_claim_admission does not treat a removal tombstone as a barrier. This is a defect in the lifecycle machinery introduced here, not merely a broader redesign opportunity. Validate the live destructive token and absence of protected claims atomically with deletion at the store boundary.
source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality); gpt-6.1-sol (phase2-reviewer: general)
5 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in packages/rs-platform-wallet/src/wallet/shielded/operations.rs, keys.rs, and file_store.rs directly implement invitation-funded spending, one-time spending-key handling, and persistent claim recovery. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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:1338-1343: Validate purge ownership in the same transaction as deletion
purge_wallet unconditionally deletes pending rows without receiving or checking the destructive-admission token. Admission and its armed-record count occurred in an earlier transaction, and the barrier expires after DESTRUCTIVE_BARRIER_MS. If the remover pauses after admission, another store can reap the barrier and admit, arm, and broadcast a new claim before this DELETE resumes. The new armed-record hold cannot protect that claim because deletion never rechecks admission or protected records. purge_all_subwallets has the same gap, and begin_claim_admission does not treat a removal tombstone as a barrier. This is a defect in the lifecycle machinery introduced here, not merely a broader redesign opportunity. Validate the live destructive token and absence of protected claims atomically with deletion at the store boundary.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1942-1955: Provide a terminating recovery path for armed claim holds
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147976)
Every armed claim blocks wallet removal and coordinator-wide Clear independently of lease expiry, but supported states have no release path. If the inviter spends the bearer note elsewhere after preflight, the victim can arm its record and receive NullifierAlreadySpent. Subsequent resumes see spent notes, invoke recovery with spend_finalized=false, find no identity, and remain unconfirmed indefinitely. Losing the invitation link also prevents entry into the only claim reconciliation path. Ordinary sync processes registered accounts rather than this reserved claim account, and identity-debit enumeration/abandonment excludes its nonempty-nullifier records. Registration failures are additionally swallowed, so success can hide a retained hold requiring another invitation retry. Add Rust-owned reconciliation from persisted data and an explicit user-acknowledged abandonment path, and surface incomplete registration unless automatic reconciliation will finish it. The tracked design discussion does not supply these required paths on this head.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1870-1873: Require identity-persistence capability before acknowledging the claim
Successful store plus store_commits_inline does not prove this backend persisted the identity. FFIPersister::store_commits_inline checks only on_changeset_end_fn, while its identity changeset is silently skipped when on_persist_identities_fn is absent. A supported partial callback configuration can therefore set registered=true and delete the recovery record without writing an identity row; calling flush would not repair the absent callback either. Process death before the result reaches the host then loses both the local identity and the retained padded identity ID. This is distinct from the fixed buffered-store ordering and does not require changing the deliberate NoPlatformPersistence contract. Expose the identity-persistence capability through the persistence abstraction and have the owning Rust claim operation refuse before spending when the required durable identity sink is unavailable.
In `packages/rs-unified-sdk-jni/src/funding.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/funding.rs:1144-1146: Keep claim domain validation in its Rust owner
The direct C empty-key guard and JNI zero-denomination guard were removed, but this new claim route still calls decode_registration_pubkeys_blob. That helper invokes check_registration_invariants, which rejects empty rows and requires key ID 0 to be MASTER/AUTHENTICATION before the wallet operation runs. IdentityPubkeyCodec.encode(emptyList()) produces a structurally valid zero-row blob, and the owning wallet operation already rejects empty public keys at operations.rs:2570–2573. The count rule therefore remains duplicated in the binding, and registration-role policy is also decided there. The book's single-owner validation and thin-binding rules apply through shared helpers, not only inline guards. Use structural-only decoding for this new claim entry point and leave registration policy in its Rust owner; older entry points can remain separate work.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:2925-2926: Initialize the native library before standalone Orchard key calls
Calling generateOneTimeOrchardKey as the first SDK operation reaches an external JNI method before libdash_sdk_jni is loaded. FundingNative has no loading initializer, this file has no top-level initialization that loads it, and mapNativeErrors only converts DashSDKException. The advertised handle-less inviter API therefore throws UnsatisfiedLinkError even when the native library is correctly packaged. orchardAddressFromSpendingKey has the same problem at lines 2947–2949. Existing standalone helpers such as Mnemonic.generate explicitly call Sdk.initialize, matching NativeLoader's documented entry-point contract. Initialize before both public calls, or initialize their native facade, and cover first-use invocation without first constructing an Sdk or wallet manager.
In `packages/rs-platform-wallet/src/wallet/shielded/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/store.rs:548-554: Document the breaking ShieldedStore API before release
The new required ShieldedStore tombstone and admission methods, plus PendingRedrive::identity_index, break Rust source compatibility for downstream storage implementations and struct literals even when they do not use invitation claims. The crate README already documents other custom-backend migrations, but neither that guidance nor the supplied PR description explains these new requirements. Add migration guidance covering the methods, the cross-instance atomicity and durability contracts, and the new field, and identify the source incompatibility in the PR/release notes. No PlatformVersion bump is needed, and the book reserves the title's ! marker for consensus-breaking changes, so that marker is not appropriate for this Rust API break.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:4200-4213: Require authenticated terminal evidence before deleting a claim record
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147966)
Resume combines a nullifier observation made before rebroadcast with a subsequent ShieldedBroadcastFailed and deletes the exact recovery record. However, broadcast_definitely_failed accepts Unavailable and explicitly documents that delivery, including an earlier accepted SDK retry, can precede that error. The transition can therefore execute after its padded identity ID has been discarded. The other retirement path also lacks authenticated evidence: broadcast_and_confirm_one_time_claim accepts a populated DAPI error cause, but broadcast.rs returns that envelope before verify_fetching_quorum_key. Recovery then collapses identity transport/proof failures into None and, with spend_finalized=true, returns the terminal result that clears the record. An endpoint can relay a successful claim, fabricate a verdict, and withhold identity proofs to trigger this cleanup. The proposed proof-only retirement rule is needed under either funding layout. Retain records across ambiguous delivery and failed lookups; retire them only on proof-verified terminal evidence.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3084-3095: Fence claim progression and deletion by reservation and record generation
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147972)
A caller receiving ClaimKeyReservation::Held still enters resume_one_time_claim before owns_claim_key is checked. Resume can rebroadcast and clear records; its chargeable-step check renews only that caller's wallet lease, not ownership of the invitation reservation. Two independent stores can consequently retain R1 while owner A clears it and arms replacement R2 under the same activity ID. B's delayed R1 cleanup then deletes R2 because clear_redrive compares only wallet/account/activity coordinates. A can subsequently broadcast without its recovery bytes. Finalization and registration acknowledgement use the same coordinate-only deletion contract. Neither funding-layout alternative is implemented here, so the linked redesign does not fence this head. Require reservation ownership before progressing a record and atomically compare the expected transition or generation when retiring it, including acknowledgement.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1140-1154: Persist removal intent before committing the detach
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147958)
on_admitted has already irreversibly detached the wallet, and account, persister, and hydration registrations are removed before record_wallet_removal runs. If the tombstone write and subsequent purge both fail, removal still returns success and only the volatile deferred_purges set remembers the unfinished wipe. Restart loses that set, leaving surviving durable pending-spend rows with no removal marker; re-registration can inherit them. The purge-only failure injection does not cover failure of the prerequisite tombstone write. The linked lifecycle pass is a suitable place to implement the fix, but this head still introduces the unsafe ordering. Persist removal intent while admission is held and before invoking on_admitted or changing registrations; abort intact if that write fails. Only failures after durable intent should use deferred completion.
In `packages/rs-platform-wallet/src/wallet/shielded/keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/keys.rs:53-56: Track secret zeroization in the Orchard owner
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4457764481)
The audited ScrubOnDrop guard is useful containment, but it remains a representation-dependent workaround for missing owner-provided secret cleanup in dashpay/orchard. Inspection of the pinned 38ac9c1 source and the current dashified branch confirms that SpendingKey and SpendAuthorizingKey expose no Zeroize implementation or scrubbing destructor; SpendingKey::from_bytes also accepts the secret array by value. The local wrapper can scrub its own storage, not construction and derivation temporaries inside Orchard. The upstream repository currently has no open issue or PR tracking this capability. Link a focused Orchard follow-up for supported secret zeroization and explicit copy/construction semantics, then replace the raw-memory shim when that API exists. This does not require removing the current containment or imply that its constrained unsafe implementation is unsound.
| let registered = stored | ||
| && (self.persister.store_commits_inline() | ||
| || match self.persister.flush() { | ||
| Ok(()) => true, |
There was a problem hiding this comment.
🔴 Blocking: Require identity-persistence capability before acknowledging the claim
Successful store plus store_commits_inline does not prove this backend persisted the identity. FFIPersister::store_commits_inline checks only on_changeset_end_fn, while its identity changeset is silently skipped when on_persist_identities_fn is absent. A supported partial callback configuration can therefore set registered=true and delete the recovery record without writing an identity row; calling flush would not repair the absent callback either. Process death before the result reaches the host then loses both the local identity and the retained padded identity ID. This is distinct from the fixed buffered-store ordering and does not require changing the deliberate NoPlatformPersistence contract. Expose the identity-persistence capability through the persistence abstraction and have the owning Rust claim operation refuse before spending when the required durable identity sink is unavailable.
source: gpt-6.1-sol (phase2-reviewer: general)
| let Some(decoded) = decode_registration_pubkeys_blob(env, &pubkeys_blob) else { | ||
| return ptr::null_mut(); | ||
| }; |
There was a problem hiding this comment.
✅ Resolved at
b42bc950; see the replies below.
🔴 Blocking: Keep claim domain validation in its Rust owner
The direct C empty-key guard and JNI zero-denomination guard were removed, but this new claim route still calls decode_registration_pubkeys_blob. That helper invokes check_registration_invariants, which rejects empty rows and requires key ID 0 to be MASTER/AUTHENTICATION before the wallet operation runs. IdentityPubkeyCodec.encode(emptyList()) produces a structurally valid zero-row blob, and the owning wallet operation already rejects empty public keys at operations.rs:2570–2573. The count rule therefore remains duplicated in the binding, and registration-role policy is also decided there. The book's single-owner validation and thin-binding rules apply through shared helpers, not only inline guards. Use structural-only decoding for this new claim entry point and leave registration policy in its Rust owner; older entry points can remain separate work.
source: gpt-6.1-sol (phase2-reviewer: general, ffi-engineer, platform-versioning, rust-quality)
There was a problem hiding this comment.
Fixed in 5466267. The claim's JNI path now decodes keys structurally only. The key-0 MASTER + AUTHENTICATION rule moved into the claim operation, before any scan or spend, with a test.
There was a problem hiding this comment.
Resolved (re-reviewed at b42bc950): The claim now uses structural-only JNI decoding, and its Rust operation checks key ID 0 for MASTER/AUTHENTICATION before deriving Orchard keys or scanning. The added regression test covers refusal without arming a record.
| fun generateOneTimeOrchardKey(): OneTimeOrchardKey { | ||
| val blob = mapNativeErrors { FundingNative.generateOneTimeOrchardKey() } |
There was a problem hiding this comment.
✅ Resolved at
b42bc950; see the replies below.
🔴 Blocking: Initialize the native library before standalone Orchard key calls
Calling generateOneTimeOrchardKey as the first SDK operation reaches an external JNI method before libdash_sdk_jni is loaded. FundingNative has no loading initializer, this file has no top-level initialization that loads it, and mapNativeErrors only converts DashSDKException. The advertised handle-less inviter API therefore throws UnsatisfiedLinkError even when the native library is correctly packaged. orchardAddressFromSpendingKey has the same problem at lines 2947–2949. Existing standalone helpers such as Mnemonic.generate explicitly call Sdk.initialize, matching NativeLoader's documented entry-point contract. Initialize before both public calls, or initialize their native facade, and cover first-use invocation without first constructing an Sdk or wallet manager.
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
There was a problem hiding this comment.
Fixed in 22d20eb. Both helpers call Sdk.initialize() before JNI, and there are first-use tests for each.
There was a problem hiding this comment.
Resolved (re-reviewed at b42bc950): Both helpers now invoke Sdk.initialize before their JNI calls. Your first-use tests check that failure occurs during library loading rather than external-method resolution.
| fn record_wallet_removal(&mut self, wallet_id: WalletId) -> Result<(), Self::Error>; | ||
|
|
||
| /// Drop `wallet_id`'s removal tombstone. Called only once its purge has | ||
| /// completed, so an outstanding tombstone means "this wallet's state may | ||
| /// still be here" and nothing weaker. Idempotent, and a no-op for a wallet | ||
| /// that has none. | ||
| fn clear_wallet_removal(&mut self, wallet_id: WalletId) -> Result<(), Self::Error>; |
There was a problem hiding this comment.
✅ Resolved at
b42bc950; see the replies below.
🟡 Suggestion: Document the breaking ShieldedStore API before release
The new required ShieldedStore tombstone and admission methods, plus PendingRedrive::identity_index, break Rust source compatibility for downstream storage implementations and struct literals even when they do not use invitation claims. The crate README already documents other custom-backend migrations, but neither that guidance nor the supplied PR description explains these new requirements. Add migration guidance covering the methods, the cross-instance atomicity and durability contracts, and the new field, and identify the source incompatibility in the PR/release notes. No PlatformVersion bump is needed, and the book reserves the title's ! marker for consensus-breaking changes, so that marker is not appropriate for this Rust API break.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Done in 2c136b5: README section 'Invitation-claim store migration'. The source incompatibility is now noted in the PR description.
There was a problem hiding this comment.
Resolved (re-reviewed at b42bc950): Your README migration section covers the required methods, cross-instance atomicity/durability contracts and identity_index field. The supplied PR description also explicitly identifies the source incompatibility.
…key helpers generateOneTimeOrchardKey and orchardAddressFromSpendingKey are handle-less and may be the first SDK call in a process; they now call Sdk.initialize() before entering JNI, like Mnemonic.generate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eys structurally in JNI The one-time-key claim's JNI entry point now decodes the pubkeys blob structurally only. The claim operation checks that key ID 0 is MASTER + AUTHENTICATION before anything is scanned or spent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gration Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: ERROR_CODE_REGISTRY.md (take v5.1-dev's row 60, keep 61/62), coordinator.rs tests (keep both sides), operations.rs imports (both). Drop two dashcore Hash imports the newer dashcore no longer needs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified b42bc95 against the source: nine prior findings are fixed, while six blocking claim-recovery/removal defects and one upstream zeroization suggestion remain. The linked design discussion tracks proposed repairs but does not resolve defects introduced by this feature. This was a static review with no local builds or tests; current CI shows Kotlin tests and the Swift frozen-schema check passing, Rust workspace tests pending, and no failed code checks.
🔴 6 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Validate purge ownership in the same transaction as deletion
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1338-1343
purge_wallet deletes pending records without accepting or validating the destructive-admission token; purge_all_subwallets has the same gap. Admission is acquired earlier under a barrier that expires after 60 seconds. If remover A is suspended after admission, peer store B can reap the expired barrier, admit a claim and arm its recovery row. When A resumes, this unconditional DELETE destroys B's row despite its live claim and the armed-record hold. The coordinator's store lock cannot prevent this across independently opened stores. Make destructive mutations atomically validate the expected live token, scope and protected-record conditions in the deletion transaction, including whole-store and deferred purges. Combining this repair with removal-intent ordering is appropriate, but writing intent earlier cannot substitute for checking ownership at deletion time.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, architecture-layering)
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes in shielded/keys.rs, shielded/operations.rs, and shielded/file_store.rs directly affect spending-key handling, funded-note spending for invitation claims, and persisted claim reservations and wallet-removal tombstones. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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:1338-1343: Validate purge ownership in the same transaction as deletion
purge_wallet deletes pending records without accepting or validating the destructive-admission token; purge_all_subwallets has the same gap. Admission is acquired earlier under a barrier that expires after 60 seconds. If remover A is suspended after admission, peer store B can reap the expired barrier, admit a claim and arm its recovery row. When A resumes, this unconditional DELETE destroys B's row despite its live claim and the armed-record hold. The coordinator's store lock cannot prevent this across independently opened stores. Make destructive mutations atomically validate the expected live token, scope and protected-record conditions in the deletion transaction, including whole-store and deferred purges. Combining this repair with removal-intent ordering is appropriate, but writing intent earlier cannot substitute for checking ownership at deletion time.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1942-1955: Provide a terminating recovery path for armed claim holds
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147976)
Every armed claim record blocks wallet removal and coordinator-wide Clear independently of its lease, but supported outcomes cannot release that hold. An inviter can spend the bearer note after the claimant's preflight and during proof construction. The claimant then arms its record and receives NullifierAlreadySpent; subsequent resumes see spent notes, call recovery with spend_finalized=false, find no matching identity and remain unconfirmed indefinitely. Ordinary sync does not reconcile the reserved claim account, and identity-debit enumeration/abandonment excludes these nonempty-nullifier records. Losing the invitation link also removes access to the only claim-resume entry point. Registration failures compound this: the registration helper logs failures and returns (), so the public claim can report success while leaving an undisclosed teardown hold. Implement Rust-owned reconciliation from durable claim data and an explicitly acknowledged abandonment/removal path, and surface incomplete registration unless automatic recovery will finish it. The proposed public API is necessary under either funding layout.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1870-1873: Require identity-persistence capability before acknowledging the claim
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4476467052)
Successful store/flush completion does not establish that the backend persisted this identity. FFIPersister::store_commits_inline checks only whether on_changeset_end_fn exists, while store silently skips identity writes when on_persist_identities_fn is absent. That configuration reaches this acknowledgement and deletes the recovery record without storing its randomized padded identity ID. NoPlatformPersistence also reaches acknowledgement through successful no-op store/flush calls. The buffered-flush and same-session retry fixes are present, but neither closes this capability gap. Require an explicit durable identity write/restore contract in the owning Rust claim entry point before scanning or spending, with FFI attestation tied to the callbacks actually installed; generic transaction completion must not authorize retirement. Deferring the funding-layout decision does not make either backend safe for the current claim path.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3102-3108: Fence claim progression and deletion by reservation and record generation
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147972)
A caller receiving ClaimKeyReservation::Held enters resume_one_time_claim before owns_claim_key is checked. Resume can rebroadcast and delete records, while its chargeable-step guard checks only the caller's wallet lease. Across two stores, owner A and non-owner B can therefore resolve old record R1 concurrently: A clears rejected R1 and arms replacement R2, then B's delayed R1 cleanup deletes R2 because clear_redrive compares only wallet/account/activity coordinates. A can subsequently broadcast without its recovery record. This also applies to rebuilding a padded single-note claim: the activity key stays the same while the identity ID changes. Require reservation ownership before existing-record progression and atomically compare the expected transition or generation on retirement. Carry that identity through finalization and registration acknowledgement too, since the acknowledgement receipt currently contains only coordinates and outlives the reservation. The proposed second-claimant refusal must precede resume, not only fresh construction.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:4218-4231: Require authenticated terminal evidence before deleting a claim record
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147966)
This branch deletes the exact transition using an Unspent snapshot taken before rebroadcast plus ShieldedBroadcastFailed. That error can originate from broadcast_definitely_failed treating Unavailable as definitive, although its own contract allows delivery followed by a lost acknowledgement or an earlier accepted SDK retry. The snapshot therefore cannot prove that the transition will not execute after deletion. A second route accepts an unauthenticated terminal verdict: dash-sdk returns wait-response error envelopes before proof/quorum verification, this operation combines a populated cause with spent nullifiers into spend_finalized=true, and both recovery lookup helpers collapse transport/proof failures into None. A DAPI endpoint can relay a successful claim, fabricate a rejection and fail the identity lookups, causing terminal cleanup of its only recovery record. Preserve records on ambiguous delivery and unknown lookups, and implement the proposed proof-verified terminal-state rule in the Rust recovery classifier. Choosing a funding layout does not authenticate either signal.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1149-1155: Persist removal intent before committing the detach
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4449147958)
on_admitted has already committed the irreversible detach and removed coordinator registrations before record_wallet_removal runs. If the tombstone write and subsequent purge both fail, removal still succeeds and only the process-local deferred_purges set remembers the residue. Restart loses that set, leaving no durable intent identifying surviving rows for cleanup before re-registration. The existing purge-only failure injection leaves tombstone writes working and does not cover this sequence. While destructive admission is held, make durable intent a fallible prerequisite before the detach hook or registration mutations; on failure, release admission and leave the wallet intact. Once intent is durable, post-detach purge failures can use deferred completion. The proposed intent-before-admission ordering also needs adjustment: an admission refusal must not leave a live wallet marked for deferred deletion.
In `packages/rs-platform-wallet/src/wallet/shielded/keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/keys.rs:56: Track secret zeroization in the Orchard owner
(existing thread: https://github.com/dashpay/platform/pull/4313#discussion_r4457764481)
ScrubOnDrop is useful containment, but it erases private representations owned by dashpay/orchard and cannot scrub secrets created inside Orchard derivation. The pinned dashified-0.14.1 source and current default keys API lack Zeroize or scrubbing Drop implementations for these key types; FullViewingKey::from also constructs an internal SpendAuthorizingKey temporary outside this guard. This leaves dependency-layout auditing and incomplete secret-lifecycle protection downstream. Track an owner-side Orchard change providing audited zeroization for secret keys and derivation temporaries, followed by dependency adoption so Platform can retire the representation-dependent wrapper. A linked upstream issue or PR is sufficient for this suggestion; the supplied discussion and the upstream issue/PR searches did not identify one. This is not a claim that the restricted local guard is itself unsound.
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.Breaking change: Rust
ShieldedStoreAPISource-incompatible for custom
ShieldedStoreimplementations, even ones that never claim invitations: new required methods (wallet-removal tombstones, claim admission and reservation, destructive admission), andPendingRedrivegainsidentity_index: Option<u32>. No consensus orPlatformVersionchange. Migration guidance is inpackages/rs-platform-wallet/README.md, section "Invitation-claim store migration".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
PR Hygiene ·
b42bc95/self-reviewedkotlin-sdk(packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.kt,packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/ShieldedDao.ktand 6 more) — HashEngineeringrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md,packages/rs-platform-wallet-ffi/src/error.rs,packages/rs-platform-wallet-ffi/src/manager.rsand 2 more) — HashEngineering or ZocoLini or llbartekll or romchornyirs-platform-wallet(packages/rs-platform-wallet/Cargo.toml,packages/rs-platform-wallet/README.md,packages/rs-platform-wallet/src/error.rsand 13 more) — HashEngineering or ZocoLini or llbartekll or romchornyipackages/rs-unified-sdk-jni/src/funding.rs,packages/rs-unified-sdk-jni/src/pubkey_rows.rs) — QuantumExplorer or shumkovswift-sdk(packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift,packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift,packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift) — llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.