Make descriptor wallets by default

achow101 commented at 11:11 pm on September 16, 2021: member

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 #20160

rpc, wallet: Descriptor wallets are no longer experimental f19ad40463

fanquake added the label Wallet on Sep 16, 2021

fanquake commented at 1:34 am on September 17, 2021: member

Concept ACK

fanquake added this to the milestone 23.0 on Sep 17, 2021

in src/wallet/rpcwallet.cpp:2769 in 723e860ddc outdated

2765@@ -2766,12 +2766,11 @@ static RPCHelpMan createwallet()
2766     if (!request.params[4].isNull() && request.params[4].get_bool()) {
2767         flags |= WALLET_FLAG_AVOID_REUSE;
2768     }
2769-    if (!request.params[5].isNull() && request.params[5].get_bool()) {
2770+    if (request.params[5].isNull() || (!request.params[5].isNull() && request.params[5].get_bool())) {

MarcoFalke commented at 6:02 am on September 17, 2021:

nit:

0    if (request.params[5].isNull() || request.params[5].get_bool())) {

achow101 commented at 4:28 pm on September 17, 2021:

Done

laanwj commented at 8:14 am on September 17, 2021: member

Concept ACK

prusnak commented at 3:17 pm on September 17, 2021: contributor

Concept ACK

sipa commented at 3:18 pm on September 17, 2021: member

Concept ACK

achow101 force-pushed on Sep 17, 2021

wallet: Default new wallets to descriptor wallets 9c1052a521

in src/wallet/wallettool.cpp:142 in 71e0af8e6d outdated

138+        if (make_legacy && make_descriptors) {
139+            tfm::format(std::cerr, "Only one of -legacy or -descriptors can be set to true, not both\n");
140+            return false;
141+        }
142+        if (!make_legacy && !make_descriptors) {
143+            tfm::format(std::cerr, "One of -legacy or -descriptors must be set to true (or ommitted)\n");

jonatack commented at 4:43 pm on September 17, 2021:

s/ommitted/omitted/

achow101 commented at 5:32 pm on September 17, 2021:

Done

achow101 force-pushed on Sep 17, 2021

DrahtBot commented at 2:40 am on September 18, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22766 (refactor: Clarify and disable unused ArgsManager flags by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/qt/forms/createwalletdialog.ui:110 in 9c1052a521

106@@ -107,6 +107,9 @@
107         <property name="text">
108          <string>Descriptor Wallet</string>
109         </property>
110+        <property name="checked">

instagibbs commented at 5:17 am on September 21, 2021:

maybe switch this around and have an unchecked “legacy wallet” option here instead?

achow101 commented at 4:47 pm on September 21, 2021:

I think that may be a little confusing?

laanwj commented at 11:51 am on September 23, 2021:

I think there’s something to be said for both options, but I think the current name is slightly better. It highlights what is new instead of what is old.

laanwj commented at 11:52 am on September 23, 2021:

I suppose you could have a tooltip text that explains in more detail, that there are “legacy wallets” and “descriptor wallets” and what is the difference from a user point of view.

darosior commented at 5:25 pm on September 21, 2021: member

Concept ACK. I’ve been using descriptor wallets in my application for a year without issue.

lsilva01 approved

lsilva01 commented at 6:25 pm on September 21, 2021: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/23002/commits/9c1052a5218e191fd23c0d9fc06f2fca34b03411 on Ubuntu 20.04

ghost commented at 6:26 pm on September 21, 2021: none

Concept ACK. Tested on Pop!_OS and had only one issue.

Descriptor wallet is created by default

 0$ bitcoin-cli createwallet "D1"
 1{
 2  "name": "D1",
 3  "warning": ""
 4}
 5
 6$ bitcoin-cli -rpcwallet=D1 getwalletinfo
 7{
 8  "walletname": "D1",
 9  "walletversion": 169900,
10  "format": "sqlite",
11  "balance": 0.00000000,
12  "unconfirmed_balance": 0.00000000,
13  "immature_balance": 0.00000000,
14  "txcount": 0,
15  "keypoolsize": 3000,
16  "keypoolsize_hd_internal": 3000,
17  "paytxfee": 0.00000000,
18  "private_keys_enabled": true,
19  "avoid_reuse": false,
20  "scanning": false,
21  "descriptors": true
22}

Descriptor wallet option is checked in GUI

Not sure where should I use -legacy? I tried using it with bitcoind, in bitcoin.conf and in bitcoin-cli -named createwallet. Nothing works. :warning:

achow101 commented at 6:35 pm on September 21, 2021: member

1. Not sure where should I use `-legacy`? I tried using it with `bitcoind`, in bitcoin.conf and in `bitcoin-cli -named createwallet`. Nothing works.

It is part of the wallet tool, bitcoin-wallet.

lsilva01 commented at 6:42 pm on September 21, 2021: contributor

Not sure where should I use -legacy? I tried using it with bitcoind, in bitcoin.conf and in bitcoin-cli -named createwallet. Nothing works.

You can test with the following command:

0src/bitcoin-wallet -legacy -wallet="test_legacy" create

unknown approved

unknown commented at 6:53 pm on September 21, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/23002/commits/9c1052a5218e191fd23c0d9fc06f2fca34b03411

meshcollider commented at 2:02 pm on September 22, 2021: contributor

Concept ACK

meshcollider commented at 2:12 am on September 30, 2021: contributor

Code review ACK 9c1052a5218e191fd23c0d9fc06f2fca34b03411

theStack commented at 10:43 am on October 11, 2021: contributor

Concept ACK

Sjors commented at 12:10 pm on October 20, 2021: member

Concept ACK, but I’d like to see #22364 land first, so we have tr() descriptors for all new users, and don’t have to worry about upgrade paths for a while.

MarcoFalke commented at 11:23 am on October 22, 2021: member

As long as #22364 lands before the next major release, it should be fine to merge this. master shouldn’t be used for anything other than testing, so we don’t have to worry about upgrade paths.

Edit: Also, (experimental) descriptor wallets already exist, so this discussion seems unrelated to this pull request either way?

MarcoFalke added the label Needs release note on Oct 22, 2021

MarcoFalke commented at 11:31 am on October 22, 2021: member

Changing the default might need release notes, but this can be done later.

MarcoFalke merged this on Oct 22, 2021

MarcoFalke closed this on Oct 22, 2021

sidhujag referenced this in commit 47c21fec08 on Oct 22, 2021

in src/wallet/wallettool.cpp:136 in 9c1052a521

129@@ -126,7 +130,19 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command)
130     if (command == "create") {
131         DatabaseOptions options;
132         options.require_create = true;
133-        if (args.GetBoolArg("-descriptors", false)) {
134+        // If -legacy is set, use it. Otherwise default to false.
135+        bool make_legacy = args.GetBoolArg("-legacy", false);
136+        // If neither -legacy nor -descriptors is set, default to true. If -descriptors is set, use its value.
137+        bool make_descriptors = (!args.IsArgSet("-descriptors") && !args.IsArgSet("-legacy")) || (args.IsArgSet("-descriptors") && args.GetBoolArg("-descriptors", true));

ryanofsky commented at 6:57 pm on October 25, 2021:

This IsArgSet logic is very strange. Why is this not just:

0bool make_descriptors = args.GetBoolArg("-descriptors", true);

achow101 commented at 9:18 pm on October 25, 2021:

To avoid setting it to true if -legacy is set.

bitcoinhodler referenced this in commit 2a8cb264a6 on Feb 7, 2022

bitcoinhodler referenced this in commit 8e9699cb10 on Feb 9, 2022

achow101 referenced this in commit 3a618c1e3b on Feb 17, 2022

hmel referenced this in commit 552a03f90e on Feb 20, 2022

prusnak referenced this in commit ade16d75d8 on Mar 13, 2022

prusnak referenced this in commit e334426816 on Mar 14, 2022

achow101 referenced this in commit 114754adf4 on Mar 16, 2022

fanquake removed the label Needs release note on Sep 15, 2022

fanquake commented at 3:17 pm on September 15, 2022: member

This was part of 23.0 and got a release note. Removing “Needs release note”.

delta1 referenced this in commit dc83a5d546 on May 14, 2023

bitcoin locked this on Sep 15, 2023

Make descriptor wallets by default #23002

Conflicts