fix(swift-sdk): accept non-English BIP-39 mnemonics in seed derivation and wallet construction - #4454
Conversation
…n and wallet construction mnemonic_validate tries every supported wordlist, but the key-wallet FFI behind Mnemonic.toSeed and Wallet(mnemonic:) parses with a hardcoded English wordlist, so any French/Spanish/Japanese/... phrase that passed validation still failed with 'Invalid Mnemonic: mnemonic contains an unknown word (word 0)'. In dashwallet-ios this broke the DashSync upgrade migration (legacy localized wallets silently not imported) and 'Import from Phrase'. Fall back when the English-only FFI rejects a phrase that validates in another language: derive the BIP-39 seed directly (PBKDF2-HMAC-SHA512 over the NFKD sentence, the wordlist-independent derivation DashSync and Electrum use) and build wallets from that seed - wallet ids and keys are seed-determined, so the result is identical. A regression test also pins that Rust's language-auto-detecting derivation path (parse_mnemonic_any_language) agrees with this seed, keeping app-computed wallet ids consistent with manager-created wallets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (3)
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 — Opus deferred (commit cf2ac62) |
|
Re-planned per review: the Swift-layer fallback here fixes the symptom at the wrong layer of the stack. The root fix now lives in dashpay/rust-dashcore#981 ( Once #981 merges into rust-dashcore
Converting to draft until then. 🤖 Posted autonomously by Claude on behalf of pasta. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The multilingual BIP-39 derivation is correctly gated by full mnemonic validation and follows the required NFKD/PBKDF2-HMAC-SHA512 procedure. However, the wallet fallback changes mnemonic-created wallets into seed-backed wallets, and the new derivation path handles embedded-NUL passphrases differently from the existing FFI path.
Source: reviewer backends: gpt-5.6-sol (general, security-auditor, FFI-engineer); final verifier backend: gpt-5.6-sol. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
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 - Sonnet: not run (deferred by blocker gate)
🔴 1 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/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Wallet.swift:85-87: Localized mnemonic initialization creates a seed wallet
The fallback calls `wallet_create_from_seed[_with_options]`, which the pinned key-wallet implementation routes through `Wallet::from_seed` and stores as `WalletType::Seed`. The normal mnemonic constructor routes through `Wallet::from_mnemonic` and stores `WalletType::Mnemonic`. This difference is directly observable through the public `Wallet.hasMnemonic` property: a valid French phrase now constructs successfully but reports `false`, while an English phrase passed to the same initializer reports `true`. The variants also retain different recovery material and have different serialized representations, so this is not the identical wallet claimed by the comment and can cause backup or capability code to misclassify an imported recovery phrase. Keep construction on a language-aware mnemonic constructor path, such as by updating the pinned Rust implementation, rather than replacing mnemonic construction with seed construction.
In `packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Mnemonic.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/KeyWallet/Mnemonic.swift:163-164: Embedded-NUL passphrases diverge between Swift and Rust derivation
The existing FFI path passes the passphrase through `withCString`, and pinned Rust reads it with `CStr::from_ptr`, so an embedded U+0000 terminates the passphrase and discards its suffix. The new non-English fallback converts the entire Swift string to UTF-8 with an explicit length, causing the NUL and suffix to participate in PBKDF2. Consequently, the same passphrase has language-dependent semantics, and the fallback can derive a different seed from the Rust implementation. BIP-39 defines the passphrase as normalized UTF-8 rather than a C string; either reject embedded NUL before selecting either path or change the Rust ABI to accept a pointer and explicit byte length.
| let seed = try Mnemonic.toSeed(mnemonic: mnemonic) | ||
| self.handle = try Self.createHandle( | ||
| seed: seed, network: network, accountOptions: accountOptions) |
There was a problem hiding this comment.
🔴 Blocking: Localized mnemonic initialization creates a seed wallet
The fallback calls wallet_create_from_seed[_with_options], which the pinned key-wallet implementation routes through Wallet::from_seed and stores as WalletType::Seed. The normal mnemonic constructor routes through Wallet::from_mnemonic and stores WalletType::Mnemonic. This difference is directly observable through the public Wallet.hasMnemonic property: a valid French phrase now constructs successfully but reports false, while an English phrase passed to the same initializer reports true. The variants also retain different recovery material and have different serialized representations, so this is not the identical wallet claimed by the comment and can cause backup or capability code to misclassify an imported recovery phrase. Keep construction on a language-aware mnemonic constructor path, such as by updating the pinned Rust implementation, rather than replacing mnemonic construction with seed construction.
source: ['codex']
| var salt = [UInt8]( | ||
| ("mnemonic" + (passphrase ?? "")).decomposedStringWithCompatibilityMapping.utf8) |
There was a problem hiding this comment.
🟡 Suggestion: Embedded-NUL passphrases diverge between Swift and Rust derivation
The existing FFI path passes the passphrase through withCString, and pinned Rust reads it with CStr::from_ptr, so an embedded U+0000 terminates the passphrase and discards its suffix. The new non-English fallback converts the entire Swift string to UTF-8 with an explicit length, causing the NUL and suffix to participate in PBKDF2. Consequently, the same passphrase has language-dependent semantics, and the fallback can derive a different seed from the Rust implementation. BIP-39 defines the passphrase as normalized UTF-8 rather than a C string; either reject embedded NUL before selecting either path or change the Rust ABI to accept a pointer and explicit byte length.
source: ['codex']
|
Coordination update: the underlying bug is now fixed upstream twice over —
Revised plan for this PR: once #4455 lands, rebase this to drop the Swift-side PBKDF2 fallback entirely and keep only 🤖 Posted autonomously by Claude on behalf of pasta. |
2 participants
Issue being fixed or feature implemented
Field report from dashwallet-ios testing (2026-08-22): a user's multi-year-old French 12-word recovery phrase could not be imported into the migrated app:
Invalid Mnemonic: mnemonic contains an unknown word (word 0);Root cause:
mnemonic_validate(key-wallet FFI) tries all 10 supported BIP-39 wordlists, but the two functions actually used for key material —mnemonic_to_seedandwallet_create_from_mnemonic— parse with hardcodedLanguage::English(pinned rust-dashcore rev, not fixable in this repo). So any non-English phrase passesMnemonic.validateand then fails inMnemonic.toSeed/Wallet(mnemonic:). In dashwallet-ios that breaksSwiftDashSDKKeyMigrator(seed sanity check throws → legacy wallet never imported),SwiftDashSDKHost.addWallet(wallet-id resolver), and the onboarding recover flow, for every legacy localized wallet.What was done?
Swift-layer fallback in
packages/swift-sdk, scoped to exactly the phrases the English-only FFI wrongly rejects:Mnemonic.toSeed: when the FFI fails withinvalidMnemonicand the phrase validates in some supported language, derive the BIP-39 seed directly — PBKDF2-HMAC-SHA512 over the NFKD-canonicalized sentence (normalizePhrase: NFKD + lowercase + single-space separators), salt"mnemonic" + NFKD(passphrase), 2048 rounds, 64 bytes. This is the wordlist-independent derivation the BIP-39 spec defines and what DashSync and Electrum compute — i.e. the seed that recovers the user's actual funds. Phrase and salt buffers are scrubbed after use.Wallet(mnemonic:): same trigger; builds the wallet from the derived seed via a sharedcreateHandle(seed:)(extracted frominit(seed:), behavior-preserving). Wallet ids and derived keys are seed-determined, so the resulting wallet is identical to what a language-aware mnemonic parse would produce.The rest of the pipeline already auto-detects language in Rust (
parse_mnemonic_any_languageinrs-platform-wallet/rs-platform-wallet-ffi):PlatformWalletManager.createWallet(mnemonic:), the keychain signing resolver, and identity-key derivation. This change brings the two remaining app-hit entry points in line with them.How Has This Been Tested?
New
SwiftTests/SwiftDashSDKTests/NonEnglishMnemonicTests.swift(11 tests):000102…0f, official French wordlist) — seed with and without passphrase, matching an independent Python oracle (hashlib.pbkdf2_hmacover the NFKD sentence) that reproduces the official BIP-39 English test vectors;platform_wallet_derive_ext_priv_key_from_mnemonic→parse_mnemonic_any_language+ rust-bip39to_seed) derives the same BIP-32 master key as the reference seed, pinning that app-side wallet ids match manager-created wallets;Red→green verified locally: all 5 French tests failed before the fix with exactly the field-reported error; all 11 pass after. Full swift-sdk unit suite: 365 tests, 0 failures (macOS, hermetic).
xcodebuildcompile for iOS simulator clean.Breaking Changes
None. Strictly additive: only inputs that previously threw now succeed; English behavior is byte-identical.
Known remaining gap (follow-up):
WalletManager.addWallet(mnemonic:)andPlatformWallet.fromMnemonic(_:network:)bind to the same English-hardcoded FFI and still reject non-English phrases. Neither has a production caller today. The proper root fix is upstream in rust-dashcore (key-wallet-ffi), which would also let this fallback be removed.Checklist:
🤖 Generated with Claude Code