fix(swift-sdk): gate the ordered bring-up on the seed actually owning the wallet - #4368
Merged
Merged
Conversation
… the wallet `startWalletSubsystems` built its own `MnemonicResolver` and `KeychainSigner` and did no binding check, so it did not inherit the wrong-seed gate `unlockWalletFromKeychain` applies. On a wallet whose identity is already known it skips discovery and goes straight to the contact sync and drain — deriving DIP-15 contact accounts from a mnemonic the wallet has already rejected. That damage does not heal. `register_contact_account` keys its existence check on `(index, owner, friend)` and never on the xpub, and unlike the external-account side there is no rotation sweep for receiving accounts, so a wrong xpub is written once and every later correct-seed pass no-ops over it. The wallet watches addresses nobody pays to, with no symptom beyond payments that never arrive. The two other drain branches are narrower: a wrong-seed ECDH product fails `RegisterExternal`'s xpub format parse and the rotation sweep rebuilds it, and the auto-accept leg cannot broadcast at all because `KeychainSigner` binds the on-chain public key and rejects a wrong-seed key before signing. **Why a new API rather than calling the unlock.** `unlockWalletFromKeychain` is not a verification primitive: when the queue is non-empty it also starts a detached drain and sets the `draining` guard. Using it as a preflight would have this call race a second drain over a second snapshot — duplicated network and ECDH work, and competing channel-broken and auto-accept writes, which is exactly what that guard exists to prevent. So step 1 is now `verifySeedBinding`, public and side-effect-free, and the unlock is that call plus the drain. No behaviour change for existing unlock callers. **Why in the SDK rather than in each host.** A host that has to remember to gate this call will eventually forget, and the published `seedMismatch` flag cannot serve as that gate: hosts commonly kick the unlock off asynchronously (iOS schedules it in a detached task and returns), so the flag races the very call it is supposed to guard. Verification performed inside the call is ordered with respect to the work it protects; a flag read outside it is not. Cost is a string comparison on the common path — the verify is marker-cached, and a match never touches the Keychain. Reported on dashpay/dashwallet-ios#961, whose call site this unblocks. Verified: `xcodebuild -scheme SwiftDashSDK -sdk iphonesimulator` clean.
romchornyi
requested review from
QuantumExplorer,
ZocoLini,
llbartekll and
shumkov
as code owners
August 11, 2026 10:41
Contributor
|
Warning Review limit reached
Next review available in: 1 minute 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a public seed-binding verification API. Wallet unlock and subsystem startup call it before creating signers or draining deferred contact cryptography. Watch-only wallets return a distinct result. ChangesSeed binding verification flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WalletSubsystems
participant PlatformWalletManager
participant SeedBindingCheck
participant ResolverSigner
WalletSubsystems->>PlatformWalletManager: startWalletSubsystems(wallet)
PlatformWalletManager->>SeedBindingCheck: verifySeedBinding(wallet)
SeedBindingCheck-->>PlatformWalletManager: .verified or mismatch error
PlatformWalletManager->>ResolverSigner: create signer after verification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
🕓 Ready for review — next in queue (commit b869720) |
romchornyi
pushed a commit
to dashpay/dashwallet-ios
that referenced
this pull request
Aug 11, 2026
From review on #961. The previous commit used `unlockWalletFromKeychain` as the preflight, which is not a verification primitive: when the contact-crypto queue is non-empty it also starts a detached drain, so the bring-up's own drain would race a second one over a second snapshot — duplicated network and ECDH work, and competing channel-broken / auto-accept writes. There was no verify-only API to call instead, so the gate moved into the SDK, where it belongs anyway: dashpay/platform#4368 adds `PlatformWalletManager.verifySeedBinding` and has `startWalletSubsystems` call it before touching any key material. Every client is now protected, not just this call site. This file goes back to what it was: the call site and its logging. A mismatch now surfaces as a throw from the bring-up, which the existing catch already handles the right way — log it and start Core SPV, because a wallet without a balance is worse than a wallet without DashPay state. Requires platform#4368; the SDK is a local path dependency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- Line 750: Update the watch-only return path in the wallet verification method
to call setDashPaySeedMismatch(walletId, false) before returning .watchOnly,
ensuring any previously published seed mismatch is cleared when the mnemonic is
removed.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift`:
- Line 144: Update the startup flow around verifySeedBinding(wallet) to call an
internal storage-aware overload with the caller-provided storage, ensuring
verification uses the same WalletStorage as the resolver and persistence logic.
Preserve the public verifySeedBinding overload as a default-storage convenience
API, and have the new overload perform the existing verification behavior
against the supplied storage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 95235abe-08fb-4ab0-8c60-b5dfb9a4b370
📒 Files selected for processing (2)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerStartup.swift
…from Both from review on #4368. **The gate could check a different Keychain than the work uses.** `startWalletSubsystems` takes a `storage` parameter and builds its resolver from it, but the verify I added constructed its own default `WalletStorage`. Where those differ — tests, or any host that injects a store — verification would approve one mnemonic while the derivation ran on another, which is the exact failure this gate exists to prevent, just harder to see. The check now takes the store as a parameter and passes it to the resolver too; the public no-argument form is the default-store convenience and is what the unlock path still uses. **A removed mnemonic left the banner up.** The watch-only early return came before `setDashPaySeedMismatch(walletId, false)`, so a wallet whose seed failed to bind and whose Keychain item was then deleted kept publishing a mismatch for a seed that no longer exists. No mnemonic is not a mismatch. Verified: `xcodebuild -scheme SwiftDashSDK -sdk iphonesimulator` clean.
QuantumExplorer
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
startWalletSubsystems(#4359) builds its ownMnemonicResolverandKeychainSigner, and did no binding check — so it never inherited the wrong-seed gateunlockWalletFromKeychainapplies. On a wallet whose identity is already known it skips discovery and goes straight to the contact sync and drain, deriving DIP-15 contact accounts from a mnemonic the wallet has already rejected.That damage does not heal.
register_contact_accountkeys its existence check on(index, owner, friend)and never on the xpub, and unlike the external-account side there is no rotation sweep for receiving accounts. A wrong xpub is written once and every later correct-seed pass no-ops over it, so the wallet watches addresses nobody pays to — with no symptom except payments that never arrive.The other two drain branches are narrower, and worth stating so the fix is not over-scoped:
RegisterExternalis self-defending — a wrong-seed ECDH product fails the 69/78/107-byte xpub format parse, the channel is marked broken, and the existing rotation sweep rebuilds it once the correct seed is present.KeychainSignerpasses the on-chain public key as a binding and the signer rejects a wrong-seed key before producing a signature.So nothing reaches Platform under a mismatched seed. What breaks is local state, silently and permanently.
Reported by review on dashpay/dashwallet-ios#961, which is the first client of this call and is blocked on this.
What was done?
PlatformWalletManager.verifySeedBinding(_:)— step 1 ofunlockWalletFromKeychain, extracted verbatim and made public. Returns.verified/.watchOnly, throws on a mismatch.unlockWalletFromKeychainis now that call plus its drain, so no existing caller changes behaviour.startWalletSubsystemscalls it before anything else, so every client is protected rather than every client having to remember.Why a new API rather than calling the unlock
unlockWalletFromKeychainis not a verification primitive. When the queue is non-empty it also starts a detached drain and sets the Swift-sidedrainingguard. Using it as a preflight would leave this call racing a second drain over a second snapshot — duplicated network and ECDH work, and competing channel-broken / auto-accept writes. That is precisely what the existing guard exists to prevent, and it is reachable both from a persisted pending queue and from work the inline contact sync queues while the startup drain runs.Why in the SDK rather than in each host
Two reasons, and the second is the load-bearing one:
seedMismatchflag cannot serve as that gate. Hosts commonly kick the unlock off asynchronously — iOS schedules it in a detached, un-awaitedTaskand returns the wallet immediately, then goes straight into the bring-up — so the flag is racing the very call it is meant to guard, and on the launch where the seed is wrong it may not be set yet. A verification performed inside the call is ordered with respect to the work it protects; a flag read outside it is not.Cost on the common path is a string comparison: the verify is marker-cached, and a match never touches the Keychain.
Considered and rejected
Putting the gate in
rs-platform-wallet'sstart_wallet_subsystemsinstead. That is the stronger home — a future JNI client inherits it — but the marker/stamp cache that keeps the check free is persisted host-side, so Rust would need both threaded through the FFI plus a new terminalSeedMismatchstatus, i.e. an ABI addition. Worth doing when a second client appears; not worth blocking a security fix behind today.How Has This Been Tested?
Plus a clean
dashpaybuild of dashwallet-ios#961 against this branch, on the iOS 26.5 simulator — that PR drops its own app-side workaround in favour of this gate.Not covered: no automated test. Exercising it needs a wallet whose persisted account-0 xpub disagrees with the Keychain mnemonic, which the SwiftDashSDK test targets have no fixture for today; the underlying verification itself is already covered Rust-side in
seed_binding.rs(verify_seed_binds_rejects_wrong_signer).Breaking Changes
None to existing callers —
unlockWalletFromKeychainkeeps its signature and behaviour.One new failure mode for adopters of
startWalletSubsystems: it now throws when the stored seed does not bind to the wallet, where it previously proceeded. That is the point of the change, and the documented contract already says Core sync must start regardless — the iOS call site catches, logs, and starts SPV.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Security
Bug Fixes