feat(platform-wallet): persist Orchard viewing keys for seedless shielded bind#4126
Conversation
…lded bind - Persist each account's raw 96-byte FVK at the first seed-backed bind (SwiftData PersistentShieldedViewingKey rows via a new persist/load callback pair); IVK / OVK / default address re-derive from it. - bind_shielded now installs viewing-grade material only; the FFI tries bind_shielded_from_persisted first, so launch / network-switch / Sync-Now rebinds never resolve the mnemonic once rows exist. - No SpendAuthorizingKey stays resident: the four shielded spend FFIs take a MnemonicResolverHandle, re-derive the keyset per operation (guarded against the bound FVK), and drop it on return. - Secret-free NSLog audit line on every resolver firing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Sonnet deferred (commit aca1d3c) |
…Explorer Satisfies scripts/check-storage-explorer.sh: list row + network-scoped list view + record detail view for the new model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Both blocking candidates are verified at the exact head. Three Swift spend wrappers can release their unretained mnemonic resolver before Rust invokes it, and the Swift persistence loader converts malformed viewing-key rows into apparent misses, triggering mnemonic fallback instead of surfacing corruption.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:541-545: Keep the mnemonic resolver alive through each spend FFI call
MnemonicResolver passes Rust an Unmanaged.passUnretained(self) callback context and destroys its native handle in deinit. Here, `_ = resolver` is the last semantic use before the FFI operation, so optimized ARC may release the captured owner before Rust calls resolve_seed_from_resolver, invalidating both resolverHandle and its callback context. This is concrete for the temporary resolvers constructed by SendViewModel and is repeated in shieldedUnshield at lines 684-686 and shieldedWithdraw at lines 740-742. The identity-create sibling already uses withExtendedLifetime because a bare `_ =` is not a guaranteed keepalive. Wrap each complete marshalling and FFI operation in withExtendedLifetime(resolver).
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3576-3581: Surface malformed persisted viewing keys instead of treating them as absent
The loader silently omits any persisted row whose wallet ID or FVK has an invalid length and reports success with only the remaining rows. Rust therefore sees a requested account as missing, bind_shielded_from_persisted returns Ok(false), and the FFI bind path resolves the mnemonic and re-persists the key. This violates the documented fail-closed behavior for present-but-malformed viewing keys and bypasses the Rust corruption test because that test supplies the malformed row directly. Validate rows before any filtering or marshalling step can discard malformed data and before allocating the FFI buffer; set resultErrored and return on corruption. The previously proposed narrow replacement is unsafe because it returns after allocation without releasing the buffer, so no exact-range suggestion is supplied.
…wing-key load - Wrap the three spend wrappers' FFI work in withExtendedLifetime(resolver) instead of the bare `_ = resolver`, which optimized ARC may elide before Rust invokes the passUnretained callback context. - loadShieldedViewingKeys now fails the load (before any allocation) on a present-but-malformed row instead of skipping it — a skipped row would read as a missing key and silently fall back to a mnemonic resolve, masking persistence corruption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fy resolver audit line Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The resolver-lifetime blocker is fixed at exact head aca1d3c. One blocking persistence issue remains: malformed wallet IDs are filtered out before validation, converting corruption into a successful missing-key result and mnemonic fallback. This is a preliminary Codex-only verification; Sonnet was not run.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3564-3587: Validate malformed wallet IDs before network filtering
`PersistentShieldedViewingKey.walletId` is unconstrained `Data`. A corrupted non-32-byte wallet ID cannot match any normal ID returned by `inNetworkWalletIds()`, so lines3564-3565 discard the row before the length check runs. If no rows remain, the callback reports a successful empty load; Rust then treats the requested key as missing and invokes the mnemonic fallback, violating this PR's documented requirement that persisted corruption fail closed. Validate wallet-ID lengths before network filtering, then validate FVK lengths after filtering so a corrupt FVK belonging to another network does not block this manager.
| if let inNetworkIds = self.inNetworkWalletIds() { | ||
| rows = rows.filter { inNetworkIds.contains($0.walletId) } | ||
| } | ||
| if rows.isEmpty { | ||
| return | ||
| } | ||
| // Fail closed on a present-but-malformed row, BEFORE any | ||
| // allocation: silently skipping it (the sync-state | ||
| // loader's pattern) would make Rust see the account as | ||
| // "no persisted key" and fall back to a mnemonic resolve, | ||
| // masking persistence corruption — the exact opposite of | ||
| // `bind_shielded_from_persisted`'s documented contract, | ||
| // which surfaces a malformed row as an error. | ||
| if let bad = rows.first(where: { | ||
| $0.walletId.count != 32 || $0.fvkBytes.count != 96 | ||
| }) { | ||
| SDKLogger.error( | ||
| "loadShieldedViewingKeys: corrupt row " | ||
| + "(walletId \(bad.walletId.count)B, fvk \(bad.fvkBytes.count)B) — " | ||
| + "failing the load rather than masking it as a missing key" | ||
| ) | ||
| resultErrored = true | ||
| return | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Validate malformed wallet IDs before network filtering
PersistentShieldedViewingKey.walletId is unconstrained Data. A corrupted non-32-byte wallet ID cannot match any normal ID returned by inNetworkWalletIds(), so lines3564-3565 discard the row before the length check runs. If no rows remain, the callback reports a successful empty load; Rust then treats the requested key as missing and invokes the mnemonic fallback, violating this PR's documented requirement that persisted corruption fail closed. Validate wallet-ID lengths before network filtering, then validate FVK lengths after filtering so a corrupt FVK belonging to another network does not block this manager.
| if let inNetworkIds = self.inNetworkWalletIds() { | |
| rows = rows.filter { inNetworkIds.contains($0.walletId) } | |
| } | |
| if rows.isEmpty { | |
| return | |
| } | |
| // Fail closed on a present-but-malformed row, BEFORE any | |
| // allocation: silently skipping it (the sync-state | |
| // loader's pattern) would make Rust see the account as | |
| // "no persisted key" and fall back to a mnemonic resolve, | |
| // masking persistence corruption — the exact opposite of | |
| // `bind_shielded_from_persisted`'s documented contract, | |
| // which surfaces a malformed row as an error. | |
| if let bad = rows.first(where: { | |
| $0.walletId.count != 32 || $0.fvkBytes.count != 96 | |
| }) { | |
| SDKLogger.error( | |
| "loadShieldedViewingKeys: corrupt row " | |
| + "(walletId \(bad.walletId.count)B, fvk \(bad.fvkBytes.count)B) — " | |
| + "failing the load rather than masking it as a missing key" | |
| ) | |
| resultErrored = true | |
| return | |
| } | |
| if let bad = rows.first(where: { $0.walletId.count !=32 }) { | |
| SDKLogger.error( | |
| "loadShieldedViewingKeys: corrupt wallet id " | |
| + "(\(bad.walletId.count)B) — failing the load rather than masking it" | |
| ) | |
| resultErrored = true | |
| return | |
| } | |
| if let inNetworkIds = self.inNetworkWalletIds() { | |
| rows = rows.filter { inNetworkIds.contains($0.walletId) } | |
| } | |
| if rows.isEmpty { | |
| return | |
| } | |
| if let bad = rows.first(where: { $0.fvkBytes.count !=96 }) { | |
| SDKLogger.error( | |
| "loadShieldedViewingKeys: corrupt FVK " | |
| + "(\(bad.fvkBytes.count)B) — failing the load rather than masking it" | |
| ) | |
| resultErrored = true | |
| return | |
| } |
source: ['codex']
…nd-example-app Conflicts: Cargo.lock (theirs + re-resolve) and identity_top_up.rs (base #4093's rework — MIN_TOP_UP_DUFFS floor, out_new_balance sentinel, guard tests — supersedes the branch's older zero-guard). Breaking-FFI adaptations for the Android JNI/Kotlin side: - #4093/#4126: shielded identity-create/transfer/unshield/withdraw gained a mnemonic_resolver_handle param — threaded through the JNI exports, FundingNative externs, and PlatformWalletManager; the transfer/unshield/withdraw trio now runs under teardownGate.op (they borrow the manager's resolver, so the round-46 ungated rationale no longer holds). - #4127 typed provider keys: the pool-entry write trampoline forwards exactly public_key_len meaningful bytes (blob self-describing by length: 33 ECDSA / 48 BLS / 32 EdDSA); the load path rebuilds the 48-byte slot + key_type_tag from the stored blob length. No Kotlin bridge/descriptor change needed. - AccountSpecFFI lost derived_platform_node_keys(+count) (moved into the typed core-address rows) — the round-42 null-init is dropped. - #4126 Orchard viewing keys: the three new vtable slots are None — binds keep resolving the seed via the mnemonic resolver (the documented fallback); the seedless-bind persistence port is tracked as a follow-up. Verified: cargo fmt, JNI check (shielded), workspace clippy --all-features -D warnings, platform-wallet-ffi lib tests (170), gradle :sdk+:app tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
At every app launch,
rebindWalletScopedServicesshielded-bound every loaded wallet through the mnemonic resolver: the mnemonic was pulled from the Keychain, the seed derived, and a resident OrchardSpendAuthorizingKeystayed in memory for the whole session — re-firing on network switch and Sync Now. The mnemonic should not be touched at launch for shielded sync, and no spend authority should be resident between spends.What was done?
rs-platform-wallet
PlatformWallet.shielded_keysnow holdsAccountViewingKeys(FVK / IVK / OVK / default address) — everything trial-decryption and sync need, nothing that can spend.bind_shieldedpersists each account's raw 96-byte FVK through the persister (ShieldedChangeSet.viewing_keys).AccountViewingKeys::from_fvk_bytesreconstructs the whole viewing set from those bytes (IVK / OVK / default address are pure functions of the FVK).bind_shielded_from_persistedrebinds from those rows with no seed parameter at all; a corrupt row is an error, a missing row means "fall back to the seed path".transfer/unshield/withdraw/identity_create_from_pool) take aseedand re-derive the fullOrchardKeySetper operation viaderive_spend_keyset, which also guards derived-FVK == bound-FVK so a wrong seed fails before the ~30 s Halo 2 proof. The keyset is dropped (evicted) when the call returns.shieldand fund-from-asset-lock need only viewing-grade material and take none.rs-platform-wallet-ffi
platform_wallet_manager_bind_shieldedkeeps its ABI but tries the persisted-keys path first — the resolver fires only when a requested account has no persisted row (first bind after create/import). Launch, network switch, and Sync Now all ride this and never resolve the mnemonic once rows exist.ShieldedViewingKeyFFI/ShieldedViewingKeyRestoreFFI+on_persist_shielded_viewing_keys_fn/on_load_shielded_viewing_keys_fn(+free) callback pair, modeled on the platform-node pubkey batch pattern.MnemonicResolverHandleparameter (reusingresolve_seed_from_resolver; seed lives inZeroizingbuffers), mirroring the transparentMnemonicResolverCoreSignerflow.swift-sdk / SwiftExampleApp
PersistentShieldedViewingKeySwiftData model (viewing-grade key material stored like the other derived-key batches, not Keychain) + persistence-handler arms; wallet deletion and shielded Clear purge the rows (Clear so a corrupt row can't outlive it — the next bind re-resolves once and re-persists).MnemonicResolver; a secret-freeNSLogaudit line fires on every resolver invocation so the no-mnemonic-at-launch property is checkable from the unified log.How Has This Been Tested?
cargo test -p platform-wallet -p platform-wallet-ffi --all-features— 753 tests pass, including new ones: FVK byte round-trip reproduces identical FVK/IVK/OVK/default address; bind→persist→simulated-restart→bind_shielded_from_persistedyields identical addresses and a working coordinator balance read with no seed anywhere (the API takes none); missing rows reportfalsewith no state change; corrupt rows error.Breaking Changes
None on-chain / consensus. The four shielded spend FFI signatures gained a resolver parameter (pre-release SDK surface, Swift call sites updated in this PR).
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code