Make descriptor wallets by default by achow101 · Pull Request #23002 · bitcoin/bitcoin

achow101 · 2021-09-16T23:11:58Z

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

fanquake · 2021-09-17T01:34:21Z

Concept ACK

src/wallet/rpcwallet.cpp

laanwj · 2021-09-17T08:14:29Z

Concept ACK

prusnak · 2021-09-17T15:17:28Z

Concept ACK

sipa · 2021-09-17T15:18:33Z

Concept ACK

src/wallet/wallettool.cpp

DrahtBot · 2021-09-18T02:40:44Z

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.

instagibbs · 2021-09-21T05:17:45Z

src/qt/forms/createwalletdialog.ui

        <property name="text">
         <string>Descriptor Wallet</string>
        </property>
+        <property name="checked">


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

I think that may be a little confusing?

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.

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

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

lsilva01

Tested ACK 9c1052a on Ubuntu 20.04

ghost · 2021-09-21T18:26:52Z

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

Descriptor wallet is created by default

$ bitcoin-cli createwallet "D1"
{
  "name": "D1",
  "warning": ""
}

$ bitcoin-cli -rpcwallet=D1 getwalletinfo
{
  "walletname": "D1",
  "walletversion": 169900,
  "format": "sqlite",
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 0,
  "keypoolsize": 3000,
  "keypoolsize_hd_internal": 3000,
  "paytxfee": 0.00000000,
  "private_keys_enabled": true,
  "avoid_reuse": false,
  "scanning": false,
  "descriptors": true
}

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. ⚠️

achow101 · 2021-09-21T18:35:05Z

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 · 2021-09-21T18:42:16Z

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:

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

ghost

tACK 9c1052a

meshcollider · 2021-09-22T14:02:31Z

Concept ACK

meshcollider

Code review ACK 9c1052a

theStack · 2021-10-11T10:43:17Z

Concept ACK

Sjors · 2021-10-20T12:10:12Z

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.

maflcko · 2021-10-22T11:23:06Z

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?

maflcko · 2021-10-22T11:31:53Z

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

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

ryanofsky · 2021-10-25T18:57:08Z

src/wallet/wallettool.cpp

+        // 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));


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

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

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

Which changed in bitcoin#23002.

8e9699c Update doc to match new default wallet type (Bitcoin Hodler) Pull request description: #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

Which changed in bitcoin#23002.

because descriptors wallets are created by default since bitcoin#23002

…y default 5347c97 doc: update multisig-tutorial.md to default wallet type (Jon Atack) Pull request description: Follow-up to #24281 and #24281 (comment). The default wallet type was changed to descriptor wallets in #23002. ACKs for top commit: laanwj: ACK 5347c97 michaelfolkson: ACK 5347c97 achow101: ACK 5347c97 theStack: Concept and code-review ACK 5347c97 Tree-SHA512: 8074a33ad253ecb7d3f78645a00c808c7c224996cc1748067928aa59ef31a58f24fcfc75169494b26a19c7fbbf23bbd78516ab4102bc52fa92f08f1f49b18b63

fanquake · 2022-09-15T15:17:26Z

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

rpc, wallet: Descriptor wallets are no longer experimental

f19ad40

fanquake added the Wallet label Sep 16, 2021

fanquake added this to the 23.0 milestone Sep 17, 2021

maflcko reviewed Sep 17, 2021

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the default-desc-wallets branch from 723e860 to 71e0af8 Compare September 17, 2021 16:28

jonatack reviewed Sep 17, 2021

View reviewed changes

src/wallet/wallettool.cpp Outdated Show resolved Hide resolved

wallet: Default new wallets to descriptor wallets

9c1052a

achow101 force-pushed the default-desc-wallets branch from 71e0af8 to 9c1052a Compare September 17, 2021 17:32

DrahtBot mentioned this pull request Sep 18, 2021

refactor: Clarify and disable unused ArgsManager flags #22766

Merged

instagibbs reviewed Sep 21, 2021

View reviewed changes

darosior reviewed Sep 21, 2021

View reviewed changes

lsilva01 approved these changes Sep 21, 2021

View reviewed changes

ghost approved these changes Sep 21, 2021

View reviewed changes

meshcollider reviewed Sep 30, 2021

View reviewed changes

achow101 mentioned this pull request Oct 6, 2021

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

maflcko added the Needs release note label Oct 22, 2021

maflcko merged commit 02feae5 into bitcoin:master Oct 22, 2021

ryanofsky reviewed Oct 25, 2021

View reviewed changes

laanwj mentioned this pull request Nov 5, 2021

doc: Document use of legacy wallet in USAGE.md JoinMarket-Org/joinmarket-clientserver#1063

Closed

bitcoinhodler added a commit to bitcoinhodler/bitcoin that referenced this pull request Feb 7, 2022

Update doc to match new default wallet type

2a8cb26

Which changed in bitcoin#23002.

bitcoinhodler mentioned this pull request Feb 7, 2022

docs: Update to match new default wallet type #24281

Merged

bitcoinhodler added a commit to bitcoinhodler/bitcoin that referenced this pull request Feb 9, 2022

Update doc to match new default wallet type

8e9699c

Which changed in bitcoin#23002.

ghost mentioned this pull request Feb 16, 2022

wallet: ensure wallet files are not reused across chains #18554

Merged

hmel pushed a commit to hmel/bitcoin that referenced this pull request Feb 20, 2022

Update doc to match new default wallet type

552a03f

Which changed in bitcoin#23002.

jamesob mentioned this pull request Mar 2, 2022

0.3.0 release jamesob/coldcore#53

Merged

jonatack mentioned this pull request Mar 10, 2022

doc: update multisig-tutorial.md to descriptor wallet by default #24519

Merged

theStack mentioned this pull request Mar 13, 2022

remove explicit descriptors=true parameter for createwallet RPCs in functional tests #24550

Closed

prusnak mentioned this pull request Mar 13, 2022

test: remove explicit descriptors=True for createwallet RPC calls #24551

Closed

prusnak added a commit to prusnak/bitcoin that referenced this pull request Mar 14, 2022

test: remove explicit descriptors=True for createwallet RPC calls

e334426

because descriptors wallets are created by default since bitcoin#23002

stickies-v mentioned this pull request Mar 28, 2022

Connection via Bitcoin RPC fails in Bitcoin v23 rc2 sparrowwallet/sparrow#488

Closed

chappjc mentioned this pull request Apr 29, 2022

client/asset/btc: enable descriptor wallets for bitcoin core v23 decred/dcrdex#1601

Closed

fanquake removed the Needs release note label Sep 15, 2022

delta1 added a commit to delta1/elements that referenced this pull request May 14, 2023

Merge 02feae5 into merged_master (Bitcoin PR bitcoin/bitcoin#23002)

dc83a5d

bitcoin locked and limited conversation to collaborators Sep 15, 2023

Conversation

achow101 commented Sep 16, 2021

Uh oh!

fanquake commented Sep 17, 2021

Uh oh!

Uh oh!

laanwj commented Sep 17, 2021

Uh oh!

prusnak commented Sep 17, 2021

Uh oh!

sipa commented Sep 17, 2021

Uh oh!

Uh oh!

DrahtBot commented Sep 18, 2021

Conflicts

Uh oh!

instagibbs Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

achow101 Sep 21, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Sep 23, 2021

Choose a reason for hiding this comment

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

lsilva01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 21, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Sep 21, 2021

Uh oh!

lsilva01 commented Sep 21, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Sep 22, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Oct 11, 2021

Uh oh!

Sjors commented Oct 20, 2021

Uh oh!

maflcko commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Oct 22, 2021

Uh oh!

ryanofsky Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

achow101 Oct 25, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

lsilva01 left a comment •

edited

Loading

ghost commented Sep 21, 2021 •

edited by ghost

Loading

maflcko commented Oct 22, 2021 •

edited

Loading