feat(platform-wallet): skip startup mnemonic touches via seed-binding marker + gated contact-crypto drain#4125
Conversation
… marker + gated contact-crypto drain The unlock path ran platform_wallet_verify_seed_binds_to_wallet for every restored wallet at every launch, resolving the Keychain mnemonic and re-deriving the BIP44 account-0 xpub even though the outcome is a pure function of (mnemonic, network) vs the persisted xpub. Verify once, persist the verified xpub as a marker on the wallet row, and let Rust skip the resolver on later launches while the marker still matches (re-import invalidates it and re-runs the full check). Also gate the per-wallet background contact-crypto drain on the signerless pending-ops probe so an empty queue schedules nothing at unlock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 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 (2)
📝 WalkthroughWalkthroughThe change adds marker-aware seed-binding verification across Rust, FFI, and Swift. Swift persists markers between unlocks, passes them to the cached FFI entry point, stores newly returned markers, and skips contact-crypto draining when no operations are pending. ChangesSeed-binding verification cache
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformWalletManager
participant PlatformWalletPersistenceHandler
participant WalletStorage
participant CachedFFI as platform_wallet_verify_seed_binds_to_wallet_cached
participant PlatformWallet
participant ContactCryptoProvider
PlatformWalletManager->>PlatformWalletPersistenceHandler: Read seed-binding marker
PlatformWalletManager->>WalletStorage: Read mnemonic keychain stamp
PlatformWalletManager->>CachedFFI: Verify wallet with marker and stamp
CachedFFI->>PlatformWallet: Call verify_seed_binds_with_marker
PlatformWallet->>ContactCryptoProvider: Derive account-0 xpub when marker is stale or absent
ContactCryptoProvider-->>PlatformWallet: Return derived xpub
PlatformWallet-->>CachedFFI: Return verification outcome and marker
CachedFFI-->>PlatformWalletManager: Return status and optional marker
PlatformWalletManager->>PlatformWalletPersistenceHandler: Persist newly returned marker
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 8a2b541) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/dashpay.rs (1)
1020-1024: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider wrapping
CStr::from_ptr(verified_marker)in an explicitunsafeblock.
It keeps the unsafe scope consistent with the rest of this function and makes the FFI boundary easier to audit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/dashpay.rs` around lines 1020 - 1024, Update the marker conversion in the verified_marker handling to wrap the CStr::from_ptr call in an explicit unsafe block. Keep the existing null check and string conversion behavior unchanged, limiting the unsafe scope to the FFI pointer dereference.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/dashpay.rs`:
- Around line 1020-1024: Update the marker conversion in the verified_marker
handling to wrap the CStr::from_ptr call in an explicit unsafe block. Keep the
existing null check and string conversion behavior unchanged, limiting the
unsafe scope to the FFI pointer dereference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97c011e7-a514-438f-8a30-d91b601482d5
📒 Files selected for processing (8)
packages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/MnemonicResolverAndPersister.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The seed-binding cache is not bound to the current Keychain mnemonic, so it bypasses the wrong-slot gate after that mnemonic changes. The drain gate also treats ContactInfoDecrypt-only queues as empty, leaving cross-device contact metadata unapplied unless a payment later triggers a drain. Both are blocking regressions.
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/high— general (failed),gpt-5.6-sol/high— security-auditor (failed),gpt-5.6-sol/high— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— ffi-engineer (failed),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 - 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/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/seed_binding.rs:87-90: Cached xpub marker bypasses current-seed verification
`expected_marker` is only the wallet's persisted account-0 xpub, and a match returns before consulting `crypto`. Swift persists that marker independently of the mutable Keychain entry, while the public `WalletStorage.storeMnemonic(_:for:)` API can replace the mnemonic for an existing wallet ID without invalidating it. After one successful verification, a replacement or mis-mapped mnemonic therefore passes warm unlock, clears `seedMismatch`, and is used by later resolver-backed cryptography. Bind the marker to a Keychain-item identity or generation that is atomically invalidated on every mnemonic write and deletion, or continue deriving the resolver xpub before trusting the seed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:689-699: Drain gate strands queued contact metadata indefinitely
This early return uses `platform_wallet_pending_contact_crypto_count`, whose Rust implementation deliberately excludes `ContactInfoDecrypt`. Seedless recurring sync enqueues that operation but cannot decrypt or apply its alias, note, and hidden metadata. The only other production path that calls `drain_pending_contact_crypto` is payment sending, and the unlock banner uses the same filtered count, so users who do not send a payment retain stale cross-device metadata across subsequent syncs and launches. Gate on any drainable work, including `ContactInfoDecrypt`, or provide another guaranteed signer-present drain for those entries.
… + gate drain on total drainable count Review follow-ups (PR #4125): - The cached verification marker is now bound to an identity stamp of the mnemonic Keychain item (creation+modification dates, attribute-only read) in addition to the account-0 xpub. Any rewrite or re-creation of the item changes the stamp and forces the full resolver check, so a replaced or mis-mapped mnemonic can never coast on an old verification. No stamp → cache disabled (full check every launch), never bypassed. - The unlock drain gate now uses a new total drainable count that includes ContactInfoDecrypt, so cross-device contact metadata still applies at launch; the filtered count remains for the user-facing banner. - Wrap CStr::from_ptr in explicit unsafe blocks in the cached verify FFI. 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 contact-crypto drain blocker is fixed: the unlock path now counts all queued operations, including ContactInfoDecrypt, before deciding whether to schedule the drain. The seed-binding cache remains unsafe on supported native macOS because its generation stamp uses second-resolution legacy Keychain timestamps, allowing a rapid mnemonic replacement to retain the cached marker and bypass current-seed verification.
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 (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/Core/Wallet/WalletStorage.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift:186-190: Cached xpub marker bypasses current-seed verification
The Swift package supports native macOS15, but these Keychain operations omit `kSecUseDataProtectionKeychain`, so SecItem defaults to the file-based Keychain on macOS ([Apple TN3137](https://developer.apple.com/documentation/Technotes/tn3137-on-mac-keychains)). Its creation and modification attributes are whole-second `YYYYMMDDhhmmSSZ` values ([Apple Security documentation](https://developer.apple.com/documentation/security/secitemattr/creationdateitemattr)); converting the resulting Date to milliseconds only appends zeroes. Because public `storeMnemonic` deletes and re-adds the same item, a correct mnemonic can be verified and then replaced by a different mnemonic within the same second without changing this stamp. The persisted `xpub|stamp` marker is not cleared, and `seed_binding.rs` returns `MarkerMatched` before invoking the resolver, so the wrong-seed bypass remains. Store a random generation value alongside the Keychain item and rotate it on every mnemonic write, or invalidate the persisted marker whenever `storeMnemonic` writes.
| let created = attrs[kSecAttrCreationDate as String] as? Date ?? modified | ||
| // Millisecond precision; both dates so delete-then-add and in-place | ||
| // update are each guaranteed to change the stamp. | ||
| return "c\(Int64(created.timeIntervalSince1970 * 1000))" | ||
| + "-m\(Int64(modified.timeIntervalSince1970 * 1000))" |
There was a problem hiding this comment.
🔴 Blocking: Cached xpub marker bypasses current-seed verification
The Swift package supports native macOS15, but these Keychain operations omit kSecUseDataProtectionKeychain, so SecItem defaults to the file-based Keychain on macOS (Apple TN3137). Its creation and modification attributes are whole-second YYYYMMDDhhmmSSZ values (Apple Security documentation); converting the resulting Date to milliseconds only appends zeroes. Because public storeMnemonic deletes and re-adds the same item, a correct mnemonic can be verified and then replaced by a different mnemonic within the same second without changing this stamp. The persisted xpub|stamp marker is not cleared, and seed_binding.rs returns MarkerMatched before invoking the resolver, so the wrong-seed bypass remains. Store a random generation value alongside the Keychain item and rotate it on every mnemonic write, or invalidate the persisted marker whenever storeMnemonic writes.
source: ['codex']
2 participants
Issue being fixed or feature implemented
Two unconditional mnemonic/FFI touches ran on the app-launch unlock path for every restored wallet, every launch:
platform_wallet_verify_seed_binds_to_walletpulled the mnemonic from the Keychain through the resolver and derived mnemonic → seed → master xpriv → BIP44 account-0 xpub, just to compare against the persisted xpub. The result is a pure function of (mnemonic, network) vs a fixed persisted xpub — it cannot change between launches, and the sign-time pubkey binding remains the ultimate guard.platform_wallet_drain_pending_contact_cryptowas scheduled unconditionally per wallet, costing a detached task + FFI round-trips (and a resolver-driven contactInfo refresh) even when nothing actionable was queued.What was done?
Seed-bind verification marker (decision in Rust, storage in Swift):
rs-platform-wallet: newPlatformWallet::verify_seed_binds_with_marker— the marker binds the previously verified account-0 xpub to an identity stamp of the mnemonic Keychain item (its creation+modification dates, read attribute-only — the secret is never materialized). A match proves the binding was verified on an earlier launch AND the Keychain item is untouched since, and only then is the signer skipped. A missing/stale marker (first launch, wallet re-import) or a changed stamp (any rewrite/re-creation of the mnemonic item, includingstoreMnemonic's delete-then-add) falls through to the full derive-and-compare; a missing stamp disables the cache entirely (full check every launch — fail-safe, never bypassed).verify_seed_bindsnow delegates to it. NewSeedBindingVerificationoutcome enum.rs-platform-wallet-ffi: newplatform_wallet_verify_seed_binds_to_wallet_cachedtaking the nullable marker + nullable Keychain stamp;*out_markeris set (host must persist) only when a full verification ran, bound, and a stamp was supplied.SeedMismatchstill maps toErrorInvalidParameterexactly like the non-cached entry point, which is kept.swift-sdk: new optionalPersistentWallet.seedBindingVerifiedMarkercolumn plus load/store accessors on the persistence handler (mirroringsetWalletName); newWalletStorage.mnemonicKeychainStamp(for:)attribute-only reader;unlockWalletFromKeychainround-trips marker + stamp through the cached FFI. Swift only loads/stores — match-vs-verify is decided in Rust.Gated contact-crypto drain:
platform_wallet_drainable_contact_crypto_count/drainable_contact_crypto_count) that includesContactInfoDecrypt— so cross-device contact metadata (alias/note) still applies at launch and metadata-only queues are never stranded. The filteredpending_contact_crypto_countkeeps its existing semantics for the user-facing "waiting to finish setup" banner. On a probe failure the drain is scheduled anyway (old behavior — always safe).Observability: the resolver's Swift vtable entry (
MnemonicResolver.resolve), the unlock outcome, the marker persist, and the drain decision now log viaNSLogso "what touched the seed at launch" is auditable off-Xcode.How Has This Been Tested?
cargo test -p platform-wallet -p platform-wallet-ffi— green. New library tests pin the marker decision: no marker → full check + returns the stamped marker; matching marker+stamp →MarkerMatchedproven to skip the signer (a wrong-seed signer passes untouched); changed Keychain stamp → full check re-runs (replaced mnemonic rejected withSeedMismatch, same mnemonic re-verifies with the new stamp); no stamp → never matches, nothing cached; stale marker → falls through (wrong signer stillSeedMismatch). New FFI tests cover the null-pointer / unknown-handle marshalling contract of the cached entry point../build_ios.sh --target sim— Rust xcframework + SwiftDashSDK + SwiftExampleApp all build.tpub…/xpub…). Wiping one wallet's marker and relaunching produced exactly one full verify + marker re-persist for that wallet.5× "wallet unlock … seed verified",0×full verify, zero resolver calls on the unlock path. The only resolver calls in the session belonged to other startup paths (Orchard shielded bind — separate track) and to signing.🔑 resolve+🔐 marker persisted, marker persisted astpub…|c<created>-m<modified>); launch 2 unlocked via the marker path with zero resolver calls and no re-persist — proving the Keychain stamp is stable across launches (a drifting stamp would have silently disabled the cache).Breaking Changes
None. The marker column is optional/nullable (pre-release, no migration needed); rows without it simply re-verify once. The non-cached FFI entry point is unchanged and retained.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance
Bug Fixes