feat: make Descriptor wallets to be created by default#7204
feat: make Descriptor wallets to be created by default#7204knst wants to merge 4 commits intodashpay:developfrom
Conversation
9c1052a wallet: Default new wallets to descriptor wallets (Andrew Chow) f19ad40 rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow) Pull request description: Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental. This follows the timeline proposed in bitcoin#20160 ACKs for top commit: lsilva01: Tested ACK bitcoin@9c1052a on Ubuntu 20.04 prayank23: tACK bitcoin@9c1052a meshcollider: Code review ACK 9c1052a Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a
…allet by default 5347c97 doc: update multisig-tutorial.md to default wallet type (Jon Atack) Pull request description: Follow-up to bitcoin#24281 and bitcoin#24281 (comment). The default wallet type was changed to descriptor wallets in bitcoin#23002. ACKs for top commit: laanwj: ACK 5347c97 michaelfolkson: ACK 5347c97 achow101: ACK 5347c97 theStack: Concept and code-review ACK 5347c97 Tree-SHA512: 8074a33ad253ecb7d3f78645a00c808c7c224996cc1748067928aa59ef31a58f24fcfc75169494b26a19c7fbbf23bbd78516ab4102bc52fa92f08f1f49b18b63
8e9699c Update doc to match new default wallet type (Bitcoin Hodler) Pull request description: bitcoin#23002 changed the default wallet type to descriptors, so this doc was out of date. ACKs for top commit: achow101: ACK 8e9699c Tree-SHA512: 2f69b23c153163bf2a091dbf728b713d28f795cc81e031bf201160882d2456494e94955ff6385634615fdcfece11542749ad1c982e2994e64ed69011380a2353
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request flips the default wallet type to descriptor wallets for new wallets, adds a -legacy CLI option to explicitly request legacy wallets (restricted to the create command), makes the GUI descriptor checkbox checked by default, changes the createwallet RPC default for the descriptors parameter to true, updates wallettool logic to derive legacy/descriptors behavior, and updates documentation and tests to use positional createwallet arguments and omit explicit descriptors flags. Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as dash-cli / GUI
participant WalletTool as dash-wallet / wallettool
participant RPC as createwallet RPC
participant DB as Wallet DB
rect rgba(200,200,255,0.5)
User->>CLI: createwallet "name" (no flags)
CLI->>RPC: RPC createwallet(descriptors=true)
RPC->>WalletTool: request create (descriptors=true)
WalletTool->>DB: create descriptor wallet (WALLET_FLAG_DESCRIPTORS, SQLITE)
DB-->>WalletTool: success
WalletTool-->>RPC: created
RPC-->>CLI: result
CLI-->>User: created
end
sequenceDiagram
participant User
participant CLI as dash-cli / GUI
participant WalletTool as dash-wallet / wallettool
participant RPC as createwallet RPC
participant DB as Wallet DB
rect rgba(255,200,200,0.5)
User->>CLI: createwallet "name" -legacy
CLI->>RPC: RPC createwallet(descriptors=false)
RPC->>WalletTool: request create (descriptors=false)
WalletTool->>DB: create legacy wallet (legacy format)
DB-->>WalletTool: success
WalletTool-->>RPC: created
RPC-->>CLI: result
CLI-->>User: created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
src/bitcoin-wallet.cpp (1)
42-42: Clarify that-legacyoverrides the new default.The help text does not say that omitting both flags now creates a descriptor wallet, so
dash-wallet -helpstill hides the behavior change.Suggested tweak
- argsman.AddArg("-legacy", "Create legacy wallet. Only for 'create'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + argsman.AddArg("-legacy", "Create legacy wallet instead of the default descriptor wallet. Only for 'create'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bitcoin-wallet.cpp` at line 42, Update the help text passed to argsman.AddArg for "-legacy" (the call argsman.AddArg("-legacy", "Create legacy wallet. Only for 'create'", ...)) to explicitly state that descriptor wallets are now the default when no wallet-type flags are provided and that "-legacy" forces the old legacy wallet format (overriding the new default) for create operations; keep note that it only applies to the "create" subcommand. Ensure the new string clearly mentions both the default behavior (descriptor wallet when omitted) and that "-legacy" overrides it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/wallettool.cpp`:
- Around line 152-162: The CLI logic must preserve explicit descriptor opt-outs
as the legacy path: change the decision so that make_legacy becomes true when
-legacy is true OR when the user explicitly set -descriptors to false (e.g.
-descriptors=0) or provided -nodescriptors, and make_descriptors becomes true
only when descriptors are selected (either by default when neither flag is set,
or when -descriptors=true). Concretely, compute descriptors_set =
args.IsArgSet("-descriptors"), descriptors_val = args.GetBoolArg("-descriptors",
true), legacy_set = args.IsArgSet("-legacy"), legacy_val =
args.GetBoolArg("-legacy", false), nodescriptors_set =
args.IsArgSet("-nodescriptors"); then set make_legacy = legacy_val ||
nodescriptors_set || (descriptors_set && !descriptors_val) and set
make_descriptors = (!descriptors_set && !legacy_set && !nodescriptors_set) ||
(descriptors_set && descriptors_val). Update the checks that use make_legacy and
make_descriptors (the error messages) accordingly.
---
Nitpick comments:
In `@src/bitcoin-wallet.cpp`:
- Line 42: Update the help text passed to argsman.AddArg for "-legacy" (the call
argsman.AddArg("-legacy", "Create legacy wallet. Only for 'create'", ...)) to
explicitly state that descriptor wallets are now the default when no wallet-type
flags are provided and that "-legacy" forces the old legacy wallet format
(overriding the new default) for create operations; keep note that it only
applies to the "create" subcommand. Ensure the new string clearly mentions both
the default behavior (descriptor wallet when omitted) and that "-legacy"
overrides it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f8dc761-3360-41eb-9329-41be3d545f66
📒 Files selected for processing (7)
doc/managing-wallets.mddoc/multisig-tutorial.mdsrc/bitcoin-wallet.cppsrc/qt/forms/createwalletdialog.uisrc/wallet/rpc/wallet.cppsrc/wallet/wallettool.cpptest/functional/tool_wallet.py
| // If -legacy is set, use it. Otherwise default to false. | ||
| bool make_legacy = args.GetBoolArg("-legacy", false); | ||
| // If neither -legacy nor -descriptors is set, default to true. If -descriptors is set, use its value. | ||
| bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", true)); | ||
| if (make_legacy && make_descriptors) { | ||
| tfm::format(std::cerr, "Only one of -legacy or -descriptors can be set to true, not both\n"); | ||
| return false; | ||
| } | ||
| if (!make_legacy && !make_descriptors) { | ||
| tfm::format(std::cerr, "One of -legacy or -descriptors must be set to true (or omitted)\n"); | ||
| return false; |
There was a problem hiding this comment.
Keep -descriptors=0 and -nodescriptors working as the legacy path.
This now turns an explicit descriptor opt-out into a hard error because both make_legacy and make_descriptors end up false. That is a larger CLI break than just changing the default, and it will break scripts that were already being explicit about creating a BDB wallet.
Compatibility-preserving approach
- // If -legacy is set, use it. Otherwise default to false.
- bool make_legacy = args.GetBoolArg("-legacy", false);
- // If neither -legacy nor -descriptors is set, default to true. If -descriptors is set, use its value.
- bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", true));
- if (make_legacy && make_descriptors) {
+ const bool legacy_requested = args.GetBoolArg("-legacy", false);
+ const bool descriptors_requested = args.GetBoolArg("-descriptors", true);
+
+ if (legacy_requested && args.IsArgSet("-descriptors") && descriptors_requested) {
tfm::format(std::cerr, "Only one of -legacy or -descriptors can be set to true, not both\n");
return false;
}
- if (!make_legacy && !make_descriptors) {
- tfm::format(std::cerr, "One of -legacy or -descriptors must be set to true (or omitted)\n");
- return false;
- }
+
+ // Preserve explicit descriptor opt-out for older automation.
+ const bool make_legacy = legacy_requested || (args.IsArgSet("-descriptors") && !descriptors_requested);
+ const bool make_descriptors = !make_legacy;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallettool.cpp` around lines 152 - 162, The CLI logic must
preserve explicit descriptor opt-outs as the legacy path: change the decision so
that make_legacy becomes true when -legacy is true OR when the user explicitly
set -descriptors to false (e.g. -descriptors=0) or provided -nodescriptors, and
make_descriptors becomes true only when descriptors are selected (either by
default when neither flag is set, or when -descriptors=true). Concretely,
compute descriptors_set = args.IsArgSet("-descriptors"), descriptors_val =
args.GetBoolArg("-descriptors", true), legacy_set = args.IsArgSet("-legacy"),
legacy_val = args.GetBoolArg("-legacy", false), nodescriptors_set =
args.IsArgSet("-nodescriptors"); then set make_legacy = legacy_val ||
nodescriptors_set || (descriptors_set && !descriptors_val) and set
make_descriptors = (!descriptors_set && !legacy_set && !nodescriptors_set) ||
(descriptors_set && descriptors_val). Update the checks that use make_legacy and
make_descriptors (the error messages) accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/release-notes-7204.md`:
- Line 8: Update the sentence "Changes to wallet related RPCs can be found in
the Wallet section below." to use the compound adjective form by replacing
"wallet related" with "wallet-related" so it reads "Changes to wallet-related
RPCs can be found in the Wallet section below."; ensure only that hyphenation is
changed and surrounding punctuation/spacing remains unchanged.
- Around line 13-15: Update the release note sentence that begins "Descriptor
wallets are now the default wallet type..." to explicitly state that new
descriptor wallets use the SQLite wallet backend instead of the legacy BDB
format, so operators know backups/file-format change; retain the existing
mention of `descriptors=false` (createwallet) and the "Descriptor wallet" GUI
checkbox as ways to avoid the new default, and add a short note that existing
BDB wallets remain unchanged unless migrated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c13e7d73-8326-4a3b-83a2-5e80066ea8ea
📒 Files selected for processing (1)
doc/release-notes-7204.md
| ------------ | ||
|
|
||
|
|
||
| Changes to wallet related RPCs can be found in the Wallet section below. |
There was a problem hiding this comment.
Hyphenate “wallet-related”.
Line 8 should use the compound adjective form for consistency and readability.
Suggested wording
-Changes to wallet related RPCs can be found in the Wallet section below.
+Changes to wallet-related RPCs can be found in the Wallet section below.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Changes to wallet related RPCs can be found in the Wallet section below. | |
| Changes to wallet-related RPCs can be found in the Wallet section below. |
🧰 Tools
🪛 LanguageTool
[grammar] ~8-~8: Use a hyphen to join words.
Context: ...ed RPCs ------------ Changes to wallet related RPCs can be found in the Wallet ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/release-notes-7204.md` at line 8, Update the sentence "Changes to wallet
related RPCs can be found in the Wallet section below." to use the compound
adjective form by replacing "wallet related" with "wallet-related" so it reads
"Changes to wallet-related RPCs can be found in the Wallet section below.";
ensure only that hyphenation is changed and surrounding punctuation/spacing
remains unchanged.
There was a problem hiding this comment.
it's copy-paste of bitcoin core's release note. I'd like to see human's feedback here
There was a problem hiding this comment.
@knst, confirmed — the phrase is a direct copy-paste from Bitcoin Core. Bitcoin Core's v23.0 release notes use the exact unhyphenated form "Changes to wallet related RPCs can be found in the Wallet section below." The same wording appears verbatim across v24.0.1, v26.0, and v25.0 release notes as well.
So while the LanguageTool grammar flag is technically valid, Bitcoin Core has consistently used this unhyphenated style across multiple releases. For a downstream project like Dash Core that tracks Bitcoin Core's release notes, keeping the text identical to upstream is a reasonable choice — it eases future diffs and attribution. I'll defer to your judgment on whether to apply the hyphen or preserve the upstream wording as-is.
✏️ Learnings added
Learnt from: knst
Repo: dashpay/dash PR: 7204
File: doc/release-notes-7204.md:8-8
Timestamp: 2026-03-06T08:37:29.753Z
Learning: In the Dash Core repository, release notes text copied verbatim from Bitcoin Core upstream (e.g. "Changes to wallet related RPCs can be found in the Wallet section below." in doc/release-notes-7204.md) should be kept identical to the upstream source to ease future diffs and attribution. Do not flag grammar/style issues in such copy-pasted upstream text.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
What was done?
Descriptor wallets are created by default since Dash Core v24
References:
How Has This Been Tested?
createwallet "aha"succeed, descriptor walletcreatewallet "fail" false false "" false falsefailed, because load_on_startup must be specifiedcreatewallet "fail" false false "" false truefailed, because load_on_startup must be specifiedcreatewallet "legacy" false false "" false false falsesucceed, created legacy walletcreatewallet "desc" false false "" false true falsesucceed, created descriptor walletBreaking Changes
Descriptor wallets are now the default wallet type. Newly created wallets will use descriptors unless
descriptors=falseis set duringcreatewallet, or theDescriptor walletcheckbox is unchecked in the GUI.Note that wallet RPC commands like
importmultianddumpprivkeycannot be used with descriptor wallets, so if your client code relies on these commands without specifyingdescriptors=falseduring wallet creation, you will need to update your code.Checklist: