BIP 350: Implement Bech32m and use it for v1+ segwit addresses

sipa commented at 9:43 pm on January 5, 2021: member

This implements BIP 350:

For segwit v1+ addresses, a new checksum algorithm called Bech32m is used.
Segwit v0 address keep using Bech32 as specified in BIP 173.

DrahtBot commented at 10:05 pm on January 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:

#21279 (scripted-diff: Regenerate key_io data deterministically by MarcoFalke)

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.

sipa force-pushed on Jan 5, 2021

DrahtBot added the label Utils/log/libs on Jan 5, 2021

sipa force-pushed on Jan 5, 2021

in src/key_io.cpp:102 in 863a5c2eba outdated

100+    if ((encoding == bech32::Encoding::BECH32 || encoding == bech32::Encoding::BECH32M) &&
101+        encdata.size() > 0 && hrp == params.Bech32HRP()) {
102         // Bech32 decoding
103-        int version = bech.second[0]; // The first 5 bit symbol is the witness version (0-16)
104+        int version = encdata[0]; // The first 5 bit symbol is the witness version (0-16)
105+        if ((version == 0 && encoding != bech32::Encoding::BECH32) || (version != 0 && encoding != bech32::Encoding::BECH32M)) {

luke-jr commented at 0:52 am on January 6, 2021:

Why forbid Bech32m for v0? Seems like it might be nice to (very slowly) migrate to Bech32m exclusively, and drop Bech32 someday?

sipa commented at 0:55 am on January 6, 2021:

I think it’s a bad idea to have multiple addresses valid for the same scriptPubKey. It only results in confusion when decoding/recoding doesn’t roundtrip. v0 outputs use bech32, and I don’t think this should ever change, for better or for worse.

It also worsens error detection to 29 bits (as every v0 output now has two valid checksums).

luke-jr commented at 1:40 am on January 6, 2021:

Roundtrips (by uninvolved third parties) seem like strictly a bad thing IMO, but loss of error detection precision is a good reason. :/

NicolasDorier commented at 12:40 pm on January 6, 2021: contributor

By implementing Bech32m, I found out that it is not obvious how to locate errors, because the residue was polymod^1, but now is either polimod^1 or polimod^0x2bc830a3… I am tempted to try both and take the residue with the least errors. I think this should be specified in the bip. That said by doing this, the error patterns with failure probability in the bip are probably different. As I think you assume in your table that the decoder knows it is decoding bech32m. Unsure if this is really changing things by a lot.

This is not the right place to comment for it, but did not found elsewhere.

luke-jr commented at 5:16 pm on January 6, 2021: member

@NicolasDorier That isn’t solved by sipa only allowing one polimod based on the witness version?

sipa commented at 5:29 pm on January 6, 2021: member

@luke-jr It’s not, because the error may be one that affects the witness version symbol. @NicolasDorier Good point, I will elaborate on that.

felipsoarez commented at 12:45 pm on January 9, 2021: none

Concept ACK

michaelfolkson commented at 8:35 pm on January 9, 2021: contributor

Concept ACK, Approach ACK. Skimmed code but will hopefully go through more thoroughly at a later date.

https://bitcoin.stackexchange.com/questions/101117/what-problems-identified-with-bech32-addresses-have-been-resolved-with-the-updat

in src/bech32.h:27 in 6b28812881 outdated

26+    INVALID,
27 
28-/** Decode a Bech32 string. Returns (hrp, data). Empty hrp means failure. */
29-std::pair<std::string, std::vector<uint8_t>> Decode(const std::string& str);
30+    BECH32,  //! Bech32 encoding as defined in BIP173
31+    BECH32M, //! Bech32m encoding as defined in bip-bech32m

kallewoof commented at 10:44 am on January 18, 2021:

Commit 6b28812881375311891e80c1d4e9df6aaa0b85d5

Perhaps too unorthodox, but it would theoretically be possible to do

0   BECH32 = 1, //...
1   BECH32M = 0x2bc830a3, // ...

and get rid of EncodingConstant. (But the amount of casting required is probably not worth it.)

sipa commented at 10:05 pm on January 18, 2021:

I prefer not to do that just for encapsulation reason. Nothing outside of bech32.cpp should care or see the constants.

kallewoof commented at 11:00 am on January 18, 2021: member

Slightly tested ACK 863a5c2eba77ca571813b33b9fa6237a9f3efbd7

I honestly don’t really like the std::tuple stuff. It’s pretty ugly and a simple struct containing the 3 elements passed as a copy ~~by reference~~ (edit: I meant like the tuples are passed now) in the same way would be more straightforward.

sipa force-pushed on Jan 18, 2021

sipa commented at 10:05 pm on January 18, 2021: member

@kallewoof Agree, a struct is definitely warranted here. Updated.

in src/bech32.h:17 in 936322d351 outdated

13@@ -14,16 +14,35 @@
14 
15 #include <stdint.h>
16 #include <string>
17+#include <tuple>

btcslade commented at 0:56 am on January 19, 2021:

Don’t think this is needed anymore.

sipa commented at 4:06 am on January 19, 2021:

Fixed.

btcslade commented at 0:58 am on January 19, 2021: none

utACK 936322d351ad8ad2b0497bdb0651d5d969944f9f

sipa force-pushed on Jan 19, 2021

kallewoof commented at 4:15 am on January 19, 2021: member

re-slightly-tested-ACK 2827cf86559a9310ca3fb8b19e03422586a21a2b

decryp2kanon approved

decryp2kanon commented at 6:08 pm on January 19, 2021: contributor

Code Review ACK 2827cf8

benthecarman approved

benthecarman commented at 10:49 pm on January 20, 2021: contributor

utACK 2827cf86559a9310ca3fb8b19e03422586a21a2b

gruve-p commented at 11:41 am on January 21, 2021: contributor

utACK https://github.com/bitcoin/bitcoin/pull/20861/commits/2827cf86559a9310ca3fb8b19e03422586a21a2b

DrahtBot added the label Needs rebase on Jan 26, 2021

sipa force-pushed on Jan 27, 2021

sipa commented at 0:23 am on January 27, 2021: member

Rebased now #20832 is merged.

I’ve also addressed some comments on the BIP draft itself, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-January/018362.html.

DrahtBot removed the label Needs rebase on Jan 27, 2021

kallewoof commented at 7:11 am on January 27, 2021: member

re-ACK 3fee858d4732c57355ec9d18c66e84ed5d2a5961

benthecarman commented at 7:42 am on January 27, 2021: contributor

re-ACK 3fee858

sipa force-pushed on Jan 28, 2021

sipa renamed this:
~~Implement Bech32m and use it for v1+ segwit addresses~~
BIP 350: Implement Bech32m and use it for v1+ segwit addresses
on Jan 28, 2021

sipa force-pushed on Jan 28, 2021

sipa commented at 11:22 pm on February 3, 2021: member

BIP draft is now published as BIP 350.

in src/bech32.cpp:36 in 50769c8085 outdated

29@@ -27,6 +30,12 @@ const int8_t CHARSET_REV[128] = {
30      1,  0,  3, 16, 11, 28, 12, 14,  6,  4,  2, -1, -1, -1, -1, -1
31 };
32 
33+/* Determine the final constant to use for the specified encoding. */
34+uint32_t EncodingConstant(Encoding encoding) {
35+    assert(encoding == Encoding::BECH32 || encoding == Encoding::BECH32M);
36+    return encoding == Encoding::BECH32 ? 1 : 0x2bc830a3;

dr-orlovsky commented at 6:05 pm on February 5, 2021:

Shouldn’t 0x2bc830a3 be made a global constant value?

sipa commented at 7:05 pm on February 11, 2021:

IMO, no. There is no reason why it should be exported from this module.

in src/bech32.cpp:136 in 50769c8085 outdated

135+    return Encoding::INVALID;
136 }
137 
138 /** Create a checksum. */
139-data CreateChecksum(const std::string& hrp, const data& values)
140+data CreateChecksum(const std::string& hrp, const data& values, Encoding encoding)

dr-orlovsky commented at 6:07 pm on February 5, 2021:

Why here Encoding is added as the last parameter why in https://github.com/bitcoin/bitcoin/pull/20861/files#diff-f146300624c06d2e08aadf500952294148a1785edd6ff2e8b50f13b2c08255edR152 it became first? Is there any reason behind API inconsistency?

sipa commented at 10:49 pm on February 17, 2021:

Fixed.

dr-orlovsky commented at 6:17 pm on February 5, 2021: none

A small API inconsistency caught and a global constant proposed

in doc/bips.md:53 in 50769c8085 outdated

45@@ -46,3 +46,4 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.21.0**):
46 * [`BIP 325`](https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki): Signet test network is supported as of **v0.21.0** ([PR 18267](https://github.com/bitcoin/bitcoin/pull/18267)).
47 * [`BIP 339`](https://github.com/bitcoin/bips/blob/master/bip-0339.mediawiki): Relay of transactions by wtxid is supported as of **v0.21.0** ([PR 18044](https://github.com/bitcoin/bitcoin/pull/18044)).
48 * [`BIP 340`](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki) [`341`](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki) [`342`](https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki): Validation rules for Taproot (including Schnorr signatures and Tapscript leaves) are implemented as of **v0.21.0** ([PR 19953](https://github.com/bitcoin/bitcoin/pull/19953)), without mainnet activation.
49+* [`BIP 350`](https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki): Addresses for native v1+ segregated Witness outputs use Bech32m instead of Bech32 as of **v22.0** ([PR 20861](https://github.com/bitcoin/bitcoin/pull/20861)).

benthecarman commented at 12:43 pm on February 6, 2021:

Won’t this need to be backported to 0.21.1 with taproot activation?

sipa commented at 7:05 pm on February 11, 2021:

I have no opinion on that, but even if we do that, that’s not part of this PR.

Sjors commented at 3:39 pm on February 17, 2021:

Also note that if 0.21.1 contains Taproot activation logic, it won’t contain any wallet support. Being able to send to a Taproot address is probably worth back-porting, but that could also wait for 0.21.2.

in src/test/data/key_io_valid.json:493 in 50769c8085 outdated

485@@ -486,7 +486,25 @@
486         }
487     ],
488     [
489-        "bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx",
490+        "tb1qqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvsesrxh6hy",
491+        "0020000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
492+        {
493+            "isPrivkey": false,
494+            "chain": "test",

benthecarman commented at 12:45 pm on February 6, 2021:

Couldn’t this be signet as well?

sipa commented at 10:49 pm on February 17, 2021:

Added a signet copy of this.

in src/test/data/key_io_valid.json:590 in 50769c8085 outdated

535-        "0020000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
536+        "tb1pqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvsesf3hn0c",
537+        "5120000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
538         {
539             "isPrivkey": false,
540             "chain": "test",

benthecarman commented at 12:46 pm on February 6, 2021:

ditto signet comment

sipa commented at 10:49 pm on February 17, 2021:

And this.

laanwj added this to the "Blockers" column in a project

sipa force-pushed on Feb 17, 2021

sipa commented at 10:49 pm on February 17, 2021: member

Rebased and addressed review comments.

benthecarman commented at 1:31 am on February 18, 2021: contributor

ACK f15513d4184e637eda76eb23e0d6d47d7fb25075

Sjors commented at 2:55 pm on February 18, 2021: member

ACK f15513d4184e637eda76eb23e0d6d47d7fb25075

I’m confused how key_io_valid.json and key_io_invalid.json are used. It doesn’t seem to be referenced in any of the (functional) tests or CI scripts. If I use the suggested incantation in gen_key_io_test_vectors.py it fails at line 131 with bech32_encode() missing 1 required positional argument: 'spec'.

sipa force-pushed on Feb 19, 2021

sipa commented at 10:39 pm on February 19, 2021: member

@Sjors The key_io_{in,}valid.json tests are executed in src/test/key_io_tests.cpp. I had no idea there was a script to automatically generate them, though. I’ve updated that now too and regenerated them using it.

in src/key_io.cpp:99 in 835ff6b856 outdated

 94@@ -95,20 +95,26 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
 95         error_str = "Invalid prefix for Base58-encoded address";
 96     }
 97     data.clear();
 98-    auto bech = bech32::Decode(str);
 99-    if (bech.second.size() > 0) {
100+    const auto dec = bech32::Decode(str);
101+    if ((dec.encoding == bech32::Encoding::BECH32 || dec.encoding == bech32::Encoding::BECH32M) && dec.data.size() > 0) {

dr-orlovsky commented at 1:44 am on February 20, 2021:

There might be a Base58 string which is a valid Bech32m with future Witness version, and since Base58 is the first decoder, a future valid Bech32m address may be incorrectly parsed as Base58. So I propose to change their order.

Sjors commented at 1:44 pm on February 23, 2021:

That seems like something for a separate PR.

sipa commented at 11:19 pm on March 4, 2021:

I don’t think there is any concern about that.

The decoder only accepts Base58Check inputs that encode 20 data bytes, which means addresses of at most 35 characters (ceil(log(256)/log(58)*20) == 20). A Bech32(m) segwit address of 35 characters (assuming 2+-character HRP) would encode at most (35-6(checksum)-1(separator)-2(hrp)-1(version byte))*5 = 125 bits, rounded down to a byte, 120 bits. I don’t think anyone is planning on proposing 120-bit addresses for Bitcoin any time soon.

jnewbery commented at 7:41 pm on March 10, 2021:

I honestly don’t really like the std::tuple stuff. It’s pretty ugly and a simple struct containing the 3 elements passed as a copy by reference (edit: I meant like the tuples are passed now) in the same way would be more straightforward. (@kallewoof)

@kallewoof Agree, a struct is definitely warranted here. Updated. (@sipa)

No need to change this again, but in future you may find that structured bindings (since c++17) offer a clean and readable way to avoid the boilerplate of declaring a struct return. Combining with an if init statement:

0    if (const auto [encoding, hrp, data] = bech32::Decode(str);
1        (encoding == bech32::Encoding::BECH32 || encoding == bech32::Encoding::BECH32M) && data.size() > 0) {
2        // [... do stuff ]
3    } // encoding, hrp and data fall out of scope at end of if block

sipa commented at 7:54 pm on March 10, 2021:

Agree, though another reason why we may not want to do this: we’ll want this PR backported as far as possible.

jnewbery commented at 8:00 pm on March 10, 2021:

Makes sense. I’ll mark this as resolved.

Also, TIL: you can use structured bindings to unpack a struct, so this works even when the return value is a DecodeResult: const auto [encoding, hrp, d] = bech32::Decode(str);

in src/key_io.cpp:117 in 835ff6b856 outdated

121+        }
122         // The rest of the symbols are converted witness program bytes.
123-        data.reserve(((bech.second.size() - 1) * 5) / 8);
124-        if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, bech.second.begin() + 1, bech.second.end())) {
125+        data.reserve(((dec.data.size() - 1) * 5) / 8);
126+        if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec.data.begin() + 1, dec.data.end())) {

dr-orlovsky commented at 1:50 am on February 20, 2021:

in case of Bech32 address error_str will still be set to "Invalid prefix for Base58-encoded address" because it is not cleaned when we return https://github.com/bitcoin/bitcoin/pull/20861/files#diff-a83ac7cb4a9d60b647db6378ccfad815005724e594bf53f49745b0f40701292fR152

So I propose

0        if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec.data.begin() + 1, dec.data.end())) {
1            error_str = "";

sipa commented at 3:51 am on February 20, 2021:

It’s already wiped in line 102, I think?

dr-orlovsky commented at 9:38 am on February 20, 2021:

True, don’t know how I missed that

dr-orlovsky commented at 1:51 am on February 20, 2021: none

Found few tricky places, which are from the previous implementation

Sjors commented at 1:54 pm on February 23, 2021: member

re-ACK 835ff6b

We should make gen_key_io_test_vectors.py deterministic, because right now it’s impractical to verify the JSON changes in ea3995aa3cd8f04d7328c95a43c79a5b20fa18a5 and 835ff6b8568291870652ca0d33d934039e7b84a8. All I could do is run it again and see if the tests still pass.

benthecarman approved

benthecarman commented at 3:03 pm on March 1, 2021: contributor

reACK 835ff6b8568291870652ca0d33d934039e7b84a8

in src/bech32.h:31 in 835ff6b856 outdated

30+    BECH32M, //!< Bech32m encoding as defined in BIP350
31+};
32+
33+/** Encode a Bech32 or Bech32m string. If hrp contains uppercase characters, this will cause an
34+ *  assertion error. Encoding must be one of BECH32 or BECH32M. */
35+std::string Encode(Encoding, const std::string& hrp, const std::vector<uint8_t>& values);

jnewbery commented at 4:58 pm on March 10, 2021:

Makes no difference to the compiled code, but the convention is to give the declaration’s parameters names (which match the parameter names in the function definition).

0std::string Encode(Encoding encoding, const std::string& hrp, const std::vector<uint8_t>& values);

sipa commented at 6:00 am on March 11, 2021:

Done.

in src/bech32.h:1 in 835ff6b856 outdated

0@@ -1,4 +1,4 @@
1-// Copyright (c) 2017 Pieter Wuille
2+// Copyright (c) 2017, 2021 Pieter Wuille

jnewbery commented at 4:59 pm on March 10, 2021:

Perhaps update line 10:

0-// For more information, see BIP 173.
1+// For more information, see BIPs 173 and 350.

sipa commented at 6:00 am on March 11, 2021:

Done.

in src/test/fuzz/bech32.cpp:34 in 835ff6b856 outdated

45-        assert(data == input);
46+    for (auto encoding : {bech32::Encoding::BECH32, bech32::Encoding::BECH32M}) {
47+        const std::string encoded = bech32::Encode(encoding, "bc", input);
48+        assert(!encoded.empty());
49+        const auto r2 = bech32::Decode(encoded);
50+        if (r2.hrp.empty()) {

jnewbery commented at 7:19 pm on March 10, 2021:

Should a round trip encode/decode always result in the same hrp (“bc”) here? Am I missing some way that this could legitimately fail?

sipa commented at 6:01 am on March 11, 2021:

This already existed in the code before this PR, but it’s a good point. The only failure case is when the data is too big to fit in a valid Bech32(m) encoding (max 90 characters).

I’ve added a check for that, and then dropped the if (r2.hrp.empty()) { branch.

jnewbery commented at 7:46 pm on March 10, 2021: member

Just a few comments on the first commit so far. Nothing blocking or that needs to be changed.

in src/test/bech32_tests.cpp:97 in f0e1595542 outdated

92+        "16plkw9",
93+        "1p2gdwpf"
94+    };
95+    for (const std::string& str : CASES) {
96+        const auto dec = bech32::Decode(str);
97+        BOOST_CHECK(dec.encoding != bech32::Encoding::BECH32);

ryanofsky commented at 3:50 am on March 11, 2021:

In commit “Add Bech32m test vectors” (f0e15955421870c297d4ddb523431dae711e27d3)

I think this means to check != BECH32M, instead of != BECH32. Alternately could check == INVALID, if it is not a problem to be more strict.

sipa commented at 6:01 am on March 11, 2021:

Indeed. Replaced with == INVALID.

in src/key_io.cpp:65 in ea3995aa3c outdated

61@@ -62,7 +62,7 @@ class DestinationEncoder
62         std::vector<unsigned char> data = {(unsigned char)id.version};
63         data.reserve(1 + (id.length * 8 + 4) / 5);
64         ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, id.program, id.program + id.length);
65-        return bech32::Encode(bech32::Encoding::BECH32, m_params.Bech32HRP(), data);
66+        return bech32::Encode(bech32::Encoding::BECH32M, m_params.Bech32HRP(), data);

ryanofsky commented at 4:14 am on March 11, 2021:

In commit “Use Bech32m encoding for v1+ segwit addresses” (ea3995aa3cd8f04d7328c95a43c79a5b20fa18a5)

Should AddressDescriptor::GetOutputType should now return BECH32M instead of BECH32 for WitnessUnknown addresses to be consistent with this change?

https://github.com/bitcoin/bitcoin/blob/ea3995aa3cd8f04d7328c95a43c79a5b20fa18a5/src/script/descriptor.cpp#L656

Also, this change seems like the first change that affects user-visible behavior, so it would be great if it added release notes or at least mentioned what the effects are in the commit message. I think the only thing current software can do with V1 addresses is validate them and do watchonly scans for them. So this only affects validate and import RPCs?

sipa commented at 6:07 am on March 11, 2021:

Very good point.

We can’t return GetOutputType::BECH32M because no such constant exists. It could be added, but I’d rather not do that in this PR, because I don’t think it actually matters at this stage. The purpose of output types is so the user can select what kind of address to request from a wallet. You’re right that technically a addr(...) descriptor could be imported (but I think that in most cases a ranged descriptor is required, so addr(...) won’t even work), and it might matter to classify that as bech32m - but this seems like a very unrealistic edge case.

However, I think this will matter once we want to add actual taproot support to the wallet (#21365, and follow-ups that at one point might want to construct such wallet descriptors by default). I suspect we may want an OutputType::BECH32M then, and permit wallets to have separate -addresstype=bech32 and -addresstype=bech32m active descriptors, as one may know that the sender they want to receive from supports bech32 but not bech32m yet.

I’ve also added release notes.

ping @achow101: thoughts on OutputType::BECH32M?

Sjors commented at 10:41 am on March 11, 2021:

I think -addresstype=bech32m makes sense, e.g. getnewaddress bech32m as a way to request a taproot p2pk address. However it might be more future proof to use -addresstype=tap and getnewaddress tap.

Longer term I suspect it might be better to give wallet descriptors a name and then call getnewaddress NAME.

See also #15590 for some earlier discussions around this issue.

sipa commented at 7:43 pm on March 11, 2021:

However it might be more future proof to use -addresstype=tap and getnewaddress tap.

I’m not sure. If Bech32m gets adopted as I hope it does, we won’t need a new address type for post-taproot things anymore.

achow101 commented at 8:20 pm on March 11, 2021:

Since bech32m is (and bech32 was) intended to be used for different segwit versions, I think that we should actually move away from having bech32 or bech32m as the address type as with future segwit versions, it becomes ambiguous as to what type of address you want.

For example, suppose we introduce a segwit v2 that, like segwit v1, will encode a pubkey in the address. Now the wallet would have a descriptor for segwit v1, and another for segwit v2. If I wanted a segwit v1 address, what do I provide as the address type? If I provide bech32m, that’s ambiguous because both segwit v1 and segwit v2 use bech32m addresses. We would have to come up with another name for segwit v2 addresses, and even then, it is still confusing because technically both are bech32m addresses.

With that in mind, I think we need to use a different and more unique naming scheme. Luckily this is pretty easy because segwit has version numbers, and proposals all have unique names. So for segwit v1 addresses, we could call the address type tap and just not expose bech32m in it.

I have done a similar thing in HWI where segwit v0 is wit rather than bech32, and segwit v1 will be tap.

Looking at it now, I do also think that having bech32 as the address type for segwit v0 was not the right thing to do. It really should have just be wit or something like that that makes it unique to just segwit v0. Since bech32 is now segwit v0 only, it’s fine, but in the future, we should not do the same thing.

sipa commented at 8:31 pm on March 11, 2021:

Now the wallet would have a descriptor for segwit v1, and another for segwit v2. If I wanted a segwit v1 address, what do I provide as the address type?

I think that’s the crucial point: why would you want that? If every sender supports both equally, you’d always produce whatever you have a descriptor for that is compatible with that.

If there is a reason for being more flexible, no objection to just using names or something. But then also, would you support having multiple distinct active descriptors that produce the same address type within the same wallet? I think that’s the same problem.

If anything, I think we should have done things in the other direction; OutputTypes should have been LEGACY, BIP16, BECH32 or so (and BIP16 should have been both usable for multisig and segwit v0). Because those are the classes senders care about.

achow101 commented at 9:22 pm on March 11, 2021:

I think that’s the crucial point: why would you want that? If every sender supports both equally, you’d always produce whatever you have a descriptor for that is compatible with that.

In the ideal world, sure. But as we have seen with segwit, that’s not guaranteed. Even though senders were supposed to allow any segwit version with bech32, they didn’t. I’m not sure that everyone will understand to do that with bech32m.

The address type option is not just for senders. It’s also for receivers and what they are comfortable with receiving. I could conceive of a situation where someone sets up a new wallet but wants to use v1 instead of v2 because, e.g., v2 introduces some new signature algo that has assumptions they aren’t comfortable with using. What would they have to do to use v1?

sipa commented at 9:30 pm on March 11, 2021:

In the ideal world, sure. But as we have seen with segwit, that’s not guaranteed. Even though senders were supposed to allow any segwit version with bech32, they didn’t. I’m not sure that everyone will understand to do that with bech32m.

Sure, that’s why I suggest we may want to add an OutputType::BECH32M - precisely because Bech32 and Bech32m may differ for senders. But p2sh vs. p2sh-p2wpk shouldn’t be distinguished, for example.

I could conceive of a situation where someone sets up a new wallet but wants to use v1 instead of v2 because, e.g., v2 introduces some new signature algo that has assumptions they aren’t comfortable with using. What would they have to do to use v1?

That’s fair, and was a good argument for segwit vs non-segwit in pre-descriptor wallets. But in descriptor wallets if you don’t want witness v9 addresses because you’re not comfortable with them, simply don’t import/create such a descriptor.

achow101 commented at 10:30 pm on March 11, 2021:

Sure, that’s why I suggest we may want to add an OutputType::BECH32M - precisely because Bech32 and Bech32m may differ for senders.

Yes, I agree with doing that.

But in descriptor wallets if you don’t want witness v9 addresses because you’re not comfortable with them, simply don’t import/create such a descriptor.

That’s reasonable, but I think it’s too onerous on users to have to figure out which and how to create the descriptor that they want. I think it’s reasonable that user would know about different address types and, at a high level, the consequences of using each type, but also not know (or be afraid of) the descriptor syntax, which derivation paths to use, safely handling xprvs for import, etc. I suppose we could add more options to the wallet creation stuff, but that still means the user has to deal with descriptors directly once they feel comfortable enough to upgrade, or they have to make a wholly new wallet.

If instead the wallet generated v1 and v2 descriptors, then the user could switch to the new address type by choosing the option and it’s very simple and easy to do so.

sipa commented at 11:47 pm on March 12, 2021:

I’m going to mark this as resolved; we’ve discussed this in today’s wallet meeting. It’s not exactly clear how to proceed, but in any case, I don’t think this is something for this PR. Please respond if you don’t agree.

jonatack commented at 11:55 pm on March 12, 2021:

Discussion: http://www.erisian.com.au/bitcoin-core-dev/log-2021-03-12.html#l-286

in contrib/testgen/gen_key_io_test_vectors.py:71 in ea3995aa3c outdated

66@@ -65,29 +67,39 @@
67 ]
68 # templates for valid bech32 sequences
69 bech32_templates = [
70-  # hrp, version, witprog_size, metadata, output_prefix
71-  ('bc',    0, 20, (False, 'main',    None, True), p2wpkh_prefix),
72-  ('bc',    0, 32, (False, 'main',    None, True), p2wsh_prefix),
73-  ('bc',    1,  2, (False, 'main',    None, True), (OP_1, 2)),

ryanofsky commented at 4:22 am on March 11, 2021:

In commit “Use Bech32m encoding for v1+ segwit addresses” (ea3995aa3cd8f04d7328c95a43c79a5b20fa18a5)

I probably just need to stare at this more, but maybe you could summarize the changes here. Like where did the 1,2 and 2,16 cases go?

sipa commented at 6:11 am on March 11, 2021:

They’re just gone and replaced by better cases. I got rid of (1,2) as (1,32) is a more relevant case (that’s taproot) and added (2,2) too. To test a bit larger variety, some other (2, _) were replaced with (3, _) as well.

ryanofsky commented at 4:30 am on March 11, 2021: member

Partial code review ACK 835ff6b8568291870652ca0d33d934039e7b84a8 for c++/python implementations and unit and functional tests, but I didn’t look at closely at test vector changes yet or look at fuzz tests at all

sipa force-pushed on Mar 11, 2021

Sjors commented at 10:44 am on March 11, 2021: member

re-utACK 92ec3fcdd2f79949839ce269403396362bea9d54

in doc/release-notes-20861.md:10 in d1054b98e5 outdated

 5+  being implemented, behavior for all RPCs that accept addresses is changed when
 6+  a native witness version 1 (or higher) is passed. These now require a Bech32m
 7+  encoding instead of a Bech32 one, and Bech32m encoding will be used for such
 8+  addresses in RPC output as well. No version 1 addresses should be created
 9+  for mainnet until consensus rules are adopted that give them meaning
10+  (e.g. through [BIP 341](https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki)).

ryanofsky commented at 12:15 pm on March 11, 2021:

s/350/341/

d1054b98e5ec04cd3cdb487eaaf561926ef6c82d

achow101 commented at 6:40 pm on March 15, 2021:

In d1054b98e5ec04cd3cdb487eaaf561926ef6c82d “Use Bech32m encoding for v1+ segwit addresses”

I think you meant https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki (341, not 350)

sipa commented at 0:19 am on March 16, 2021:

Done.

sipa commented at 9:17 pm on March 17, 2021:

Already fixed.

in src/test/fuzz/bech32.cpp:36 in 402cbb2951 outdated

35-    const std::string encoded = bech32::Encode("bc", input);
36-    assert(!encoded.empty());
37 
38-    const std::pair<std::string, std::vector<uint8_t>> r2 = bech32::Decode(encoded);
39-    if (r2.first.empty()) {
40-        assert(r2.second.empty());

achow101 commented at 8:26 pm on March 11, 2021:

How come this scenario is no longer relevant?

sipa commented at 8:58 pm on March 11, 2021:

The added if condition around it (which checks that the input is within acceptable range).

achow101 commented at 8:26 pm on March 11, 2021: member

Code Review ACK 92ec3fc

Code changes look good, I didn’t go too deep into the test vector changes, but the changes to the tests themselves look good.

jonatack commented at 8:30 pm on March 12, 2021: member

Approach ACK, reviewing.

in src/bech32.h:31 in 92ec3fcdd2 outdated

30+    BECH32M, //!< Bech32m encoding as defined in BIP350
31+};
32+
33+/** Encode a Bech32 or Bech32m string. If hrp contains uppercase characters, this will cause an
34+ *  assertion error. Encoding must be one of BECH32 or BECH32M. */
35+std::string Encode(Encoding encoding, const std::string& hrp, const std::vector<uint8_t>& values);

jonatack commented at 9:36 pm on March 12, 2021:

402cbb2 Perhaps move the data type alias to the header file to use it uniformly in bech32.{h, cpp}

 0diff --git a/src/bech32.cpp b/src/bech32.cpp
 1index 80794ec4f1..60d4d72ca5 100644
 2--- a/src/bech32.cpp
 3+++ b/src/bech32.cpp
 4@@ -13,8 +13,6 @@ namespace bech32
 5
 6-typedef std::vector<uint8_t> data;
 7-
 8 /** The Bech32 character set for encoding. */
 9 const char* CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l";
10 
11diff --git a/src/bech32.h b/src/bech32.h
12index 3679ea8ccb..7a601a7b97 100644
13--- a/src/bech32.h
14+++ b/src/bech32.h
15@@ -16,6 +16,8 @@
16 #include <string>
17 #include <vector>
18 
19+typedef std::vector<uint8_t> data;
20+
21 namespace bech32
22 {
23 
24@@ -28,16 +30,16 @@ enum class Encoding {
25 
26 /** Encode a Bech32 or Bech32m string. If hrp contains uppercase characters, this will cause an
27  *  assertion error. Encoding must be one of BECH32 or BECH32M. */
28-std::string Encode(Encoding encoding, const std::string& hrp, const std::vector<uint8_t>& values);
29+std::string Encode(Encoding encoding, const std::string& hrp, const data& values);
30 
31 struct DecodeResult
32 {
33-    Encoding encoding;         //!< What encoding was detected in the result; Encoding::INVALID if failed.
34-    std::string hrp;           //!< The human readable part
35-    std::vector<uint8_t> data; //!< The payload (excluding checksum)
36+    Encoding encoding; //!< What encoding was detected in the result; Encoding::INVALID if failed.
37+    std::string hrp;   //!< The human readable part
38+    data data;         //!< The payload (excluding checksum)
39 
40     DecodeResult() : encoding(Encoding::INVALID) {}
41-    DecodeResult(Encoding enc, std::string&& h, std::vector<uint8_t>&& d) : encoding(enc), hrp(std::move(h)), data(std::move(d)) {}
42+    DecodeResult(Encoding enc, std::string&& h, ::data&& d) : encoding(enc), hrp(std::move(h)), data(std::move(d)) {}
43 };

jnewbery commented at 9:00 am on March 15, 2021:

I don’t think we want to pollute the global namespace with a type called data.

jonatack commented at 6:19 pm on March 15, 2021:

Good point indeed: https://google.github.io/styleguide/cppguide.html#Aliases. “However, local convenience aliases are fine in function definitions, private sections of classes, explicitly marked internal namespaces, and in .cc (.cpp) files.”

sipa commented at 11:09 pm on March 15, 2021:

Perhaps it’s worth having a global typedef for this (maybe with a more descriptive name than data), but if so, I think it’s something for a different PR.

Closing this.

in src/bech32.cpp:129 in 92ec3fcdd2 outdated

127     // if we required that the checksum was 0, it would be the case that appending a 0 to a valid
128     // list of values would result in a new valid list. For that reason, Bech32 requires the
129-    // resulting checksum to be 1 instead.
130-    return PolyMod(Cat(ExpandHRP(hrp), values)) == 1;
131+    // resulting checksum to be 1 instead. In Bech32m, this constant was amended.
132+    uint32_t check = PolyMod(Cat(ExpandHRP(hrp), values));

jonatack commented at 9:39 pm on March 12, 2021:

402cbb29516d5bf03a18822af9f1b4b574f319ac pico-nits, check can be const (and braced initialization for type safety)

0    const uint32_t check{PolyMod(Cat(ExpandHRP(hrp), values))};

Idem for mod on line 140, result on line 195, and version in key_io.cpp::L106

sipa commented at 11:08 pm on March 15, 2021:

Perhaps this is more personal style. I don’t think this is an improvement.

sipa commented at 0:18 am on March 16, 2021:

Added const.

in doc/release-notes-20861.md:12 in 92ec3fcdd2 outdated

 7+  encoding instead of a Bech32 one, and Bech32m encoding will be used for such
 8+  addresses in RPC output as well. No version 1 addresses should be created
 9+  for mainnet until consensus rules are adopted that give them meaning
10+  (e.g. through [BIP 341](https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki)).
11+  Once that happens, Bech32m is expected to be used for them, so this shouldn't
12+  affect any production system, but may be observed on other networks where such

jonatack commented at 9:49 pm on March 12, 2021:

d1054b98e nit, s/system/systems/ and omit comma

0  affect any production systems but may be observed on other networks where such

sipa commented at 0:18 am on March 16, 2021:

Done.

in src/bech32.cpp:35 in 92ec3fcdd2 outdated

29@@ -27,6 +30,12 @@ const int8_t CHARSET_REV[128] = {
30      1,  0,  3, 16, 11, 28, 12, 14,  6,  4,  2, -1, -1, -1, -1, -1
31 };
32 
33+/* Determine the final constant to use for the specified encoding. */
34+uint32_t EncodingConstant(Encoding encoding) {
35+    assert(encoding == Encoding::BECH32 || encoding == Encoding::BECH32M);

jonatack commented at 9:52 pm on March 12, 2021:

Checking for encoding type may be done frequently enough to consider adding helper functions (not necessarily in this PR).

sipa commented at 11:22 pm on March 15, 2021:

I’m not sure what you’re suggesting concretely here.

kallewoof commented at 0:04 am on March 16, 2021:

I think explicitly asserting is purposefully done to give a sort of code-visual cue on where updates are required, after adding additional encodings in the future, so I don’t think helper functions would be appropriate here.

jonatack commented at 10:06 pm on March 12, 2021: member

Light first-pass ACK 92ec3fcdd2f79949839ce269403396362bea9d54

Mostly looked at the first commit. A few comments, feel free to ignore or address in a follow-up.

in src/bech32.h:25 in 402cbb2951 outdated

24+enum class Encoding {
25+    INVALID, //!< Failed decoding
26 
27-/** Decode a Bech32 string. Returns (hrp, data). Empty hrp means failure. */
28-std::pair<std::string, std::vector<uint8_t>> Decode(const std::string& str);
29+    BECH32,  //!< Bech32 encoding as defined in BIP173

fjahr commented at 6:07 pm on March 14, 2021:

Not necessarily for this PR but we may want to think about renaming this to BECH32Legacy or BECH32v0 or so. In the code right now sometimes “bech32” refers to BIP173 specifically and sometimes to both BIP173 and BIP350. In this line it’s the former case, in the namespace it’s the latter case. I can see people getting confused by that in the future.

sipa commented at 11:11 pm on March 15, 2021:

I don’t think that’s the right solution. The bech32 module is independent of the concept of segwit or bitcoin or addresses or witness versions. If there are other places where the name bech32 is used in a confusing way to include bech32m, feel free to point them out so they can be fixed (in this PR or elsewhere).

fjahr commented at 8:01 pm on March 17, 2021:

Ok, my naming suggestions were just what came to my head at that moment, I did not want to intentionally peg the encoding name to one of its use cases. I just think it’s unfortunate that everything including the BIP173 encoding was called “Bech32” and now there is a new encoding called “Bech32m” that is also a Bech32 string. I think it might be helpful to have a naming distinction between Bech32 in general and the BIP173 encoding. But I also hope this code doesn’t have to change much in the future and if no one else shares that view, I am happy to have it ignored :)

sipa commented at 8:10 pm on March 17, 2021:

there is a new encoding called “Bech32m” that is also a Bech32 string.

That’s wrong. We should fix places in the code where Bech32m is referred to as Bech32. I know of one, addresstype/OutputType::BECH32, which I’m intentionally ignoring in this to minimize code changes (see earlier discussion), but should be fixed still.

ryanofsky commented at 8:45 pm on March 17, 2021:

re: #20861 (review)

The name “bech32m” does maybe seem unfortunate. It might have been nice to chose a completely different name (“fatfinger32, now catching more stupid typos!”) since from a user perspective you can’t use a bech32 address when you need a bech32m address, or vice versa. It’s just behind the scenes that they share an extremely similar implementation, and the “human readable” parts can be interpreted the same way.

sipa commented at 9:17 pm on March 17, 2021:

You are hereby promoted to Official Namer Of Stuff.

ryanofsky commented at 10:46 pm on March 17, 2021:

Nice, so much easier than my other jobs!

fjahr commented at 11:57 pm on March 17, 2021:

That’s wrong. We should fix places in the code where Bech32m is referred to as Bech32.

Ok, re-reading the module now I think this is mostly clear and I was probably bringing in my own preconceived ideas or looking at older code like the comment 2 lines above that was actually removed. I nit-picked through the module and here a few suggestions where this could be clarified further: https://github.com/fjahr/bitcoin/commit/2156afd69e6e5bd587a9d734b21c3e068a037156 but obviously feel free to ignore or keep for a follow-up together with the OutputType maybe.

fjahr commented at 6:09 pm on March 14, 2021: member

tested ACK 92ec3fcdd2f79949839ce269403396362bea9d54

Reviewed code after re-reading BIP350 and the Review Club discussion. Then did some manual testing with address imports and some test mutations.

in src/bech32.h:44 in 92ec3fcdd2 outdated

43+    DecodeResult() : encoding(Encoding::INVALID) {}
44+    DecodeResult(Encoding enc, std::string&& h, std::vector<uint8_t>&& d) : encoding(enc), hrp(std::move(h)), data(std::move(d)) {}
45+};
46+
47+/** Decode a Bech32 string. */
48+DecodeResult Decode(const std::string& str);

jnewbery commented at 9:18 am on March 15, 2021:

The caller can’t do anything with a DecodeResult where encoding is Encoding::INVALID, so changing this return type to std::optional<DecodeResult> and removing the Encoding::INVALID enum type feels more natural to me.

  0diff --git a/src/bech32.cpp b/src/bech32.cpp
  1index 80794ec4f1..2fd84b395b 100644
  2--- a/src/bech32.cpp
  3+++ b/src/bech32.cpp
  4@@ -6,6 +6,7 @@
  5 #include <util/vector.h>
  6 
  7 #include <assert.h>
  8+#include <optional>
  9 
 10 namespace bech32
 11 {
 12@@ -120,7 +121,7 @@ data ExpandHRP(const std::string& hrp)
 13 }
 14 
 15 /** Verify a checksum. */
 16-Encoding VerifyChecksum(const std::string& hrp, const data& values)
 17+std::optional<Encoding> VerifyChecksum(const std::string& hrp, const data& values)
 18 {
 19     // PolyMod computes what value to xor into the final values to make the checksum 0. However,
 20     // if we required that the checksum was 0, it would be the case that appending a 0 to a valid
 21@@ -129,7 +130,7 @@ Encoding VerifyChecksum(const std::string& hrp, const data& values)
 22     uint32_t check = PolyMod(Cat(ExpandHRP(hrp), values));
 23     if (check == EncodingConstant(Encoding::BECH32)) return Encoding::BECH32;
 24     if (check == EncodingConstant(Encoding::BECH32M)) return Encoding::BECH32M;
 25-    return Encoding::INVALID;
 26+    return {};
 27 }
 28 
 29 /** Create a checksum. */
 30@@ -165,7 +166,7 @@ std::string Encode(Encoding encoding, const std::string& hrp, const data& values
 31 }
 32 
 33 /** Decode a Bech32 or Bech32m string. */
 34-DecodeResult Decode(const std::string& str) {
 35+std::optional<DecodeResult> Decode(const std::string& str) {
 36     bool lower = false, upper = false;
 37     for (size_t i = 0; i < str.size(); ++i) {
 38         unsigned char c = str[i];
 39@@ -192,9 +193,9 @@ DecodeResult Decode(const std::string& str) {
 40     for (size_t i = 0; i < pos; ++i) {
 41         hrp += LowerCase(str[i]);
 42     }
 43-    Encoding result = VerifyChecksum(hrp, values);
 44-    if (result == Encoding::INVALID) return {};
 45-    return {result, std::move(hrp), data(values.begin(), values.end() - 6)};
 46+    auto result = VerifyChecksum(hrp, values);
 47+    if (!result) return {};
 48+    return {{*result, std::move(hrp), data(values.begin(), values.end() - 6)}};
 49 }
 50 
 51 } // namespace bech32
 52diff --git a/src/bech32.h b/src/bech32.h
 53index 3679ea8ccb..f2f294bc3f 100644
 54--- a/src/bech32.h
 55+++ b/src/bech32.h
 56@@ -12,6 +12,7 @@
 57 #ifndef BITCOIN_BECH32_H
 58 #define BITCOIN_BECH32_H
 59 
 60+#include <optional>
 61 #include <stdint.h>
 62 #include <string>
 63 #include <vector>
 64@@ -20,8 +21,6 @@ namespace bech32
 65 {
 66 
 67 enum class Encoding {
 68-    INVALID, //!< Failed decoding
 69-
 70     BECH32,  //!< Bech32 encoding as defined in BIP173
 71     BECH32M, //!< Bech32m encoding as defined in BIP350
 72 };
 73@@ -32,16 +31,15 @@ std::string Encode(Encoding encoding, const std::string& hrp, const std::vector<
 74 
 75 struct DecodeResult
 76 {
 77-    Encoding encoding;         //!< What encoding was detected in the result; Encoding::INVALID if failed.
 78+    Encoding encoding;         //!< What encoding was detected in the result
 79     std::string hrp;           //!< The human readable part
 80     std::vector<uint8_t> data; //!< The payload (excluding checksum)
 81 
 82-    DecodeResult() : encoding(Encoding::INVALID) {}
 83     DecodeResult(Encoding enc, std::string&& h, std::vector<uint8_t>&& d) : encoding(enc), hrp(std::move(h)), data(std::move(d)) {}
 84 };
 85 
 86 /** Decode a Bech32 string. */
 87-DecodeResult Decode(const std::string& str);
 88+std::optional<DecodeResult> Decode(const std::string& str);
 89 
 90 } // namespace bech32
 91 
 92diff --git a/src/key_io.cpp b/src/key_io.cpp
 93index dbcbfa1f29..dbb4711d42 100644
 94--- a/src/key_io.cpp
 95+++ b/src/key_io.cpp
 96@@ -96,25 +96,25 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
 97     }
 98     data.clear();
 99     const auto dec = bech32::Decode(str);
100-    if ((dec.encoding == bech32::Encoding::BECH32 || dec.encoding == bech32::Encoding::BECH32M) && dec.data.size() > 0) {
101+    if (dec && dec->data.size() > 0) {
102         // Bech32 decoding
103         error_str = "";
104-        if (dec.hrp != params.Bech32HRP()) {
105+        if (dec->hrp != params.Bech32HRP()) {
106             error_str = "Invalid prefix for Bech32 address";
107             return CNoDestination();
108         }
109-        int version = dec.data[0]; // The first 5 bit symbol is the witness version (0-16)
110-        if (version == 0 && dec.encoding != bech32::Encoding::BECH32) {
111+        int version = dec->data[0]; // The first 5 bit symbol is the witness version (0-16)
112+        if (version == 0 && dec->encoding != bech32::Encoding::BECH32) {
113             error_str = "Version 0 witness address must use Bech32 checksum";
114             return CNoDestination();
115         }
116-        if (version != 0 && dec.encoding != bech32::Encoding::BECH32M) {
117+        if (version != 0 && dec->encoding != bech32::Encoding::BECH32M) {
118             error_str = "Version 1+ witness address must use Bech32m checksum";
119             return CNoDestination();
120         }
121         // The rest of the symbols are converted witness program bytes.
122-        data.reserve(((dec.data.size() - 1) * 5) / 8);
123-        if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec.data.begin() + 1, dec.data.end())) {
124+        data.reserve(((dec->data.size() - 1) * 5) / 8);
125+        if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec->data.begin() + 1, dec->data.end())) {
126             if (version == 0) {
127                 {
128                     WitnessV0KeyHash keyid;
129diff --git a/src/test/bech32_tests.cpp b/src/test/bech32_tests.cpp
130index 2651e46430..835d3a1c79 100644
131--- a/src/test/bech32_tests.cpp
132+++ b/src/test/bech32_tests.cpp
133@@ -23,8 +23,9 @@ BOOST_AUTO_TEST_CASE(bech32_testvectors_valid)
134     };
135     for (const std::string& str : CASES) {
136         const auto dec = bech32::Decode(str);
137-        BOOST_CHECK(dec.encoding == bech32::Encoding::BECH32);
138-        std::string recode = bech32::Encode(bech32::Encoding::BECH32, dec.hrp, dec.data);
139+        BOOST_CHECK(dec);
140+        BOOST_CHECK(dec->encoding == bech32::Encoding::BECH32);
141+        std::string recode = bech32::Encode(bech32::Encoding::BECH32, dec->hrp, dec->data);
142         BOOST_CHECK(!recode.empty());
143         BOOST_CHECK(CaseInsensitiveEqual(str, recode));
144     }
145@@ -43,8 +44,9 @@ BOOST_AUTO_TEST_CASE(bech32m_testvectors_valid)
146     };
147     for (const std::string& str : CASES) {
148         const auto dec = bech32::Decode(str);
149-        BOOST_CHECK(dec.encoding == bech32::Encoding::BECH32M);
150-        std::string recode = bech32::Encode(bech32::Encoding::BECH32M, dec.hrp, dec.data);
151+        BOOST_CHECK(dec);
152+        BOOST_CHECK(dec->encoding == bech32::Encoding::BECH32M);
153+        std::string recode = bech32::Encode(bech32::Encoding::BECH32M, dec->hrp, dec->data);
154         BOOST_CHECK(!recode.empty());
155         BOOST_CHECK(CaseInsensitiveEqual(str, recode));
156     }
157@@ -70,7 +72,7 @@ BOOST_AUTO_TEST_CASE(bech32_testvectors_invalid)
158     };
159     for (const std::string& str : CASES) {
160         const auto dec = bech32::Decode(str);
161-        BOOST_CHECK(dec.encoding == bech32::Encoding::INVALID);
162+        BOOST_CHECK(!dec);
163     }
164 }
165 
166@@ -94,7 +96,7 @@ BOOST_AUTO_TEST_CASE(bech32m_testvectors_invalid)
167     };
168     for (const std::string& str : CASES) {
169         const auto dec = bech32::Decode(str);
170-        BOOST_CHECK(dec.encoding == bech32::Encoding::INVALID);
171+        BOOST_CHECK(!dec);
172     }
173 }
174 
175diff --git a/src/test/fuzz/bech32.cpp b/src/test/fuzz/bech32.cpp
176index 15a3875828..c794fdafc3 100644
177--- a/src/test/fuzz/bech32.cpp
178+++ b/src/test/fuzz/bech32.cpp
179@@ -17,10 +17,8 @@ FUZZ_TARGET(bech32)
180 {
181     const std::string random_string(buffer.begin(), buffer.end());
182     const auto r1 = bech32::Decode(random_string);
183-    if (r1.hrp.empty()) {
184-        assert(r1.data.empty());
185-    } else {
186-        const std::string reencoded = bech32::Encode(r1.encoding, r1.hrp, r1.data);
187+    if (r1) {
188+        const std::string reencoded = bech32::Encode(r1->encoding, r1->hrp, r1->data);
189         assert(CaseInsensitiveEqual(random_string, reencoded));
190     }
191 
192@@ -33,9 +31,10 @@ FUZZ_TARGET(bech32)
193             const std::string encoded = bech32::Encode(encoding, "bc", input);
194             assert(!encoded.empty());
195             const auto r2 = bech32::Decode(encoded);
196-            assert(r2.encoding == encoding);
197-            assert(r2.hrp == "bc");
198-            assert(r2.data == input);
199+            assert(r2);
200+            assert(r2->encoding == encoding);
201+            assert(r2->hrp == "bc");
202+            assert(r2->data == input);
203         }
204     }
205 }

Note that we’ve removed the following check from the fuzz tester:

0-    if (r1.hrp.empty()) {
1-        assert(r1.data.empty());

That inconsistent state can’t exist where DecodeResult only contains a successfully decoded bech32{m} string.

EDIT: I guess you’re not using std::optional so it’s easier to backport to versions that don’t require c++17?

sipa commented at 11:12 pm on March 15, 2021:

@jnewbery Yes, exactly. I considered that, but decided against it, as I prefer to have something maximally backportable.

jnewbery commented at 11:37 am on March 16, 2021:

Thanks. Marking resolved.

in src/bech32.cpp:34 in 92ec3fcdd2 outdated

29@@ -27,6 +30,12 @@ const int8_t CHARSET_REV[128] = {
30      1,  0,  3, 16, 11, 28, 12, 14,  6,  4,  2, -1, -1, -1, -1, -1
31 };
32 
33+/* Determine the final constant to use for the specified encoding. */
34+uint32_t EncodingConstant(Encoding encoding) {

jnewbery commented at 9:27 am on March 15, 2021:

Is there a reason that this isn’t simply a std::map<Encoding, uint32_t>?

This can be constexpred to evaluate this in compile time for the VerifyChecksum() case:

0constexpr uint32_t EncodingConstant(Encoding encoding) {

sipa commented at 11:14 pm on March 15, 2021:

Hmm, a map feels like a lot more heavyweight/overkill to me.

sipa commented at 11:20 pm on March 15, 2021:

Making it constexpr will prevent backporting to pre-C++17 code. It’s also unnecessary (constexpr doesn’t control whether the compiler can evaluate at compile time as an optimization, only whether you can use it in compile-time expressions).

in test/functional/test_framework/segwit_addr.py:45 in 92ec3fcdd2 outdated

43+        return Encoding.BECH32M
44+    else:
45+        return None
46 
47-def bech32_create_checksum(hrp, data):
48+def bech32_create_checksum(hrp, data, spec):

jnewbery commented at 9:34 am on March 15, 2021:

It’s slightly inconsistent that in the c++ code, this is called encoding and is the first argument, and here it’s spec and the last argument.

sipa commented at 0:19 am on March 16, 2021:

Changed the test_framework code to also use (encoding,hrp,data) everywhere.

in test/functional/test_framework/segwit_addr.py:38 in 92ec3fcdd2 outdated

33@@ -27,38 +34,45 @@ def bech32_hrp_expand(hrp):
34 
35 def bech32_verify_checksum(hrp, data):
36     """Verify a checksum given HRP and converted data characters."""
37-    return bech32_polymod(bech32_hrp_expand(hrp) + data) == 1
38-
39+    check = bech32_polymod(bech32_hrp_expand(hrp) + data)
40+    if check == 1:

jnewbery commented at 9:34 am on March 15, 2021:

Perhaps add constant BECH32_CONST = 1

sipa commented at 0:19 am on March 16, 2021:

Done.

jnewbery commented at 9:48 am on March 15, 2021: member

ACK 92ec3fcdd2f79949839ce269403396362bea9d54

A few style comments. Definitely don’t change the branch to take these changes now, but some things to think about once we don’t have to worry about backporting to pre-c++17.

in src/test/fuzz/bech32.cpp:22 in 402cbb2951 outdated

15@@ -16,28 +16,26 @@
16 FUZZ_TARGET(bech32)
17 {
18     const std::string random_string(buffer.begin(), buffer.end());
19-    const std::pair<std::string, std::vector<uint8_t>> r1 = bech32::Decode(random_string);
20-    if (r1.first.empty()) {
21-        assert(r1.second.empty());
22+    const auto r1 = bech32::Decode(random_string);
23+    if (r1.hrp.empty()) {
24+        assert(r1.data.empty());

achow101 commented at 6:39 pm on March 15, 2021:

In 402cbb29516d5bf03a18822af9f1b4b574f319ac “Implement Bech32m encoding/decoding”

It would be nice to also check that the encoding is Encoding::INVALID

sipa commented at 0:19 am on March 16, 2021:

Done.

in contrib/testgen/README.md:8 in 92ec3fcdd2 outdated

 3@@ -4,5 +4,5 @@ Utilities to generate test vectors for the data-driven Bitcoin tests.
 4 
 5 Usage:
 6 
 7-    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py valid 50 > ../../src/test/data/key_io_valid.json
 8-    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py invalid 50 > ../../src/test/data/key_io_invalid.json
 9+    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py valid 70 > ../../src/test/data/key_io_valid.json
10+    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py invalid 70 > ../../src/test/data/key_io_invalid.json

achow101 commented at 7:06 pm on March 15, 2021:

In 92ec3fcdd2f79949839ce269403396362bea9d54 “Add signet support to gen_key_io_test_vectors.py”

There is a similar comment at the top of contrib/testgen/gen_key_io_test_vectors.py which should either be updated or removed. Also that comment says the file is for Base58 addresses when clearly both Base58 and Bech32 things are generated in it.

sipa commented at 0:19 am on March 16, 2021:

Done.

achow101 commented at 7:08 pm on March 15, 2021: member

Just a few additional comments for some things I saw in further review. Not invalidating my previous ACK and these shouldn’t hold up this PR.

sipa force-pushed on Mar 16, 2021

sipa commented at 0:17 am on March 16, 2021: member

Addressed comments. Rebased + made the following changes:

  0diff --git a/contrib/testgen/gen_key_io_test_vectors.py b/contrib/testgen/gen_key_io_test_vectors.py
  1index 6b985b8ec2..2f3189eca9 100755
  2--- a/contrib/testgen/gen_key_io_test_vectors.py
  3+++ b/contrib/testgen/gen_key_io_test_vectors.py
  4@@ -3,11 +3,11 @@
  5 # Distributed under the MIT software license, see the accompanying
  6 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
  7 '''
  8-Generate valid and invalid base58 address and private key test vectors.
  9+Generate valid and invalid base58/bech32(m) address and private key test vectors.
 10 
 11 Usage:
 12-    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py valid 50 > ../../src/test/data/key_io_valid.json
 13-    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py invalid 50 > ../../src/test/data/key_io_invalid.json
 14+    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py valid 70 > ../../src/test/data/key_io_valid.json
 15+    PYTHONPATH=../../test/functional/test_framework ./gen_key_io_test_vectors.py invalid 70 > ../../src/test/data/key_io_invalid.json
 16 '''
 17 # 2012 Wladimir J. van der Laan
 18 # Released under MIT License
 19diff --git a/doc/release-notes-20861.md b/doc/release-notes-20861.md
 20index 2c270f0df5..5c68e4ab0c 100644
 21--- a/doc/release-notes-20861.md
 22+++ b/doc/release-notes-20861.md
 23@@ -7,7 +7,7 @@ Updated RPCs
 24   encoding instead of a Bech32 one, and Bech32m encoding will be used for such
 25   addresses in RPC output as well. No version 1 addresses should be created
 26   for mainnet until consensus rules are adopted that give them meaning
 27-  (e.g. through [BIP 341](https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki)).
 28+  (e.g. through [BIP 341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki)).
 29   Once that happens, Bech32m is expected to be used for them, so this shouldn't
 30-  affect any production system, but may be observed on other networks where such
 31+  affect any production systems, but may be observed on other networks where such
 32   addresses already have meaning (like signet).
 33diff --git a/src/bech32.cpp b/src/bech32.cpp
 34index 80794ec4f1..289e0213e8 100644
 35--- a/src/bech32.cpp
 36+++ b/src/bech32.cpp
 37@@ -126,7 +126,7 @@ Encoding VerifyChecksum(const std::string& hrp, const data& values)
 38     // if we required that the checksum was 0, it would be the case that appending a 0 to a valid
 39     // list of values would result in a new valid list. For that reason, Bech32 requires the
 40     // resulting checksum to be 1 instead. In Bech32m, this constant was amended.
 41-    uint32_t check = PolyMod(Cat(ExpandHRP(hrp), values));
 42+    const uint32_t check = PolyMod(Cat(ExpandHRP(hrp), values));
 43     if (check == EncodingConstant(Encoding::BECH32)) return Encoding::BECH32;
 44     if (check == EncodingConstant(Encoding::BECH32M)) return Encoding::BECH32M;
 45     return Encoding::INVALID;
 46diff --git a/src/test/fuzz/bech32.cpp b/src/test/fuzz/bech32.cpp
 47index 15a3875828..63f05c4474 100644
 48--- a/src/test/fuzz/bech32.cpp
 49+++ b/src/test/fuzz/bech32.cpp
 50@@ -19,9 +19,11 @@ FUZZ_TARGET(bech32)
 51     const auto r1 = bech32::Decode(random_string);
 52     if (r1.hrp.empty()) {
 53         assert(r1.data.empty());
 54+        assert(r1.encoding == Encoding::INVALID);
 55     } else {
 56         const std::string reencoded = bech32::Encode(r1.encoding, r1.hrp, r1.data);
 57         assert(CaseInsensitiveEqual(random_string, reencoded));
 58+        assert(r1.encoding != Encoding::INVALID);
 59     }
 60 
 61     std::vector<unsigned char> input;
 62diff --git a/test/functional/test_framework/segwit_addr.py b/test/functional/test_framework/segwit_addr.py
 63index 78b175a69e..3c3d242ad2 100644
 64--- a/test/functional/test_framework/segwit_addr.py
 65+++ b/test/functional/test_framework/segwit_addr.py
 66@@ -7,6 +7,7 @@ import unittest
 67 from enum import Enum
 68 
 69 CHARSET = "qpzry9x8gf2tvdw0s3jn54khce6mua7l"
 70+BECH32_CONST = 1
 71 BECH32M_CONST = 0x2bc830a3
 72 
 73 class Encoding(Enum):
 74@@ -35,24 +36,24 @@ def bech32_hrp_expand(hrp):
 75 def bech32_verify_checksum(hrp, data):
 76     """Verify a checksum given HRP and converted data characters."""
 77     check = bech32_polymod(bech32_hrp_expand(hrp) + data)
 78-    if check == 1:
 79+    if check == BECH32_CONST:
 80         return Encoding.BECH32
 81     elif check == BECH32M_CONST:
 82         return Encoding.BECH32M
 83     else:
 84         return None
 85 
 86-def bech32_create_checksum(hrp, data, spec):
 87+def bech32_create_checksum(encoding, hrp, data):
 88     """Compute the checksum values given HRP and data."""
 89     values = bech32_hrp_expand(hrp) + data
 90-    const = BECH32M_CONST if spec == Encoding.BECH32M else 1
 91+    const = BECH32M_CONST if encoding == Encoding.BECH32M else 1
 92     polymod = bech32_polymod(values + [0, 0, 0, 0, 0, 0]) ^ const
 93     return [(polymod >> 5 * (5 - i)) & 31 for i in range(6)]
 94 
 95 
 96-def bech32_encode(hrp, data, spec):
 97+def bech32_encode(encoding, hrp, data):
 98     """Compute a Bech32 or Bech32m string given HRP and data values."""
 99-    combined = data + bech32_create_checksum(hrp, data, spec)
100+    combined = data + bech32_create_checksum(encoding, hrp, data)
101     return hrp + '1' + ''.join([CHARSET[d] for d in combined])
102 
103 
104@@ -69,10 +70,10 @@ def bech32_decode(bech):
105         return (None, None, None)
106     hrp = bech[:pos]
107     data = [CHARSET.find(x) for x in bech[pos+1:]]
108-    spec = bech32_verify_checksum(hrp, data)
109-    if spec is None:
110+    encoding = bech32_verify_checksum(hrp, data)
111+    if encoding is None:
112         return (None, None, None)
113-    return (hrp, data[:-6], spec)
114+    return (encoding, hrp, data[:-6])
115 
116 
117 def convertbits(data, frombits, tobits, pad=True):
118@@ -100,7 +101,7 @@ def convertbits(data, frombits, tobits, pad=True):
119 
120 def decode_segwit_address(hrp, addr):
121     """Decode a segwit address."""
122-    hrpgot, data, spec = bech32_decode(addr)
123+    encoding, hrpgot, data = bech32_decode(addr)
124     if hrpgot != hrp:
125         return (None, None)
126     decoded = convertbits(data[1:], 5, 8, False)
127@@ -110,15 +111,15 @@ def decode_segwit_address(hrp, addr):
128         return (None, None)
129     if data[0] == 0 and len(decoded) != 20 and len(decoded) != 32:
130         return (None, None)
131-    if (data[0] == 0 and spec != Encoding.BECH32) or (data[0] != 0 and spec != Encoding.BECH32M):
132+    if (data[0] == 0 and encoding != Encoding.BECH32) or (data[0] != 0 and encoding != Encoding.BECH32M):
133         return (None, None)
134     return (data[0], decoded)
135 
136 
137 def encode_segwit_address(hrp, witver, witprog):
138     """Encode a segwit address."""
139-    spec = Encoding.BECH32 if witver == 0 else Encoding.BECH32M
140-    ret = bech32_encode(hrp, [witver] + convertbits(witprog, 8, 5), spec)
141+    encoding = Encoding.BECH32 if witver == 0 else Encoding.BECH32M
142+    ret = bech32_encode(encoding, hrp, [witver] + convertbits(witprog, 8, 5))
143     if decode_segwit_address(hrp, ret) == (None, None):
144         return None
145     return ret

Implement Bech32m encoding/decoding da2bb6976d

Add Bech32m test vectors 25b1c6e13d

sipa force-pushed on Mar 16, 2021

Sjors commented at 10:15 am on March 16, 2021: member

re-utACK c85b2fce57a49133ec652a3ec95d67511cde311c

in test/functional/test_framework/segwit_addr.py:49 in c85b2fce57 outdated

48-def bech32_create_checksum(hrp, data):
49+def bech32_create_checksum(encoding, hrp, data):
50     """Compute the checksum values given HRP and data."""
51     values = bech32_hrp_expand(hrp) + data
52-    polymod = bech32_polymod(values + [0, 0, 0, 0, 0, 0]) ^ 1
53+    const = BECH32M_CONST if encoding == Encoding.BECH32M else 1

jnewbery commented at 11:50 am on March 16, 2021:

0    const = BECH32M_CONST if encoding == Encoding.BECH32M else BECH32_CONST

sipa commented at 5:49 pm on March 16, 2021:

Done.

jnewbery commented at 11:52 am on March 16, 2021: member

utACK c85b2fce57a49133ec652a3ec95d67511cde311c

Verified range-diff from 92ec3fcdd2

gruve-p commented at 12:07 pm on March 16, 2021: contributor

re-utACK https://github.com/bitcoin/bitcoin/pull/20861/commits/c85b2fce57a49133ec652a3ec95d67511cde311c

Use Bech32m encoding for v1+ segwit addresses

This also includes updates to the Python test framework implementation,
test vectors, and release notes.

fe5e495c31

Add signet support to gen_key_io_test_vectors.py 2e7c80fb5b

sipa force-pushed on Mar 16, 2021

jnewbery commented at 6:26 pm on March 16, 2021: member

utACK 2e7c80fb5be82ad4a3f737cab65b31f70a772a23

Sjors commented at 6:59 pm on March 16, 2021: member

re-utACK 2e7c80fb5be82ad4a3f737cab65b31f70a772a23

achow101 commented at 8:05 pm on March 16, 2021: member

ACK 2e7c80fb5be82ad4a3f737cab65b31f70a772a23

fjahr commented at 9:51 pm on March 16, 2021: member

re-ACK 2e7c80fb5be82ad4a3f737cab65b31f70a772a23

Check range-diff since last review.

SomberNight referenced this in commit 4315fa4371 on Mar 17, 2021

naming nits 03346022d6

sipa commented at 1:00 am on March 18, 2021: member

@fjahr Included your naming nits commit.

Sjors commented at 8:05 am on March 18, 2021: member

utACK 0334602

Apologies for the meta naming nit, but let’s call that commit bech32m: naming nits? :-)

jnewbery commented at 11:11 am on March 18, 2021: member

utACK 03346022d6

This seems ready for merge (and had 4 ACKs prior to the last push). Can we settle on this and resolve any additional style comments in follow-up PRs?

achow101 commented at 7:05 pm on March 18, 2021: member

ACK 0334602

fjahr commented at 7:05 pm on March 18, 2021: member

re-ACK 0334602

benthecarman approved

benthecarman commented at 7:37 pm on March 18, 2021: contributor

ACK 03346022d611871f2cc185440b19d928b9264d9d

laanwj merged this on Mar 18, 2021

laanwj closed this on Mar 18, 2021

sipa referenced this in commit bfeae19699 on Mar 18, 2021

sipa referenced this in commit a815f2d615 on Mar 18, 2021

sipa referenced this in commit 9bbd52e14a on Mar 18, 2021

sipa referenced this in commit 89f5230166 on Mar 18, 2021

sipa referenced this in commit d520f73d10 on Mar 18, 2021

sipa referenced this in commit 5f9537b2d9 on Mar 18, 2021

sipa referenced this in commit cf18ac9665 on Mar 18, 2021

sipa referenced this in commit d7aba29077 on Mar 18, 2021

sipa referenced this in commit 3ce09ce4c8 on Mar 18, 2021

sipa referenced this in commit d1b1f61303 on Mar 18, 2021

sipa referenced this in commit 7d8b5f564a on Mar 18, 2021

sipa referenced this in commit 28b3a32060 on Mar 18, 2021

sipa referenced this in commit 2d0a8bc59a on Mar 18, 2021

sipa referenced this in commit b51f67398a on Mar 18, 2021

sipa referenced this in commit 47d4601344 on Mar 18, 2021

sipa referenced this in commit 0dd36c6c61 on Mar 18, 2021

fanquake referenced this in commit 47d79c941a on Mar 19, 2021

sidhujag referenced this in commit 752dba1890 on Mar 19, 2021

in src/test/fuzz/bech32.cpp:32 in 03346022d6

43-    } else {
44-        const std::string& hrp = r2.first;
45-        const std::vector<uint8_t>& data = r2.second;
46-        assert(hrp == "bc");
47-        assert(data == input);
48+    if (input.size() + 3 + 6 <= 90) {

MarcoFalke commented at 7:13 pm on March 19, 2021:

nit: Any reason to remove the branch that checks bech32 for larger input sizes? I know this isn’t relevant for bitcoin addresses, but there are other applications that use bech32 for encoding and checking it here doesn’t cost us anything, no?

sipa commented at 7:21 pm on March 19, 2021:

Bech32 and Bech32m do not permit encoded strings over 90 characters (the checksum properties break down past that point). BOLT11 uses a relaxed version of the spec that drops this requirement.

in test/functional/wallet_labels.py:147 in 03346022d6

147         }
148         BECH32_INVALID = {
149-            '❌_VER15_PROG41': 'bcrt10qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqzc7xyq',
150-            '❌_VER16_PROB01': 'bcrt1sqqpl9r5c',
151+            '❌_VER15_PROG41': 'bcrt1sqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqajlxj8',
152+            '❌_VER16_PROB01': 'bcrt1sqq5r4036',

MarcoFalke commented at 7:14 pm on March 19, 2021:

nit: obviously unrelated to your changes, but the B should be a G. (blame me)

in src/bench/bech32.cpp:22 in 03346022d6

18@@ -19,7 +19,7 @@ static void Bech32Encode(benchmark::Bench& bench)
19     tmp.reserve(1 + 32 * 8 / 5);
20     ConvertBits<8, 5, true>([&](unsigned char c) { tmp.push_back(c); }, v.begin(), v.end());
21     bench.batch(v.size()).unit("byte").run([&] {
22-        bech32::Encode("bc", tmp);
23+        bech32::Encode(bech32::Encoding::BECH32, "bc", tmp);

MarcoFalke commented at 7:15 pm on March 19, 2021:

nit: Could add a benchmark for bech32m?

sipa commented at 7:21 pm on March 19, 2021:

Yeah, could do. The performance should be exactly identical to Bech32 though.

MarcoFalke commented at 7:15 pm on March 19, 2021: member

left some minor nits

fanquake removed this from the "Blockers" column in a project

sipa referenced this in commit ce14a52285 on Mar 24, 2021

sipa referenced this in commit 66009fc45a on Mar 27, 2021

sipa referenced this in commit 26347db408 on Mar 27, 2021

sipa referenced this in commit 1a4e88e0e8 on Mar 27, 2021

sipa referenced this in commit c6709867d3 on Mar 27, 2021

sipa referenced this in commit c0f85fd850 on Mar 27, 2021

sipa referenced this in commit 1485533092 on Mar 27, 2021

sipa referenced this in commit 8944aaa6d6 on Mar 27, 2021

sipa referenced this in commit 593e206627 on Mar 27, 2021

sipa referenced this in commit 7dfe406e20 on Mar 27, 2021

sipa referenced this in commit 1e9671116f on Mar 27, 2021

sipa referenced this in commit f2195d7c4a on Mar 27, 2021

MarcoFalke referenced this in commit 65fa43bda1 on Apr 1, 2021

laanwj referenced this in commit 56311988bf on Apr 23, 2021

apoelstra referenced this in commit 114297611a on Aug 5, 2021

apoelstra referenced this in commit 44dff10c22 on Aug 5, 2021

apoelstra referenced this in commit a4e778cd3a on Aug 5, 2021

apoelstra referenced this in commit b1d1d94e01 on Aug 18, 2021

apoelstra referenced this in commit 42f43a1bcb on Aug 18, 2021

apoelstra referenced this in commit b3df66f82e on Aug 18, 2021

apoelstra referenced this in commit 64194498cf on Aug 30, 2021

DrahtBot locked this on Aug 18, 2022

BIP 350: Implement Bech32m and use it for v1+ segwit addresses #20861

Conflicts