Make bech32m the default for RPC, opt-in for GUI

Sjors commented at 3:09 pm on June 16, 2021: member

RPC This makes bech32m the default whenever possible. For legacy wallets the default remains bech32. Same for descriptor wallets without an active Taproot descriptor.

GUI A new wallet setting allows users to opt-in to Taproot. If they opt in, the GUI bech32 checkbox now produces either bech32 or bech32m depending on the wallet. This avoids having to explain the difference.

Considerations This lets users and developers experiment with taproot wallets. When merged before #22364, the user will have to manually import a taproot descriptor. After that PR, taproot descriptors will be present for all new wallets.

It is better to stick to bech32 addresses for the time being, because:

It is expected that it will take a while for enough wallets and exchanges to correctly handle sending to bech32m addresses
Taproot savings for a single signature wallet are not huge
Explaining the difference between bech32 and bech32m is not great UX

But we don’t want to prevent users and developers who do understand the difference to use Taproot. Without this PR they can only do this via the RPC (getnewaddress some_label bech32m). With this PR it’s a simple checkbox.

Changing the RPC default to bech32m seems acceptable to me, since those users will have a better understanding of the wallet. Though I can see the case against it because it might be a breaking change, e.g. if someone has an automated setup that creates new wallets (existing wallets currently don’t auto upgrade to taproot, so their behavior is not changed).

Suggested testing

call getnewaddress without type and getnewaddress Test bech32 / bech32m
create a receive address in the GUI with and without the bech32 checkbox

Legacy wallet
Default descriptor wallet (current does not have taproot descriptor)
Wallet with taproot descriptor:
- build #22364
- create new descriptor wallet
- build this PR again

DrahtBot added the label Descriptors on Jun 16, 2021

DrahtBot added the label GUI on Jun 16, 2021

DrahtBot added the label RPC/REST/ZMQ on Jun 16, 2021

DrahtBot added the label Wallet on Jun 16, 2021

Sjors force-pushed on Jun 16, 2021

DrahtBot commented at 11:44 pm on June 16, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23308 (Update basic multisig test/docs to use multipath descriptor by mjdietzx)
#22928 (refactor: Remove gArgs from wallet.h and wallet.cpp (2) by kiminuo)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#22805 (refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack)
#22787 (refactor: actual immutable pointing by kallewoof)

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.

Sjors force-pushed on Jun 17, 2021

DrahtBot added the label Needs rebase on Jun 24, 2021

Sjors force-pushed on Jun 24, 2021

DrahtBot removed the label Needs rebase on Jun 24, 2021

DrahtBot added the label Needs rebase on Jul 27, 2021

michaelfolkson commented at 10:16 am on August 2, 2021: contributor

My understanding from discussion at they July 30th Core wallet meeting is that this will only be merged once the (super)majority of the ecosystem is able to make payments to bech32m addresses. So definitely post Taproot activation but uncertain at this stage when exactly.

Assuming I’ve understood above, Concept ACK. Thanks for the instructions for testing in the PR description, will do this testing at a later date.

Sjors commented at 11:37 am on August 2, 2021: member

@michaelfolkson note that this doesn’t do anything unless there’s actually a taproot descriptor in the wallet, which this PR doesn’t add.

Sjors force-pushed on Aug 4, 2021

DrahtBot removed the label Needs rebase on Aug 4, 2021

unknown approved

unknown commented at 5:12 am on August 11, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/22260/commits/2c2a8c0e29f3ccf9a4907c5870ccb41aa4fd1f42

https://bitcoin.stackexchange.com/a/108013/

ghost commented at 7:58 am on August 11, 2021: none

Not sure why I don’t see QR in bitcoin-qt

in src/qt/receivecoinsdialog.cpp:154 in 2c2a8c0e29 outdated

