Add OutputType::BECH32M and related wallet support for fetching bech32m addresses #22154

achow101 · 2021-06-04T21:47:42Z

Currently bech32m addresses are classfied as bech32. Because bech32m is incompatible with bech32, we need to define a new OutputType for it so that it can be handled correctly. This PR adds OutputType::BECH32M, updates all of the relevant OutputType classifications, and handle requests for bech32m addresses. There is now a bech32m address type string that can be used.

tr() descriptors now report their output type as OutputType::BECH32M. WtinessV1Taproot and WitnessUnknown are also classified as OutputType::BECH32M.
Bech32m addresses are completely disabled for legacy wallets. They cannot be imported (explicitly disallowed in importaddress and importmulti), will not be created when getting all destinations for a pubkey, and will not be added with addmultisigaddress. Additional protections have been added to LegacyScriptPubKeyMan to disallow attempting to retrieve bech32m addresses.
Since Taproot multisigs are not implemented yet, createmultisig will also disallow the bech32m address type.
As Taproot is not yet active, DescriptorScriptPubKeyMan cannot and will not create a tr() descriptor. Protections have been added to make sure this cannot occur.
The change address type detection algorithm has been updated to return bech32m when there is a segwit v1+ output script and the wallet has a bech32m ScriptPubKeyMan, falling back to bech32 if one is not available.

DrahtBot · 2021-06-05T05:28:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Make bech32m the default for RPC, opt-in for GUI #22260 (Make bech32m the default, except where needed. Update GUI checkbox. by Sjors)
wallet, rpc: add an option to list private descriptors #21500 (wallet, rpc: add an option to list private descriptors by S3RK)
wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag #20191 (wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag by S3RK)
wallet: Migrate legacy wallets to descriptor wallets #19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)

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.

src/rpc/misc.cpp

fjahr

Looks close, only some style nits and open questions/suggestions.

fjahr · 2021-06-10T23:02:19Z

src/wallet/scriptpubkeyman.h

nit: could use constexpr I think

Tried that but it doesn't work.

You can't constexpr things that need allocation. You could use an array instead of an unordered_set, and use std::find to look up (which will likely be faster too, due to the tiny size), but it really doesn't matter here.

test/functional/wallet_address_types.py

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.cpp

fjahr · 2021-06-13T20:11:48Z

utACK 05ff76c3aad6905d984ba1764c8045ca530fbf8f

Sjors

tACK 05ff76c

I made a followup PR that makes the defaults a bit more friendly, and adds GUI support: #22260

Sjors · 2021-06-16T13:21:39Z

src/wallet/scriptpubkeyman.cpp

a1373469af96bf77bad65415c4b456f182dbc3fe : is this just belt and suspenders? When I do getnewaddress Test bech32m on legacy wallet for this commit, it's caught earlier and returns No bech32m addresses available. That's because CWallet::GetNewDestination tries GetScriptPubKeyMan first for the requested type.

Yes, just belt and suspenders.

Sjors · 2021-06-16T13:32:03Z

src/wallet/wallet.cpp

As an extra safety measure, maybe double-check that Taproot is active like in #22156

Since this is generic for future segwit versions, I'm not sure doing that makes sense. For taproot specifically, there won't be a bech32m ScriptPubKeyMan until taproot activates anyways.

src/wallet/wallet.cpp

Sjors · 2021-06-17T09:19:37Z

re-utACK f70d55c136d9fc0bc986cc86e619570e0417d188

maflcko

left two comments.

src/wallet/wallet.cpp

maflcko · 2021-06-17T09:57:57Z

src/wallet/wallet.cpp

Not sure how difficult it would be to fix, but the general idea for translations is to pass up the bilingual string and let the caller pick the right version. Otherwise the RPC responses (and debug log) might be partially translated. Functional test might fail when run with the gui.

I think that will be a bit invasive since several of the functions involved use std::string error rather than bilingual_str. Perhaps for a followup.

Yes, if you're going to translate anything, it's better to use bilingual_str. I would prefer to do this right in the first go, instead of having another follow-up commit to prevent RPC messages from being translated.

Alternatively just leave this untranslated for now. This is marked for 22.0 and we've passed the translation string freeze.

For the strings I added, I was just following the pattern already in use in these functions. For this change in particular, it was to make the style unified. Unless this is considered a blocker, I'm going to leave it as is.

practicalswift · 2021-06-17T19:53:26Z

Concept ACK

Sjors · 2021-06-18T13:39:55Z

re-utACK f33361c17f3937ee8819fcea76548e19f6ac6558

