fix(wallet): don't hold launch on DashSync mnemonics orphaned by an 8.x reset - #1179
llbartekll wants to merge 2 commits into
Conversation
….x reset DashSync's Reset (DSChain.unregisterWallet) removes the wallet id from CHAIN_WALLETS_KEY_<genesis> and deletes the PIN, but never deletes WALLET_MNEMONIC_KEY_<id>. 8.x loads wallets only from those lists, so it stopped showing such a wallet. The key migrator treated that mnemonic as a wallet on an unknown chain: it never set the done sentinel, and the launch probe counted the mnemonic as pending. A device whose old wallets had all been reset therefore showed "Couldn't move your wallet" on every launch, with no way to create or restore a wallet, and reinstalling did not help because the keychain survives app deletion. With a live wallet next to an orphan the upgrade opened, but done was never set and the migrator re-processed the orphan on every launch. A mnemonic that no chain list names is now orphaned: the migrator skips it, leaves it in the keychain, and marks migration done when nothing else is pending, and the launch probe reports a keychain holding only orphans as absent, so setup is offered at once. That verdict needs every list read and decoded: a list that cannot be read or decoded defers as a failure, and a keychain with no chain list at all keeps the unknown-chain card. The lists are read one item at a time instead of pulling every keychain secret once per wallet. Each migration run past the done check also writes one summary line with counts, never wallet ids, to the app log: the migrator's os_log lines do not reach a diagnostic export. The LEGACY_KEYCHAIN_INVALID fixture now lists its entry on mainnet, so it still reproduces the failure card. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughMigration and launch probing now classify DashSync mnemonic accounts using per-chain wallet lists. Orphaned mnemonics remain in Keychain and do not block completion. Unreadable lists, unresolved wallets, and unsupported chains can defer migration or keep the launch hold. ChangesMnemonic migration classification
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant SwiftDashSDKKeyMigrator
participant Keychain
participant DashSyncChainWalletLists
SwiftDashSDKKeyMigrator->>Keychain: Enumerate accounts and read chain lists
Keychain-->>SwiftDashSDKKeyMigrator: Account attributes and chain-list data
SwiftDashSDKKeyMigrator->>DashSyncChainWalletLists: Decode lists and classify wallet IDs
DashSyncChainWalletLists-->>SwiftDashSDKKeyMigrator: Membership and material state
SwiftDashSDKKeyMigrator->>Keychain: Import supported listed wallets and record outcomes
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change stops launch from being held by leftover DashSync mnemonics after a reset, and it keeps ambiguous or unreadable states fail-closed. No merge-blocking risk was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change preserves legacy secrets and keeps missing or unreadable wallet-list information from being treated as a reset. No introduced security finding was established. Risk remains low rather than minimal because recovery after interrupted imports and ambiguous multi-chain membership are not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit e448300) · triage: critical |
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
Reviewed at faa55294. No blockers: the orphan classification matches how DashSync 8.x itself loaded wallets (only from the CHAIN_WALLETS_KEY_* lists), orphaned seeds stay in the keychain, and an unlisted mnemonic is never judged orphaned unless every list was read. Approving.
Non-blocking recommendations (none of these needs to land in this PR):
- Fail-closed scope is wider than it needs to be (
SwiftDashSDKKeyMigrator.swift:205, probe at:337). One unreadable or undecodable list, even an unrelated devnet one, now defers the whole run and makes the probe report.unreadable, including for a wallet that is plainly listed on a readable mainnet/testnet list. Before this PR that wallet still migrated (the bad list was skipped). Only the orphan verdict needs every list; a wallet found on a readable mainnet/testnet list could migrate regardless. Low likelihood, since DashSync writes every list the same way, but when it happens it is permanent and Try Again cannot clear it. - Devnet-only mnemonics still hold the launch forever (
:253). A mnemonic listed only on a devnet/regtest/evonet list keepsdeferredUnknownChain, done is never set, and the card has no way out. This predates the PR, but it is the same stuck-forever mechanism, and internal test phones are where it would show up. Worth a follow-up: treat it like an orphan for the done/launch decision (keep the seed, don't import it), or give the card an exit. - The orphan verdict cannot be undone. Once done is set, a mnemonic that was missing from the lists for some reason other than Reset (an older list layout, a failed
setKeychainArrayinregisterWallet) is never looked at again, and setup gives no hint that an old phrase exists. The removeddetectNetworklog of "chains present" was what separated a layout change from a real orphan; the new summary line keeps the chain suffixes, which helps. Consider at least a per-run count of orphans in the log export after done (already there for the run that sets done) and documenting how support recovers such a seed. - Legacy-layout knowledge moved outside the migrator.
DashSyncChainWalletLists.decodeWalletIDs(DashSync's NSKeyedArchiver list format) now lives inWalletLifecycleTransitionState.swift, whileSwiftDashSDKKeyMigrator.swift:23still says it is the only file that knows the layout (CLAUDE.md, guardrail 7). Either fix the header comment or keep the decoder in the migrator and pass decoded lists in. - Debug fixture accessibility (
:663, also:711). The fixture writes the chain list with.whenUnlockedThisDeviceOnly; DashSync uses AfterFirstUnlockThisDeviceOnly, andKeychainStore.set's update path would change a real list's class on a device that has an 8.x keychain. Afterwards a locked background launch fails the list read, which a real 8.x device never does. The comment "as DashSync would" is then not quite true. The two fixture branches also each archive and write the list separately; one helper would keep them consistent. - Card codes for the same condition disagree (
:211). An unreadable list givesdeferredFailure(KeyMigrator:failed) in the migrator but.unreadable(unreadableKeychain) in the probe. An undecodable list is not a lock or read problem, and retrying won't help.chainListUndecodablealso inheritsLegacyMnemonicCleanupError's "could not be removed" description. - Launch-time keychain I/O. The probe now also reads each chain list's data, synchronously on main, and launch calls it up to three times (
DWInitialViewController.viewDidLoad,shouldDisplayOnboarding,DWAppRootViewController). That is cheap at today's sizes, but computing it once and passing it down would remove the repeats. - Small cleanup. The "filter
WALLET_MNEMONIC_KEY_, drop prefix" mapping is repeated inperformMigration, the probe andstrictlyEnumerateDashSyncMnemonicAccounts; onemnemonicWalletIDs(in:)helper would stop them drifting.enumerateDashSyncMnemonicAccounts()(:542) appears to have no callers.
🤖 Generated with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The migrator and launch probe consistently skip orphaned mnemonics without deleting them, while missing chain lists, unsupported membership, and list read or decoding failures retain fail-closed behavior. Inspection of the complete base-to-head diff and relevant launch, import, runtime, and wipe callers found no actionable in-scope defects. Verification was static only: the supplied exact-head CI checks passed, while the build, test-harness results, and simulator migration smoke tests remain author-reported rather than independently verified.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — SwiftDashSDKKeyMigrator.swift substantially reworks legacy keychain enumeration, chain-list decoding, mnemonic migration eligibility and completion decisions, making this an intricate change to key handling and storage migration. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 51% left, 5h 84% left - 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— security-auditor (completed, effort xhigh); agentphase2-reviewer
Three review follow-ups on the orphaned-mnemonic change. An unreadable or undecodable chain list no longer defers the whole run. It is recorded as unreadable, never as empty, so a wallet that a readable mainnet or testnet list names still migrates, and a mnemonic no readable list names is undetermined rather than orphaned: the run defers as a failure, and the launch probe reports the keychain unreadable only when undetermined mnemonics are all that is left. Before, one bad list (even an unrelated devnet one) held back every wallet, permanently. The migrator header said it was the only file that knows DashSync's keychain layout, but decoding a wallet list moved to DashSyncChainWalletLists. It now says the migrator is the only reader of DashSync's wallet items and points at the pure decoder. The DEBUG fixtures wrote chain lists WhenUnlockedThisDeviceOnly; DashSync wrote them AfterFirstUnlockThisDeviceOnly, so the INVALID fixture would also change a real list's class. Both fixtures now write lists through one helper that matches DashSync's archive format and accessibility. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
Approving: no blockers. Nothing here deletes a mnemonic, and every case below is either rare or no worse than before this PR. The points are non-blocking, roughly in order of how much they matter:
- A list that never decodes still leaves an orphan-only device on the card.
DashSyncChainWalletLists.membershipreturns.undeterminedwhenever any list is inunreadableChains, including a devnet list that can't name a mainnet wallet. If oneCHAIN_WALLETS_KEY_*has bytes that don't decode as[NSString], a device with only reset wallets gets.unreadableon every launch, and Try Again reads the same bytes. A decode failure is permanent, unlikeerrSecInteractionNotAllowed, so it might be worth blocking only on a read error, or only on mainnet/testnet lists. It's the documented fail-closed choice and not a regression, but it is the same dead end the PR removes, in a narrower case. - "No list names it" isn't always a user Reset. DashSync also unlists wallets after the PIN-lockout wipe (
unregisterAllWalletsafterMAX_FAIL_COUNT), inunregisterAllWalletsMissingExtendedPublicKeys, and inregisterWalletwhengetKeychainArrayreturns nil (the list gets rewritten with only the new id). Those mnemonics now count as orphans. The seed stays, but setup is offered without any hint that an old recovery phrase is still on the device. Maybe just reword the doc comments ("reset in the previous app") so they don't overclaim, and consider a follow-up for surfacing it. - Launch now reads the keychain more.
legacyWalletMaterialPresent/PendingMigration(DWInitialViewController.m:62,:222),DWAppRootViewController.m:214andevaluate()each redo the enumeration plus one data read per chain list on the main thread, about 16 securityd round trips before the first screen with three lists. Caching the inventory per launch decision would remove that. - The
LEGACY_KEYCHAIN_MNEMONICDEBUG fixture replaces the mainnet list with only its own id (the INVALID fixture keeps the existing ids). On a simulator that holds a real 8.x wallet, that wallet becomes an orphan and is skipped silently. Before, that failed closed and a tester would have noticed. - Smaller things:
.listed(chains)wins even when the mainnet/testnet list is unreadable, so the card can sayunknownChaininstead ofunreadable.- The probe ignores
migratedDashSyncWalletIds, so it and the migrator can disagree on the card reason (.failedvs.unreadableKeychain). LEGACY_KEYCHAIN_ORPHAN=1now produces the "no list at all" state, not an orphan, and the real orphan layout (empty list, no PIN) has no fixture.- The decoding of DashSync's frozen layout now lives in
WalletLifecycleTransitionState.swift, next to the migrator's copy (CLAUDE.md guardrails #4/#7). The prefix filter and network resolution are written twice. Movingnetwork(for:)intoDashSyncChainWalletListswould keep the probe and the migrator in sync and make it testable.
This was a static review of the code at the PR head; I didn't build or run the branch.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
No in-scope defects were found in the complete base-to-head diff or the relevant launch, runtime, host-import, and cleanup callers. The shared classifier preserves orphaned mnemonics without blocking setup, fails closed on unresolved or unreadable membership, and allows supported wallets named by readable lists to migrate independently. Validation was static only: the supplied exact-head CI snapshot shows successful title, accessibility, and CodeRabbit checks but no build or test results; the author-reported build and simulator validation were not independently reproduced.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — SwiftDashSDKKeyMigrator.swift substantially reworks legacy keychain enumeration and migration eligibility, introducing multi-state chain-list classification that determines which stored mnemonics are imported or skipped and when migration is marked complete. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 23% left, 5h 10% left),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 final gate: an independent Phase-2 review ran after iterative findings were reconciled
- 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— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (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 the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
This branch has not been deployed
3 participants
Issue being fixed or feature implemented
A user who reset their wallet in the DashSync app (8.x) and then updates to 9.x, or reinstalls it, is stuck on Couldn't move your wallet on every launch. Try Again fails the same way, nothing leads to Create or Restore, and reinstalling does not help. Two internal test phones are in this state.
Root cause, reproduced end to end on a simulator with a real 8.7.0 build, its in-app Reset, and develop installed over it:
DSChain.unregisterWallet→DSWallet.wipeWalletInfo) removes the wallet id fromCHAIN_WALLETS_KEY_<genesis>(an emptied list stays) and deletes the PIN, but never deletesWALLET_MNEMONIC_KEY_<id>. 8.x loads wallets only from those lists, so it stopped showing that wallet.unknownChain). It never set the done sentinel, and the launch probe counted every mnemonic as pending, so the hold showed the card forever. The keychain survives app deletion, so reinstalling does not help.What was done?
DashSyncChainWalletLists(inWalletLifecycleTransitionState.swift, so the harness covers it) classifies each mnemonic against the chain lists:detectNetworkpulled every item in the service, including every mnemonic secret, once per wallet, and silently skipped lists it could not decode.log collectfrom the device.LEGACY_KEYCHAIN_INVALIDfixture now lists its entry on mainnet, so it still reproduces the failure card (KeyMigrator:failed). Before, it actually producedunknownChain, and with this change it would have been skipped as an orphan. Both fixtures now write chain lists the way DashSync did (same archive,AfterFirstUnlockThisDeviceOnly), so a fixture appending to a real list keeps that list's protection class.DASHSYNC_KEY_MIGRATION.mddescribes the orphan case, the fail-closed cases and the launch decision.Unchanged on purpose:
How Has This Been Tested?
python3 scripts/test_wallet_preparation.py: 53 tests, 0 failures (9 new: membership, launch verdict, list decoding of DashSync'sNSMutableArrayarchives, undecodable lists never read as empty, and an unreadable list neither blocking wallets that other lists name nor turning an unnamed mnemonic into an orphan).dashpayDebug build for the arm64 simulator, on top of fix(wallet): adopt the UIScene life cycle so the app launches on iOS 27 #1145.dashwalletscheme, DashSyncd3c1497a) and its in-app Reset:deferredUnknownChainand no done; then this build. The wallet opens, done is set, the flag is cleared, and the orphan is not processed again.LEGACY_KEYCHAIN_INVALID=1: the card shows withKeyMigrator:failed, and Export Logs explains it needs the PIN.LEGACY_KEYCHAIN_MNEMONIC+LEGACY_KEYCHAIN_ORPHAN=1, which leaves no chain list: the card shows withunknownChain, and Export Logs asks for the PIN.unreadableKeychain.LEGACY_KEYCHAIN_INVALIDcases were rerun on the second commit.Not tested on a physical device or with App Store binaries.
Breaking Changes
None. Devices whose old wallets were all reset in 8.x now reach setup instead of the failure card. Their leftover phrases stay in the keychain.
🤖 Generated with Claude Code
Summary by CodeRabbit