Full BIP173 (Bech32) support by sipa · Pull Request #11167 · bitcoin/bitcoin

sipa · 2017-08-26T22:18:27Z

Builds on top of #11117.

This adds support for:

Creating BIP173 addresses for testing (through addwitnessaddress, though by default it still produces P2SH versions)
Sending to BIP173 addresses (including non-v0 ones)
Analysing BIP173 addresses (through validateaddress)

It includes a reformatted version of the C++ Bech32 reference code and an independent implementation of the address encoding/decoding logic (integrated with CTxDestination). All BIP173 test vectors are included.

Not included (and intended for other PRs):

Full wallet support for SegWit (which would include automatically adding witness scripts to the wallet during automatic keypool topup, SegWit change outputs, ...) [see SegWit wallet support #11403]
Splitting base58.cpp and tests/base58_tests.cpp up into base58-specific code, and "address encoding"-code [see Address encoding cleanup #11372]
Error locating in UI for BIP173 addresses.

luke-jr · 2017-08-26T22:37:47Z

Would prefer to have simply sending-to (maybe validating/analyzing too?) as a separate PR, before wallet upgrades.

I'm not sure when it would make sense to convert between P2SH and BIP173...

sipa · 2017-08-26T22:38:57Z

@luke-jr I agree, but I consider addwitnessaddress an RPC to aid with testing, not full support.

I'm not sure when it would make sense to convert between P2SH and BIP173...

I think you're right. I'll remove that.

gmaxwell · 2017-08-26T22:45:33Z

addwitnessaddress is very much not actual support, it's a test shim.

luke-jr · 2017-08-26T22:58:29Z

But it modifies the wallet, no? Seems useful to review independently from the rest. Especially since it has the additional considerations of what happens if you try to use it and then downgrade to an older version...

sipa · 2017-08-26T23:00:39Z

@luke-jr Consider that we've since 0.13.1 had support for receiving and spending native witness outputs in the wallet (without that, testing the consensus logic for it would have been much harder), just no way to encode such outputs as strings. So I think the encoding is somewhat orthogonal.

It does modify the wallet, but I'm not sure it's worth trying to separate the logic. We only have one data type (CTxDestination) to encode things we can receive on or send to. Having the ability to send to something but not being able to encode it ourselves would require separating the two, and I expect would be more work then just implementing it all.

Wallet backward compatibility is only affected when you use an import or addwitnessaddress with p2sh=false (the default is `true).

sipa · 2017-08-27T07:49:28Z

Added support in Python framework, and some integrated some functional tests into the segwit.py test.

promag

Fast code review. Move 4bc71a6 to #11117??

promag · 2017-08-27T13:12:49Z

src/bech32.cpp

IMO bool Decode(const std::string& str, const std::string& hrp, data& d) feels better, and this way below it can early return.

It's not often used in the Bitcoin Core codebase, but using pairs for multiple returned values is very typical in C++ (see the return type of std::map::insert for example). In C++03 it was a bit verbose to use, but with C++11's auto types and std::tie for assigning to multiple variables, it's pretty convenient. I'd rather stick with the current approach.

promag · 2017-08-27T13:16:30Z

src/base58.cpp

Return in new line?

promag · 2017-08-27T13:17:36Z

src/base58.cpp

promag · 2017-08-27T13:19:25Z

src/bech32.h

promag · 2017-08-27T13:22:52Z

src/base58.cpp

Must come first? If not which is the cheapest?

bech32 is far cheaper (no basis conversion, no SHA256).

promag · 2017-08-27T13:23:35Z

src/base58.cpp

Must come first? If not which is the cheapest?

I haven't benchmarked, but Bech32 should be far cheaper (no SHA256, no basis conversion). There should never be overlap between the addresses, so the order shouldn't matter.

Right, in terms of functionality the order doesn't matter. But at the moment most addresses (don't know numbers) are base58 so for now move bech32::Decode() after DecodeBase58Check()?

It would be cool to move this out of base58.cpp, follow up maybe?

Right, in terms of functionality the order doesn't matter. But at the moment most addresses (don't know numbers) are base58 so for now move bech32::Decode() after DecodeBase58Check()?

I was using a fail-fast approach, making the thing that most quickly fails first. You're right that as long as there are hardly any bech32 addresses, putting Base58 would be overall faster. But none of this is performance critical anyway...

sipa · 2017-08-27T20:11:53Z

src/chainparams.cpp

I arbitrarily chose a bech32 prefix for regtest. Feel free to bikeshed (it doesn't even need to be just 2 characters).

promag · 2017-08-28T10:30:56Z

src/bech32.cpp

Early return?

promag · 2017-08-28T10:31:29Z

src/bech32.cpp

Early return?

promag · 2017-08-28T10:32:14Z

src/bech32.cpp

Early return?

jimpo · 2017-09-01T23:28:55Z

src/bech32.h

Could you leave a comment referring the reader to more complete documentation on bech32 (either the BIP or reference repo)? Also would be nice to document somewhere in the codebase what hrp stands for.

I'm thinking something as simple as: "Bech32 is a data encoding used for some newer address formats. Output consists of a human-readable part (HRP) followed by a separator, then the data itself with a checksum. For more details, see documentation of BIP 173."

Done again - I somehow lost this change.

I don't see the comment... I think you lost it again.

It's there, just a bit higher up in the file.

jimpo · 2017-09-03T18:55:24Z

src/test/base58_tests.cpp

nit: Maybe use toupper here to be more descriptive and succinct.

Unfortunately, toupper is locale-dependent, so it can't be used for consistent behaviour.

std::toupper(c, std::locale::classic())?

Seems overkill.

instagibbs · 2017-09-05T23:22:48Z

Include a test with v1+ address?

92f1f8b Split off key_io_tests from base58_tests (Pieter Wuille) 119b0f8 Split key_io (address/key encodings) off from base58 (Pieter Wuille) ebfe217 Stop using CBase58Data for ext keys (Pieter Wuille) 32e69fa Replace CBitcoinSecret with {Encode,Decode}Secret (Pieter Wuille) Pull request description: This PR contains some of the changes left as TODO in #11167 (and built on top of that PR). They are not intended for backporting. This removes the `CBase58`, `CBitcoinSecret`, `CBitcoinExtKey`, and `CBitcoinExtPubKey` classes, in favor of simple `Encode`/`Decode` functions. Furthermore, all Bitcoin-specific logic (addresses, WIF, BIP32) is moved to `key_io.{h,cpp}`, leaving `base58.{h,cpp}` as a pure utility that implements the base58 encoding/decoding logic. Tree-SHA512: a5962c0ed27ad53cbe00f22af432cf11aa530e3efc9798e25c004bc9ed1b5673db5df3956e398ee2c085e3a136ac8da69fe7a7d97a05fb2eb3be0b60d0479655

Bech32 encoding support and t-addr encoding refactor Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7922 - bitcoin/bitcoin#7825 - bitcoin/bitcoin#8317 - bitcoin/bitcoin#9804 - Only the commit that changed `base58.cpp` - bitcoin/bitcoin#11117 - bitcoin/bitcoin#11259 - bitcoin/bitcoin#11167 - Only the first three commits (the fourth commit depends on #2390, later ones are SegWit-specific). Part of #3058.

Refactor t-address encoding Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#11117 - bitcoin/bitcoin#11259 - Only the second commit (first is for QT code) - bitcoin/bitcoin#11167 - Only the first commit (the rest are not part of the t-address encoding refactor). Part of #3058. Precursor to #3202.

bitcoin/bitcoin#11167

Bech32 encoding support Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#8578 - bitcoin/bitcoin#11167 - Only the second and third commits (first is in #3228, fourth depends on #2390, later ones are SegWit-specific). - bitcoin/bitcoin#12757 - Only the change to `src/bech32.h` Part of #3058.

Function extracted from upstream: PR bitcoin/bitcoin#11167 Commit c091b99

Sapling address encodings This PR enables Sapling keys and addresses to be passed in anywhere Sprout keys and addresses are used. Doing so will cause crashes until those places are updated with Sapling support. Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#11167 - bitcoin/bitcoin#11630 Closes #3058.

Sapling address encodings This PR enables Sapling keys and addresses to be passed in anywhere Sprout keys and addresses are used. Doing so will cause crashes until those places are updated with Sapling support. Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#11167 - Only the `ConvertBits()` function. - bitcoin/bitcoin#11630 Closes #3058.

sipa force-pushed the 201708_bech32 branch from 5c42247 to 433389b Compare August 26, 2017 22:24

sipa force-pushed the 201708_bech32 branch from 433389b to 47c4421 Compare August 26, 2017 22:46

sipa force-pushed the 201708_bech32 branch from 47c4421 to 6128648 Compare August 27, 2017 05:26

FelixWeis referenced this pull request in richardkiss/pycoin Aug 27, 2017

Fix comment.

65ac986

promag reviewed Aug 27, 2017

View reviewed changes

sipa force-pushed the 201708_bech32 branch from ab8d509 to 7b7f7e9 Compare August 27, 2017 17:12

sipa mentioned this pull request Aug 27, 2017

Prepare for non-Base58 addresses #11117

Merged

sipa force-pushed the 201708_bech32 branch from 7b7f7e9 to 5d9dbc1 Compare August 27, 2017 17:39

sipa commented Aug 27, 2017

View reviewed changes

sipa force-pushed the 201708_bech32 branch from 5d9dbc1 to bfada9e Compare August 28, 2017 00:02

laanwj added the Wallet label Aug 28, 2017

promag reviewed Aug 28, 2017

View reviewed changes

sipa force-pushed the 201708_bech32 branch from bfada9e to ab8942c Compare August 29, 2017 00:57

jimpo reviewed Sep 3, 2017

View reviewed changes

sipa force-pushed the 201708_bech32 branch 3 times, most recently from d7dd8a5 to f77636b Compare September 3, 2017 20:14

sipa added Needs backport Needs release note labels Sep 5, 2017

sipa added this to the 0.15.1 milestone Sep 5, 2017

sipa force-pushed the 201708_bech32 branch from f77636b to 4a82181 Compare September 5, 2017 18:57

meshcollider mentioned this pull request Jan 17, 2018

TODO for release notes 0.16.0 #11054

Closed

str4d mentioned this pull request Mar 6, 2018

Implement Sapling Bech32 address format zcash/zcash#3058

Closed

str4d mentioned this pull request Apr 23, 2018

Bech32 encoding support zcash/zcash#3202

Merged

str4d mentioned this pull request May 1, 2018

Refactor t-address encoding zcash/zcash#3228

Merged

str4d mentioned this pull request May 4, 2018

Key encoding refactor zcash/zcash#3242

Merged

mkjekk added a commit to mkjekk/zcash that referenced this pull request May 4, 2018

https://github.com/bitcoin/bitcoin/pull/11167

3d3904f

bitcoin/bitcoin#11167

str4d added a commit to str4d/zcash that referenced this pull request Jun 7, 2018

ConvertBits() - convert from one power-of-2 number base to another.

6b759fb

Function extracted from upstream: PR bitcoin/bitcoin#11167 Commit c091b99

str4d mentioned this pull request Jun 7, 2018

Sapling address encodings zcash/zcash#3326

Merged

underdarkskies mentioned this pull request Jul 3, 2018

Extended RPC to support Insight-API RavenProject/Ravencoin#106

Closed

underdarkskies mentioned this pull request Jul 18, 2018

Insight final RavenProject/Ravencoin#131

Merged

UdjinM6 mentioned this pull request Dec 25, 2020

Backport 11167 (partial), 11372, 11630 and 14242 dashpay/dash#3903

Merged

Conversation

sipa commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Aug 26, 2017

Uh oh!

sipa commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Aug 26, 2017

Uh oh!

luke-jr commented Aug 26, 2017

Uh oh!

sipa commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Aug 27, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sipa commented Aug 26, 2017 •

edited

Loading

sipa commented Aug 26, 2017 •

edited

Loading

sipa commented Aug 26, 2017 •

edited

Loading

sipa Aug 27, 2017 •

edited

Loading

sipa Aug 27, 2017 •

edited

Loading

sipa Sep 15, 2017 •

edited

Loading

jimpo Sep 3, 2017 •

edited

Loading