fjahr · 2021-06-20T19:31:03Z

re-ACK f33361c

The only changes since my last review was a reorganization of the code changes in TransactionChangeType() and ensuring there that BECH32 address types are also available before using them. That commit was also slightly renamed.

laanwj · 2021-06-21T07:03:08Z

Code review ACK f33361c17f3937ee8819fcea76548e19f6ac6558
re-review ACK 754f134

I think the potential RPC error message translation issue mentioned in #22154 (comment) needs to be addressed at some point, but also think it's not a blocker for 22.0.

meshcollider · 2021-06-23T01:50:12Z

I get an error in wallet_taproot.py:

Remaining jobs: [wallet_taproot.py]
1/1 - wallet_taproot.py failed, Duration: 1 s

stdout:
2021-06-23T01:47:26.095000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20210623_134725/wallet_taproot_0
2021-06-23T01:47:26.359000Z TestFramework (INFO): Creating wallets...
2021-06-23T01:47:26.605000Z TestFramework (INFO): Mining blocks...
2021-06-23T01:47:26.694000Z TestFramework (INFO): Testing tr(XPRV) address derivation
2021-06-23T01:47:26.745000Z TestFramework (INFO): Testing tr(XPRV) through sendtoaddress
2021-06-23T01:47:26.770000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
  File "/home/meshcollider/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
    self.run_test()
  File "/home/meshcollider/bitcoin/test/functional/wallet_taproot.py", line 347, in run_test
    1
  File "/home/meshcollider/bitcoin/test/functional/wallet_taproot.py", line 314, in do_test
    self.do_test_sendtoaddress(comment, pattern, privmap, treefn, keys[0:nkeys], keys[nkeys:2*nkeys])
  File "/home/meshcollider/bitcoin/test/functional/wallet_taproot.py", line 262, in do_test_sendtoaddress
    addr_g = self.rpc_online.getnewaddress(address_type='bech32')
  File "/home/meshcollider/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/meshcollider/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Error: No bech32 addresses available. (-12)
2021-06-23T01:47:26.821000Z TestFramework (INFO): Stopping nodes
2021-06-23T01:47:27.028000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20210623_134725/wallet_taproot_0
2021-06-23T01:47:27.028000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20210623_134725/wallet_taproot_0/test_framework.log

Make sure that LegacyScriptPubKeyMan can only be used for legacy, p2sh-segwit, and bech32 address types.

Bech32m addresses need their own OutputType We are not ready to create DescriptorScriptPubKeyMans which produce bech32m addresses. So don't allow generating them.

If a transaction as a segwit output, use a bech32m change address if they are available. If not, fallback to bech32. If bech32 change addresses are unavailable, fallback to the default address type.

The tr() descriptor, WitnessV1Taproot CTxDestination, and WitnessUnknown CTxDestination are OutputType::BECH32M so they should report as such.

We don't want the legacy wallet to ever have bech32m addresses so don't allow importing them. This includes addmultisigaddress as that is a legacy wallet only RPC Additionally, bech32m multisigs are not available yet, so disallow them in createmultisig.

Adds an error output parameter to all GetReservedDestination functions so that callers can get the actual reason that a change address could not be fetched. This more closely matches GetNewDestination. This allows for more granular error messages, such as one that indicates that bech32m addresses cannot be generated yet.

achow101 · 2021-06-23T01:57:08Z

Silent merge conflict, rebased and fixed.

Sjors · 2021-06-23T12:36:36Z

re-utACK 754f134: only change is switching to bech32m in two wallet_taproot.py test cases.

jonatack

ACK 754f134

When should bech32m be added to the -addresstype and -changetype wallet config option helps?

jonatack · 2021-06-23T21:40:13Z

src/script/descriptor.cpp

    std::optional<OutputType> GetOutputType() const override { return std::nullopt; }
 };

