feat: change descriptor type wallets to non-experimental #7038
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughBoth files remove "experimental" references from descriptor wallet features. The UI dialog shortens the descriptor checkbox tooltip and removes the "(EXPERIMENTAL)" label suffix. The RPC help text similarly removes experimental language from the descriptor wallet description and warning message. Additionally, two new help examples are added for the listaddressbalances command. No functional behavior or control flow is modified. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes These are straightforward text and documentation updates with no logic changes. The modifications follow a consistent pattern of removing "experimental" terminology and adding help examples. Both files contain only UI text and documentation string updates. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/bitcoin-wallet.cpp(1 hunks)src/qt/forms/createwalletdialog.ui(1 hunks)src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/wallettool.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/wallet/wallettool.cppsrc/bitcoin-wallet.cppsrc/wallet/rpc/wallet.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/wallettool.cppsrc/wallet/rpc/wallet.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
📚 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/wallettool.cppsrc/bitcoin-wallet.cppsrc/wallet/rpc/wallet.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/wallet/wallettool.cppsrc/wallet/rpc/wallet.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/wallettool.cppsrc/wallet/rpc/wallet.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/wallet/wallettool.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/rpc/wallet.cpp
🧬 Code graph analysis (1)
src/bitcoin-wallet.cpp (1)
src/dummywallet.cpp (1)
argsman(31-31)
🔇 Additional comments (5)
src/bitcoin-wallet.cpp (1)
41-41: LGTM!The
-legacyoption is properly registered alongside the existing-descriptorsoption, with appropriate help text indicating it's for the 'create' command.src/qt/forms/createwalletdialog.ui (1)
149-156: LGTM!The UI text updates appropriately remove the experimental designation from descriptor wallets, aligning with the PR objective. The tooltip and label are now cleaner and reflect the stable status of the feature.
src/wallet/wallettool.cpp (1)
138-141: LGTM!The validation correctly restricts the
-legacyoption to the 'create' command, maintaining consistency with the existing-descriptorsvalidation.src/wallet/rpc/wallet.cpp (2)
60-62: LGTM!The additional help examples improve the documentation by demonstrating usage with the
minamountparameter.
629-629: LGTM!The help text update removes the experimental designation from descriptor wallets, aligning with the PR objective to mark descriptor wallets as stable.
src/wallet/wallettool.cpp
Outdated
| // If -legacy is set, use it. Otherwise default to true | ||
| bool make_legacy = args.GetBoolArg("-legacy", true); | ||
| // If neither -legacy nor -descriptors is set, default to false. If -descriptors is set, use its value. | ||
| bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", false)); | ||
| 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_descriptors) { | ||
| options.create_flags |= WALLET_FLAG_DESCRIPTORS; | ||
| options.require_format = DatabaseFormat::SQLITE; | ||
| } |
There was a problem hiding this comment.
Critical logic error: default behavior triggers mutual exclusivity error.
When no flags are provided, the current logic sets both make_legacy and make_descriptors to true, triggering the mutual exclusivity error. This breaks the default wallet creation flow.
Analysis:
- Line 154:
make_legacy = args.GetBoolArg("-legacy", true)→truewhen flag not set - Line 156:
make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || ...→truewhen neither flag is set (first part of OR evaluates totrue) - Line 157-160: Both are
true→ error
This contradicts:
- Line 155-156 comment stating "default to false"
- PR objective that "legacy wallets remain the default"
Apply this diff to fix the logic:
- // If -legacy is set, use it. Otherwise default to true
- bool make_legacy = args.GetBoolArg("-legacy", true);
- // If neither -legacy nor -descriptors is set, default to false. If -descriptors is set, use its value.
- bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", false));
+ // Determine wallet type from flags
+ bool make_descriptors = args.GetBoolArg("-descriptors", false);
+ bool make_legacy = args.GetBoolArg("-legacy", !make_descriptors);
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;This ensures:
- No flags:
make_descriptors=false,make_legacy=true(legacy default) -descriptorsonly:make_descriptors=true,make_legacy=false-legacyonly:make_legacy=true,make_descriptors=false- Both set to
true: error
🤖 Prompt for AI Agents
In src/wallet/wallettool.cpp around lines 153-164 the flag-default logic
currently sets both make_legacy and make_descriptors true when no flags are
provided; change the logic so legacy is the default and descriptors default to
false: compute make_descriptors as false unless -descriptors is explicitly set
(e.g. if args.IsArgSet("-descriptors") then use args.GetBoolArg("-descriptors",
false) else false), and compute make_legacy as true unless -legacy is explicitly
set to false (e.g. if args.IsArgSet("-legacy") then use
args.GetBoolArg("-legacy", true) else true); then keep the mutual-exclusion
check (error if both true) and the descriptor-specific option setting if
make_descriptors is true.
There was a problem hiding this comment.
it doesn't because -legacy haven't existed before this PR.
There was a problem hiding this comment.
🧩 Analysis chain
bool make_legacy = args.GetBoolArg("-legacy", true);
bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", false));
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;
}
When user runs: `dash-wallet create` (no flags)
---
Step 1: Evaluate make_legacy
make_legacy = args.GetBoolArg("-legacy", true);
Since -legacy is NOT set, use default value → true
Result: make_legacy = TRUE
Step 2: Evaluate make_descriptors
Expression: (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", false))
Part A: (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy"))
args.IsArgSet("-descriptors") = false (not set)
args.IsArgSet("-legacy") = false (not set)
Part A = (!false && !false) = (true && true) = TRUE
Part B: (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", false))
args.IsArgSet("-descriptors") = false
Part B = (false && ...) = FALSE
Result: make_descriptors = (TRUE || FALSE) = TRUE
Step 3: Check mutual exclusivity
if (make_legacy && make_descriptors)
if (TRUE && TRUE) → CONDITION IS TRUE
→ Error: "Only one of -legacy or -descriptors can be set to true, not both"
→ return false
CONCLUSION: The default case (no flags) will FAIL with an error!
This contradicts the PR goal that "legacy wallets remain the default."
EOF
# Let's also check if there are any tests for this functionality
echo ""
echo "=== Searching for related tests ==="
rg -n "dash-wallet.*create" test/ --type py --type sh -A 2 -B 2 | head -30
# Look for bitcoin-wallet tests
echo ""
echo "=== Searching for wallet tool tests ==="
rg -n "ExecuteWalletToolFunc|wallettool" test/ --type py --type cpp -C 2 | head -30
🏁 Script executed:
#!/bin/bash
# Let's trace through the exact logic to demonstrate the issue
cat << 'EOF'
=== Logic Analysis for Default Case (No Flags) ===
Current code (lines 153-160):
Length of output: 173
@knst I understand that -legacy is NEW in this PR. My concern is about the NEW logic being introduced - let me demonstrate the issue step by step:
When a user runs dash-wallet create -wallet=test with NO flags:
Line 154: make_legacy = args.GetBoolArg("-legacy", true);
- Since
-legacyis NOT set → uses default valuetrue - Result:
make_legacy = true
Line 156: make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", false));
Breaking this down:
args.IsArgSet("-descriptors")=false(not set)args.IsArgSet("-legacy")=false(not set)- First part:
(!false && !false)=true - Second part:
(false && ...)=false - Result:
make_descriptors = true || false = true
Lines 157-160: if (make_legacy && make_descriptors)
if (true && true)→ condition is TRUE- Triggers error: "Only one of -legacy or -descriptors can be set to true, not both"
This means the default case (no flags) will fail! This contradicts:
- Your comment on line 156 stating "default to false"
- The PR objective that "legacy wallets remain the default"
The fix should ensure that when neither flag is set, only make_legacy is true and make_descriptors is false. The suggested fix in my original comment accomplishes this.
🧬 Code Graph Analysis Results
[src/wallet/wallettool.cpp] (approx. lines 90-170)
- Snippet: Handling of -format, -dumpfile, -descriptors, and -legacy options within ExecuteWalletToolFunc
"""
bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
{
if (args.IsArgSet("-format") && command != "createfromdump") {
tfm::format(std::cerr, "The -format option can only be used with the "createfromdump" command.\n");
return false;
}
if (args.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") {
tfm::format(std::cerr, "The -dumpfile option can only be used with the "dump" and "createfromdump" commands.\n");
return false;
}
if (args.IsArgSet("-descriptors") && command != "create") {
tfm::format(std::cerr, "The -descriptors option can only be used with the 'create' command.\n");
return false;
}
if (args.IsArgSet("-legacy") && command != "create") {
tfm::format(std::cerr, "The -legacy option can only be used with the 'create' command.\n");
return false;
}
if (command == "create" && !args.IsArgSet("-wallet")) {
tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n");
return false;
}
const std::string name = args.GetArg("-wallet", "");
const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(name));
if (command == "create") {
DatabaseOptions options;
ReadDatabaseArgs(args, options);
options.require_create = true;
// If -legacy is set, use it. Otherwise default to true
bool make_legacy = args.GetBoolArg("-legacy", true);
// If neither -legacy nor -descriptors is set, default to false. If -descriptors is set, use its value.
bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", false));
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_descriptors) {
options.create_flags |= WALLET_FLAG_DESCRIPTORS;
options.require_format = DatabaseFormat::SQLITE;
}
const std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, args, options);
if (wallet_instance) {
WalletShowInfo(wallet_instance.get());
wallet_instance->Close();
}
} else if (command == "info") {
// ...
"""
🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
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
knst
force-pushed
the
feat-descriptor-stable
branch
from
299484d to
9a488c0
Compare
2 participants
Issue being fixed or feature implemented
Descriptor wallets supports all required features, and has been released >1 year ago.
Time to remove flag "non-experimental"
As a reference, there's bitcoin's timeline for deprecating legacy wallets: bitcoin#20160
What was done?
Removed mark "experimental" from descriptor wallet:
To Do for v23.1 also: bitcoin#19602
How Has This Been Tested?
Call RPC
help createwalletRun qt app and click "Create Wallet"
Breaking Changes
N/A, by default still legacy wallets are created; left-over from bitcoin#23002 will be merged with Dash Core v24
Checklist: