Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full BIP173 (Bech32) support #11167

Merged
merged 9 commits into from Sep 29, 2017

Conversation

@sipa
Copy link
Member

sipa commented Aug 26, 2017

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 #11403]
  • Splitting base58.cpp and tests/base58_tests.cpp up into base58-specific code, and "address encoding"-code [see #11372]
  • Error locating in UI for BIP173 addresses.

@sipa sipa force-pushed the sipa:201708_bech32 branch Aug 26, 2017

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 26, 2017

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

This comment has been minimized.

Copy link
Member Author

sipa commented Aug 26, 2017

@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

This comment has been minimized.

Copy link
Member

gmaxwell commented Aug 26, 2017

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

@sipa sipa force-pushed the sipa:201708_bech32 branch Aug 26, 2017

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 26, 2017

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

This comment has been minimized.

Copy link
Member Author

sipa commented Aug 26, 2017

@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 sipa force-pushed the sipa:201708_bech32 branch Aug 27, 2017

@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Aug 27, 2017

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

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

@promag
Copy link
Member

promag left a comment

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

}

/** Decode a Bech32 string. */
std::pair<std::string, data> Decode(const std::string& str) {

This comment has been minimized.

@promag

promag Aug 27, 2017

Member

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

This comment has been minimized.

@sipa

sipa Aug 27, 2017

Author Member

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.

src/base58.cpp Outdated
}
return CNoDestination();
}
if (version > 16 || data.size() < 2 || data.size() > 40) return CNoDestination();

This comment has been minimized.

@promag

promag Aug 27, 2017

Member

Return in new line?

This comment has been minimized.

@sipa

sipa Aug 27, 2017

Author Member

done

src/base58.cpp Outdated

#include <assert.h>
#include <stdint.h>
#include <string.h>
#include <vector>
#include <string>
#include <algorithm>

This comment has been minimized.

@promag

promag Aug 27, 2017

Member

Sort.

This comment has been minimized.

@sipa

sipa Aug 27, 2017

Author Member

Done.

src/bech32.h Outdated
*/

#include <stdint.h>
#include <vector>

This comment has been minimized.

@promag

promag Aug 27, 2017

Member

Sort.

This comment has been minimized.

@sipa

sipa Aug 27, 2017

Author Member

Done.

src/base58.cpp Outdated
std::string operator()(const CNoDestination& no) const { return ""; }
};

