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

achow101 commented at 9:47 pm on June 4, 2021: member

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.

achow101 force-pushed on Jun 4, 2021

DrahtBot added the label Descriptors on Jun 4, 2021

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

DrahtBot added the label Wallet on Jun 4, 2021

DrahtBot commented at 5:28 am on June 5, 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:

#22260 (Make bech32m the default, except where needed. Update GUI checkbox. by Sjors)
#21500 (wallet, rpc: add an option to list private descriptors by S3RK)
#20191 (wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag by S3RK)
#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.

in src/rpc/misc.cpp:135 in 3f68bb5d64 outdated

130@@ -131,6 +131,9 @@ static RPCHelpMan createmultisig()
131         if (!ParseOutputType(request.params[2].get_str(), output_type)) {
132             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[2].get_str()));
133         }
134+        if (output_type == OutputType::BECH32M) {
135+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Bech32m multisig addresses cannot be created yet");

sipa commented at 5:24 pm on June 5, 2021:

Do we intend to ever create those through this RPC? I think you’d use deriveaddress or so instead.

achow101 commented at 7:41 pm on June 6, 2021:

Probably not. Changed the message.

achow101 force-pushed on Jun 6, 2021

meshcollider added this to the milestone 22.0 on Jun 9, 2021

in src/wallet/scriptpubkeyman.h:258 in fe7f3ee0e7 outdated

253@@ -254,6 +254,13 @@ class ScriptPubKeyMan
254     boost::signals2::signal<void ()> NotifyCanGetAddressesChanged;
255 };
256 
257+/** OutputTypes supported by the LegacyScriptPubKeyMan */
258+static const std::unordered_set<OutputType> LEGACY_OUTPUT_TYPES {

fjahr commented at 11:02 pm on June 10, 2021:

nit: could use constexpr I think

achow101 commented at 1:04 am on June 13, 2021:

Tried that but it doesn’t work.

sipa commented at 1:44 am on June 13, 2021:

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.

in test/functional/wallet_address_types.py:380 in 73ddcb81fc outdated

372@@ -373,5 +373,15 @@ def run_test(self):
373         self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit')
374         self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32')
375 
376+        if self.options.descriptors:
377+            self.log.info("Descriptor wallets do not have bech32m addreses by default yet")
378+            # TODO: Remove this when they do
379+            assert_raises_rpc_error(-12, "Error: No bech32m addresses available", self.nodes[0].getnewaddress, "", "bech32m")
380+            assert_raises_rpc_error(-12, "Error: Keypool ran out, please call keypoolrefill first", self.nodes[0].getrawchangeaddress, "bech32m")

fjahr commented at 11:37 pm on June 10, 2021:

Hm, these error messages could be more clear, even if they are only temporary since they will be probably what people might run into with 0.22. Maybe return something similar as the log info above in both cases if the in this case?

achow101 commented at 1:48 am on June 13, 2021:

I’ve added a commit which allows GetReservedDestination to return real error messages. So this is now No bech32m addresses available.

in test/functional/wallet_address_types.py:377 in 73ddcb81fc outdated

372@@ -373,5 +373,15 @@ def run_test(self):
373         self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit')
374         self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32')
375 
376+        if self.options.descriptors:
377+            self.log.info("Descriptor wallets do not have bech32m addreses by default yet")

fjahr commented at 11:37 pm on June 10, 2021:

addresses

achow101 commented at 1:48 am on June 13, 2021:

Fixed

in src/wallet/scriptpubkeyman.cpp:25 in fe7f3ee0e7 outdated

22@@ -23,6 +23,11 @@ const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;
23 
24 bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
25 {
26+    if (LEGACY_OUTPUT_TYPES.count(type) == 0) {

fjahr commented at 0:08 am on June 13, 2021:

Have you thought about placing asserts in LegacyScriptPubKeyMan::KeepDestination, LegacyScriptPubKeyMan::GetKeyFromPool, and LegacyScriptPubKeyMan::LearnRelatedScripts?

achow101 commented at 1:50 am on June 13, 2021:

Added asserts for all of the LegacyScriptPubKeyMan functions which take an OutputType.

in src/wallet/wallet.cpp:1909 in 26597ff64d outdated

1905@@ -1906,7 +1906,12 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
1906         // Check if any destination contains a witness program:
1907         int witnessversion = 0;
1908         std::vector<unsigned char> witnessprogram;
1909-        if (recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
1910+        bool is_wit = recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram);

fjahr commented at 0:12 am on June 13, 2021:

Can be const

achow101 commented at 1:50 am on June 13, 2021:

Done

fjahr commented at 0:36 am on June 13, 2021: member

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

achow101 force-pushed on Jun 13, 2021

fjahr commented at 8:11 pm on June 13, 2021: member

utACK 05ff76c3aad6905d984ba1764c8045ca530fbf8f

in src/wallet/scriptpubkeyman.cpp:26 in a1373469af outdated

22@@ -23,6 +23,11 @@ const uint32_t BIP32_HARDENED_KEY_LIMIT = 0x80000000;
23 
24 bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
25 {
26+    if (LEGACY_OUTPUT_TYPES.count(type) == 0) {
27+        error = _("Error: Legacy wallets only support the \"legacy\", \"p2sh-segwit\", and \"bech32\" address types").translated;

Sjors commented at 1:21 pm on June 16, 2021:

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.

achow101 commented at 4:37 pm on June 16, 2021:

Yes, just belt and suspenders.

in src/wallet/wallet.cpp:1910 in fe262ac06b outdated

1905@@ -1906,7 +1906,12 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
1906         // Check if any destination contains a witness program:
1907         int witnessversion = 0;
1908         std::vector<unsigned char> witnessprogram;
1909-        if (recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
1910+        const bool is_wit = recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram);
1911+        if (is_wit && witnessversion >= 1 && GetScriptPubKeyMan(OutputType::BECH32M, true)) {

Sjors commented at 1:32 pm on June 16, 2021:

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

achow101 commented at 4:41 pm on June 16, 2021:

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.

MarcoFalke commented at 9:56 am on June 17, 2021:

4bf448fc2528d9df371b2ca6e9b13319441a21e5: I don’t understand what the witnessversion >= 1 is doing.

Assuming witnessversion is 2, then a bech32m address will be selected (possibly a taproot address). However, if the witnessversion is 0, even with a taproot wallet, a segwit_0 address will be enforced. This seems inconsistent and confusing, unless I am missing something?

MarcoFalke commented at 10:02 am on June 17, 2021:

Also, if the first output is a witnessversion_0 and the second a witnessversion_1, this function will incorrectly return witnessversion_0?

Sjors commented at 2:40 pm on June 17, 2021:

This opportunistic change code was initially intended to upgrade change addresses to bech32 if there was no privacy downside. With this new change it seems that we downgrade a v1+ witness to v0 just in order to blend in. I’m also not sure if we should do that. It could actually hurt privacy when that v0 change address is spent later on and combined with a v1 input.

achow101 commented at 4:58 pm on June 17, 2021:

Indeed. I’ve changed this to just return BECH32M or BECH32 depending on what is available, with a preference for BECH32M.

However I think this function (and generally how change address type is determined) should be revisited because not all address types are available in descriptor wallets. But I think that is out of scope for this PR.

Sjors approved

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

tACK 05ff76c

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

in src/wallet/wallet.cpp:1914 in fe262ac06b outdated

1910+        const bool is_wit = recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram);
1911+        if (is_wit && witnessversion >= 1 && GetScriptPubKeyMan(OutputType::BECH32M, true)) {
1912+            // If this is a segwit v1+ address and this wallet supports bech32m addresses, use bech32m
1913+            return OutputType::BECH32M;
1914+        } else if (is_wit) {
1915+            // Otherwise if this is witness, use bech32

Sjors commented at 8:05 pm on June 16, 2021:

This assumes there is always a bech32 descriptor, which may not be the case if you’re using a custom taproot descriptor without fallback.

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 3eca506f08..c0b28acd47 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -1911,9 +1911,11 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
 5         if (is_wit && witnessversion >= 1 && GetScriptPubKeyMan(OutputType::BECH32M, true)) {
 6             // If this is a segwit v1+ address and this wallet supports bech32m addresses, use bech32m
 7             return OutputType::BECH32M;
 8-        } else if (is_wit) {
 9-            // Otherwise if this is witness, use bech32
10+        } else if (is_wit && GetScriptPubKeyMan(OutputType::BECH32, true)) {
11+            // Otherwise if this is witness, use bech32 if we have a descriptor:
12             return OutputType::BECH32;
13+        } else {
14+            return m_default_address_type;
15         }
16     }

achow101 commented at 8:56 pm on June 16, 2021:

The original code did not check for a bech32 ScriptPubKeyMan, but I have added that check. I did not include the else suggested in your diff as that would be incorrect.

achow101 force-pushed on Jun 16, 2021

Sjors commented at 9:19 am on June 17, 2021: member

re-utACK f70d55c136d9fc0bc986cc86e619570e0417d188

in src/wallet/wallet.cpp:2121 in f70d55c136 outdated

2114@@ -2115,7 +2115,7 @@ bool CWallet::GetNewDestination(const OutputType type, const std::string label,
2115         spk_man->TopUp();
2116         result = spk_man->GetNewDestination(type, dest, error);
2117     } else {
2118-        error = strprintf("Error: No %s addresses available.", FormatOutputType(type));
2119+        error = strprintf(_("Error: No %s addresses available."), FormatOutputType(type)).translated;

MarcoFalke commented at 9:57 am on June 17, 2021:

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.

achow101 commented at 5:00 pm on June 17, 2021:

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.

laanwj commented at 5:54 pm on June 18, 2021:

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.

achow101 commented at 6:11 pm on June 18, 2021:

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.

laanwj commented at 6:42 am on June 21, 2021:

Ok…

MarcoFalke commented at 9:58 am on June 17, 2021: member

left two comments.

achow101 force-pushed on Jun 17, 2021

practicalswift commented at 7:53 pm on June 17, 2021: contributor

Concept ACK

Sjors commented at 1:39 pm on June 18, 2021: member

re-utACK f33361c17f3937ee8819fcea76548e19f6ac6558

fjahr commented at 7:31 pm on June 20, 2021: member

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 commented at 7:03 am on June 21, 2021: member

Code review ACK f33361c17f3937ee8819fcea76548e19f6ac6558 re-review ACK 754f134a50cc56cdf0baf996d909c992770fcc97

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

meshcollider commented at 1:50 am on June 23, 2021: contributor

I get an error in wallet_taproot.py:

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

Limit LegacyScriptPubKeyMan address types

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

177c15d2f7

Add OutputType::BECH32M

Bech32m addresses need their own OutputType

We are not ready to create DescriptorScriptPubKeyMans which produce
bech32m addresses. So don't allow generating them.

0262536c34

Opportunistically use bech32m change addresses if available

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.

699dfcd8ad

achow101 force-pushed on Jun 23, 2021

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

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

6dbe4d1072

Disallow bech32m addresses for legacy wallet things

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.

87a0e7a3b7

wallet: Add error message to GetReservedDestination

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.

754f134a50

achow101 commented at 1:57 am on June 23, 2021: member

Silent merge conflict, rebased and fixed.

achow101 force-pushed on Jun 23, 2021

Sjors commented at 12:36 pm on June 23, 2021: member

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

in src/script/descriptor.cpp:643 in 754f134a50

639@@ -640,20 +640,6 @@ class DescriptorImpl : public Descriptor
640     std::optional<OutputType> GetOutputType() const override { return std::nullopt; }
641 };
642 
643-static std::optional<OutputType> OutputTypeFromDestination(const CTxDestination& dest) {

jonatack commented at 9:40 pm on June 23, 2021:

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

0--- a/src/script/descriptor.cpp
1+++ b/src/script/descriptor.cpp
2@@ -5,6 +5,7 @@
3 #include <script/descriptor.h>
4 
5 #include <key_io.h>
6+#include <outputtype.h>
7 #include <pubkey.h>
8 #include <script/script.h>
9 #include <script/standard.h>

in test/functional/wallet_address_types.py:377 in 754f134a50

372@@ -373,5 +373,15 @@ def run_test(self):
373         self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit')
374         self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32')
375 
376+        if self.options.descriptors:
377+            self.log.info("Descriptor wallets do not have bech32m addresses by default yet")

jonatack commented at 10:11 pm on June 23, 2021:

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

jonatack commented at 10:18 pm on June 23, 2021: member

ACK 754f134a50cc56cdf0baf996d909c992770fcc97

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

achow101 commented at 10:59 pm on June 23, 2021: member

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

When we make bech32m descriptors by default.

fjahr commented at 11:03 pm on June 23, 2021: member

re-ACK 754f134a50cc56cdf0baf996d909c992770fcc97

Confirmed only changes were rebasing and fixing wallet_taproot.py.

laanwj merged this on Jun 24, 2021

laanwj closed this on Jun 24, 2021

sidhujag referenced this in commit 66e52de009 on Jun 24, 2021

Sjors commented at 5:52 pm on June 24, 2021: member

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

gwillen referenced this in commit b3bba59059 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

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

Conflicts