153+            address_type = OutputType::BECH32;
154+        }
155     } else {
156-        address_type = model->wallet().getDefaultAddressType();
157-        if (address_type == OutputType::BECH32) {
158+        if (address_type == OutputType::BECH32 ||address_type == OutputType::BECH32M) {

benthecarman commented at 2:17 am on August 31, 2021:

spacing/formatting is weird here

Sjors force-pushed on Aug 31, 2021

Sjors commented at 10:31 am on August 31, 2021: member

Rebased. @prayank23 do you have libqrencode on your system? See https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#dependencies

ghost commented at 9:53 pm on September 1, 2021: none

libqrencode was missing. I installed it with sudo apt-get install qrencode, still QR doesn’t work. Its not related to this PR and maybe something wrong with this VM. I will try to fix this.

ACK https://github.com/bitcoin/bitcoin/commit/1ba76c2ce3446d0fd95f89bb91089c6664b29afc

benthecarman commented at 10:10 pm on September 1, 2021: contributor

libqrencode was missing. I installed it with sudo apt-get install qrencode, still QR doesn’t work. Its not related to this PR and maybe something wrong with this VM. I will try to fix this.

you probably need to reconfigure and rebuild

fanquake deleted a comment on Sep 11, 2021

Sjors force-pushed on Oct 20, 2021

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

Rebased.

My understanding from discussion at they July 30th Core wallet meeting is that this will only be merged once the (super)majority of the ecosystem is able to make payments to bech32m addresses.

I added a note in the PR description. I tend to agree it’s preferable to wait a little bit until more wallets and services support sending to bech32m, otherwise we have to add another checkbox for bech32 fallback and explain all that.

schildbach commented at 1:06 pm on October 20, 2021: contributor

I wonder why this isn’t already merged for Testnet and Signet (not Mainnet), both of which have Taproot activated already. It would make Taproot testing much easier.

ghost commented at 5:09 pm on October 20, 2021: none

reACK https://github.com/bitcoin/bitcoin/pull/22260/commits/9ca663e7ee3dc6d5312e839c28b125bf67051146

sipa commented at 5:21 pm on October 20, 2021: member

@schildbach I don’t think we can merge this until there is wide support for sending to bech32m in commonly deployed clients/services, which will probably take a while after activation even.

Sjors commented at 5:40 pm on October 20, 2021: member

I’m open to the idea of enabling this behavior on signet (and testnet) and turning it off on mainnet until a later release.

Sjors renamed this:
~~Make bech32m the default, except where needed. Update GUI checkbox.~~
Make bech32m the default for RPC, opt-in for GUI
on Oct 20, 2021

Sjors commented at 6:38 pm on October 20, 2021: member

I added a GUI settings checkbox to enable “taproot” (maybe needs rewording). See updated PR description.

DrahtBot added the label Needs rebase on Oct 22, 2021

wallet: make bech32m default address type

Except for legacy wallets and descriptor wallets without taproot. Update the default after importing a taproot descriptor.

7bfa223753

gui: interpret bech32 checkbox as bech32m where needed d6b3f6e982

Sjors force-pushed on Oct 22, 2021

DrahtBot removed the label Needs rebase on Oct 22, 2021

Rspigler commented at 9:54 pm on October 22, 2021: contributor

Use Taproot -> “Generate taproot addresses for wallets that support it…” Instead, could the new GUI option only be shown to users who have updated to taproot capable wallets? Then once clicking ‘Use Taproot’, the GUI bech32 checkbox should auto-select and produce bech32m addresses.

Sjors commented at 10:39 am on October 23, 2021: member

Do you mean moving it from the wallet Settings view to the Receive view? (and then only showing it for taproot capable wallets)

Rspigler commented at 2:51 pm on October 23, 2021: contributor

I didn’t, but I believe that’s a better idea

DrahtBot added the label Needs rebase on Oct 29, 2021

DrahtBot commented at 10:01 am on October 29, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

Sjors commented at 12:11 pm on October 29, 2021: member

Closing this in favor of a GUI-only PR: https://github.com/bitcoin-core/gui/pull/459

Sjors closed this on Oct 29, 2021

MarcoFalke referenced this in commit 63b5dfac21 on Dec 22, 2021

sidhujag referenced this in commit b960097cf4 on Dec 22, 2021

RandyMcMillan referenced this in commit aaa89c92e4 on Jan 27, 2022

DrahtBot locked this on Oct 30, 2022

Make bech32m the default for RPC, opt-in for GUI #22260

Conflicts