CTxDestination DecodeDestination(const std::string& str, const CChainParams& params)
{
std::vector<unsigned char> data;
uint160 hash;
auto bech = bech32::Decode(str);

This comment has been minimized.

@promag

promag Aug 27, 2017

Member

Must come first? If not which is the cheapest?

This comment has been minimized.

@sipa

sipa Aug 27, 2017

Author Member

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

src/base58.cpp Outdated
else
std::vector<unsigned char> data;
uint160 hash;
auto bech = bech32::Decode(str);

This comment has been minimized.

@promag

promag Aug 27, 2017

Member

Must come first? If not which is the cheapest?

This comment has been minimized.

@sipa

sipa Aug 27, 2017

Author Member

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.

This comment has been minimized.

@promag

promag Aug 28, 2017

Member

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?

This comment has been minimized.

@sipa

sipa Aug 28, 2017

Author Member

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 sipa force-pushed the sipa:201708_bech32 branch Aug 27, 2017

@sipa sipa force-pushed the sipa:201708_bech32 branch Aug 27, 2017

src/chainparams.cpp Outdated
@@ -330,6 +334,8 @@ class CRegTestParams : public CChainParams {
base58Prefixes[SECRET_KEY] = std::vector<unsigned char>(1,239);
base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};

bech32_hrp = "rt";

This comment has been minimized.

@sipa

sipa Aug 27, 2017

Author Member

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

@sipa sipa force-pushed the sipa:201708_bech32 branch Aug 28, 2017

@laanwj laanwj added the Wallet label Aug 28, 2017

src/bech32.cpp Outdated
bool ok = true;
for (size_t i = 0; ok && i < str.size(); ++i) {
unsigned char c = str[i];
if (c < 33 || c > 126) ok = false;

This comment has been minimized.

@promag

promag Aug 28, 2017

Member

Early return?

This comment has been minimized.

@sipa

sipa Aug 29, 2017

Author Member

Done.

src/bech32.cpp Outdated
if (c >= 'a' && c <= 'z') lower = true;
if (c >= 'A' && c <= 'Z') upper = true;
}
if (lower && upper) ok = false;

This comment has been minimized.

@promag

promag Aug 28, 2017

Member

Early return?

This comment has been minimized.

@sipa

sipa Aug 29, 2017

Author Member

Done.

src/bech32.cpp Outdated
values.resize(str.size() - 1 - pos);
for (size_t i = 0; i < str.size() - 1 - pos; ++i) {
unsigned char c = str[i + pos + 1];
if (CHARSET_REV[c] == -1) ok = false;

This comment has been minimized.

@promag

promag Aug 28, 2017

Member

Early return?

This comment has been minimized.

@sipa

sipa Aug 29, 2017

Author Member

Done.

@sipa sipa force-pushed the sipa:201708_bech32 branch Aug 29, 2017

#include <string>
#include <vector>

namespace bech32

This comment has been minimized.

@jimpo

jimpo Sep 3, 2017

Contributor

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."

This comment has been minimized.

@sipa

sipa Sep 3, 2017

Author Member

Done.

This comment has been minimized.

@sipa

sipa Sep 9, 2017

Author Member

Done again - I somehow lost this change.

This comment has been minimized.

@achow101

achow101 Sep 15, 2017

Member

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

This comment has been minimized.

@sipa

sipa Sep 15, 2017

Author Member

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

src/test/base58_tests.cpp Outdated
BOOST_CHECK_MESSAGE(dest == dest2, strprintf("mismatch in encoding: %s", strTest));

for (char& c : address) {
if (c >= 'a' && c <= 'z') {

This comment has been minimized.

@jimpo

jimpo Sep 3, 2017

Contributor

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

This comment has been minimized.

@sipa

sipa Sep 3, 2017

Author Member

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

This comment has been minimized.

@jimpo

jimpo Sep 3, 2017

Contributor

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

This comment has been minimized.

@sipa

sipa Sep 3, 2017

Author Member

Seems overkill.

@sipa sipa force-pushed the sipa:201708_bech32 branch 3 times, most recently Sep 3, 2017

@sipa sipa added this to the 0.15.1 milestone Sep 5, 2017

@sipa sipa force-pushed the sipa:201708_bech32 branch Sep 5, 2017

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Sep 5, 2017

Include a test with v1+ address?

zkbot added a commit to zcash/zcash that referenced this pull request Apr 25, 2018

Auto merge of #3202 - str4d:3058-sapling-bech32, r=<try>
Bech32 encoding support and address encoding refactor

Includes code 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).
- bitcoin/bitcoin#8578
- bitcoin/bitcoin#11372
  - Only the first three commits (the fourth commit depends on #2390)

Part of #3058.

@str4d str4d referenced this pull request May 1, 2018

Merged

Refactor t-address encoding #3228

zkbot added a commit to zcash/zcash that referenced this pull request May 3, 2018

Auto merge of #3228 - str4d:3058-taddr-encoding-refactor, r=<try>
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.

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

Auto merge of #3228 - str4d:3058-taddr-encoding-refactor, r=str4d
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.

@str4d str4d referenced this pull request May 4, 2018

Merged

Key encoding refactor #3242

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

zkbot added a commit to zcash/zcash that referenced this pull request May 7, 2018

Auto merge of #3202 - str4d:3058-sapling-bech32, r=<try>
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.

zkbot added a commit to zcash/zcash that referenced this pull request May 7, 2018

Auto merge of #3202 - str4d:3058-sapling-bech32, r=str4d
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.

zkbot added a commit to zcash/zcash that referenced this pull request May 8, 2018

Auto merge of #3202 - str4d:3058-sapling-bech32, r=str4d
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.

zkbot added a commit to zcash/zcash that referenced this pull request May 8, 2018

Auto merge of #3202 - str4d:3058-sapling-bech32, r=str4d
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.

zkbot added a commit to zcash/zcash that referenced this pull request May 8, 2018

Auto merge of #3202 - str4d:3058-sapling-bech32, r=str4d
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.

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.
Function extracted from upstream:
  PR bitcoin/bitcoin#11167
  Commit c091b99379b97cb314c9fa123beabdbc324cf7a4

@str4d str4d referenced this pull request Jun 7, 2018

Merged

Sapling address encodings #3326

zkbot added a commit to zcash/zcash that referenced this pull request Jun 11, 2018

Auto merge of #3326 - str4d:3058-sapling-addresses, r=<try>
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.

zkbot added a commit to zcash/zcash that referenced this pull request Jun 19, 2018

Auto merge of #3326 - str4d:3058-sapling-addresses, r=str4d
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.

arcalinea pushed a commit to arcalinea/zcash that referenced this pull request Jul 5, 2018

ConvertBits() - convert from one power-of-2 number base to another.
Function extracted from upstream:
  PR bitcoin/bitcoin#11167
  Commit c091b99379b97cb314c9fa123beabdbc324cf7a4

underdarkskies referenced this pull request in underdarkskies/Ravencoin Jul 14, 2018

Revert "Merge #11167: Full BIP173 (Bech32) support"
This reverts commit aa624b6, reversing
changes made to a72003d.

underdarkskies referenced this pull request in underdarkskies/Ravencoin Jul 14, 2018

Revert "Merge #11167: Full BIP173 (Bech32) support"
This reverts commit aa624b6, reversing
changes made to a72003d.

underdarkskies referenced this pull request in underdarkskies/Ravencoin Jul 18, 2018

Revert "Merge #11167: Full BIP173 (Bech32) support"
This reverts commit aa624b6, reversing
changes made to a72003d.

@underdarkskies underdarkskies referenced this pull request Jul 18, 2018

Merged

Insight final #131

underdarkskies referenced this pull request in underdarkskies/Ravencoin Aug 1, 2018

Revert "Merge #11167: Full BIP173 (Bech32) support"
This reverts commit aa624b6, reversing
changes made to a72003d.

mkjekk added a commit to mkjekk/zcash that referenced this pull request Aug 12, 2018

Auto merge of zcash#3228 - str4d:3058-taddr-encoding-refactor, r=str4d
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 zcash#3058. Precursor to zcash#3202.

mkjekk added a commit to mkjekk/zcash that referenced this pull request Aug 12, 2018

Auto merge of zcash#3228 - str4d:3058-taddr-encoding-refactor, r=str4d
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 zcash#3058. Precursor to zcash#3202.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.