-static std::optional<OutputType> OutputTypeFromDestination(const CTxDestination& dest) {


In commit 87a0e7a, src/script/descriptor.cpp should include outputtype.h where OutputTypeFromDestination() is moved.

--- a/src/script/descriptor.cpp +++ b/src/script/descriptor.cpp @@ -5,6 +5,7 @@ #include <script/descriptor.h> #include <key_io.h> +#include <outputtype.h> #include <pubkey.h> #include <script/script.h> #include <script/standard.h>

jonatack · 2021-06-23T22:11:30Z

test/functional/wallet_address_types.py

        self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32')

+        if self.options.descriptors:
+            self.log.info("Descriptor wallets do not have bech32m addresses by default yet")


s/addreses/addresses/ should be fixed one commit earlier in 87a0e7a

achow101 · 2021-06-23T22:59:16Z

When should bech32m be added to the -addresstype and -changetype wallet config option helps?

When we make bech32m descriptors by default.

fjahr · 2021-06-23T23:03:46Z

re-ACK 754f134

Confirmed only changes were rebasing and fixing wallet_taproot.py.

…rt for …

Sjors · 2021-06-24T17:52:59Z

I rebased my followup in #22260 (which probably works best if you don't set -addresstype)

achow101 force-pushed the outputtype-bech32m branch from 4549a29 to 3f68bb5 Compare June 4, 2021 22:39

DrahtBot added Descriptors RPC/REST/ZMQ Wallet labels Jun 4, 2021

This was referenced Jun 5, 2021

wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag #20191

Merged

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

sipa reviewed Jun 5, 2021

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the outputtype-bech32m branch from 3f68bb5 to 73ddcb8 Compare June 6, 2021 19:41

meshcollider added this to the 22.0 milestone Jun 9, 2021

achow101 mentioned this pull request Jun 12, 2021

Allow tr() import only when Taproot is active #22156

Merged

fjahr reviewed Jun 13, 2021

View reviewed changes

achow101 force-pushed the outputtype-bech32m branch 4 times, most recently from 02657d1 to 05ff76c Compare June 13, 2021 01:56

DrahtBot mentioned this pull request Jun 13, 2021

wallet, rpc: add an option to list private descriptors #21500

Merged

Sjors mentioned this pull request Jun 16, 2021

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

Closed

Sjors approved these changes Jun 16, 2021

View reviewed changes

Sjors reviewed Jun 16, 2021

View reviewed changes

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

achow101 force-pushed the outputtype-bech32m branch from 05ff76c to f70d55c Compare June 16, 2021 20:56

maflcko reviewed Jun 17, 2021

View reviewed changes

achow101 force-pushed the outputtype-bech32m branch 2 times, most recently from cae6672 to f33361c Compare June 17, 2021 18:39

achow101 added 3 commits June 22, 2021 21:53

Limit LegacyScriptPubKeyMan address types

177c15d

Make sure that LegacyScriptPubKeyMan can only be used for legacy, p2sh-segwit, and bech32 address types.

Add OutputType::BECH32M

0262536

Bech32m addresses need their own OutputType We are not ready to create DescriptorScriptPubKeyMans which produce bech32m addresses. So don't allow generating them.

Opportunistically use bech32m change addresses if available

699dfcd

If a transaction as a segwit output, use a bech32m change address if they are available. If not, fallback to bech32. If bech32 change addresses are unavailable, fallback to the default address type.

achow101 force-pushed the outputtype-bech32m branch from f33361c to fbf142e Compare June 23, 2021 01:56

achow101 added 3 commits June 22, 2021 21:57

Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests

6dbe4d1

The tr() descriptor, WitnessV1Taproot CTxDestination, and WitnessUnknown CTxDestination are OutputType::BECH32M so they should report as such.

achow101 force-pushed the outputtype-bech32m branch from fbf142e to 754f134 Compare June 23, 2021 01:57

jonatack reviewed Jun 23, 2021

View reviewed changes

laanwj merged commit 0553d75 into bitcoin:master Jun 24, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 24, 2021

Merge bitcoin#22154: Add OutputType::BECH32M and related wallet suppo…

66e52de

…rt for …

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 0553d75 into merged_master (Bitcoin PR bitcoin/bitcoin#22154)

b3bba59

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Add OutputType::BECH32M and related wallet support for fetching bech32m addresses #22154

Add OutputType::BECH32M and related wallet support for fetching bech32m addresses #22154

Uh oh!

Conversation

achow101 commented Jun 4, 2021

Uh oh!

DrahtBot commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjahr commented Jun 13, 2021

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jun 17, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jun 17, 2021

Uh oh!

Sjors commented Jun 18, 2021

Uh oh!

fjahr commented Jun 20, 2021

Uh oh!

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

Uh oh!

Uh oh!

meshcollider commented Jun 23, 2021

Uh oh!

achow101 commented Jun 23, 2021

Uh oh!

Sjors commented Jun 23, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jun 23, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 23, 2021

Uh oh!

DrahtBot commented Jun 5, 2021 •

edited

Loading

laanwj commented Jun 21, 2021 •

edited

Loading

jonatack Jun 23, 2021 •

edited

Loading