feat: show seed on wallet creation #6946
Conversation
…yleSheet Collect regex matches first, compute replacements using capturedStart/capturedEnd, then apply in reverse order to prevent iterator invalidation and QString corruption when processing <os=...></os> sections. Also include <algorithm> for std::sort.
- Add collapsible advanced options toggle in CreateWalletDialog - Add mnemonic verification dialog after wallet creation - Implement getMnemonic() interface method for retrieving mnemonic phrases - Add proper error handling for wallet lock state restoration - Add styling for both light and dark themes - Show verification dialog for HD wallets after creation - Skip verification for blank wallets and wallets with disabled private keys
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request adds mnemonic (recovery phrase) verification and viewing to the Qt wallet UI. It introduces a new MnemonicVerificationDialog (UI form, header, implementation, and MOC), extends the Wallet interface with Sequence Diagram(s)sequenceDiagram
participant User
participant BitcoinGUI
participant WalletFrame
participant WalletView
participant WalletImpl
participant MnemonicDialog
User->>BitcoinGUI: Click "Settings > Show Recovery Phrase"
BitcoinGUI->>WalletFrame: showMnemonic()
WalletFrame->>WalletView: showMnemonic()
WalletView->>WalletImpl: getMnemonic() (may prompt unlock)
alt Mnemonic retrieved
WalletView->>MnemonicDialog: Create (viewOnly=true)
Note over MnemonicDialog: Step 1: Display mnemonic for recording
User->>MnemonicDialog: Click Continue
Note over MnemonicDialog: Step 2: Prompt 3 random words, validate input
User->>MnemonicDialog: Enter words & OK
MnemonicDialog->>MnemonicDialog: Validate & clear sensitive memory
else Retrieval failed or not permitted
WalletView->>User: Show warning / abort
end
sequenceDiagram
participant User
participant WalletController
participant WalletImpl
participant Wallet
participant MnemonicDialog
User->>WalletController: Finish wallet creation
WalletController->>Wallet: Is HD & has mnemonic?
alt Yes
WalletController->>Wallet: Unlock if encrypted
alt Unlock success
WalletController->>WalletImpl: getMnemonic()
alt Mnemonic retrieved
WalletController->>MnemonicDialog: Create (viewOnly=false)
Note over MnemonicDialog: Two-step verification flow
User->>MnemonicDialog: Complete or Cancel
alt Accepted
WalletController->>User: Proceed with creation
else Rejected
WalletController->>User: Notify, proceed/abort as flow dictates
end
MnemonicDialog->>WalletController: Securely clear sensitive data
else Retrieval failed
WalletController->>User: Log warning, restore lock state
end
else Unlock fails
WalletController->>User: Warn and finish creation path
end
else Non-HD or no keys
WalletController->>WalletController: Skip verification
end
WalletController->>WalletController: Emit finished signal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Key areas requiring careful attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/qt/guiutil.cpp (1)
990-996: Guard against substring matches when filtering OS blocks.Using
contains()means a value like-uiplatform=oswould match both<os="windows">and<os="macosx">. Please split the attribute on commas, trim the tokens, and compare for exact equality (ideally case-insensitive) so we only keep blocks for the intended platform.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Makefile.qt.include(4 hunks)src/interfaces/wallet.h(1 hunks)src/qt/createwalletdialog.cpp(2 hunks)src/qt/forms/createwalletdialog.ui(2 hunks)src/qt/forms/mnemonicverificationdialog.ui(1 hunks)src/qt/guiutil.cpp(2 hunks)src/qt/mnemonicverificationdialog.cpp(1 hunks)src/qt/mnemonicverificationdialog.h(1 hunks)src/qt/res/css/dark.css(1 hunks)src/qt/res/css/light.css(1 hunks)src/qt/walletcontroller.cpp(2 hunks)src/qt/walletcontroller.h(1 hunks)src/wallet/interfaces.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/walletcontroller.cppsrc/qt/createwalletdialog.cppsrc/wallet/interfaces.cppsrc/interfaces/wallet.hsrc/qt/walletcontroller.hsrc/qt/mnemonicverificationdialog.hsrc/qt/guiutil.cppsrc/qt/mnemonicverificationdialog.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/qt/walletcontroller.cppsrc/Makefile.qt.includesrc/wallet/interfaces.cppsrc/interfaces/wallet.h
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/Makefile.qt.include
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/Makefile.qt.include
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/interfaces.cpp
🧬 Code graph analysis (5)
src/qt/walletcontroller.cpp (1)
src/qt/walletcontroller.h (1)
m_wallet_model(103-106)
src/interfaces/wallet.h (1)
src/wallet/interfaces.cpp (2)
mnemonic_out(528-578)mnemonic_out(528-528)
src/qt/walletcontroller.h (1)
src/qt/walletcontroller.cpp (2)
restoreWalletLockState(360-388)restoreWalletLockState(360-360)
src/qt/mnemonicverificationdialog.h (1)
src/qt/mnemonicverificationdialog.cpp (36)
MnemonicVerificationDialog(24-94)MnemonicVerificationDialog(96-101)accept(301-310)accept(301-301)onShowMnemonicClicked(227-235)onShowMnemonicClicked(227-227)onHideMnemonicClicked(237-245)onHideMnemonicClicked(237-237)onWord1Changed(256-256)onWord1Changed(256-256)onWord2Changed(257-257)onWord2Changed(257-257)onWord3Changed(258-258)onWord3Changed(258-258)onShowMnemonicAgainClicked(247-254)onShowMnemonicAgainClicked(247-247)setupStep1(103-137)setupStep1(103-103)setupStep2(139-212)setupStep2(139-139)generateRandomPositions(214-225)generateRandomPositions(214-214)updateWordValidation(272-299)updateWordValidation(272-272)validateWord(260-270)validateWord(260-260)clearMnemonic(312-316)clearMnemonic(312-312)buildMnemonicGrid(364-426)buildMnemonicGrid(364-364)parseWords(318-334)parseWords(318-318)clearWordsSecurely(336-346)clearWordsSecurely(336-336)getWordCount(348-362)getWordCount(348-348)
src/qt/mnemonicverificationdialog.cpp (1)
src/qt/mnemonicverificationdialog.h (1)
MnemonicVerificationDialog(18-56)
…ication - Guard against short mnemonics (< 3 words) to prevent infinite loop - Add validation in setupStep2() to check word count before proceeding - Add safety check after generateRandomPositions() to ensure positions were generated - Add missing #include <algorithm> for std::sort usage
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/qt/mnemonicverificationdialog.cpp (1)
349-357: Zero the temporary mnemonic buffer before releasing it.
mnemonicStr.clear()/squeeze()frees capacity but doesn’t scrub the characters, leaving the seed words in heap memory. Please overwrite the buffer (e.g.,mnemonicStr.fill(QChar(0))) before clearing—mirroring whatclearWordsSecurely()already does—so the temporary copy is actually wiped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/qt/mnemonicverificationdialog.cpp(1 hunks)src/qt/walletcontroller.cpp(2 hunks)src/wallet/interfaces.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/interfaces.cppsrc/qt/mnemonicverificationdialog.cppsrc/qt/walletcontroller.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/interfaces.cppsrc/qt/walletcontroller.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/interfaces.cpp
🧬 Code graph analysis (2)
src/qt/mnemonicverificationdialog.cpp (1)
src/qt/mnemonicverificationdialog.h (1)
MnemonicVerificationDialog(18-56)
src/qt/walletcontroller.cpp (1)
src/qt/walletcontroller.h (1)
m_wallet_model(103-106)
🔇 Additional comments (1)
src/wallet/interfaces.cpp (1)
528-577: Mnemonic retrieval logic looks solid.Descriptor and legacy SPK managers are both covered under the wallet lock, with clean failure paths. Nicely done.
| SecureString mnemonic_passphrase; | ||
| bool has_mnemonic = m_wallet_model->wallet().getMnemonic(mnemonic, mnemonic_passphrase); | ||
|
|
||
| if (!has_mnemonic || mnemonic.empty()) { | ||
| // No mnemonic found - log warning and skip verification | ||
| restoreWalletLockState(was_locked, was_unlocked_for_mixing); | ||
| // Clear sensitive data before showing message | ||
| mnemonic.assign(mnemonic.size(), 0); | ||
| mnemonic_passphrase.assign(mnemonic_passphrase.size(), 0); | ||
| QMessageBox::warning(m_parent_widget, tr("Mnemonic retrieval failed"), | ||
| tr("Could not retrieve mnemonic phrase from wallet. Wallet creation completed but verification skipped.")); | ||
| Q_EMIT created(m_wallet_model); | ||
| Q_EMIT finished(); | ||
| return; | ||
| } | ||
|
|
||
| // Wallet has mnemonic - show verification dialog | ||
| MnemonicVerificationDialog verify_dialog(mnemonic, m_parent_widget); | ||
| verify_dialog.setWindowModality(Qt::ApplicationModal); | ||
|
|
||
| // Clear mnemonic from local variables after dialog has copied it | ||
| // The dialog will manage its own copy securely | ||
| const size_t mnemonic_size = mnemonic.size(); | ||
| const size_t passphrase_size = mnemonic_passphrase.size(); | ||
| mnemonic.assign(mnemonic_size, 0); | ||
| mnemonic_passphrase.assign(passphrase_size, 0); | ||
|
|
||
| if (verify_dialog.exec() == QDialog::Accepted) { | ||
| // Verification successful | ||
| restoreWalletLockState(was_locked, was_unlocked_for_mixing); | ||
| Q_EMIT created(m_wallet_model); | ||
| } else { | ||
| // User cancelled verification | ||
| restoreWalletLockState(was_locked, was_unlocked_for_mixing); | ||
| QMessageBox::warning(m_parent_widget, tr("Verification cancelled"), | ||
| tr("You cancelled mnemonic verification. Please make sure you have saved your mnemonic phrase safely.")); | ||
| Q_EMIT created(m_wallet_model); | ||
| } |
There was a problem hiding this comment.
Don't drop the mnemonic passphrase.
Line 314 stores mnemonic_passphrase, but nothing ever shows it to the user—Line 338 wipes it and the dialog only receives the word list. If the wallet is born with a non-empty BIP39 passphrase (today via RPC, tomorrow via advanced GUI options), the backup produced here is incomplete and the wallet becomes unrecoverable. Please surface the passphrase in the dialog (with explicit acknowledgement) or abort the flow when it’s non-empty instead of silently discarding it.
🤖 Prompt for AI Agents
In src/qt/walletcontroller.cpp around lines 314-351, the code reads and then
immediately zeroes the mnemonic_passphrase while only passing the mnemonic word
list to the MnemonicVerificationDialog, which silently drops a non-empty BIP39
passphrase and makes the backup unrecoverable; either surface the passphrase in
the verification flow or abort when it exists. Fix by passing
mnemonic_passphrase into the verification dialog (or a new ctor/param) so the
dialog can display/require explicit acknowledgement of the passphrase (masked by
default with a reveal control and a confirmation checkbox), do not zero
mnemonic_passphrase until after the dialog has consumed/acknowledged it, and if
you choose not to show it then abort the flow with a clear warning message
instructing the user to back up the passphrase before proceeding; in all cases
ensure sensitive buffers are securely zeroed only after the dialog finishes
handling them and restore the existing lock state consistently.
There was a problem hiding this comment.
I think just exposing the generated seed is sufficient. Any mnemonic_passphrase would be user provided; and we don't need to re-display that
There was a problem hiding this comment.
@PastaPastaPasta, that's a fair point—BIP39 passphrases are typically optional and user-provided, so if someone explicitly set one during wallet creation, they would already know it and be responsible for backing it up separately.
However, to future-proof this flow, it might be worth adding a simple check: if mnemonic_passphrase is non-empty when entering the verification dialog, show a brief informational message reminding the user that their wallet also uses a BIP39 passphrase (which they provided) and that it must be backed up separately alongside the mnemonic. This way, if a wallet is ever created via RPC or future UI with a passphrase, the user gets a reminder without the code silently dropping the passphrase context.
Something minimal like:
if (!mnemonic_passphrase.empty()) {
QMessageBox::information(m_parent_widget, tr("Passphrase Reminder"),
tr("This wallet was created with a BIP39 passphrase. Make sure you have backed it up separately—it is required along with the mnemonic to restore your wallet."));
}This keeps the flow simple while providing a safety net for edge cases. Would that work for you?
🧠 Learnings used
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
src/wallet/interfaces.cpp
Outdated
| SecureString ssMnemonic; | ||
| SecureString ssMnemonicPassphrase; | ||
| if (!hdChainCurrent.GetMnemonic(ssMnemonic, ssMnemonicPassphrase)) { | ||
| return false; | ||
| } | ||
|
|
||
| mnemonic_out = ssMnemonic; | ||
| mnemonic_passphrase_out = ssMnemonicPassphrase; |
There was a problem hiding this comment.
nit: why not just
- SecureString ssMnemonic;
- SecureString ssMnemonicPassphrase;
+ return hdChainCurrent.GetMnemonic(mnemonic_out, mnemonic_passphrase_out));
- if (!hdChainCurrent.GetMnemonic(ssMnemonic, ssMnemonicPassphrase)) {
- return false;
- }
-
- mnemonic_out = ssMnemonic;
- mnemonic_passphrase_out = ssMnemonicPassphrase;
Move theme-independent styling from inline C++ code to general.css following Dash Core CSS architecture (general.css for layout, dark/light.css for colors). Changes: - Move font sizes, weights, and families to general.css - Move widget size constraints to general.css - Move label margins to general.css - Remove inline HTML style attributes from translated strings - Simplify C++ code to only handle dynamic theme colors via GUIUtil Benefits: - Better separation of concerns (styling vs logic) - Easier theme customization without recompiling - Consistent with Dash Core styling patterns - Cleaner, more maintainable C++ code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
When clicking "Back" in the verification step (Step 2) to return to the word list display (Step 1), restore the original dialog size. Issue: Step 2 reduces minimum size to 460x280 for compact verification form, but setupStep1() didn't restore the original 550x360 size, causing the dialog to remain small when returning to the larger word list view. Fix: - Restore minimum size to 550x360 in setupStep1() - Resize to m_defaultSize when returning from Step 2 - Keep adjustSize() only for initial setup This ensures a consistent dialog size throughout the user flow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add "Show Recovery Phrase" menu item under Settings that allows users to view their mnemonic seed phrase for existing HD wallets with a streamlined view-only interface. Menu Integration: - Added Settings → Show Recovery Phrase menu option - Menu item positioned between "Change Passphrase" and "Unlock Wallet" - Enabled for HD wallets with private keys - Disabled for watch-only wallets (NoKeys status) View-Only Dialog Mode: - New optional viewOnly parameter in MnemonicVerificationDialog constructor - Simplified interface without verification step for viewing existing mnemonics - Window title: "Your Recovery Phrase" (vs "Save Your Mnemonic" for new wallets) - Button: Single "Close" button (Cancel button hidden) - Checkbox: "I have written down" checkbox hidden - Flow: Single-step view without word verification requirement - Adjusted warning text appropriate for viewing existing phrase Security Features: - Prompts for passphrase if wallet is encrypted (unlocks temporarily) - Validates wallet capabilities (HD enabled, has private keys) - Clears mnemonic from memory immediately after dialog creation - Properly restores wallet lock state after viewing: * Returns to locked state if was locked * Returns to unlocked-for-mixing state if was in that mode - Secure memory handling throughout with SecureString Error Handling: - Clear error messages for non-HD wallets - Clear error messages for watch-only wallets - Graceful handling of user cancellation during unlock - Proper error recovery with lock state restoration Backward Compatibility: - viewOnly parameter defaults to false - Original verification flow unchanged for new wallet creation - Existing MnemonicVerificationDialog behavior preserved This allows users to backup their recovery phrase after wallet creation without needing to recreate the wallet, with a cleaner UX that skips unnecessary verification steps when viewing an existing mnemonic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Simplify the lock state tracking logic in CreateWalletActivity::finish() since mixing-only unlock state is impossible for newly created wallets. Changes: - Use getEncryptionStatus() instead of tracking multiple state variables - Replace was_locked and was_unlocked_for_mixing with single was_locked check - Remove restoreWalletLockState() helper method - inline simple lock call - Add comment explaining why mixing state is impossible at creation time Before: Tracked both was_locked and was_unlocked_for_mixing states After: Only track was_locked since new wallets can only be: * Unencrypted (unlocked) * Encrypted and locked (need to unlock to show mnemonic) This is clearer and removes unnecessary complexity for an impossible state. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/qt/walletcontroller.cpp (1)
312-340: Don't silently drop the BIP39 passphrase.Here we fetch
mnemonic_passphrase, immediately zero it, and never surface it to the user. If the wallet was created with a non-empty BIP39 passphrase (possible today via RPC and very likely once advanced UI enables it), the backup produced by this flow is incomplete—the user leaves without the passphrase required to ever restore the wallet. Please either pass the passphrase into the dialog (with explicit acknowledgement) or at least show a blocking reminder before wiping it so the user confirms they have already backed it up.Example reminder before zeroing:
if (!has_mnemonic || mnemonic.empty()) { ... return; } + if (!mnemonic_passphrase.empty()) { + QMessageBox::information(m_parent_widget, tr("Passphrase Reminder"), + tr("This wallet was created with a BIP39 passphrase. Make sure you have backed it up separately; it is required together with the mnemonic to restore your wallet.")); + } + // Wallet has mnemonic - show verification dialog MnemonicVerificationDialog verify_dialog(mnemonic, m_parent_widget);…and only clear
mnemonic_passphraseafter that message (or after the dialog consumes it). Otherwise we create unrecoverable wallets for anyone using a passphrase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/qt/bitcoingui.cpp(5 hunks)src/qt/bitcoingui.h(1 hunks)src/qt/mnemonicverificationdialog.cpp(1 hunks)src/qt/mnemonicverificationdialog.h(1 hunks)src/qt/res/css/general.css(1 hunks)src/qt/walletcontroller.cpp(2 hunks)src/qt/walletframe.cpp(1 hunks)src/qt/walletframe.h(1 hunks)src/qt/walletview.cpp(3 hunks)src/qt/walletview.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/walletframe.cppsrc/qt/walletframe.hsrc/qt/bitcoingui.cppsrc/qt/walletview.cppsrc/qt/walletview.hsrc/qt/mnemonicverificationdialog.cppsrc/qt/bitcoingui.hsrc/qt/mnemonicverificationdialog.hsrc/qt/walletcontroller.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/qt/bitcoingui.cppsrc/qt/walletview.cppsrc/qt/walletcontroller.cpp
🔇 Additional comments (7)
src/qt/res/css/general.css (1)
1988-2038: LGTM! CSS styling looks appropriate.The styling for the new dialogs follows existing patterns and uses appropriate design choices (e.g., monospace font for mnemonic display, reasonable max-width constraints).
src/qt/bitcoingui.h (1)
160-160: LGTM! Consistent with existing action declarations.The new
showMnemonicActionmember follows the established pattern and is properly positioned alongside related wallet actions.src/qt/walletview.h (1)
110-111: LGTM! Slot declaration follows established patterns.The new
showMnemonic()slot is properly declared and positioned consistently with other wallet action slots.src/qt/walletframe.h (1)
103-104: LGTM! Slot declaration follows established patterns.The new
showMnemonic()slot is properly declared and positioned consistently with other wallet action slots.src/qt/walletframe.cpp (1)
313-318: LGTM! Implementation follows established delegation pattern.The method correctly retrieves the current wallet view and delegates to it, consistent with other wallet action methods like
changePassphrase()andbackupWallet().src/qt/bitcoingui.cpp (1)
397-398: LGTM! Action properly integrated into GUI.The
showMnemonicActionis correctly:
- Created with appropriate text and tooltip (lines 397-398)
- Connected to the wallet frame slot (line 507)
- Added to the Settings menu in a logical position (line 626)
- Enabled/disabled with other wallet actions (line 1047)
- Explicitly disabled in
NoKeysstate (line 1944), which is appropriate for watch-only walletsThe integration follows existing patterns and properly handles different wallet states.
Also applies to: 507-507, 626-626, 1047-1047, 1944-1944
src/qt/walletview.cpp (1)
330-376: LGTM with a minor observation about unused mnemonic_passphrase.The implementation correctly:
- Validates wallet capabilities (private keys, HD mode)
- Uses RAII pattern with
UnlockContextto manage wallet lock state- Retrieves mnemonic securely
- Clears sensitive data from local memory after passing to dialog
The memory clearing at lines 368-371 occurs after the dialog constructor copies the mnemonic (line 364) but before
exec()(line 373), which is correct.Minor observation: The
mnemonic_passphraseis retrieved and cleared (lines 354, 369, 371) but appears unused. If the passphrase is not displayed or used by the dialog, consider removing this retrieval to simplify the code. Otherwise, consider passing it to the dialog if it's relevant information for the user.
Replace QStringList with std::vector<SecureString> for m_words member to ensure mnemonic seed words are stored in secure memory that: - Locks memory to prevent paging to disk - Automatically zeros memory on deallocation - Uses secure allocators throughout the lifetime of sensitive data This addresses a security concern where mnemonic words were stored in non-secure memory (QStringList) which does not guarantee secure memory allocation or zeroing on deallocation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/qt/mnemonicverificationdialog.cpp (1)
48-70: Consider refactoring manual layout adjustments to stylesheets.The extensive manual layout adjustments (margins, spacing, size policies) make the code fragile and harder to maintain. Where feasible, consider moving these styling decisions to the CSS stylesheets or handling them in the UI designer. This would centralize layout rules and make them easier to adjust consistently across themes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/qt/mnemonicverificationdialog.cpp(1 hunks)src/qt/mnemonicverificationdialog.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/mnemonicverificationdialog.cppsrc/qt/mnemonicverificationdialog.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
🧬 Code graph analysis (1)
src/qt/mnemonicverificationdialog.h (1)
src/qt/mnemonicverificationdialog.cpp (36)
MnemonicVerificationDialog(26-100)MnemonicVerificationDialog(102-107)accept(354-366)accept(354-354)onShowMnemonicClicked(272-282)onShowMnemonicClicked(272-272)onHideMnemonicClicked(284-292)onHideMnemonicClicked(284-284)onWord1Changed(303-303)onWord1Changed(303-303)onWord2Changed(304-304)onWord2Changed(304-304)onWord3Changed(305-305)onWord3Changed(305-305)onShowMnemonicAgainClicked(294-301)onShowMnemonicAgainClicked(294-294)setupStep1(109-168)setupStep1(109-109)setupStep2(170-253)setupStep2(170-170)generateRandomPositions(255-270)generateRandomPositions(255-255)updateWordValidation(326-352)updateWordValidation(326-326)validateWord(307-324)validateWord(307-307)clearMnemonic(368-372)clearMnemonic(368-368)buildMnemonicGrid(434-500)buildMnemonicGrid(434-434)parseWords(374-404)parseWords(374-374)clearWordsSecurely(406-415)clearWordsSecurely(406-406)getWordCount(417-432)getWordCount(417-417)
🔇 Additional comments (9)
src/qt/mnemonicverificationdialog.h (1)
50-51: Security improvement verified: mnemonic data now uses secure storage.The previous security concern about
m_wordsusingQStringListhas been properly addressed. Bothm_mnemonic(line 50) andm_words(line 51) now useSecureStringandstd::vector<SecureString>respectively, ensuring sensitive mnemonic data is stored in secure memory that will be properly zeroed on deallocation.src/qt/mnemonicverificationdialog.cpp (8)
24-24: Build fix verified:<algorithm>header included.The missing
<algorithm>header that was causing build failures withstd::sortandstd::maxhas been properly added.
102-107: LGTM: Proper cleanup of sensitive data in destructor.The destructor correctly clears sensitive mnemonic data by calling
clearWordsSecurely()andclearMnemonic()before deleting the UI. This ensures the secure memory is zeroed before the object is destroyed.
255-270: Infinite loop fix verified: proper guard against short mnemonics.The early return when
n < 3(lines 259-262) properly prevents the infinite loop that could occur with invalid/short mnemonics. The safety check insetupStep2()(lines 178-183, 188-193) ensures the dialog shows an appropriate error message when positions cannot be generated.
374-404: Good security hygiene within Qt's constraints.The
parseWords()method makes a reasonable effort to minimize exposure time of sensitive mnemonic data. While there are temporary copies in non-secure memory (QString,QStringList,std::string), this is unavoidable when working with Qt's QString-based APIs. The code:
- Caches parsed words to avoid repeated parsing (line 377-379)
- Immediately clears temporary
std::stringcopies after conversion (line 394)- Attempts to zero and clear temporary
QStringobjects (lines 398-401)- Uses
SecureStringfor persistent storageNote that Qt's memory management (copy-on-write, internal buffering) means the
fill(0),clear(),squeeze()operations don't guarantee the sensitive data is completely removed from memory, but this is a known limitation when using Qt widgets to display secret data. The implementation does the best it can within these constraints.
406-415: LGTM: Secure clearing of word vector.The
clearWordsSecurely()method properly overwrites eachSecureStringwith zeros before clearing the vector, ensuring sensitive data is zeroed in secure memory.
307-366: LGTM: Sound validation logic with good UX.The word validation flow is well-implemented:
validateWord()performs bounds checking before comparing user input against stored wordsupdateWordValidation()provides real-time feedback with checkmark/X indicatorsaccept()performs final validation to prevent bypassing verification in non-view-only mode- View-only mode correctly skips verification (line 357)
The temporary QString exposure during comparison (line 317) is minimal and immediately cleared afterward.
170-253: LGTM: Robust step 2 setup with good defensive checks.The
setupStep2()method includes appropriate safety checks:
- Validates mnemonic has at least 3 words before proceeding (lines 178-183)
- Verifies random positions were successfully generated (lines 188-193)
- Rejects the dialog with clear error messages if validation fails
These defensive checks ensure the verification flow fails gracefully rather than presenting a broken UI to the user.
417-432: LGTM: Efficient word counting avoids unnecessary parsing.The
getWordCount()method efficiently returns the cached count when words are already parsed, and only creates temporary QString objects when necessary. The immediate clearing of temporary objects (lines 426-428) minimizes exposure time.
9531028 security: Store mnemonic words in secure memory (pasta) 5024589 doc: Add release notes for PR dashpay#6946 (pasta) a0b15cf refactor: Simplify wallet lock state handling in CreateWalletActivity (UdjinM6) 34914f1 feat: Add menu option to show recovery phrase for existing wallets (UdjinM6) 3b16a88 fix: Restore dialog size when going back from verification to word list (UdjinM6) f79a750 refactor: Move wallet creation dialog styles from C++ to CSS (UdjinM6) fd3c1eb Simplify mnemonic retrieval (pasta) 9894f47 Fix trailing whitespace in wallet creation files (pasta) e08e29f qt: Fix potential infinite loop and missing include in mnemonic verification (pasta) 77c3aeb qt: Improve wallet creation UI flow (pasta) f3219ee qt: avoid modifying QString during regex iteration in GUIUtil::loadStyleSheet (pasta) Pull request description: ## Issue being fixed or feature implemented Improve QT wallet creation process to display the seed to users. Release notes tbd ## What was done? <img width="438" height="236" alt="Screenshot 2025-11-05 at 21 07 38" src="https://github.com/user-attachments/assets/c7fda5bf-cb5f-4ccb-a3a3-7c74c1f01ce8" /> <img width="425" height="332" alt="Screenshot 2025-11-05 at 21 07 50" src="https://github.com/user-attachments/assets/3861b0bc-6f53-41e9-b800-12e7e81b1524" /> <img width="573" height="403" alt="Screenshot 2025-11-05 at 21 08 09" src="https://github.com/user-attachments/assets/bffd2400-5cf0-4d34-9ec4-f8f2a875cbfa" /> <img width="560" height="396" alt="Screenshot 2025-11-05 at 21 08 18" src="https://github.com/user-attachments/assets/36a54cc2-9422-4f79-82c4-cbadd1127910" /> <img width="480" height="383" alt="Screenshot 2025-11-05 at 21 08 42" src="https://github.com/user-attachments/assets/02e31e2c-f7bf-49ca-a13e-2a04a824c7ad" /> <img width="474" height="379" alt="Screenshot 2025-11-05 at 21 09 10" src="https://github.com/user-attachments/assets/35cc1169-b616-4678-ab64-f05a871032b7" /> <img width="479" height="378" alt="Screenshot 2025-11-05 at 21 09 26" src="https://github.com/user-attachments/assets/b124b15f-f0f3-4dc7-a327-3861dd6ac5ab" /> ## Breaking Changes None ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: ACK 9531028 Tree-SHA512: eb618eb68bc055a0fd0e434f7039efe75c420ffd01422211acde21e622d5ce0243a7a9d96e9bd654c7015f18b1beff044526cbe804f01dee9c961a7a0ebe1204
6fd7059 chore: mark v23 as release (pasta) ae08f53 docs: integrate 6946 release notes into final (pasta) 74a222d Merge #6946: feat: show seed on wallet creation (pasta) 877343a Merge #6943: fix: don't treat arrays/objects as string literals for composite methods (pasta) 00368fb Merge #6940: fix: reuse best clsig to avoid potential race condition (pasta) 8eceb98 Merge #6938: fix: logic error in `CheckDecryptionKey` (pasta) 3f30664 Merge #6929: fix: repair `makeseeds.py`, `getblockchaininfo[softforks]` help text, drop extra `generate`s from test, resolve macOS GID issue (pasta) 7ba4f1c Merge #6928: docs: update man pages for 23.0 (pasta) a6c1d6a Merge #6920: chore: update minimum chain work, tx stats, checkpoints, seeds and SECURITY.md for v23 (pasta) 84df1f0 Merge #6909: chore: Translations 2025-10 (pasta) a6449b1 Merge #6885: fix: improve governance/proposal dialog strings (pasta) Pull request description: ## Issue being fixed or feature implemented See commits ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 6fd7059 Tree-SHA512: a0d93a61a4f4978fbe120bea832ce683a8ae7c16c892a381d91ddc4b25344c0f3ad3306a1a30a166a7dfa6e38e4532708587cc23cc372126828b8e22d141dc85
3 participants
Issue being fixed or feature implemented
Improve QT wallet creation process to display the seed to users. Release notes tbd
What was done?
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.